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

[Sam Ravnborg <sam@ravnborg.org>]

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).

> +
> +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.
>  
> +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