Messages in this thread | | | Date | Tue, 24 Jan 2006 13:13:12 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH -mm] swsusp: userland interface (rev 2) |
| |
"Rafael J. Wysocki" <rjw@sisk.pl> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |