Hi Sam, Thanks for the feedback. On 07/16/2014 01:04 PM, Sam Ravnborg wrote:
Hi Kahlid. On Wed, Jul 16, 2014 at 08:02:03AM -0600, Khalid Aziz wrote:/dev/mdesc on Linux does not support reading arbitrary number of bytes and seeking while /dev/mdesc on Solaris does. This causes tools that work on Solaris to break on Linux. This patch adds these two capabilities to /dev/mdesc. Signed-off-by: Khalid Aziz <khalid.aziz@xxxxxxxxxx> --- arch/sparc/kernel/mdesc.c | 77 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 11 deletions(-) +/* mdesc_open() - Grab a reference to mdesc_handle when /dev/mdesc is + * opened. Hold this reference until /dev/mdesc is closed to ensure + * mdesc data structure is not released underneath us. Store the + * pointer to mdesc structure in private_data for read and seek to use + */ +static int mdesc_open(struct inode *inode, struct file *file) { struct mdesc_handle *hp = mdesc_grab(); if (!hp) return -ENODEV; + file->private_data = hp; + return 0; +}Do we know the open/close always come in pairs? I assume so - but there is no check fo this (at least on this level).
Most likely yes, but I wouldn't assume that to be guaranteed. Is that a concern? Isn't "struct file" unique for each instance of open?
+ +static ssize_t mdesc_read(struct file *file, char __user *buf, + size_t len, loff_t *offp) +{ + struct mdesc_handle *hp = file->private_data; + unsigned char *mdesc; + int err, bytes_left; + + if (*offp >= hp->handle_size) + return 0; + err = len; + bytes_left = hp->handle_size - *offp; + if (len > bytes_left) + err = bytes_left; + mdesc = (unsigned char *)&hp->mdesc; + mdesc += *offp; + if (copy_to_user(buf, mdesc, err)) err = -EFAULT; - mdesc_release(hp); + else + *offp += err; + + return err; +}When reading your code it is confusing to read that err is set to len, and then maybe later set to an error value or a new len. See the following refactoring of mdesc_read() that avoids the err local variable resulting in more readable code. static ssize_t mdesc_read(struct file *file, char __user *buf, size_t len, loff_t *offp) { struct mdesc_handle *hp = file->private_data; unsigned char *mdesc; int bytes_left; if (*offp >= hp->handle_size) return 0; bytes_left = hp->handle_size - *offp; if (len > bytes_left) len = bytes_left; mdesc = (unsigned char *)&hp->mdesc; mdesc += *offp; if (!copy_to_user(buf, mdesc, len)) { *offp += len; return len; } else { return -EFAULT; } } The above is IMO more readable.
I was simply following how err was used in the original code, but I agree this is more readable. I can redo the patch.
+static loff_t mdesc_llseek(struct file *file, loff_t offset, int whence) +{ + struct mdesc_handle *hp; + int err; + + switch (whence) { + case SEEK_CUR: + offset += file->f_pos; + break; + case SEEK_SET: + break; + default: + return -EINVAL; + } + + err = offset; + hp = file->private_data; + if (offset > hp->handle_size) + err = -EINVAL; + else + file->f_pos = offset; return err; }Same story here with err. Sam -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
-- Khalid -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
