Re: [PATCH] FS: nfs: removed goto statement

[Trond Myklebust]

On Fri, 2022-05-20 at 00:51 +0200, Javier Abrego wrote:
> [You don't often get email from javier.abrego.lorente@xxxxxxxxx.
> Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification.]
> 
> In this function goto can be replaced. Avoiding goto will improve the
> readability
> 
> Signed-off-by: Javier Abrego<javier.abrego.lorente@xxxxxxxxx>
> ---
>  fs/nfs/nfs42xattr.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
> index e7b34f7e0..2fc806454 100644
> --- a/fs/nfs/nfs42xattr.c
> +++ b/fs/nfs/nfs42xattr.c
> @@ -743,25 +743,19 @@ void nfs4_xattr_cache_set_list(struct inode
> *inode, const char *buf,
>         struct nfs4_xattr_entry *entry;
> 
>         cache = nfs4_xattr_get_cache(inode, 1);
> -       if (cache == NULL)
> -               return;
> -
> -       entry = nfs4_xattr_alloc_entry(NULL, buf, NULL, buflen);
> -       if (entry == NULL)
> -               goto out;
> -
> -       /*
> -        * This is just there to be able to get to bucket->cache,
> -        * which is obviously the same for all buckets, so just
> -        * use bucket 0.
> -        */
> -       entry->bucket = &cache->buckets[0];
> -
> -       if (!nfs4_xattr_set_listcache(cache, entry))
> -               kref_put(&entry->ref, nfs4_xattr_free_entry_cb);
> -
> -out:
> -       kref_put(&cache->ref, nfs4_xattr_free_cache_cb);
> +       if (cache == NULL) {
> +               kref_put(&cache->ref, nfs4_xattr_free_cache_cb);
> +       } else {
> +              /*
> +               * This is just there to be able to get to bucket-
> >cache,
> +               * which is obviously the same for all buckets, so
> just
> +               * use bucket 0.
> +               */
> +               entry->bucket = &cache->buckets[0];
> +
> +               if (!nfs4_xattr_set_listcache(cache, entry))
> +                       kref_put(&entry->ref,
> nfs4_xattr_free_entry_cb);
> +       }
>  }
> 

This "cleanup" has clearly not seen any testing at all, since it
appears to call kref_put(&cache->ref,) when cache == NULL, and it leaks
the same reference in the case where cache != NULL. Not to mention the
fact that it removes the allocation of entry altogether.

Thanks, but I think we'll pass.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx