Skip to contentSkip to conversation

hfsplus: add missing curly braces in hfsplus_delete_cat()

PATCH In review Dan Carpenter 3 participants 8 messages Feb 25, 2015

Subsystems (3)
VFS Kernel janitors Hfsplus filesystem
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 hfsplus: add missing curly braces in hfsplus_delete_cat()

This doesn't change how the code works, but clearly the curly braces were intended.

Signed-off-by Dan Carpenter <[email protected]>
fs/hfsplus/catalog.c +2 -1
index 7892e6f..022974a 100644
@@ -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)
#1 lore raw
Quoted 12 lines · Dan Carpenter

This doesn't change how the code works, but clearly the curly braces were intended.

Signed-off-by Dan Carpenter <[email protected]>
fs/hfsplus/catalog.c +0 -0
index 7892e6f..022974a 100644
@@ -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);

fs/hfsplus/catalog.c int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str) · line 360

Quoted 10 lines
fs/hfsplus/catalog.c +2 -1
                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 <[email protected]>

author
#2 lore raw
Quoted 3 lines · Sergei Antonov

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.

Quoted 3 lines

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

#3 lore raw
Quoted 8 lines · Dan Carpenter
Quoted 3 lines · Sergei Antonov

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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 1 line

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

author
#4 lore raw

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

#5 lore raw
Quoted 6 lines · Dan Carpenter

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.

author
#6 lore raw
Quoted 10 lines · Sergei Antonov
Quoted 6 lines · Dan Carpenter

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

maintainer
#7 lore raw
In reply to Dan Carpenter
Quoted 4 lines · Dan Carpenter

This doesn't change how the code works, but clearly the curly braces were intended.

Signed-off-by Dan Carpenter <[email protected]>

Looks good for me.

Thanks,
Vyacheslav Dubeyko.

Quoted 18 lines
fs/hfsplus/catalog.c +2 -1
index 7892e6f..022974a 100644
@@ -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)