diff mbox series

FS: nfs: removed goto statement

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

Commit Message

Javier Abrego May 20, 2022, 11:57 a.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 | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Comments

Javier Abrego May 20, 2022, noon UTC | #1
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
>
Benjamin Coddington May 20, 2022, 12:41 p.m. UTC | #2
On 20 May 2022, at 8:00, Javier Abrego Lorente wrote:

> final version, I promise.

Javier, please read "7) Centralized exiting of functions" in
Documentation/process/coding-style.rst.  The maintainers are unlikely to
take this patch because it doesn't (my opinion) improve the readability, and
removing a goto isn't a valid reason on its own.  The use of goto is an
acceptable practice in the linux kernel for some patterns.

Ben
Javier Abrego May 20, 2022, 10:44 p.m. UTC | #3
Thanks for the link.
I think that goto is really useful and makes a lot of sense when there
are different parts in the function where you want to go to the exit
block.

I think this function is not one of those cases since there's only one
exit point and the goto can be easily removed, in my case 3rd try :)
Maybe at some point, this function had different parts in the code
where it went to the "out" block. But not anymore.

However, I donĀ“t have a strong opinion on this patch. If it's accepted
that's great but mostly I wanted to learn the process of how to
contribute to the kernel and I did it!

Thanks,
Javier

On Fri, 20 May 2022 at 14:41, Benjamin Coddington <bcodding@redhat.com> wrote:
>
> On 20 May 2022, at 8:00, Javier Abrego Lorente wrote:
>
> > final version, I promise.
>
> Javier, please read "7) Centralized exiting of functions" in
> Documentation/process/coding-style.rst.  The maintainers are unlikely to
> take this patch because it doesn't (my opinion) improve the readability, and
> removing a goto isn't a valid reason on its own.  The use of goto is an
> acceptable practice in the linux kernel for some patterns.
>
> Ben
>
diff mbox series

Patch

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);
 }