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

[Khalid Aziz]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/