| Message ID | 20150225133644.GW19745@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 February 2015 at 14:36, Dan Carpenter <dan.carpenter@oracle.com> wrote: > This doesn't change how the code works, but clearly the curly braces > were intended. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c > index 7892e6f..022974a 100644 > --- a/fs/hfsplus/catalog.c > +++ b/fs/hfsplus/catalog.c > @@ -350,10 +350,11 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str) > &fd.search_key->cat.name.unicode, > off + 2, len); > fd.search_key->key_len = cpu_to_be16(6 + len); > - } else > + } else { > err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str); > if (unlikely(err)) > goto out; > + } > > err = hfs_brec_find(&fd, hfs_find_rec_by_key); > if (err) Right you are. I would also add 2 things: 1. CC the author of the last patch (the one which introduced it). 2. Unify the way the return code from hfsplus_cat_build_key() is checked. Now it has two flavours: "if (unlikely(err < 0))" and "if (unlikely(err))". The latter is better. If you do so and resubmit, then it is Reviewed-by: Sergei Antonov <saproj@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2015 at 03:50:19PM +0100, Sergei Antonov wrote: > Right you are. > I would also add 2 things: > 1. CC the author of the last patch (the one which introduced it). Huh? Sougata is CC'd. I didn't add a fixes: tag because this is just a cleanup and has no effect on runtime. > 2. Unify the way the return code from hfsplus_cat_build_key() is > checked. Now it has two flavours: "if (unlikely(err < 0))" and "if > (unlikely(err))". The latter is better. I'm a bit confused. 1) This function uses "if (unlikely(err)) " consistently. 2) I don't see how any of that relates to this patch?? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2015-02-25 at 16:36 +0300, Dan Carpenter wrote: > This doesn't change how the code works, but clearly the curly braces > were intended. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Looks good for me. Thanks, Vyacheslav Dubeyko. > > diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c > index 7892e6f..022974a 100644 > --- a/fs/hfsplus/catalog.c > +++ b/fs/hfsplus/catalog.c > @@ -350,10 +350,11 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str) > &fd.search_key->cat.name.unicode, > off + 2, len); > fd.search_key->key_len = cpu_to_be16(6 + len); > - } else > + } else { > err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str); > if (unlikely(err)) > goto out; > + } > > err = hfs_brec_find(&fd, hfs_find_rec_by_key); > if (err) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 February 2015 at 18:13, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Wed, Feb 25, 2015 at 03:50:19PM +0100, Sergei Antonov wrote: >> Right you are. >> I would also add 2 things: >> 1. CC the author of the last patch (the one which introduced it). > > Huh? Sougata is CC'd. I didn't add a fixes: tag because this is just a > cleanup and has no effect on runtime. > >> 2. Unify the way the return code from hfsplus_cat_build_key() is >> checked. Now it has two flavours: "if (unlikely(err < 0))" and "if >> (unlikely(err))". The latter is better. > > I'm a bit confused. > 1) This function uses "if (unlikely(err)) " consistently. The last patch https://github.com/torvalds/linux/commit/89ac9b4d3d1a049ae1054f99b1aed81092cd0a82#diff-37a74f715b10ff2d442d82812c89e874 intruduced "if (unlikely(err < 0))" in fs/hfsplus/dir.c for example, but "if (unlikely(err))" in fs/hfsplus/catalog.c > 2) I don't see how any of that relates to this patch?? This patch is not bad. But I'd rather see a bigger "Fix the last commit" patch rather than "Add missing braces". -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Gar. No, I don't care about "if (unlikely(err < 0)" or "if (unlikely(err)) ". Those seem like petty things to me so I'm not getting involved with the argument between the two of you. (My secret real opinion, is that I doubt anyone benchmarked it so probably the unlikely() annotations hurt readability for no good reason. In other words, "if (err)" is correct.) regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 February 2015 at 19:30, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Gar. No, I don't care about "if (unlikely(err < 0)" or > "if (unlikely(err)) ". Those seem like petty things to me so I'm not > getting involved with the argument between the two of you. (My secret > real opinion, is that I doubt anyone benchmarked it so probably the > unlikely() annotations hurt readability for no good reason. In other > words, "if (err)" is correct.) Thanks for drawing attention to this code anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 25, 2015 at 07:37:47PM +0100, Sergei Antonov wrote: > On 25 February 2015 at 19:30, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Gar. No, I don't care about "if (unlikely(err < 0)" or > > "if (unlikely(err)) ". Those seem like petty things to me so I'm not > > getting involved with the argument between the two of you. (My secret > > real opinion, is that I doubt anyone benchmarked it so probably the > > unlikely() annotations hurt readability for no good reason. In other > > words, "if (err)" is correct.) > > > Thanks for drawing attention to this code anyway. No problem. Please give me a Reported-by if you fix the static checker warning. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c index 7892e6f..022974a 100644 --- a/fs/hfsplus/catalog.c +++ b/fs/hfsplus/catalog.c @@ -350,10 +350,11 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str) &fd.search_key->cat.name.unicode, off + 2, len); fd.search_key->key_len = cpu_to_be16(6 + len); - } else + } else { err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str); if (unlikely(err)) goto out; + } err = hfs_brec_find(&fd, hfs_find_rec_by_key); if (err)
This doesn't change how the code works, but clearly the curly braces were intended. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html