Re: [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc

Khalid Aziz
2014-07-16T20:35:52Z

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@oracle.com>
>> ---
>> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
Khalid