"Rafael J. Wysocki" <[email protected]> wrote:
>
> Hi,
>
> This patch introduces a user space interface for swsusp.
How will we know if/when this feature is ready for mainline? What criteria
can we use to judge that?
Will you be developing and long-term maintaining the userspace tools? Is
it your expectation/hope that distros will migrate onto using them? etc.
> +
> +static int snapshot_open(struct inode *inode, struct file *filp)
> +{
> + struct snapshot_data *data;
> +
> + if (!atomic_dec_and_test(&device_available)) {
> + atomic_inc(&device_available);
You may find that atomic_add_unless(..., -1, ...) is neater here, and
closes the tiny race.
> + return -EBUSY;
> + }
> +
> + if ((filp->f_flags & O_ACCMODE) == O_RDWR)
> + return -ENOSYS;
> +
> + nonseekable_open(inode, filp);
> + data = &snapshot_state;
> + filp->private_data = data;
> + memset(&data->handle, 0, sizeof(struct snapshot_handle));
<goes off hunting elsewhere for the defn of data->handle. grr>
> +static ssize_t snapshot_read(struct file *filp, char __user *buf,
> + size_t count, loff_t *offp)
> +{
> + struct snapshot_data *data;
> + ssize_t res;
> +
> + data = filp->private_data;
> + res = snapshot_read_next(&data->handle, count);
> + if (res > 0) {
> + if (copy_to_user(buf, data_of(data->handle), res))
> + res = -EFAULT;
> + else
> + *offp = data->handle.offset;
> + }
> + return res;
> +}
It's more conventional for a read() to return less-than-was-asked-for when
it hits a fault. Doesn't matter though - lots of drivers do it this way.
> +static ssize_t snapshot_write(struct file *filp, const char __user *buf,
> + size_t count, loff_t *offp)
> +{
> + struct snapshot_data *data;
> + ssize_t res;
> +
> + data = filp->private_data;
> + res = snapshot_write_next(&data->handle, count);
> + if (res > 0) {
> + if (copy_from_user(data_of(data->handle), buf, res))
> + res = -EFAULT;
> + else
> + *offp = data->handle.offset;
> + }
> + return res;
> +}
Ditto.
> +static int snapshot_ioctl(struct inode *inode, struct file *filp,
> + unsigned int cmd, unsigned long arg)
> +{
>
> ...
>
> + case SNAPSHOT_ATOMIC_RESTORE:
> + if (data->mode != O_WRONLY || !data->frozen ||
> + !snapshot_image_loaded(&data->handle)) {
> + error = -EPERM;
> + break;
> + }
> + down(&pm_sem);
> + pm_prepare_console();
> + error = device_suspend(PMSG_FREEZE);
> + if (!error) {
> + mb();
> + error = swsusp_resume();
> + device_resume();
> + }
whee, what does the mystery barrier do? It'd be nice to comment this
(please always comment open-coded barriers).
> + case SNAPSHOT_GET_SWAP_PAGE:
> + if (!access_ok(VERIFY_WRITE, (unsigned long __user *)arg, _IOC_SIZE(cmd))) {
> + error = -EINVAL;
> + break;
> + }
Why do we need an access_ok() here?
Should it return -EFAULT?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
