[PATCH v2 0/6] Patch series to make lustre safe(r) for W=1 compiles

[PATCH v2 0/6] Patch series to make lustre safe(r) for W=1 compiles

[Valdis Kletnieks]

Start of a batch series to clean up the Lustre tree. Other people have
done some sparse and checkpatch cleanups, but I found a bunch of
stuff building with W=1. There's probably more, but this was the
really low-hanging obvious fruit.

Valdis Kletnieks (6):
  staging/lustre: Silence warning about 'inline'
  staging/lustre: Fix set-but-unused whinge.
  staging/lustre: Clean up another C warnining: set but not used
  staging/lustre: Fix another C compiler whine: set but not used
  staging/lustre: Nuke an unsigned >= 0 assert
  staging/lustre: Nuke another unsigned >= 0 assert

 drivers/staging/lustre/lustre/fid/lproc_fid.c      |  1 +
 drivers/staging/lustre/lustre/include/lu_object.h  |  2 +-
 drivers/staging/lustre/lustre/include/lustre_cfg.h |  4 --
 drivers/staging/lustre/lustre/libcfs/module.c      | 15 ++++----
 drivers/staging/lustre/lustre/llite/rw.c           |  1 -
 drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    |  1 -
 6 files changed, 9 insertions(+), 15 deletions(-)

-- 
2.6.3

[PATCH v2 1/6] staging/lustre: Silence warning about 'inline'

[Valdis Kletnieks]

Low-hanging fruit first:

  CC [M]  drivers/staging/lustre/lustre/fid/fid_request.o
In file included from drivers/staging/lustre/lustre/fid/../include/lustre_net.h:66:0,
                 from drivers/staging/lustre/lustre/fid/../include/lustre_lib.h:64,
                 from drivers/staging/lustre/lustre/fid/../include/obd.h:52,
                 from drivers/staging/lustre/lustre/fid/fid_request.c:48:
drivers/staging/lustre/lustre/fid/../include/lu_object.h:765:1: warning: 'inline' is not at beginning of declaration [-Wold-style-declaration]
 static const inline struct lu_device_operations *
 ^

So we just swap inline and const.  272 warnings gone. :)

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/lustre/lustre/include/lu_object.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/include/lu_object.h b/drivers/staging/lustre/lustre/include/lu_object.h
index fa78689748a9..176724f60c1b 100644
--- a/drivers/staging/lustre/lustre/include/lu_object.h
+++ b/drivers/staging/lustre/lustre/include/lu_object.h
@@ -756,7 +756,7 @@ static inline const struct lu_fid *lu_object_fid(const struct lu_object *o)
 /**
  * return device operations vector for this object
  */
-static const inline struct lu_device_operations *
+static inline const struct lu_device_operations *
 lu_object_ops(const struct lu_object *o)
 {
 	return o->lo_dev->ld_ops;
-- 
2.6.3

[PATCH v2 2/6] Fix set-but-unused whinge.

[Valdis Kletnieks]

drivers/staging/lustre/lustre/fid/lproc_fid.c: In function 'ldebugfs_fid_write_common':
drivers/staging/lustre/lustre/fid/lproc_fid.c:67:6: warning: variable 'rc' set but not used [-Wunused-but-set-variable]
  int rc;

We fix it by *using* the return code to help bulletproof it.  It says it's
test code - it should be *more* bulletproof than production, not less.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/lustre/lustre/fid/lproc_fid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/lustre/lustre/fid/lproc_fid.c b/drivers/staging/lustre/lustre/fid/lproc_fid.c
index ce90c1c54a63..eff011f30fa5 100644
--- a/drivers/staging/lustre/lustre/fid/lproc_fid.c
+++ b/drivers/staging/lustre/lustre/fid/lproc_fid.c
@@ -85,6 +85,8 @@ ldebugfs_fid_write_common(const char __user *buffer, size_t count,
 	rc = sscanf(kernbuf, "[%llx - %llx]\n",
 		    (unsigned long long *)&tmp.lsr_start,
 		    (unsigned long long *)&tmp.lsr_end);
+	if (rc != 2)
+		return -EINVAL;
 	if (!range_is_sane(&tmp) || range_is_zero(&tmp) ||
 	    tmp.lsr_start < range->lsr_start || tmp.lsr_end > range->lsr_end)
 		return -EINVAL;
-- 
2.6.3

[PATCH v2 3/6] Clean up another C warnining: set but not used

[Valdis Kletnieks]

drivers/staging/lustre/lustre/fid/../include/lustre_cfg.h: In function 'lustre_cfg_free':
drivers/staging/lustre/lustre/fid/../include/lustre_cfg.h:253:6: warning: variable 'len' set but not used [-Wunused-but-set-variable]
  int len;

Yep, we're just gonna call kfree, no need to calculate len. Bye-bye.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/lustre/lustre/include/lustre_cfg.h |  4 --
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/lustre_cfg.h b/drivers/staging/lustre/lustre/include/lustre_cfg.h
index eb6b292b7b25..d30d8b054c92 100644
--- a/drivers/staging/lustre/lustre/include/lustre_cfg.h
+++ b/drivers/staging/lustre/lustre/include/lustre_cfg.h
@@ -252,10 +252,6 @@ static inline struct lustre_cfg *lustre_cfg_new(int cmd,
 
 static inline void lustre_cfg_free(struct lustre_cfg *lcfg)
 {
-	int len;
-
-	len = lustre_cfg_len(lcfg->lcfg_bufcount, lcfg->lcfg_buflens);
-
 	kfree(lcfg);
 	return;
 }

[PATCH v2 4/6] Fix another C compiler whine: set but not used

[Valdis Kletnieks]

  CC [M]  drivers/staging/lustre/lustre/libcfs/module.o
drivers/staging/lustre/lustre/libcfs/module.c: In function 'lustre_insert_debugfs':
drivers/staging/lustre/lustre/libcfs/module.c:670:17: warning: variable 'entry' set but not used [-Wunused-but-set-variable]
  struct dentry *entry;
                     ^

Just ignore the dentry returned, and add a comment that we *know*
we're not really leaking the dentry because something else will be able
to reap it via recursion.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/lustre/lustre/libcfs/module.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/lustre/lustre/libcfs/module.c b/drivers/staging/lustre/lustre/libcfs/module.c
index 96d9d4651a51..4438dc426b54 100644
--- a/drivers/staging/lustre/lustre/libcfs/module.c
+++ b/drivers/staging/lustre/lustre/libcfs/module.c
@@ -667,8 +667,6 @@ static const struct file_operations lnet_debugfs_file_operations = {
 void lustre_insert_debugfs(struct ctl_table *table,
 			   const struct lnet_debugfs_symlink_def *symlinks)
 {
-	struct dentry *entry;
-
 	if (lnet_debugfs_root == NULL)
 		lnet_debugfs_root = debugfs_create_dir("lnet", NULL);
 
@@ -676,15 +674,17 @@ void lustre_insert_debugfs(struct ctl_table *table,
 	if (IS_ERR_OR_NULL(lnet_debugfs_root))
 		return;
 
+	/* We don't save the dentry returned in next two calls, because
+	 * we don't call debugfs_remove() but rather remove_recursive()
+	 */
 	for (; table->procname; table++)
-		entry = debugfs_create_file(table->procname, table->mode,
-					    lnet_debugfs_root, table,
-					    &lnet_debugfs_file_operations);
+		debugfs_create_file(table->procname, table->mode,
+				    lnet_debugfs_root, table,
+				    &lnet_debugfs_file_operations);
 
 	for (; symlinks && symlinks->name; symlinks++)
-		entry = debugfs_create_symlink(symlinks->name,
-					       lnet_debugfs_root,
-					       symlinks->target);
+		debugfs_create_symlink(symlinks->name, lnet_debugfs_root,
+				       symlinks->target);
 
 }
 EXPORT_SYMBOL_GPL(lustre_insert_debugfs);
-- 
2.6.3

[PATCH v2 5/6] Nuke an unsigned >= 0 assert

[Valdis Kletnieks]

Writing asserts for almost-never-can-happen things can be valuable.
Writing an assert that tests that an "unsigned int" hasn't gone negative
isn't.

And it generates an *ugly* message:

drivers/staging/lustre/lustre/llite/rw.c:763:20: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
  LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
                    ^
include/linux/compiler.h:137:45: note: in definition of macro 'unlikely'
 #  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 0))
                                             ^
drivers/staging/lustre/lustre/llite/rw.c:763:2: note: in expansion of macro 'LASSERTF'
  LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
  ^
drivers/staging/lustre/lustre/llite/rw.c:763:20: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
  LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
                    ^
include/linux/compiler.h:137:53: note: in definition of macro 'unlikely'
 #  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 0))
                                                     ^
drivers/staging/lustre/lustre/llite/rw.c:763:2: note: in expansion of macro 'LASSERTF'
  LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
  ^
drivers/staging/lustre/lustre/llite/rw.c:763:20: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
  LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
                    ^
include/linux/compiler.h:110:47: note: in definition of macro 'likely_notrace'
 #define likely_notrace(x) __builtin_expect(!!(x), 1)
                                               ^
include/linux/compiler.h:137:58: note: in expansion of macro '__branch_check__'
 #  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : __branch_check__(x, 0))
                                                          ^
drivers/staging/lustre/lustre/llite/../include/linux/../../../include/linux/libcfs/libcfs_private.h:58:6: note: in expansion of macro 'unlikely'
  if (unlikely(!(cond))) {     \
      ^
drivers/staging/lustre/lustre/llite/rw.c:763:2: note: in expansion of macro 'LASSERTF'
  LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
  ^

Umm, thank you, GCC.  We'll delete the problem line so we never see that spew again.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/lustre/lustre/llite/rw.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/rw.c b/drivers/staging/lustre/lustre/llite/rw.c
index f79193fa2fb7..39390aab9da2 100644
--- a/drivers/staging/lustre/lustre/llite/rw.c
+++ b/drivers/staging/lustre/lustre/llite/rw.c
@@ -764,7 +764,6 @@ int ll_readahead(const struct lu_env *env, struct cl_io *io,
 	ret = ll_read_ahead_pages(env, io, queue,
 				  ria, &reserved, mapping, &ra_end);
 
-	LASSERTF(reserved >= 0, "reserved %lu\n", reserved);
 	if (reserved != 0)
 		ll_ra_count_put(ll_i2sbi(inode), reserved);
 
-- 
2.6.3

[PATCH v2 6/6] Nuke another unsigned >= 0 assert

[Valdis Kletnieks]

Clean up another case of the compiler remininding the programmer they
are an idiot:

drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c:308:34: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
  LASSERT(page_pools.epp_waitqlen >= 0);

Just lose the assert, and save a page of compiler spew.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c b/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
index cd8a9987f7ac..1f326673f089 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c
@@ -304,7 +304,6 @@ static unsigned long enc_pools_cleanup(struct page ***pools, int npools)
 static inline void enc_pools_wakeup(void)
 {
 	assert_spin_locked(&page_pools.epp_lock);
-	LASSERT(page_pools.epp_waitqlen >= 0);
 
 	if (unlikely(page_pools.epp_waitqlen)) {
 		LASSERT(waitqueue_active(&page_pools.epp_waitq));
-- 
2.6.3

Re: [PATCH v2 0/6] Patch series to make lustre safe(r) for W=1 compiles

[Greg KH]

On Fri, Dec 11, 2015 at 06:04:19PM -0500, Valdis Kletnieks wrote:
> Start of a batch series to clean up the Lustre tree. Other people have
> done some sparse and checkpatch cleanups, but I found a bunch of
> stuff building with W=1. There's probably more, but this was the
> really low-hanging obvious fruit.
> 
> Valdis Kletnieks (6):
>   staging/lustre: Silence warning about 'inline'
>   staging/lustre: Fix set-but-unused whinge.
>   staging/lustre: Clean up another C warnining: set but not used
>   staging/lustre: Fix another C compiler whine: set but not used
>   staging/lustre: Nuke an unsigned >= 0 assert
>   staging/lustre: Nuke another unsigned >= 0 assert

These subject names don't match what you actually sent out, the 'prefix'
got stripped.  Please resend them all so that I can properly apply them.

thanks,

greg k-h