[patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Dan Carpenter]

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)

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Sergei Antonov]

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>

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Dan Carpenter]

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

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Viacheslav Dubeyko]

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)

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Sergei Antonov]

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

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[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.)

regards,
dan carpenter

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Sergei Antonov]

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.

Re: [patch] hfsplus: add missing curly braces in hfsplus_delete_cat()

[Dan Carpenter]

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