| Message ID | 20220520115714.47321-1-javier.abrego.lorente@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | FS: nfs: removed goto statement | expand |
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 >
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
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 --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); }
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(-)