All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/72] multipath-tools: cleanup and warning enablement
@ 2019-10-12 21:27 Martin Wilck
  2019-10-12 21:27 ` [PATCH 01/72] multipath tests: move condlog test wrappers to separate file Martin Wilck
                   ` (71 more replies)
  0 siblings, 72 replies; 85+ messages in thread
From: Martin Wilck @ 2019-10-12 21:27 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

here is a series with cleanup patches and minor fixes for multipath-tools.
Sorry about the number of patches, I hope this way the series will be easier
to review. There are lots of obvious, short hunks. In terms of LoC, most
of the changes are in a new unit test, in the NVMe code update, and in
a (necessary) indentation change in the VPD code.

Patch 01-14 are part of a recent effort to go over the multipath-tools
code, re-review, and modernize the code a bit. Part of that is adding "const"
qualifiers to function arguments, as I did before. I happened to start with
"alias.c", for alphabetic reasons. Other parts of the code will hopefully
follow.

15-20 are misc fixes for stuff I came across while working on the
"-Wclobbered" flag (see below).

The rest of the series is an attempt to get rid of the disablement of
warnings that we had so far in multipath-tools. I believe we agree that
warning-free code is a good thing and that disabling warnings should be
avoided if possible. My goal was to be able to set "-Werror" and compile
successfully with all currently relevant compilers.

Patch 21-42 fix issues with -Wunused-parameter and finally enable that
warning. -Wno-unused-parameter is only kept in place for
libmultipath/dict.c and multipathd/cli-handlers.c, which basically consist
only of implementations of certain prototypes where many functions don't
use every argument, and hundreds of "unused" attributes would pollute the
code too much. Patch 53-58 fix issues with "-Wsign-compare". This was
actually a good excercise, because I was forced to think twice which
signedness was correct for certain variables and expressions. Patch 59-64
fix some warnings that are issued by clang with our current warning settings
(in particular, -Wformat-literal).

Patch 65 is an update of our nvme code from nvme-cli 1.9. Patch 66-71
contain some make file fixes and cleanups, and adaptations for older
compilers. Finally, Patch 71 enables -Werror, and patch 72 tests for
"-Wno-clobbered", which clang doesn't support.

I can proudly say that multipath-tools now compiles without warnings or
errors with -Werror and with a large set of compilers. I tested gcc 4.8,
7, 8, 9 and clang 3.9, 6, 7, and 8.

The only "-Wno" option that now remains is "-Wclobbered". I have put
considerable work into trying to eliminate that as well. The result
can be examined in the "Wclobbered" branch of my github fork:
https://github.com/mwilck/multipath-tools/commits/Wclobbered
(yes, that's another 37 patches on top of this already long series).

I have pondered this back and forth whether to submit that part of the
series, too. All the -Wclobbered warnings are caused by pthread_cleanup_push()
calls, of which our code has a lot, and which glibc implements using a
sigsetjmp() call. These warnings are arguably a false positives, and
a bug of either gcc, glibc, or both
(see e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61118). 

Eliminating these warnings is possible, but it requires a lot of changes
in the code. Some of them are actually beneficial for readability, but
others are rather not. Some are outright mysterious (e.g.
https://github.com/mwilck/multipath-tools/commit/bb53d666777f072e60372979eed51752db03cec4),
and finding the workarounds was trial-and error work. Also, there are
variations between gcc versions.

The bottom line is, while I feel sorry about the vain effort 
I put into this, my personal opinion is that silencing just this single
warning isn't worth that big amount of changes.

Reviews and opinions welcome.

Regards
Martin

Martin Wilck (72):
  multipath tests: move condlog test wrappers to separate file
  multipath tests: add tests for alias handling
  libmultipath: alias.c: constify function arguments
  libmultipath: alias.c: use strlcpy(), and 2 minor fixes
  libmultipath: format_devname: avoid buffer overflow
  libmultipath: scan_devname: fix int overflow detection
  libmultipath: lookup_binding(): don't rely on int overflow
  libmultipath: rlookup_binding(): removed unused parameter
  libmultipath: allocate_binding(): error out for id=0
  libmultipath: allocate_binding(): detect line overflow
  multipath tests: alias: add tests for allocate_binding()
  multipath tests: alias: add format/scan test
  libmultipath: alias.c: prepare for cancel-safe allocation
  multipath tests: use -lpthread for alias test
  libmultipath: path_discovery: handle libudev errors
  libmultipath: make path_discovery() pthread_cancel()-safe
  libmultipath: uevent_listen(): fix poll() retval check
  libmultipath: replace_wwids(): fix possible fd leak
  libmultipath: remove_wwids(): fix possible leaks
  libmultipath: _init_foreign(): fix possible memory leak
  libmultipath: process_config_dir(): remove unused argument
  libmultipath: mark unused arguments in partmap functions
  libmultipath: scsi_ioctl_pathinfo(): remove unused argument
  multipath-tools: mark unused params in signal and cleanup handlers
  libmultipath: get_ana_info(): remove unused parameter
  libmultipath: mark unused params in getprio() methods
  libmultipath: hp_sw: remove usused argument in do_inq()
  libmultipath: checkers: mark unused arguments in methods
  multipathd: stop_waiter_thread(): removed unused parameter
  multipath tools: mark unused arguments in thread routines
  kpartx: gpt: remove unused arg in read_lastoddsector()
  kpartx: mark unused arguments in ptreader methods
  libmultipath: mark missing arguments in functions matching prototypes
  libmultipath: get_pgpolicy_name(): use "len" parameter
  libmultipath: snprint_multipath_map_json(): remove unused argument
  multipath: delegate_to_multipathd: mark unused arguments
  libmultipath: use -Wno-unused-parameter for dict.c
  multipathd: use -Wno-unused-parameter for cli_handlers.c
  libmpathpersist: remove unused "noisy" argument in various functions
  libmpathpersist: fix copy-paste error in mpath_format_readresv()
  multipath-tools tests: add -Wno-unused-parameter
  multipath-tools: Makefile.inc: remove -Wno-unused-parameter
  libmultipath: dev_loss_tmo is unsigned
  libmultipath: trivial changes for -Wsign-compare
  libmultipath: fix -Wsign-compare warnings with snprintf()
  libmultipath: parse_vpd_pg83(): sanitize indentation
  libmultipath: parse_vpd_pg83(): fix -Wsign-compare warnings
  libmultipath: print: use unsigned int for "width" field
  libmultipath: vector_for_each_slot: fix -Wsign-compare warnings
  libmultipath: set_int(): add error check and set_uint()
  libmultipath: make "checkint" unsigned
  libmultipath: use unsigned blksize in directio context
  libmultipath, kpartx: byteorder: always use unsigned types
  libmpathcmd: fix -Wsign-compare warnings
  libmpathpersist: fix -Wsign-compare warnings
  kpartx: use unsigned arguments in dm_devn() and dm_prereq()
  kpartx: use unsigned int for "ns" argument of ptreader
  multipath-tools: Makefile.inc: enable -Wsign-compare
  libdmmp: fix clang -Wformat-nonliteral warnings
  libmultipath: fix clang -Wformat-literal warnings
  multipath tests: blacklist: remove always-true condition
  multipath tests: hwtable: fix strncat() invocation
  multipath tests: fix -Wformat-literal warning
  multipath tests: util: fix clang strlcpy warnings
  libmultipath: nvme: update to nvme-cli v1.9
  multipath-tools: Makefile.inc: fix TEST_CC_OPTION
  multipath-tools: Makefile.inc: use -Wp,... for compiling only
  multipath-tools: Makefile: use proper directory recursion
  multipath tests: Makefile: fix "clean" target
  multipath tests: Makefile: avoid gcc 4.8 missing initializers failure
  multipath-tools: Makefile.inc: enable -Werror
  multipath-tools: Makefile.inc: test for -Wno-clobbered support

 Makefile                                 |  38 +-
 Makefile.inc                             |  15 +-
 kpartx/bsd.c                             |   4 +-
 kpartx/dasd.c                            |   3 +-
 kpartx/devmapper.c                       |  13 +-
 kpartx/devmapper.h                       |   7 +-
 kpartx/dos.c                             |   4 +-
 kpartx/gpt.c                             |  15 +-
 kpartx/gpt.h                             |   2 +-
 kpartx/kpartx.h                          |  11 +-
 kpartx/mac.c                             |   5 +-
 kpartx/ps3.c                             |   5 +-
 kpartx/solaris.c                         |   4 +-
 kpartx/sun.c                             |   4 +-
 kpartx/unixware.c                        |   4 +-
 libdmmp/libdmmp_private.h                |   8 +-
 libmpathcmd/mpath_cmd.c                  |   5 +-
 libmpathpersist/mpath_persist.c          |   3 +-
 libmpathpersist/mpath_pr_ioctl.c         |  40 +-
 libmultipath/Makefile                    |   5 +
 libmultipath/alias.c                     | 134 ++--
 libmultipath/alias.h                     |  12 +-
 libmultipath/byteorder.h                 |  12 +-
 libmultipath/checkers/cciss_tur.c        |   4 +-
 libmultipath/checkers/directio.c         |   2 +-
 libmultipath/checkers/hp_sw.c            |   8 +-
 libmultipath/checkers/rdac.c             |   2 +-
 libmultipath/checkers/readsector0.c      |   4 +-
 libmultipath/config.c                    |   4 +-
 libmultipath/config.h                    |   4 +-
 libmultipath/defaults.h                  |   4 +-
 libmultipath/devmapper.c                 |  10 +-
 libmultipath/dict.c                      |  52 +-
 libmultipath/discovery.c                 | 284 +++++----
 libmultipath/discovery.h                 |   2 +-
 libmultipath/dm-generic.c                |   6 +-
 libmultipath/file.c                      |   5 +-
 libmultipath/foreign.c                   |  20 +-
 libmultipath/foreign/nvme.c              |  24 +-
 libmultipath/generic.c                   |   2 +-
 libmultipath/io_err_stat.c               |  10 +-
 libmultipath/log.h                       |   3 +-
 libmultipath/log_pthread.c               |   2 +-
 libmultipath/log_pthread.h               |   3 +-
 libmultipath/nvme/linux/nvme.h           | 136 ++++-
 libmultipath/nvme/nvme-ioctl.c           | 229 ++++---
 libmultipath/nvme/nvme-ioctl.h           |  31 +-
 libmultipath/nvme/nvme.h                 | 121 +++-
 libmultipath/parser.c                    |   2 +-
 libmultipath/pgpolicies.c                |   2 +-
 libmultipath/print.c                     |  14 +-
 libmultipath/print.h                     |   8 +-
 libmultipath/prioritizers/alua_rtpg.c    |   2 +-
 libmultipath/prioritizers/ana.c          |  14 +-
 libmultipath/prioritizers/const.c        |   4 +-
 libmultipath/prioritizers/datacore.c     |   3 +-
 libmultipath/prioritizers/emc.c          |   3 +-
 libmultipath/prioritizers/hds.c          |   3 +-
 libmultipath/prioritizers/hp_sw.c        |   3 +-
 libmultipath/prioritizers/iet.c          |   3 +-
 libmultipath/prioritizers/ontap.c        |   3 +-
 libmultipath/prioritizers/random.c       |   4 +-
 libmultipath/prioritizers/rdac.c         |   3 +-
 libmultipath/prioritizers/sysfs.c        |   3 +-
 libmultipath/prioritizers/weightedpath.c |   3 +-
 libmultipath/structs.c                   |   4 +-
 libmultipath/structs.h                   |   3 +-
 libmultipath/structs_vec.c               |   2 +-
 libmultipath/sysfs.c                     |  10 +-
 libmultipath/time-util.c                 |   6 +-
 libmultipath/uevent.c                    |   5 +-
 libmultipath/util.c                      |   7 +-
 libmultipath/util.h                      |   8 +-
 libmultipath/uxsock.c                    |   3 +-
 libmultipath/vector.h                    |   4 +-
 libmultipath/wwids.c                     |  39 +-
 mpathpersist/main.c                      |   2 +-
 multipath/main.c                         |  10 +-
 multipathd/Makefile                      |   3 +
 multipathd/cli_handlers.c                |   2 +-
 multipathd/dmevents.c                    |   4 +-
 multipathd/main.c                        |  36 +-
 multipathd/pidfile.c                     |   2 +-
 multipathd/waiter.c                      |   2 +-
 multipathd/waiter.h                      |   2 +-
 tests/Makefile                           |  19 +-
 tests/alias.c                            | 744 +++++++++++++++++++++++
 tests/blacklist.c                        |  22 +-
 tests/hwtable.c                          |   2 +-
 tests/test-log.c                         |  27 +
 tests/test-log.h                         |   7 +
 tests/util.c                             |  16 +-
 92 files changed, 1802 insertions(+), 591 deletions(-)
 create mode 100644 tests/alias.c
 create mode 100644 tests/test-log.c
 create mode 100644 tests/test-log.h

-- 
2.23.0

^ permalink raw reply	[flat|nested] 85+ messages in thread

end of thread, other threads:[~2019-11-06 20:51 UTC | newest]

Thread overview: 85+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-12 21:27 [PATCH 00/72] multipath-tools: cleanup and warning enablement Martin Wilck
2019-10-12 21:27 ` [PATCH 01/72] multipath tests: move condlog test wrappers to separate file Martin Wilck
2019-10-12 21:27 ` [PATCH 02/72] multipath tests: add tests for alias handling Martin Wilck
2019-10-12 21:27 ` [PATCH 03/72] libmultipath: alias.c: constify function arguments Martin Wilck
2019-10-12 21:27 ` [PATCH 04/72] libmultipath: alias.c: use strlcpy(), and 2 minor fixes Martin Wilck
2019-10-12 21:27 ` [PATCH 05/72] libmultipath: format_devname: avoid buffer overflow Martin Wilck
2019-10-12 21:27 ` [PATCH 06/72] libmultipath: scan_devname: fix int overflow detection Martin Wilck
2019-10-12 21:27 ` [PATCH 07/72] libmultipath: lookup_binding(): don't rely on int overflow Martin Wilck
2019-10-12 21:27 ` [PATCH 08/72] libmultipath: rlookup_binding(): removed unused parameter Martin Wilck
2019-10-12 21:27 ` [PATCH 09/72] libmultipath: allocate_binding(): error out for id=0 Martin Wilck
2019-10-12 21:27 ` [PATCH 10/72] libmultipath: allocate_binding(): detect line overflow Martin Wilck
2019-10-12 21:27 ` [PATCH 11/72] multipath tests: alias: add tests for allocate_binding() Martin Wilck
2019-10-12 21:27 ` [PATCH 12/72] multipath tests: alias: add format/scan test Martin Wilck
2019-10-12 21:27 ` [PATCH 13/72] libmultipath: alias.c: prepare for cancel-safe allocation Martin Wilck
2019-10-30 14:47   ` Benjamin Marzinski
2019-10-12 21:27 ` [PATCH 14/72] multipath tests: use -lpthread for alias test Martin Wilck
2019-10-12 21:27 ` [PATCH 15/72] libmultipath: path_discovery: handle libudev errors Martin Wilck
2019-10-12 21:27 ` [PATCH 16/72] libmultipath: make path_discovery() pthread_cancel()-safe Martin Wilck
2019-10-30 14:53   ` Benjamin Marzinski
2019-11-04  8:29     ` Martin Wilck
2019-11-06 20:51       ` Benjamin Marzinski
2019-10-12 21:27 ` [PATCH 17/72] libmultipath: uevent_listen(): fix poll() retval check Martin Wilck
2019-10-12 21:27 ` [PATCH 18/72] libmultipath: replace_wwids(): fix possible fd leak Martin Wilck
2019-10-12 21:28 ` [PATCH 19/72] libmultipath: remove_wwids(): fix possible leaks Martin Wilck
2019-10-12 21:28 ` [PATCH 20/72] libmultipath: _init_foreign(): fix possible memory leak Martin Wilck
2019-10-12 21:28 ` [PATCH 21/72] libmultipath: process_config_dir(): remove unused argument Martin Wilck
2019-10-12 21:28 ` [PATCH 22/72] libmultipath: mark unused arguments in partmap functions Martin Wilck
2019-10-12 21:28 ` [PATCH 23/72] libmultipath: scsi_ioctl_pathinfo(): remove unused argument Martin Wilck
2019-10-12 21:28 ` [PATCH 24/72] multipath-tools: mark unused params in signal and cleanup handlers Martin Wilck
2019-10-12 21:28 ` [PATCH 25/72] libmultipath: get_ana_info(): remove unused parameter Martin Wilck
2019-10-12 21:28 ` [PATCH 26/72] libmultipath: mark unused params in getprio() methods Martin Wilck
2019-10-12 21:28 ` [PATCH 27/72] libmultipath: hp_sw: remove usused argument in do_inq() Martin Wilck
2019-10-12 21:28 ` [PATCH 28/72] libmultipath: checkers: mark unused arguments in methods Martin Wilck
2019-10-12 21:28 ` [PATCH 29/72] multipathd: stop_waiter_thread(): removed unused parameter Martin Wilck
2019-10-12 21:28 ` [PATCH 30/72] multipath tools: mark unused arguments in thread routines Martin Wilck
2019-10-12 21:28 ` [PATCH 31/72] kpartx: gpt: remove unused arg in read_lastoddsector() Martin Wilck
2019-10-12 21:28 ` [PATCH 32/72] kpartx: mark unused arguments in ptreader methods Martin Wilck
2019-10-12 21:28 ` [PATCH 33/72] libmultipath: mark missing arguments in functions matching prototypes Martin Wilck
2019-10-12 21:28 ` [PATCH 34/72] libmultipath: get_pgpolicy_name(): use "len" parameter Martin Wilck
2019-10-12 21:28 ` [PATCH 35/72] libmultipath: snprint_multipath_map_json(): remove unused argument Martin Wilck
2019-10-12 21:28 ` [PATCH 36/72] multipath: delegate_to_multipathd: mark unused arguments Martin Wilck
2019-10-12 21:28 ` [PATCH 37/72] libmultipath: use -Wno-unused-parameter for dict.c Martin Wilck
2019-10-12 21:28 ` [PATCH 38/72] multipathd: use -Wno-unused-parameter for cli_handlers.c Martin Wilck
2019-10-12 21:28 ` [PATCH 39/72] libmpathpersist: remove unused "noisy" argument in various functions Martin Wilck
2019-10-12 21:28 ` [PATCH 40/72] libmpathpersist: fix copy-paste error in mpath_format_readresv() Martin Wilck
2019-10-12 21:28 ` [PATCH 41/72] multipath-tools tests: add -Wno-unused-parameter Martin Wilck
2019-10-12 23:01   ` Bart Van Assche
2019-10-12 21:28 ` [PATCH 42/72] multipath-tools: Makefile.inc: remove -Wno-unused-parameter Martin Wilck
2019-10-12 21:28 ` [PATCH 43/72] libmultipath: dev_loss_tmo is unsigned Martin Wilck
2019-10-12 21:28 ` [PATCH 44/72] libmultipath: trivial changes for -Wsign-compare Martin Wilck
2019-10-12 21:28 ` [PATCH 45/72] libmultipath: fix -Wsign-compare warnings with snprintf() Martin Wilck
2019-10-12 22:59   ` Bart Van Assche
2019-10-22 16:07     ` Martin Wilck
2019-10-22 16:47       ` Bart Van Assche
2019-10-22 20:34         ` Martin Wilck
2019-10-22 21:32           ` Bart Van Assche
2019-10-23  9:11             ` Martin Wilck
2019-10-12 21:28 ` [PATCH 46/72] libmultipath: parse_vpd_pg83(): sanitize indentation Martin Wilck
2019-10-12 21:28 ` [PATCH 47/72] libmultipath: parse_vpd_pg83(): fix -Wsign-compare warnings Martin Wilck
2019-10-12 21:28 ` [PATCH 48/72] libmultipath: print: use unsigned int for "width" field Martin Wilck
2019-10-12 21:28 ` [PATCH 49/72] libmultipath: vector_for_each_slot: fix -Wsign-compare warnings Martin Wilck
2019-10-12 21:28 ` [PATCH 50/72] libmultipath: set_int(): add error check and set_uint() Martin Wilck
2019-10-12 21:28 ` [PATCH 51/72] libmultipath: make "checkint" unsigned Martin Wilck
2019-10-12 21:28 ` [PATCH 52/72] libmultipath: use unsigned blksize in directio context Martin Wilck
2019-10-12 21:28 ` [PATCH 53/72] libmultipath, kpartx: byteorder: always use unsigned types Martin Wilck
2019-10-12 21:28 ` [PATCH 54/72] libmpathcmd: fix -Wsign-compare warnings Martin Wilck
2019-10-12 21:28 ` [PATCH 55/72] libmpathpersist: " Martin Wilck
2019-10-12 21:28 ` [PATCH 56/72] kpartx: use unsigned arguments in dm_devn() and dm_prereq() Martin Wilck
2019-10-12 21:28 ` [PATCH 57/72] kpartx: use unsigned int for "ns" argument of ptreader Martin Wilck
2019-10-12 21:28 ` [PATCH 58/72] multipath-tools: Makefile.inc: enable -Wsign-compare Martin Wilck
2019-10-12 21:28 ` [PATCH 59/72] libdmmp: fix clang -Wformat-nonliteral warnings Martin Wilck
2019-10-12 21:28 ` [PATCH 60/72] libmultipath: fix clang -Wformat-literal warnings Martin Wilck
2019-10-12 21:28 ` [PATCH 61/72] multipath tests: blacklist: remove always-true condition Martin Wilck
2019-10-12 21:28 ` [PATCH 62/72] multipath tests: hwtable: fix strncat() invocation Martin Wilck
2019-10-12 21:28 ` [PATCH 63/72] multipath tests: fix -Wformat-literal warning Martin Wilck
2019-10-12 21:28 ` [PATCH 64/72] multipath tests: util: fix clang strlcpy warnings Martin Wilck
2019-10-12 21:28 ` [PATCH 65/72] libmultipath: nvme: update to nvme-cli v1.9 Martin Wilck
2019-10-12 21:28 ` [PATCH 66/72] multipath-tools: Makefile.inc: fix TEST_CC_OPTION Martin Wilck
2019-10-12 21:29 ` [PATCH 67/72] multipath-tools: Makefile.inc: use -Wp, ... for compiling only Martin Wilck
2019-10-30 14:55   ` Benjamin Marzinski
2019-10-12 21:29 ` [PATCH 68/72] multipath-tools: Makefile: use proper directory recursion Martin Wilck
2019-10-12 21:29 ` [PATCH 69/72] multipath tests: Makefile: fix "clean" target Martin Wilck
2019-10-12 21:29 ` [PATCH 70/72] multipath tests: Makefile: avoid gcc 4.8 missing initializers failure Martin Wilck
2019-10-12 21:29 ` [PATCH 71/72] multipath-tools: Makefile.inc: enable -Werror Martin Wilck
2019-10-12 21:29 ` [PATCH 72/72] multipath-tools: Makefile.inc: test for -Wno-clobbered support Martin Wilck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.