diff mbox series

FS: nfs: removed goto statement

Message ID 20220519225115.35431-1-javier.abrego.lorente@gmail.com (mailing list archive)
State New
Headers show
Series FS: nfs: removed goto statement | expand

Commit Message

Javier Abrego Lorente May 19, 2022, 10:51 p.m. UTC
In this function goto can be replaced. Avoiding goto will improve the
readability

Signed-off-by: Javier Abrego<javier.abrego.lorente@gmail.com>
---
 fs/nfs/nfs42xattr.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

Comments

Frank van der Linden May 19, 2022, 11:49 p.m. UTC | #1
On Fri, May 20, 2022 at 12:51:15AM +0200, Javier Abrego wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> In this function goto can be replaced. Avoiding goto will improve the
> readability
> 
> Signed-off-by: Javier Abrego<javier.abrego.lorente@gmail.com>
> ---
>  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);
> +       }
>  }
> 
>  /*

Hm, I'm not sure what your intention was here, but this patch is wrong
in several ways. It references 'cache' when it's NULL. It removes the
allocation of 'entry' altogether, and then references an uninitialized
variable. Which, surely, gcc would have warned about.

I mean, in the original code, you could replace

if (entry == NULL)
	goto out;

with

if (entry != NULL) {
	...
}

..and remove the out label. Not sure if that would make things massively
more readable.

- Frank
Trond Myklebust May 20, 2022, 12:18 a.m. UTC | #2
On Fri, 2022-05-20 at 00:51 +0200, Javier Abrego wrote:
> [You don't often get email from javier.abrego.lorente@gmail.com.
> 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@gmail.com>
> ---
>  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.
kernel test robot May 20, 2022, 3:58 a.m. UTC | #3
Hi Javier,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on v5.18-rc7 next-20220519]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Javier-Abrego/FS-nfs-removed-goto-statement/20220520-065648
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: i386-randconfig-a015 (https://download.01.org/0day-ci/archive/20220520/202205201106.ONDKuLvW-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project e00cbbec06c08dc616a0d52a20f678b8fbd4e304)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/99dd76e4af5d61f97c1981a240cbd1d86908ac8e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Javier-Abrego/FS-nfs-removed-goto-statement/20220520-065648
        git checkout 99dd76e4af5d61f97c1981a240cbd1d86908ac8e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash fs/nfs/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/nfs/nfs42xattr.c:754:3: warning: variable 'entry' is uninitialized when used here [-Wuninitialized]
                   entry->bucket = &cache->buckets[0];
                   ^~~~~
   fs/nfs/nfs42xattr.c:743:32: note: initialize the variable 'entry' to silence this warning
           struct nfs4_xattr_entry *entry;
                                         ^
                                          = NULL
   1 warning generated.


vim +/entry +754 fs/nfs/nfs42xattr.c

95ad37f90c338e Frank van der Linden 2020-06-23  735  
95ad37f90c338e Frank van der Linden 2020-06-23  736  /*
95ad37f90c338e Frank van der Linden 2020-06-23  737   * Cache listxattr output, replacing any possible old one.
95ad37f90c338e Frank van der Linden 2020-06-23  738   */
95ad37f90c338e Frank van der Linden 2020-06-23  739  void nfs4_xattr_cache_set_list(struct inode *inode, const char *buf,
95ad37f90c338e Frank van der Linden 2020-06-23  740  			       ssize_t buflen)
95ad37f90c338e Frank van der Linden 2020-06-23  741  {
95ad37f90c338e Frank van der Linden 2020-06-23  742  	struct nfs4_xattr_cache *cache;
95ad37f90c338e Frank van der Linden 2020-06-23  743  	struct nfs4_xattr_entry *entry;
95ad37f90c338e Frank van der Linden 2020-06-23  744  
95ad37f90c338e Frank van der Linden 2020-06-23  745  	cache = nfs4_xattr_get_cache(inode, 1);
99dd76e4af5d61 Javier Abrego        2022-05-20  746  	if (cache == NULL) {
99dd76e4af5d61 Javier Abrego        2022-05-20  747  		kref_put(&cache->ref, nfs4_xattr_free_cache_cb);
99dd76e4af5d61 Javier Abrego        2022-05-20  748  	} else {
95ad37f90c338e Frank van der Linden 2020-06-23  749  	       /*
95ad37f90c338e Frank van der Linden 2020-06-23  750  		* This is just there to be able to get to bucket->cache,
95ad37f90c338e Frank van der Linden 2020-06-23  751  		* which is obviously the same for all buckets, so just
95ad37f90c338e Frank van der Linden 2020-06-23  752  		* use bucket 0.
95ad37f90c338e Frank van der Linden 2020-06-23  753  		*/
95ad37f90c338e Frank van der Linden 2020-06-23 @754  		entry->bucket = &cache->buckets[0];
95ad37f90c338e Frank van der Linden 2020-06-23  755  
95ad37f90c338e Frank van der Linden 2020-06-23  756  		if (!nfs4_xattr_set_listcache(cache, entry))
95ad37f90c338e Frank van der Linden 2020-06-23  757  			kref_put(&entry->ref, nfs4_xattr_free_entry_cb);
99dd76e4af5d61 Javier Abrego        2022-05-20  758  	}
95ad37f90c338e Frank van der Linden 2020-06-23  759  }
95ad37f90c338e Frank van der Linden 2020-06-23  760
diff mbox series

Patch

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);
+	}
 }
 
 /*