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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
: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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On 2015/12/01, 15:05, "Valdis Kletnieks" <[email protected]> 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 <[email protected]>" and also
[email protected] 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