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 <[email protected]>
---
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
