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