[PATCH] FS: nfs: removed goto statement

[PATCH] FS: nfs: removed goto statement

[Javier Abrego]

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 | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
index e7b34f7e0..9bf3a88fd 100644
--- a/fs/nfs/nfs42xattr.c
+++ b/fs/nfs/nfs42xattr.c
@@ -747,21 +747,19 @@ void nfs4_xattr_cache_set_list(struct inode *inode, const char *buf,
 		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 (entry == 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);
+	}
 }
 
 /*
-- 
2.25.1

Re: [PATCH] nfs: removed goto statement

[Benjamin Coddington]

On 20 May 2022, at 4:50, Javier Abrego wrote:

> 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 | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
> index e7b34f7e0..9bf3a88fd 100644
> --- a/fs/nfs/nfs42xattr.c
> +++ b/fs/nfs/nfs42xattr.c
> @@ -747,21 +747,19 @@ void nfs4_xattr_cache_set_list(struct inode 
> *inode, const char *buf,
>  		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 (entry == 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);
> +	}
>  }
>
>  /*
> -- 
> 2.25.1

No, NACK because you removed the kref_put of cache->ref.  I find this
function quite readable as it is.

Ben

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

[Javier Abrego Lorente]

final version, I promise.

On Fri, 20 May 2022 at 13:57, Javier Abrego
<javier.abrego.lorente@gmail.com> wrote:
>
> 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 | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
> index e7b34f7e0..78130462c 100644
> --- a/fs/nfs/nfs42xattr.c
> +++ b/fs/nfs/nfs42xattr.c
> @@ -747,20 +747,18 @@ void nfs4_xattr_cache_set_list(struct inode *inode, const char *buf,
>                 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);
> +       if (entry != NULL) {
> +              /*
> +               * 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);
>  }
>
> --
> 2.25.1
>

[PATCH] FS: nfs: removed goto statement

[Javier Abrego]

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 | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
index e7b34f7e0..78130462c 100644
--- a/fs/nfs/nfs42xattr.c
+++ b/fs/nfs/nfs42xattr.c
@@ -747,20 +747,18 @@ void nfs4_xattr_cache_set_list(struct inode *inode, const char *buf,
 		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);
+	if (entry != NULL) {
+	       /*
+		* 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);
 }
 
-- 
2.25.1

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

[kernel test robot]

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  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

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

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

[Frank van der Linden]

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

[PATCH] FS: nfs: removed goto statement

[Javier Abrego]

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