From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934970AbaGPTEm (ORCPT ); Wed, 16 Jul 2014 15:04:42 -0400 Received: from asavdk4.altibox.net ([109.247.116.15]:55860 "EHLO asavdk4.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754620AbaGPTEk (ORCPT ); Wed, 16 Jul 2014 15:04:40 -0400 Date: Wed, 16 Jul 2014 21:04:14 +0200 From: Sam Ravnborg To: Khalid Aziz Cc: davem@davemloft.net, sparclinux@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] sparc: Add support for seek and shorter read to /dev/mdesc Message-ID: <20140716190414.GA6028@ravnborg.org> References: <1405519323-3092-1-git-send-email-khalid.aziz@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1405519323-3092-1-git-send-email-khalid.aziz@oracle.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 > --- > 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