Message-ID: <CAGG-pUTqVk5JkhWDU6-EicWzQT-rhiXBuXRm6uPmVSVifQ_FzQ@mail.gmail.com>
Date:	Mon, 28 Oct 2013 23:48:11 -0200
From:	Geyslan Gregório Bem <geyslan@...il.com>
To:	Eric Van Hensbergen <ericvh@...il.com>
Cc:	Joe Perches <joe@...ches.com>, Ron Minnich <rminnich@...dia.gov>,
	Latchesar Ionkov <lucho@...kov.net>,
	V9FS Developers <v9fs-developer@...ts.sourceforge.net>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	kernel-br <kernel-br@...glegroups.com>
Subject: Re: [PATCH] 9p: unsigned/signed wrap in p9/unix modes.

2013/10/28 Geyslan Gregório Bem <geyslan@...il.com>
>
> 2013/10/27 Eric Van Hensbergen <ericvh@...il.com>
>>
>> Looks like the right approach.  The one other optional thing I mentioned was support for passing NULL for rdev and not trying to parse the device info when rdev == NULL.  Its a very slight optimization in the grand scheme of things, but would seem to be cleaner for the folks calling the function who don't touch rdev after the fact...
>>
>>      -eric
>>
> Great. Let me do the changes this afternoon.
>
>
Hi Eric and all.

You requested to avoid the parsing of device when rdev is NULL, all
right? But I'm afraid that that manner the res (return value) can be
returned wrong when the bit mode is a device. Well, I did some
changes. In this new approach, when rdev is NULL, the function only
doesn't make the device, but returns the res (umode_t) nicely.

Tell me if this approach is correct. Do I have to modify something else?

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 94de6d1..e3d56f1 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -63,7 +63,7 @@ static const struct inode_operations
v9fs_symlink_inode_operations;

 static u32 unixmode2p9mode(struct v9fs_session_info *v9ses, umode_t mode)
 {
-    int res;
+    u32 res;
     res = mode & 0777;
     if (S_ISDIR(mode))
         res |= P9_DMDIR;
@@ -125,10 +125,9 @@ static int p9mode2perm(struct v9fs_session_info *v9ses,
 static umode_t p9mode2unixmode(struct v9fs_session_info *v9ses,
                    struct p9_wstat *stat, dev_t *rdev)
 {
-    int res;
+    umode_t res;
     u32 mode = stat->mode;

-    *rdev = 0;
     res = p9mode2perm(v9ses, stat);

     if ((mode & P9_DMDIR) == P9_DMDIR)
@@ -144,10 +143,15 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
     else if ((mode & P9_DMDEVICE) && (v9fs_proto_dotu(v9ses))
          && (v9ses->nodev == 0)) {
         char type = 0, ext[32];
-        int major = -1, minor = -1;
+        u32 major = 0, minor = 0;

         strlcpy(ext, stat->extension, sizeof(ext));
-        sscanf(ext, "%c %u %u", &type, &major, &minor);
+        if (sscanf(ext, "%c %u %u", &type, &major, &minor) < 3) {
+            p9_debug(P9_DEBUG_ERROR,
+                 "It's necessary define type [%c], major [%u] and minor [%u]" \
+                 "values when mode is P9_DMDEVICE\n", type, major, minor);
+            goto err_dev;
+        }
         switch (type) {
         case 'c':
             res |= S_IFCHR;
@@ -158,11 +162,18 @@ static umode_t p9mode2unixmode(struct
v9fs_session_info *v9ses,
         default:
             p9_debug(P9_DEBUG_ERROR, "Unknown special type %c %s\n",
                  type, stat->extension);
-        };
-        *rdev = MKDEV(major, minor);
+            goto err_dev;
+        }
+        /* Only make device if rdev pointer isn't NULL */
+        if (rdev)
+            *rdev = MKDEV(major, minor);
     } else
         res |= S_IFREG;
-
+    goto ret;
+err_dev:
+    if (rdev)
+        rdev = ERR_PTR(-EIO);
+ret:
     return res;
 }

@@ -460,13 +471,12 @@ void v9fs_evict_inode(struct inode *inode)

 static int v9fs_test_inode(struct inode *inode, void *data)
 {
-    int umode;
-    dev_t rdev;
+    umode_t umode;
     struct v9fs_inode *v9inode = V9FS_I(inode);
     struct p9_wstat *st = (struct p9_wstat *)data;
     struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);

-    umode = p9mode2unixmode(v9ses, st, &rdev);
+    umode = p9mode2unixmode(v9ses, st, NULL);
     /* don't match inode of different type */
     if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
         return 0;
@@ -526,6 +536,10 @@ static struct inode *v9fs_qid_iget(struct super_block *sb,
      */
     inode->i_ino = i_ino;
     umode = p9mode2unixmode(v9ses, st, &rdev);
+    if (IS_ERR_VALUE(rdev)) {
+        retval = rdev;
+        goto error;
+    }
     retval = v9fs_init_inode(v9ses, inode, umode, rdev);
     if (retval)
         goto error;
@@ -1461,8 +1475,7 @@ v9fs_vfs_mknod(struct inode *dir, struct dentry
*dentry, umode_t mode, dev_t rde

 int v9fs_refresh_inode(struct p9_fid *fid, struct inode *inode)
 {
-    int umode;
-    dev_t rdev;
+    umode_t umode;
     loff_t i_size;
     struct p9_wstat *st;
     struct v9fs_session_info *v9ses;
@@ -1474,7 +1487,7 @@ int v9fs_refresh_inode(struct p9_fid *fid,
struct inode *inode)
     /*
      * Don't update inode if the file type is different
      */
-    umode = p9mode2unixmode(v9ses, st, &rdev);
+    umode = p9mode2unixmode(v9ses, st, NULL);
     if ((inode->i_mode & S_IFMT) != (umode & S_IFMT))
         goto out;


-- 
Regards,

Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists