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

[lustre cleanups 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.

Valdis Kletnieks (6):

  Swap inline/const to kill 272 warnings
  Fix set-but-unused whinge.
  Clean up another C warnining:
  Fix another C compiler whine:
  Nuke an unsigned >= 0 assert
  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 -
 7 files changed, 26 insertions(+), 41 deletions(-)

-- 
2.6.3

[lustre cleanups 1/6] Swap inline/const to kill 272 warnings

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

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

[lustre cleanups 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>
---
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

[lustre cleanups 3/6] Clean up another C warnining:

[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>
---
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;
 }
-- 
2.6.3

[lustre cleanups 4/6] Fix another C compiler whine:

[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>
---
:100644 100644 96d9d4651a51... 4438dc426b54... M	drivers/staging/lustre/lustre/libcfs/module.c
 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 +676,18 @@ 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

[lustre cleanups 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 ne never see that spew again.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
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

[lustre cleanups 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 u
nsigned 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>
---
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: [lustre cleanups 0/6] Patch series to make lustre safe(r) for W=1 compiles

[Dilger, Andreas]

On 2015/12/01, 15:05, "Valdis Kletnieks" <Valdis.Kletnieks@vt.edu> 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.

Hello Valdis,
thanks for these patches.  Strictly speaking, they should also be sent
to "Greg Kroah-Hartman <gregkh@linuxfoundation.org>" and also
devel@driverdev.osuosl.org because Lustre is still in the staging tree.

Also, for the patch subject line, it is standard to use something like:

    [PATCH 1/6] staging/lustre: Swap inline/const to kill 272 warnings

as generated by "git format-patch".

Could you please resubmit your patch series so that Greg sees your
patches.

Cheers, Andreas

>Valdis Kletnieks (6):
>
>  Swap inline/const to kill 272 warnings
>  Fix set-but-unused whinge.
>  Clean up another C warnining:
>  Fix another C compiler whine:
>  Nuke an unsigned >= 0 assert
>  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 -
> 7 files changed, 26 insertions(+), 41 deletions(-)
>
>-- 
>2.6.3
>
>


Cheers, Andreas
-- 
Andreas Dilger

Lustre Principal Architect
Intel High Performance Data Division