[PATCH 00/35] multipath-tools series part I: minor changes

[PATCH 00/35] multipath-tools series part I: minor changes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

This is part I of a larger patch series for multpath-tools I've been preparing.
It contains self-contained fixes and cleanups, and unit test additions.

The full series will also be available here:
https://github.com/mwilck/multipath-tools/tree/ups/submit-2007

There are tags in that repo for each part of the series.
This part is tagged "submit-200709-1".

It's based on 0.8.4, plus the following set of previously
submitted and reviewed patches:

 - libmultipath: add device to hwtable.c (Steve Schremmer)
 - [PATCH v3 0/7] Fix muitpath/multipathd flush issue (v3 7-part series, Ben)
 - [PATCH v2 0/4] misc patches (v2 4-part series, Ben)
 - multipath: Fix compiler warnings when built without systemd. (Marius Bakke)
 - [PATCH v2 0/3] multipath: change default devnode blacklist
   (v2 3-part series, Ben)
 - multipath: add "-e" option to enable foreign libraries (me)
 - libmultipath: set "enable_foreign" to NONE by default (me)
 - libmultipath: fix condlog NULL argument in uevent_get_env_var (Ben)
 - fix boolean value with json-c 0.14 (Christian Hesse) 
 - [PATCH v3 0/6] multipath: path validation library prep work
   (v3 6-part series, me)
 - [PATCH 0/2] More minor libmultipath fixes (2-part series, me)
 - [PATCH 00/11] Minor fixes for multipath-tools (11-part series, me)
 - libmpathpersist: depend on libmultipath (Christian Hesse)
 - [PATCH v2 0/2] multipath-tools: fixes for kpartx.rules and skip_kpartx
   (v2 2-part series, me)
 - libmultipath: allow force reload with no active paths (Ben)
 - libmutipath: don't close fd on dm_lib_release (Ben)
 - libmultipath: assign variable to make gcc happy (Ben)
 - [PATCH v2 0/4] libmpathpersist allocation size fixes (v2 4-part series, me)

You can find a full tree of the code this is based on here:
https://github.com/openSUSE/multipath-tools/tree/upstream-queue

Regards, Martin


Martin Wilck (35):
  multipath-tools tests/util: separate group for bitmask tests
  multipath-tools tests/directio: fix missing initializers
  tests: __wrap_dlog: use check_expected()
  multipath tools tests: add strchop() test
  libmultipath: improve strchop()
  multipath-tools tests: add test for devt2devname
  libmultipath: devt2devname(): simplify using libudev
  libmultipath: create bitfield abstraction
  libmultipath: use bitfields in group_by_match()
  libmultipath: util: constify function arguments
  multipath-tools tests: add unit tests for strlcat
  libmultipath: strlcpy()/strlcat(): use restrict qualifier
  libmultipath: constify blacklist code
  libmultipath: rlookup_binding(): remove newline in log message
  libmultipath: fix missing initializer warning from clang 3.9
  libmultipath: fix gcc -Wstringop-overflow warning
  libmultipath: remove uevent listener failback
  libmultipath: uevent: use static linkage where possible
  libmultipath: uevent: inline trivial functions
  libmultipath: decrease log level of "SCSI target" log message
  libmultipath: get_udev_uid(): more appropriate error code
  libmultipath: get_uid(): improve log message on udev failure
  libmultipath: make sysfs_pathinfo() static and use const
  libmultipath: pathinfo(): improve a log message
  libmultipath: pathinfo(): don't filter emtpy devnode names
  libmultipath: io_err_stat_handle_pathfail(): less error conditions
  libmultipath: improve libdm logging
  libmultipath: snprint_devices(): use udev_enumerate
  libmultipath: snprint_devices(): print hidden/foreign status
  libmultipath: alloc_path(): initialize pp->initialized
  libmultipath: alloc_path_with_pathinfo(): treat devname overflow as
    error
  libmultipath: log table params in addmap()
  multipathd: remove set_multipath_wwid()
  kpartx: print an error message if removing a partition fails
  kpartx: add missing newline

 kpartx/devmapper.c               |   2 +-
 kpartx/kpartx.c                  |   2 +
 libmultipath/alias.c             |   2 +-
 libmultipath/blacklist.c         |  34 +-
 libmultipath/blacklist.h         |  17 +-
 libmultipath/checkers/directio.c |   2 +-
 libmultipath/configure.c         |  11 +-
 libmultipath/defaults.h          |   2 +
 libmultipath/devmapper.c         |  27 +-
 libmultipath/discovery.c         |  30 +-
 libmultipath/dmparser.c          |   2 +-
 libmultipath/io_err_stat.c       |  25 +-
 libmultipath/parser.c            |   2 +-
 libmultipath/pgpolicies.c        |  12 +-
 libmultipath/print.c             |  90 ++---
 libmultipath/print.h             |   2 +-
 libmultipath/propsel.c           |   6 +
 libmultipath/structs.c           |   1 +
 libmultipath/uevent.c            | 324 ++---------------
 libmultipath/uevent.h            |  47 ++-
 libmultipath/util.c              | 168 ++++-----
 libmultipath/util.h              |  73 +++-
 multipathd/main.c                |  14 +-
 tests/Makefile                   |   3 +-
 tests/alias.c                    |   4 +-
 tests/devt.c                     | 192 ++++++++++
 tests/directio.c                 |  28 +-
 tests/test-log.c                 |   5 +-
 tests/uevent.c                   |   4 -
 tests/util.c                     | 586 ++++++++++++++++++++++++++++---
 30 files changed, 1081 insertions(+), 636 deletions(-)
 create mode 100644 tests/devt.c

-- 
2.26.2

[PATCH 01/35] multipath-tools tests/util: separate group for bitmask tests

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Separate these more cleanly.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/util.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/tests/util.c b/tests/util.c
index 7c486fc..48de384 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -141,6 +141,27 @@ static void test_basenamecpy_bad5(void **state)
         assert_int_equal(basenamecpy("baz/qux", NULL, sizeof(dst)), 0);
 }
 
+static int test_basenamecpy(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_basenamecpy_good0),
+		cmocka_unit_test(test_basenamecpy_good1),
+		cmocka_unit_test(test_basenamecpy_good2),
+		cmocka_unit_test(test_basenamecpy_good3),
+		cmocka_unit_test(test_basenamecpy_good4),
+		cmocka_unit_test(test_basenamecpy_good5),
+		cmocka_unit_test(test_basenamecpy_good6),
+		cmocka_unit_test(test_basenamecpy_good7),
+		cmocka_unit_test(test_basenamecpy_bad0),
+		cmocka_unit_test(test_basenamecpy_bad1),
+		cmocka_unit_test(test_basenamecpy_bad2),
+		cmocka_unit_test(test_basenamecpy_bad3),
+		cmocka_unit_test(test_basenamecpy_bad4),
+		cmocka_unit_test(test_basenamecpy_bad5),
+	};
+	return cmocka_run_group_tests(tests, NULL, NULL);
+}
+
 static void test_bitmask_1(void **state)
 {
 	uint64_t arr[BITARR_SZ];
@@ -235,23 +256,9 @@ static void test_bitmask_2(void **state)
 	}
 }
 
-int test_basenamecpy(void)
+static int test_bitmasks(void)
 {
 	const struct CMUnitTest tests[] = {
-		cmocka_unit_test(test_basenamecpy_good0),
-		cmocka_unit_test(test_basenamecpy_good1),
-		cmocka_unit_test(test_basenamecpy_good2),
-		cmocka_unit_test(test_basenamecpy_good3),
-		cmocka_unit_test(test_basenamecpy_good4),
-		cmocka_unit_test(test_basenamecpy_good5),
-		cmocka_unit_test(test_basenamecpy_good6),
-		cmocka_unit_test(test_basenamecpy_good7),
-		cmocka_unit_test(test_basenamecpy_bad0),
-		cmocka_unit_test(test_basenamecpy_bad1),
-		cmocka_unit_test(test_basenamecpy_bad2),
-		cmocka_unit_test(test_basenamecpy_bad3),
-		cmocka_unit_test(test_basenamecpy_bad4),
-		cmocka_unit_test(test_basenamecpy_bad5),
 		cmocka_unit_test(test_bitmask_1),
 		cmocka_unit_test(test_bitmask_2),
 	};
@@ -406,6 +413,7 @@ int main(void)
 	int ret = 0;
 
 	ret += test_basenamecpy();
+	ret += test_bitmasks();
 	ret += test_strlcpy();
 	return ret;
 }
-- 
2.26.2

[PATCH 02/35] multipath-tools tests/directio: fix missing initializers

[mwilck]

From: Martin Wilck <mwilck@suse.com>

clang-3.9 doesn't like the {0} initializers for complex data structures.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/directio.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tests/directio.c b/tests/directio.c
index 66aaf0e..9895409 100644
--- a/tests/directio.c
+++ b/tests/directio.c
@@ -31,7 +31,7 @@ int test_fd = 111;
 int ioctx_count = 0;
 struct io_event mock_events[AIO_GROUP_SIZE]; /* same as the checker max */
 int ev_off = 0;
-struct timespec zero_timeout = {0};
+struct timespec zero_timeout = { .tv_sec = 0 };
 struct timespec full_timeout = { .tv_sec = -1 };
 
 int __real_ioctl(int fd, unsigned long request, void *argp);
@@ -287,7 +287,7 @@ static void test_reset(void **state)
 /* tests initializing, then resetting, and then initializing again */
 static void test_init_reset_init(void **state)
 {
-	struct checker c = {0};
+	struct checker c = {.cls = NULL};
 	struct aio_group *aio_grp, *tmp_grp;
 
 	assert_true(list_empty(&aio_grp_list));
@@ -315,7 +315,7 @@ static void test_init_reset_init(void **state)
 static void test_init_free(void **state)
 {
 	int i, count = 0;
-	struct checker c[4096] = {0};
+	struct checker c[4096] = {{.cls = NULL}};
 	struct aio_group *aio_grp = NULL;
 
 	assert_true(list_empty(&aio_grp_list));
@@ -361,7 +361,7 @@ static void test_init_free(void **state)
 static void test_multi_init_free(void **state)
 {
 	int i, count;
-	struct checker c[4096] = {0};
+	struct checker c[4096] = {{.cls = NULL}};
 	struct aio_group *aio_grp;
 
 	assert_true(list_empty(&aio_grp_list));
@@ -401,7 +401,7 @@ static void test_multi_init_free(void **state)
 /* simple single checker sync test */
 static void test_check_state_simple(void **state)
 {
-	struct checker c = {0};
+	struct checker c = {.cls = NULL};
 	struct async_req *req;
 	int res = 0;
 
@@ -417,7 +417,7 @@ static void test_check_state_simple(void **state)
 /* test sync timeout */
 static void test_check_state_timeout(void **state)
 {
-	struct checker c = {0};
+	struct checker c = {.cls = NULL};
 	struct aio_group *aio_grp;
 
 	assert_true(list_empty(&aio_grp_list));
@@ -440,7 +440,7 @@ static void test_check_state_timeout(void **state)
 /* test async timeout */
 static void test_check_state_async_timeout(void **state)
 {
-	struct checker c = {0};
+	struct checker c = {.cls = NULL};
 	struct aio_group *aio_grp;
 
 	assert_true(list_empty(&aio_grp_list));
@@ -467,7 +467,7 @@ static void test_check_state_async_timeout(void **state)
 /* test freeing checkers with outstanding requests */
 static void test_free_with_pending(void **state)
 {
-        struct checker c[2] = {0};
+        struct checker c[2] = {{.cls = NULL}};
         struct aio_group *aio_grp;
 	struct async_req *req;
 	int res = 0;
@@ -500,7 +500,7 @@ static void test_free_with_pending(void **state)
 /* test removing orpahed aio_group on free */
 static void test_orphaned_aio_group(void **state)
 {
-	struct checker c[AIO_GROUP_SIZE] = {0};
+	struct checker c[AIO_GROUP_SIZE] = {{.cls = NULL}};
 	struct aio_group *aio_grp, *tmp_grp;
 	int i;
 
@@ -533,7 +533,7 @@ static void test_orphaned_aio_group(void **state)
  * checker */
 static void test_timeout_cancel_failed(void **state)
 {
-	struct checker c[2] = {0};
+	struct checker c[2] = {{.cls = NULL}};
 	struct aio_group *aio_grp;
 	struct async_req *reqs[2];
 	int res[] = {0,0};
@@ -568,7 +568,7 @@ static void test_timeout_cancel_failed(void **state)
  * checker */
 static void test_async_timeout_cancel_failed(void **state)
 {
-	struct checker c[2] = {0};
+	struct checker c[2] = {{.cls = NULL}};
 	struct async_req *reqs[2];
 	int res[] = {0,0};
 	int i;
@@ -610,7 +610,7 @@ static void test_async_timeout_cancel_failed(void **state)
 /* test orphaning a request, and having another checker clean it up */
 static void test_orphan_checker_cleanup(void **state)
 {
-	struct checker c[2] = {0};
+	struct checker c[2] = {{.cls = NULL}};
 	struct async_req *reqs[2];
 	int res[] = {0,0};
 	struct aio_group *aio_grp;
@@ -667,7 +667,7 @@ static void test_orphan_reset_cleanup(void **state)
 static void test_check_state_blksize(void **state)
 {
 	int i;
-	struct checker c[3] = {0};
+	struct checker c[3] = {{.cls = NULL}};
 	int blksize[] = {4096, 1024, 512};
 	struct async_req *reqs[3];
 	int res[] = {0,1,0};
@@ -698,7 +698,7 @@ static void test_check_state_blksize(void **state)
 static void test_check_state_async(void **state)
 {
 	int i;
-	struct checker c[257] = {0};
+	struct checker c[257] = {{.cls = NULL}};
 	struct async_req *reqs[257];
 	int res[257] = {0};
 
-- 
2.26.2

[PATCH 03/35] tests: __wrap_dlog: use check_expected()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

For function parameters, cmocka's check_expected() API should be used
rather than will_return() / mock_type().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/test-log.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/test-log.c b/tests/test-log.c
index d685d58..051491e 100644
--- a/tests/test-log.c
+++ b/tests/test-log.c
@@ -12,7 +12,7 @@ void __wrap_dlog (int sink, int prio, const char * fmt, ...)
 	char buff[MAX_MSG_SIZE];
 	va_list ap;
 
-	assert_int_equal(prio, mock_type(int));
+	check_expected(prio);
 	va_start(ap, fmt);
 	vsnprintf(buff, MAX_MSG_SIZE, fmt, ap);
 	va_end(ap);
@@ -21,7 +21,6 @@ void __wrap_dlog (int sink, int prio, const char * fmt, ...)
 
 void expect_condlog(int prio, char *string)
 {
-	will_return(__wrap_dlog, prio);
+	expect_value(__wrap_dlog, prio, prio);
 	will_return(__wrap_dlog, string);
 }
-
-- 
2.26.2

[PATCH 04/35] multipath tools tests: add strchop() test

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/tests/util.c b/tests/util.c
index 48de384..6d12fda 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -408,6 +408,68 @@ static int test_strlcpy(void)
 	return cmocka_run_group_tests(tests, NULL, NULL);
 }
 
+static void test_strchop_nochop(void **state)
+{
+	char hello[] = "hello";
+
+	assert_int_equal(strchop(hello), 5);
+	assert_string_equal(hello, "hello");
+}
+
+static void test_strchop_newline(void **state)
+{
+	char hello[] = "hello\n";
+
+	assert_int_equal(strchop(hello), 5);
+	assert_string_equal(hello, "hello");
+}
+
+static void test_strchop_space(void **state)
+{
+	char hello[] = " ello      ";
+
+	assert_int_equal(strchop(hello), 5);
+	assert_string_equal(hello, " ello");
+}
+
+static void test_strchop_mix(void **state)
+{
+	char hello[] = " el\no \t  \n\n \t    \n";
+
+	assert_int_equal(strchop(hello), 5);
+	assert_string_equal(hello, " el\no");
+}
+
+static void test_strchop_blank(void **state)
+{
+	char hello[] = "  \t  \n\n \t    \n";
+
+	assert_int_equal(strchop(hello), 0);
+	assert_string_equal(hello, "");
+}
+
+static void test_strchop_empty(void **state)
+{
+	char hello[] = "";
+
+	assert_int_equal(strchop(hello), 0);
+	assert_string_equal(hello, "");
+}
+
+static int test_strchop(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_strchop_nochop),
+		cmocka_unit_test(test_strchop_newline),
+		cmocka_unit_test(test_strchop_space),
+		cmocka_unit_test(test_strchop_mix),
+		cmocka_unit_test(test_strchop_blank),
+		cmocka_unit_test(test_strchop_empty),
+	};
+
+	return cmocka_run_group_tests(tests, NULL, NULL);
+}
+
 int main(void)
 {
 	int ret = 0;
@@ -415,5 +477,6 @@ int main(void)
 	ret += test_basenamecpy();
 	ret += test_bitmasks();
 	ret += test_strlcpy();
+	ret += test_strchop();
 	return ret;
 }
-- 
2.26.2

[PATCH 05/35] libmultipath: improve strchop()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

strchop() returns size_t, so have it work with size_t, too. Also,
avoid the unnecessary second call to strlen().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/util.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libmultipath/util.c b/libmultipath/util.c
index 51c38c8..28d0168 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -26,11 +26,11 @@
 size_t
 strchop(char *str)
 {
-	int i;
+	size_t i;
 
-	for (i=strlen(str)-1; i >=0 && isspace(str[i]); --i) ;
+	for (i = strlen(str) - 1; i != (size_t) -1 && isspace(str[i]); i--) ;
 	str[++i] = '\0';
-	return strlen(str);
+	return i;
 }
 
 int
-- 
2.26.2

[PATCH 06/35] multipath-tools tests: add test for devt2devname

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Two tests skipped because they fail, will be fixed with the following
patch.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/Makefile |   3 +-
 tests/devt.c   | 194 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+), 1 deletion(-)
 create mode 100644 tests/devt.c

diff --git a/tests/Makefile b/tests/Makefile
index 125553b..5f00a3a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -13,7 +13,7 @@ CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir) \
 LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd -lcmocka
 
 TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \
-	 alias directio valid
+	 alias directio valid devt
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
@@ -52,6 +52,7 @@ alias-test_TESTDEPS := test-log.o
 alias-test_LIBDEPS := -lpthread -ldl
 valid-test_OBJDEPS := ../libmultipath/valid.o
 valid-test_LIBDEPS := -ludev -lpthread -ldl
+devt-test_LIBDEPS := -ludev
 ifneq ($(DIO_TEST_DEV),)
 directio-test_LIBDEPS := -laio
 endif
diff --git a/tests/devt.c b/tests/devt.c
new file mode 100644
index 0000000..4be6d75
--- /dev/null
+++ b/tests/devt.c
@@ -0,0 +1,194 @@
+/*
+ * Copyright (c) 2020 Martin Wilck, SUSE
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <cmocka.h>
+#include <libudev.h>
+#include <sys/sysmacros.h>
+#include "util.h"
+#include "debug.h"
+
+#include "globals.c"
+
+static int get_one_devt(char *devt, size_t len)
+{
+	struct udev_enumerate *enm;
+	int r, ret = -1;
+	struct udev_list_entry *first;
+	struct udev_device *u_dev;
+	const char *path;
+	dev_t devnum;
+
+	enm = udev_enumerate_new(udev);
+	if (!enm)
+		return -1;
+	r = udev_enumerate_add_match_subsystem(enm, "block");
+	r = udev_enumerate_scan_devices(enm);
+	if (r < 0)
+		goto out;
+	first = udev_enumerate_get_list_entry(enm);
+	if (!first)
+		goto out;
+	path = udev_list_entry_get_name(first);
+	u_dev = udev_device_new_from_syspath(udev, path);
+	if (!u_dev)
+		goto out;
+	devnum = udev_device_get_devnum(u_dev);
+	snprintf(devt, len, "%d:%d",
+		 major(devnum), minor(devnum));
+	udev_device_unref(u_dev);
+	condlog(3, "found block device: %s", devt);
+	ret = 0;
+out:
+	udev_enumerate_unref(enm);
+	return ret;
+}
+
+int setup(void **state)
+{
+	static char dev_t[BLK_DEV_SIZE];
+
+	udev = udev_new();
+	if (udev == NULL)
+		return -1;
+	*state = dev_t;
+	return get_one_devt(dev_t, sizeof(dev_t));
+}
+
+int teardown(void **state)
+{
+	udev_unref(udev);
+	return 0;
+}
+
+static void test_devt2devname_devt_good(void **state)
+{
+	char dummy[BLK_DEV_SIZE];
+
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), *state), 0);
+}
+
+static void test_devt2devname_devname_null(void **state)
+{
+	assert_int_equal(devt2devname(NULL, 0, ""), 1);
+}
+
+/* buffer length 0 */
+static void test_devt2devname_length_0(void **state)
+{
+	char dummy[] = "";
+
+	assert_int_equal(devt2devname(dummy, 0, ""), 1);
+}
+
+/* buffer too small */
+static void test_devt2devname_length_1(void **state)
+{
+	char dummy[] = "";
+
+	skip();
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), *state), 1);
+}
+
+static void test_devt2devname_devt_null(void **state)
+{
+	char dummy[32];
+
+	skip();
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), NULL), 1);
+}
+
+static void test_devt2devname_devt_empty(void **state)
+{
+	char dummy[32];
+
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), ""), 1);
+}
+
+static void test_devt2devname_devt_invalid_1(void **state)
+{
+	char dummy[32];
+
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), "foo"), 1);
+}
+
+static void test_devt2devname_devt_invalid_2(void **state)
+{
+	char dummy[32];
+
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), "1234"), 1);
+}
+
+static void test_devt2devname_devt_invalid_3(void **state)
+{
+	char dummy[32];
+
+	assert_int_equal(devt2devname(dummy, sizeof(dummy), "0:0"), 1);
+}
+
+static void test_devt2devname_real(void **state)
+{
+	struct udev_enumerate *enm;
+	int r;
+	struct udev_list_entry *first, *item;
+	unsigned int i = 0;
+
+	enm = udev_enumerate_new(udev);
+	assert_non_null(enm);
+	r = udev_enumerate_add_match_subsystem(enm, "block");
+	assert_in_range(r, 0, INT_MAX);
+	r = udev_enumerate_scan_devices(enm);
+	first = udev_enumerate_get_list_entry(enm);
+	udev_list_entry_foreach(item, first) {
+		const char *path = udev_list_entry_get_name(item);
+		struct udev_device *u_dev;
+		dev_t devnum;
+		char devt[BLK_DEV_SIZE];
+		char devname[FILE_NAME_SIZE];
+
+		u_dev = udev_device_new_from_syspath(udev, path);
+		assert_non_null(u_dev);
+		devnum = udev_device_get_devnum(u_dev);
+		snprintf(devt, sizeof(devt), "%d:%d",
+			 major(devnum), minor(devnum));
+		r = devt2devname(devname, sizeof(devname), devt);
+		assert_int_equal(r, 0);
+		assert_string_equal(devname, udev_device_get_sysname(u_dev));
+		i++;
+		udev_device_unref(u_dev);
+	}
+	udev_enumerate_unref(enm);
+	condlog(2, "devt2devname test passed for %u block devices", i);
+}
+
+static int devt2devname_tests(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_devt2devname_devt_good),
+		cmocka_unit_test(test_devt2devname_devname_null),
+		cmocka_unit_test(test_devt2devname_length_0),
+		cmocka_unit_test(test_devt2devname_length_1),
+		cmocka_unit_test(test_devt2devname_devt_null),
+		cmocka_unit_test(test_devt2devname_devt_empty),
+		cmocka_unit_test(test_devt2devname_devt_invalid_1),
+		cmocka_unit_test(test_devt2devname_devt_invalid_2),
+		cmocka_unit_test(test_devt2devname_devt_invalid_3),
+		cmocka_unit_test(test_devt2devname_real),
+	};
+
+	return cmocka_run_group_tests(tests, setup, teardown);
+}
+
+int main(void)
+{
+	int ret = 0;
+
+	ret += devt2devname_tests();
+	return ret;
+}
-- 
2.26.2

[PATCH 07/35] libmultipath: devt2devname(): simplify using libudev

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Replace the hand-written code by a simple libudev call. The two
previously skipped tests can now be enabled again.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/util.c | 87 ++++++---------------------------------------
 libmultipath/util.h |  2 +-
 tests/devt.c        |  2 --
 3 files changed, 12 insertions(+), 79 deletions(-)

diff --git a/libmultipath/util.c b/libmultipath/util.c
index 28d0168..3c43f28 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -21,6 +21,7 @@
 #include "checkers.h"
 #include "vector.h"
 #include "structs.h"
+#include "config.h"
 #include "log.h"
 
 size_t
@@ -155,89 +156,23 @@ size_t strlcat(char *dst, const char *src, size_t size)
 	return bytes;
 }
 
-int devt2devname(char *devname, int devname_len, char *devt)
+int devt2devname(char *devname, int devname_len, const char *devt)
 {
-	FILE *fd;
-	unsigned int tmpmaj, tmpmin, major, minor;
-	char dev[FILE_NAME_SIZE];
-	char block_path[PATH_SIZE];
-	struct stat statbuf;
-
-	memset(block_path, 0, sizeof(block_path));
-	memset(dev, 0, sizeof(dev));
-	if (sscanf(devt, "%u:%u", &major, &minor) != 2) {
-		condlog(0, "Invalid device number %s", devt);
-		return 1;
-	}
+	struct udev_device *u_dev;
+	int r;
 
-	if (devname_len > FILE_NAME_SIZE)
-		devname_len = FILE_NAME_SIZE;
-
-	if (stat("/sys/dev/block", &statbuf) == 0) {
-		/* Newer kernels have /sys/dev/block */
-		sprintf(block_path,"/sys/dev/block/%u:%u", major, minor);
-		dev[FILE_NAME_SIZE - 1] = '\0';
-		if (lstat(block_path, &statbuf) == 0) {
-			if (S_ISLNK(statbuf.st_mode) &&
-			    readlink(block_path, dev, FILE_NAME_SIZE-1) > 0) {
-				char *p = strrchr(dev, '/');
-
-				if (!p) {
-					condlog(0, "No sysfs entry for %s",
-						block_path);
-					return 1;
-				}
-				p++;
-				strlcpy(devname, p, devname_len);
-				return 0;
-			}
-		}
-		condlog(4, "%s is invalid", block_path);
+	if (!devname || !devname_len || !devt)
 		return 1;
-	}
-	memset(block_path, 0, sizeof(block_path));
 
-	if (!(fd = fopen("/proc/partitions", "r"))) {
-		condlog(0, "Cannot open /proc/partitions");
+	u_dev = udev_device_new_from_devnum(udev, 'b', parse_devt(devt));
+	if (!u_dev) {
+		condlog(0, "\"%s\": invalid major/minor numbers, not found in sysfs", devt);
 		return 1;
 	}
+	r = strlcpy(devname, udev_device_get_sysname(u_dev), devname_len);
+	udev_device_unref(u_dev);
 
-	while (!feof(fd)) {
-		int r = fscanf(fd,"%u %u %*d %s",&tmpmaj, &tmpmin, dev);
-		if (!r) {
-			r = fscanf(fd,"%*s\n");
-			continue;
-		}
-		if (r != 3)
-			continue;
-
-		if ((major == tmpmaj) && (minor == tmpmin)) {
-			if (safe_sprintf(block_path, "/sys/block/%s", dev)) {
-				condlog(0, "device name %s is too long", dev);
-				fclose(fd);
-				return 1;
-			}
-			break;
-		}
-	}
-	fclose(fd);
-
-	if (strncmp(block_path,"/sys/block", 10)) {
-		condlog(3, "No device found for %u:%u", major, minor);
-		return 1;
-	}
-
-	if (stat(block_path, &statbuf) < 0) {
-		condlog(0, "No sysfs entry for %s", block_path);
-		return 1;
-	}
-
-	if (S_ISDIR(statbuf.st_mode) == 0) {
-		condlog(0, "sysfs entry %s is not a directory", block_path);
-		return 1;
-	}
-	basenamecpy((const char *)block_path, devname, devname_len);
-	return 0;
+	return !(r < devname_len);
 }
 
 /* This function returns a pointer inside of the supplied pathname string.
diff --git a/libmultipath/util.h b/libmultipath/util.h
index 56bd78c..df75c4f 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -14,7 +14,7 @@ char *get_next_string(char **temp, char *split_char);
 int get_word (char * sentence, char ** word);
 size_t strlcpy(char *dst, const char *src, size_t size);
 size_t strlcat(char *dst, const char *src, size_t size);
-int devt2devname (char *, int, char *);
+int devt2devname (char *, int, const char *);
 dev_t parse_devt(const char *dev_t);
 char *convert_dev(char *dev, int is_path_device);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
diff --git a/tests/devt.c b/tests/devt.c
index 4be6d75..fd4d74a 100644
--- a/tests/devt.c
+++ b/tests/devt.c
@@ -92,7 +92,6 @@ static void test_devt2devname_length_1(void **state)
 {
 	char dummy[] = "";
 
-	skip();
 	assert_int_equal(devt2devname(dummy, sizeof(dummy), *state), 1);
 }
 
@@ -100,7 +99,6 @@ static void test_devt2devname_devt_null(void **state)
 {
 	char dummy[32];
 
-	skip();
 	assert_int_equal(devt2devname(dummy, sizeof(dummy), NULL), 1);
 }
 
-- 
2.26.2

[PATCH 08/35] libmultipath: create bitfield abstraction

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In e32d521d ("libmultipath: coalesce_paths: fix size mismatch handling"),
we introduced simple bitmap handling functions. We can do better. This
patch introduces a bitfield type with overflow detection and a
find_first_set() method.

Use this in coalesce_paths(), and adapt the unit tests. Also, add
unit tests for "odd" bitfield sizes; so far we tested only multiples
of 64.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |   9 +-
 libmultipath/util.c      |  35 ++++++
 libmultipath/util.h      |  57 ++++++++-
 tests/util.c             | 263 +++++++++++++++++++++++++++++++++++----
 4 files changed, 327 insertions(+), 37 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 96c7961..fe590f4 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1092,7 +1092,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 	vector pathvec = vecs->pathvec;
 	struct config *conf;
 	int allow_queueing;
-	uint64_t *size_mismatch_seen;
+	struct bitfield *size_mismatch_seen;
 
 	/* ignore refwwid if it's empty */
 	if (refwwid && !strlen(refwwid))
@@ -1106,8 +1106,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 	if (VECTOR_SIZE(pathvec) == 0)
 		return CP_OK;
-	size_mismatch_seen = calloc((VECTOR_SIZE(pathvec) - 1) / 64 + 1,
-				    sizeof(uint64_t));
+	size_mismatch_seen = alloc_bitfield(VECTOR_SIZE(pathvec));
 	if (size_mismatch_seen == NULL)
 		return CP_FAIL;
 
@@ -1131,7 +1130,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 		}
 
 		/* 2. if path already coalesced, or seen and discarded */
-		if (pp1->mpp || is_bit_set_in_array(k, size_mismatch_seen))
+		if (pp1->mpp || is_bit_set_in_bitfield(k, size_mismatch_seen))
 			continue;
 
 		/* 3. if path has disappeared */
@@ -1183,7 +1182,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 					"Discard", pp2->dev, pp2->size,
 					mpp->size);
 				mpp->action = ACT_REJECT;
-				set_bit_in_array(i, size_mismatch_seen);
+				set_bit_in_bitfield(i, size_mismatch_seen);
 			}
 		}
 		verify_paths(mpp, vecs);
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 3c43f28..46cacd4 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -404,3 +404,38 @@ void close_fd(void *arg)
 {
 	close((long)arg);
 }
+
+struct bitfield *alloc_bitfield(unsigned int maxbit)
+{
+	unsigned int n;
+	struct bitfield *bf;
+
+	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
+	bf = calloc(1, sizeof(struct bitfield) + n * sizeof(bitfield_t));
+	bf->len = maxbit;
+	return bf;
+}
+
+void _log_bitfield_overflow(const char *f, unsigned int bit, unsigned int len)
+{
+	condlog(0, "%s: bitfield overflow: %u >= %u", f, bit, len);
+}
+
+unsigned int find_first_set(const struct bitfield *bf)
+{
+	unsigned int b = 0, i, n;
+
+	for (i = 0; i < bf->len; i+= bits_per_slot) {
+		b = _ffs(bf->bits[i / bits_per_slot]);
+		if (b != 0)
+			break;
+	};
+	if (b == 0)
+		return 0;
+
+	n = i + b;
+	if (n <= bf->len)
+		return n;
+
+	return 0;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index df75c4f..ec6de6d 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -1,6 +1,9 @@
 #ifndef _UTIL_H
 #define _UTIL_H
 
+#include <stdlib.h>
+#include <string.h>
+#include <limits.h>
 #include <sys/types.h>
 /* for rlim_t */
 #include <sys/resource.h>
@@ -51,19 +54,61 @@ struct scandir_result {
 };
 void free_scandir_result(struct scandir_result *);
 
-static inline bool is_bit_set_in_array(unsigned int bit, const uint64_t *arr)
+/*
+ * ffsll() is also available on glibc < 2.27 if _GNU_SOURCE is defined.
+ * But relying on that would require that every program using this header file
+ * set _GNU_SOURCE during compilation, because otherwise the library and the
+ * program would use different types for bitfield_t, causing errors.
+ * That's too error prone, so if in doubt, use ffs().
+ */
+#if __GLIBC_PREREQ(2, 27)
+typedef unsigned long long int bitfield_t;
+#define _ffs(x) ffsll(x)
+#else
+typedef unsigned int bitfield_t;
+#define _ffs(x) ffs(x)
+#endif
+#define bits_per_slot (sizeof(bitfield_t) * CHAR_BIT)
+
+struct bitfield {
+	unsigned int len;
+	bitfield_t bits[];
+};
+
+struct bitfield *alloc_bitfield(unsigned int maxbit);
+
+void _log_bitfield_overflow(const char *f, unsigned int bit, unsigned int len);
+#define log_bitfield_overflow(bit, len) \
+	_log_bitfield_overflow(__func__, bit, len)
+
+static inline bool is_bit_set_in_bitfield(unsigned int bit,
+				       const struct bitfield *bf)
 {
-	return arr[bit / 64] & (1ULL << (bit % 64)) ? 1 : 0;
+	if (bit >= bf->len) {
+		log_bitfield_overflow(bit, bf->len);
+		return false;
+	}
+	return !!(bf->bits[bit / bits_per_slot] &
+		  (1ULL << (bit % bits_per_slot)));
 }
 
-static inline void set_bit_in_array(unsigned int bit, uint64_t *arr)
+static inline void set_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
 {
-	arr[bit / 64] |= (1ULL << (bit % 64));
+	if (bit >= bf->len) {
+		log_bitfield_overflow(bit, bf->len);
+		return;
+	}
+	bf->bits[bit / bits_per_slot] |= (1ULL << (bit % bits_per_slot));
 }
 
-static inline void clear_bit_in_array(unsigned int bit, uint64_t *arr)
+static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
 {
-	arr[bit / 64] &= ~(1ULL << (bit % 64));
+	if (bit >= bf->len) {
+		log_bitfield_overflow(bit, bf->len);
+		return;
+	}
+	bf->bits[bit / bits_per_slot] &= ~(1ULL << (bit % bits_per_slot));
 }
 
+unsigned int find_first_set(const struct bitfield *bf);
 #endif /* _UTIL_H */
diff --git a/tests/util.c b/tests/util.c
index 6d12fda..db7c05f 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -164,19 +164,25 @@ static int test_basenamecpy(void)
 
 static void test_bitmask_1(void **state)
 {
-	uint64_t arr[BITARR_SZ];
+	struct bitfield *bf;
+	uint64_t *arr;
 	int i, j, k, m, b;
 
-	memset(arr, 0, sizeof(arr));
+	bf = alloc_bitfield(BITARR_SZ * 64);
+	assert_non_null(bf);
+	assert_int_equal(bf->len, BITARR_SZ * 64);
+	arr = (uint64_t *)bf->bits;
 
 	for (j = 0; j < BITARR_SZ; j++) {
 		for (i = 0; i < 64; i++) {
 			b = 64 * j + i;
-			assert(!is_bit_set_in_array(b, arr));
-			set_bit_in_array(b, arr);
+			assert(!is_bit_set_in_bitfield(b, bf));
+			set_bit_in_bitfield(b, bf);
 			for (k = 0; k < BITARR_SZ; k++) {
+#if 0
 				printf("b = %d j = %d k = %d a = %"PRIx64"\n",
 				       b, j, k, arr[k]);
+#endif
 				if (k == j)
 					assert_int_equal(arr[j], 1ULL << i);
 				else
@@ -184,39 +190,46 @@ static void test_bitmask_1(void **state)
 			}
 			for (m = 0; m < 64; m++)
 				if (i == m)
-					assert(is_bit_set_in_array(64 * j + m,
-								   arr));
+					assert(is_bit_set_in_bitfield(64 * j + m,
+								      bf));
 				else
-					assert(!is_bit_set_in_array(64 * j + m,
-								    arr));
-			clear_bit_in_array(b, arr);
-			assert(!is_bit_set_in_array(b, arr));
+					assert(!is_bit_set_in_bitfield(64 * j + m,
+								       bf));
+			assert_int_equal(find_first_set(bf), b + 1);
+			clear_bit_in_bitfield(b, bf);
+			assert(!is_bit_set_in_bitfield(b, bf));
 			for (k = 0; k < BITARR_SZ; k++)
 				assert_int_equal(arr[k], 0ULL);
 		}
 	}
+	free(bf);
 }
 
 static void test_bitmask_2(void **state)
 {
-	uint64_t arr[BITARR_SZ];
+	struct bitfield *bf;
+	uint64_t *arr;
 	int i, j, k, m, b;
 
-	memset(arr, 0, sizeof(arr));
+	bf = alloc_bitfield(BITARR_SZ * 64);
+	assert_non_null(bf);
+	assert_int_equal(bf->len, BITARR_SZ * 64);
+	arr = (uint64_t *)bf->bits;
 
 	for (j = 0; j < BITARR_SZ; j++) {
 		for (i = 0; i < 64; i++) {
 			b = 64 * j + i;
-			assert(!is_bit_set_in_array(b, arr));
-			set_bit_in_array(b, arr);
+			assert(!is_bit_set_in_bitfield(b, bf));
+			set_bit_in_bitfield(b, bf);
 			for (m = 0; m < 64; m++)
 				if (m <= i)
-					assert(is_bit_set_in_array(64 * j + m,
-								   arr));
+					assert(is_bit_set_in_bitfield(64 * j + m,
+								      bf));
 				else
-					assert(!is_bit_set_in_array(64 * j + m,
-								    arr));
-			assert(is_bit_set_in_array(b, arr));
+					assert(!is_bit_set_in_bitfield(64 * j + m,
+								       bf));
+			assert(is_bit_set_in_bitfield(b, bf));
+			assert_int_equal(find_first_set(bf), 1);
 			for (k = 0; k < BITARR_SZ; k++) {
 				if (k < j || (k == j && i == 63))
 					assert_int_equal(arr[k], ~0ULL);
@@ -232,16 +245,20 @@ static void test_bitmask_2(void **state)
 	for (j = 0; j < BITARR_SZ; j++) {
 		for (i = 0; i < 64; i++) {
 			b = 64 * j + i;
-			assert(is_bit_set_in_array(b, arr));
-			clear_bit_in_array(b, arr);
+			assert(is_bit_set_in_bitfield(b, bf));
+			clear_bit_in_bitfield(b, bf);
 			for (m = 0; m < 64; m++)
 				if (m <= i)
-					assert(!is_bit_set_in_array(64 * j + m,
-								    arr));
+					assert(!is_bit_set_in_bitfield(64 * j + m,
+								       bf));
 				else
-					assert(is_bit_set_in_array(64 * j + m,
-								   arr));
-			assert(!is_bit_set_in_array(b, arr));
+					assert(is_bit_set_in_bitfield(64 * j + m,
+								      bf));
+			assert(!is_bit_set_in_bitfield(b, bf));
+			if (b == 64 * BITARR_SZ - 1)
+				assert_int_equal(find_first_set(bf), 0);
+			else
+				assert_int_equal(find_first_set(bf), b + 2);
 			for (k = 0; k < BITARR_SZ; k++) {
 				if (k < j || (k == j && i == 63))
 					assert_int_equal(arr[k], 0ULL);
@@ -254,13 +271,207 @@ static void test_bitmask_2(void **state)
 			}
 		}
 	}
+	free(bf);
 }
 
+/*
+ *  Test operations on a 0-length bitfield
+ */
+static void test_bitmask_len_0(void **state)
+{
+	struct bitfield *bf;
+
+	bf = alloc_bitfield(0);
+	assert_non_null(bf);
+	assert_int_equal(bf->len, 0);
+	assert_int_equal(is_bit_set_in_bitfield(0, bf), 0);
+	assert_int_equal(is_bit_set_in_bitfield(1, bf), 0);
+	assert_int_equal(find_first_set(bf), 0);
+	set_bit_in_bitfield(0, bf);
+	assert_int_equal(is_bit_set_in_bitfield(0, bf), 0);
+	assert_int_equal(find_first_set(bf), 0);
+	clear_bit_in_bitfield(0, bf);
+	assert_int_equal(is_bit_set_in_bitfield(0, bf), 0);
+	set_bit_in_bitfield(11, bf);
+	assert_int_equal(find_first_set(bf), 0);
+	assert_int_equal(is_bit_set_in_bitfield(11, bf), 0);
+	clear_bit_in_bitfield(11, bf);
+	assert_int_equal(is_bit_set_in_bitfield(11, bf), 0);
+	free(bf);
+}
+
+static void _test_bitmask_small(unsigned int n)
+{
+	struct bitfield *bf;
+	uint64_t *arr;
+
+	assert(n <= 64);
+	assert(n >= 1);
+
+	bf = alloc_bitfield(n);
+	assert_non_null(bf);
+	assert_int_equal(bf->len, n);
+	arr = (uint64_t *)bf->bits;
+
+	assert_int_equal(*arr, 0);
+
+	set_bit_in_bitfield(n + 1, bf);
+	assert_int_equal(*arr, 0);
+	assert_int_equal(find_first_set(bf), 0);
+
+	set_bit_in_bitfield(n, bf);
+	assert_int_equal(*arr, 0);
+	assert_int_equal(find_first_set(bf), 0);
+
+	set_bit_in_bitfield(n - 1, bf);
+	assert_int_equal(*arr, 1ULL << (n - 1));
+	assert_int_equal(find_first_set(bf), n);
+
+	clear_bit_in_bitfield(n - 1, bf);
+	assert_int_equal(*arr, 0);
+	assert_int_equal(find_first_set(bf), 0);
+
+	set_bit_in_bitfield(0, bf);
+	assert_int_equal(*arr, 1);
+	assert_int_equal(find_first_set(bf), 1);
+
+	free(bf);
+}
+
+static void _test_bitmask_small_2(unsigned int n)
+{
+	struct bitfield *bf;
+	uint64_t *arr;
+
+	assert(n <= 128);
+	assert(n >= 65);
+
+	bf = alloc_bitfield(n);
+	assert_non_null(bf);
+	assert_int_equal(bf->len, n);
+	arr = (uint64_t *)bf->bits;
+
+	assert_int_equal(arr[0], 0);
+	assert_int_equal(arr[1], 0);
+
+	set_bit_in_bitfield(n + 1, bf);
+	assert_int_equal(arr[0], 0);
+	assert_int_equal(arr[1], 0);
+	assert_int_equal(find_first_set(bf), 0);
+
+	set_bit_in_bitfield(n, bf);
+	assert_int_equal(arr[0], 0);
+	assert_int_equal(arr[1], 0);
+	assert_int_equal(find_first_set(bf), 0);
+
+	set_bit_in_bitfield(n - 1, bf);
+	assert_int_equal(arr[0], 0);
+	assert_int_equal(arr[1], 1ULL << (n - 65));
+	assert_int_equal(find_first_set(bf), n);
+
+	set_bit_in_bitfield(0, bf);
+	assert_int_equal(arr[0], 1);
+	assert_int_equal(arr[1], 1ULL << (n - 65));
+	assert_int_equal(find_first_set(bf), 1);
+
+	set_bit_in_bitfield(64, bf);
+	assert_int_equal(arr[0], 1);
+	assert_int_equal(arr[1], (1ULL << (n - 65)) | 1);
+	assert_int_equal(find_first_set(bf), 1);
+
+	clear_bit_in_bitfield(0, bf);
+	assert_int_equal(arr[0], 0);
+	assert_int_equal(arr[1], (1ULL << (n - 65)) | 1);
+	assert_int_equal(find_first_set(bf), 65);
+
+	free(bf);
+}
+
+static void test_bitmask_len_1(void **state)
+{
+	_test_bitmask_small(1);
+}
+
+static void test_bitmask_len_2(void **state)
+{
+	_test_bitmask_small(2);
+}
+
+static void test_bitmask_len_3(void **state)
+{
+	_test_bitmask_small(3);
+}
+
+static void test_bitmask_len_23(void **state)
+{
+	_test_bitmask_small(23);
+}
+
+static void test_bitmask_len_63(void **state)
+{
+	_test_bitmask_small(63);
+}
+
+static void test_bitmask_len_64(void **state)
+{
+	_test_bitmask_small(63);
+}
+
+static void test_bitmask_len_65(void **state)
+{
+	_test_bitmask_small_2(65);
+}
+
+static void test_bitmask_len_66(void **state)
+{
+	_test_bitmask_small_2(66);
+}
+
+static void test_bitmask_len_67(void **state)
+{
+	_test_bitmask_small_2(67);
+}
+
+static void test_bitmask_len_103(void **state)
+{
+	_test_bitmask_small_2(103);
+}
+
+static void test_bitmask_len_126(void **state)
+{
+	_test_bitmask_small_2(126);
+}
+
+static void test_bitmask_len_127(void **state)
+{
+	_test_bitmask_small_2(127);
+}
+
+static void test_bitmask_len_128(void **state)
+{
+	_test_bitmask_small_2(128);
+}
+
+
 static int test_bitmasks(void)
 {
 	const struct CMUnitTest tests[] = {
 		cmocka_unit_test(test_bitmask_1),
 		cmocka_unit_test(test_bitmask_2),
+		cmocka_unit_test(test_bitmask_len_0),
+		cmocka_unit_test(test_bitmask_len_1),
+		cmocka_unit_test(test_bitmask_len_2),
+		cmocka_unit_test(test_bitmask_len_3),
+		cmocka_unit_test(test_bitmask_len_23),
+		cmocka_unit_test(test_bitmask_len_63),
+		cmocka_unit_test(test_bitmask_len_64),
+		cmocka_unit_test(test_bitmask_len_65),
+		cmocka_unit_test(test_bitmask_len_66),
+		cmocka_unit_test(test_bitmask_len_67),
+		cmocka_unit_test(test_bitmask_len_103),
+		cmocka_unit_test(test_bitmask_len_126),
+		cmocka_unit_test(test_bitmask_len_127),
+		cmocka_unit_test(test_bitmask_len_128),
 	};
 	return cmocka_run_group_tests(tests, NULL, NULL);
 }
-- 
2.26.2

[PATCH 09/35] libmultipath: use bitfields in group_by_match()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This makes "bitmap" a proper bitmap, and decreases memory consumption.
Unit tests for pgpolicy.c still pass.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/pgpolicies.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 02cafdc..0e55109 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -196,20 +196,20 @@ int group_by_match(struct multipath * mp, vector paths,
 		   bool (*path_match_fn)(struct path *, struct path *))
 {
 	int i, j;
-	int * bitmap;
+	struct bitfield *bitmap;
 	struct path * pp;
 	struct pathgroup * pgp;
 	struct path * pp2;
 
 	/* init the bitmap */
-	bitmap = (int *)MALLOC(VECTOR_SIZE(paths) * sizeof (int));
+	bitmap = alloc_bitfield(VECTOR_SIZE(paths));
 
 	if (!bitmap)
 		goto out;
 
 	for (i = 0; i < VECTOR_SIZE(paths); i++) {
 
-		if (bitmap[i])
+		if (is_bit_set_in_bitfield(i, bitmap))
 			continue;
 
 		pp = VECTOR_SLOT(paths, i);
@@ -227,11 +227,11 @@ int group_by_match(struct multipath * mp, vector paths,
 		if (store_path(pgp->paths, pp))
 			goto out1;
 
-		bitmap[i] = 1;
+		set_bit_in_bitfield(i, bitmap);
 
 		for (j = i + 1; j < VECTOR_SIZE(paths); j++) {
 
-			if (bitmap[j])
+			if (is_bit_set_in_bitfield(j, bitmap))
 				continue;
 
 			pp2 = VECTOR_SLOT(paths, j);
@@ -240,7 +240,7 @@ int group_by_match(struct multipath * mp, vector paths,
 				if (store_path(pgp->paths, pp2))
 					goto out1;
 
-				bitmap[j] = 1;
+				set_bit_in_bitfield(j, bitmap);
 			}
 		}
 	}
-- 
2.26.2

[PATCH 10/35] libmultipath: util: constify function arguments

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Use "const" for function arguments where possible.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dmparser.c |  2 +-
 libmultipath/util.c     | 12 ++++++------
 libmultipath/util.h     | 10 +++++-----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index b856a07..27581cd 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -18,7 +18,7 @@
 #define WORD_SIZE 64
 
 static int
-merge_words(char **dst, char *word)
+merge_words(char **dst, const char *word)
 {
 	char * p = *dst;
 	int len, dstlen;
diff --git a/libmultipath/util.c b/libmultipath/util.c
index 46cacd4..957fb97 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -53,7 +53,7 @@ basenamecpy (const char *src, char *dst, size_t size)
 }
 
 int
-filepresent (char * run) {
+filepresent (const char *run) {
 	struct stat buf;
 
 	if(!stat(run, &buf))
@@ -61,7 +61,7 @@ filepresent (char * run) {
 	return 0;
 }
 
-char *get_next_string(char **temp, char *split_char)
+char *get_next_string(char **temp, const char *split_char)
 {
 	char *token = NULL;
 	token = strsep(temp, split_char);
@@ -71,9 +71,9 @@ char *get_next_string(char **temp, char *split_char)
 }
 
 int
-get_word (char * sentence, char ** word)
+get_word (const char *sentence, char **word)
 {
-	char * p;
+	const char *p;
 	int len;
 	int skip = 0;
 
@@ -316,7 +316,7 @@ int get_linux_version_code(void)
 	return _linux_version_code;
 }
 
-int parse_prkey(char *ptr, uint64_t *prkey)
+int parse_prkey(const char *ptr, uint64_t *prkey)
 {
 	if (!ptr)
 		return 1;
@@ -333,7 +333,7 @@ int parse_prkey(char *ptr, uint64_t *prkey)
 	return 0;
 }
 
-int parse_prkey_flags(char *ptr, uint64_t *prkey, uint8_t *flags)
+int parse_prkey_flags(const char *ptr, uint64_t *prkey, uint8_t *flags)
 {
 	char *flagstr;
 
diff --git a/libmultipath/util.h b/libmultipath/util.h
index ec6de6d..ae18d8b 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -12,9 +12,9 @@
 
 size_t strchop(char *);
 int basenamecpy (const char *src, char *dst, size_t size);
-int filepresent (char * run);
-char *get_next_string(char **temp, char *split_char);
-int get_word (char * sentence, char ** word);
+int filepresent (const char *run);
+char *get_next_string(char **temp, const char *split_char);
+int get_word (const char * sentence, char ** word);
 size_t strlcpy(char *dst, const char *src, size_t size);
 size_t strlcat(char *dst, const char *src, size_t size);
 int devt2devname (char *, int, const char *);
@@ -23,8 +23,8 @@ char *convert_dev(char *dev, int is_path_device);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
 int systemd_service_enabled(const char *dev);
 int get_linux_version_code(void);
-int parse_prkey(char *ptr, uint64_t *prkey);
-int parse_prkey_flags(char *ptr, uint64_t *prkey, uint8_t *flags);
+int parse_prkey(const char *ptr, uint64_t *prkey);
+int parse_prkey_flags(const char *ptr, uint64_t *prkey, uint8_t *flags);
 int safe_write(int fd, const void *buf, size_t count);
 void set_max_fds(rlim_t max_fds);
 
-- 
2.26.2

[PATCH 11/35] multipath-tools tests: add unit tests for strlcat

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Also, use some constants for both strlcpy and strlcat tests.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/util.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 210 insertions(+), 12 deletions(-)

diff --git a/tests/util.c b/tests/util.c
index db7c05f..3c4113e 100644
--- a/tests/util.c
+++ b/tests/util.c
@@ -476,48 +476,54 @@ static int test_bitmasks(void)
 	return cmocka_run_group_tests(tests, NULL, NULL);
 }
 
-static const char src_str[] = "Hello";
+#define DST_STR "Hello"
+static const char dst_str[] = DST_STR;
+/* length of src_str and dst_str should be different */
+static const char src_str[] = " World";
+/* Must be big enough to hold dst_str and src_str */
+#define ARRSZ 16
+#define FILL '@'
 
 /* strlcpy with length 0 */
 static void test_strlcpy_0(void **state)
 {
-	char tst[] = "word";
+	char tst[] = DST_STR;
 	int rc;
 
 	rc = strlcpy(tst, src_str, 0);
 	assert_int_equal(rc, strlen(src_str));
-	assert_string_equal(tst, "word");
+	assert_string_equal(tst, dst_str);
 }
 
 /* strlcpy with length 1 */
 static void test_strlcpy_1(void **state)
 {
-	char tst[] = "word";
+	char tst[] = DST_STR;
 	int rc;
 
 	rc = strlcpy(tst, src_str, 1);
 	assert_int_equal(rc, strlen(src_str));
 	assert_int_equal(tst[0], '\0');
-	assert_string_equal(tst + 1, "ord");
+	assert_string_equal(tst + 1, dst_str + 1);
 }
 
 /* strlcpy with length 2 */
 static void test_strlcpy_2(void **state)
 {
-	char tst[] = "word";
+	char tst[] = DST_STR;
 	int rc;
 
 	rc = strlcpy(tst, src_str, 2);
 	assert_int_equal(rc, strlen(src_str));
 	assert_int_equal(tst[0], src_str[0]);
 	assert_int_equal(tst[1], '\0');
-	assert_string_equal(tst + 2, "rd");
+	assert_string_equal(tst + 2, dst_str + 2);
 }
 
 /* strlcpy with dst length < src length */
 static void test_strlcpy_3(void **state)
 {
-	char tst[] = "word";
+	char tst[] = DST_STR;
 	int rc;
 
 	rc = strlcpy(tst, src_str, sizeof(tst));
@@ -580,26 +586,26 @@ static void test_strlcpy_6(void **state)
 /* strlcpy with empty src */
 static void test_strlcpy_7(void **state)
 {
-	char tst[] = "word";
+	char tst[] = DST_STR;
 	static const char empty[] = "";
 	int rc;
 
 	rc = strlcpy(tst, empty, sizeof(tst));
 	assert_int_equal(rc, strlen(empty));
 	assert_string_equal(empty, tst);
-	assert_string_equal(tst + 1, "ord");
+	assert_string_equal(tst + 1, dst_str + 1);
 }
 
 /* strlcpy with empty src, length 0 */
 static void test_strlcpy_8(void **state)
 {
-	char tst[] = "word";
+	char tst[] = DST_STR;
 	static const char empty[] = "";
 	int rc;
 
 	rc = strlcpy(tst, empty, 0);
 	assert_int_equal(rc, strlen(empty));
-	assert_string_equal("word", tst);
+	assert_string_equal(dst_str, tst);
 }
 
 static int test_strlcpy(void)
@@ -619,6 +625,197 @@ static int test_strlcpy(void)
 	return cmocka_run_group_tests(tests, NULL, NULL);
 }
 
+
+/* 0-terminated string, filled with non-0 after the terminator */
+static void prep_buf(char *buf, size_t size, const char *word)
+{
+	memset(buf, FILL, size);
+	assert_in_range(strlen(word), 0, size - 1);
+	memcpy(buf, word, strlen(word) + 1);
+}
+
+/* strlcat with size 0, dst not 0-terminated  */
+static void test_strlcat_0(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, 0);
+	assert_int_equal(rc, strlen(src_str));
+	assert_string_equal(tst, dst_str);
+	assert_int_equal(tst[sizeof(dst_str)], FILL);
+}
+
+/* strlcat with length 1, dst not 0-terminated */
+static void test_strlcat_1(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, 1);
+	assert_int_equal(rc, 1 + strlen(src_str));
+	assert_string_equal(tst, dst_str);
+	assert_int_equal(tst[sizeof(dst_str)], FILL);
+}
+
+/* strlcat with length = dst - 1 */
+static void test_strlcat_2(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, strlen(dst_str));
+	assert_int_equal(rc, strlen(src_str) + strlen(dst_str));
+	assert_string_equal(tst, dst_str);
+	assert_int_equal(tst[sizeof(dst_str)], FILL);
+}
+
+/* strlcat with length = dst */
+static void test_strlcat_3(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, strlen(dst_str) + 1);
+	assert_int_equal(rc, strlen(src_str) + strlen(dst_str));
+	assert_string_equal(tst, dst_str);
+	assert_int_equal(tst[sizeof(dst_str)], FILL);
+}
+
+/* strlcat with len = dst + 1 */
+static void test_strlcat_4(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, strlen(dst_str) + 2);
+	assert_int_equal(rc, strlen(src_str) + strlen(dst_str));
+	assert_false(strncmp(tst, dst_str, strlen(dst_str)));
+	assert_int_equal(tst[strlen(dst_str)], src_str[0]);
+	assert_int_equal(tst[strlen(dst_str) + 1], '\0');
+	assert_int_equal(tst[strlen(dst_str) + 2], FILL);
+}
+
+/* strlcat with len = needed - 1 */
+static void test_strlcat_5(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, strlen(dst_str) + strlen(src_str));
+	assert_int_equal(rc, strlen(src_str) + strlen(dst_str));
+	assert_false(strncmp(tst, dst_str, strlen(dst_str)));
+	assert_false(strncmp(tst + strlen(dst_str), src_str,
+			     strlen(src_str) - 1));
+	assert_int_equal(tst[strlen(dst_str) + strlen(src_str) - 1], '\0');
+	assert_int_equal(tst[strlen(dst_str) + strlen(src_str)], FILL);
+}
+
+/* strlcat with exactly sufficient space */
+static void test_strlcat_6(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, strlen(dst_str) + strlen(src_str) + 1);
+	assert_int_equal(rc, strlen(src_str) + strlen(dst_str));
+	assert_false(strncmp(tst, dst_str, strlen(dst_str)));
+	assert_string_equal(tst + strlen(dst_str), src_str);
+	assert_int_equal(tst[strlen(dst_str) + strlen(src_str) + 1], FILL);
+}
+
+/* strlcat with sufficient space */
+static void test_strlcat_7(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, src_str, sizeof(tst));
+	assert_int_equal(rc, strlen(src_str) + strlen(dst_str));
+	assert_false(strncmp(tst, dst_str, strlen(dst_str)));
+	assert_string_equal(tst + strlen(dst_str), src_str);
+}
+
+/* strlcat with 0-length string */
+static void test_strlcat_8(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), dst_str);
+	rc = strlcat(tst, "", sizeof(tst));
+	assert_int_equal(rc, strlen(dst_str));
+	assert_string_equal(tst, dst_str);
+	assert_int_equal(tst[sizeof(dst_str)], FILL);
+}
+
+/* strlcat with empty dst */
+static void test_strlcat_9(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), "");
+	rc = strlcat(tst, src_str, ARRSZ);
+	assert_int_equal(rc, strlen(src_str));
+	assert_string_equal(tst, src_str);
+	assert_int_equal(tst[sizeof(src_str)], FILL);
+}
+
+/* strlcat with empty dst and src */
+static void test_strlcat_10(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), "");
+	rc = strlcat(tst, "", ARRSZ);
+	assert_int_equal(rc, 0);
+	assert_string_equal(tst, "");
+	assert_int_equal(tst[1], FILL);
+}
+
+/* strlcat with no space to store 0 */
+static void test_strlcat_11(void **state)
+{
+	char tst[ARRSZ];
+	int rc;
+
+	prep_buf(tst, sizeof(tst), "");
+	tst[0] = FILL;
+	rc = strlcat(tst, src_str, 0);
+	assert_int_equal(rc, strlen(src_str));
+	assert_int_equal(tst[0], FILL);
+}
+
+static int test_strlcat(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_strlcat_0),
+		cmocka_unit_test(test_strlcat_1),
+		cmocka_unit_test(test_strlcat_2),
+		cmocka_unit_test(test_strlcat_3),
+		cmocka_unit_test(test_strlcat_4),
+		cmocka_unit_test(test_strlcat_5),
+		cmocka_unit_test(test_strlcat_6),
+		cmocka_unit_test(test_strlcat_7),
+		cmocka_unit_test(test_strlcat_8),
+		cmocka_unit_test(test_strlcat_9),
+		cmocka_unit_test(test_strlcat_10),
+		cmocka_unit_test(test_strlcat_11),
+	};
+
+	return cmocka_run_group_tests(tests, NULL, NULL);
+}
+
 static void test_strchop_nochop(void **state)
 {
 	char hello[] = "hello";
@@ -688,6 +885,7 @@ int main(void)
 	ret += test_basenamecpy();
 	ret += test_bitmasks();
 	ret += test_strlcpy();
+	ret += test_strlcat();
 	ret += test_strchop();
 	return ret;
 }
-- 
2.26.2

[PATCH 12/35] libmultipath: strlcpy()/strlcat(): use restrict qualifier

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Also remove the redundant local variables. It's not necessary to
make "restrict" work, but it makes the intention more clear.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/util.c | 28 ++++++++++++----------------
 libmultipath/util.h |  4 ++--
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/libmultipath/util.c b/libmultipath/util.c
index 957fb97..f965094 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -113,46 +113,42 @@ get_word (const char *sentence, char **word)
 	return skip + len;
 }
 
-size_t strlcpy(char *dst, const char *src, size_t size)
+size_t strlcpy(char * restrict dst, const char * restrict src, size_t size)
 {
 	size_t bytes = 0;
-	char *q = dst;
-	const char *p = src;
 	char ch;
 
-	while ((ch = *p++)) {
-		if (bytes+1 < size)
-			*q++ = ch;
+	while ((ch = *src++)) {
+		if (bytes + 1 < size)
+			*dst++ = ch;
 		bytes++;
 	}
 
 	/* If size == 0 there is no space for a final null... */
 	if (size)
-		*q = '\0';
+		*dst = '\0';
 	return bytes;
 }
 
-size_t strlcat(char *dst, const char *src, size_t size)
+size_t strlcat(char * restrict dst, const char * restrict src, size_t size)
 {
 	size_t bytes = 0;
-	char *q = dst;
-	const char *p = src;
 	char ch;
 
-	while (bytes < size && *q) {
-		q++;
+	while (bytes < size && *dst) {
+		dst++;
 		bytes++;
 	}
 	if (bytes == size)
 		return (bytes + strlen(src));
 
-	while ((ch = *p++)) {
-		if (bytes+1 < size)
-		*q++ = ch;
+	while ((ch = *src++)) {
+		if (bytes + 1 < size)
+			*dst++ = ch;
 		bytes++;
 	}
 
-	*q = '\0';
+	*dst = '\0';
 	return bytes;
 }
 
diff --git a/libmultipath/util.h b/libmultipath/util.h
index ae18d8b..7e29b26 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -15,8 +15,8 @@ int basenamecpy (const char *src, char *dst, size_t size);
 int filepresent (const char *run);
 char *get_next_string(char **temp, const char *split_char);
 int get_word (const char * sentence, char ** word);
-size_t strlcpy(char *dst, const char *src, size_t size);
-size_t strlcat(char *dst, const char *src, size_t size);
+size_t strlcpy(char * restrict dst, const char * restrict src, size_t size);
+size_t strlcat(char * restrict dst, const char * restrict src, size_t size);
 int devt2devname (char *, int, const char *);
 dev_t parse_devt(const char *dev_t);
 char *convert_dev(char *dev, int is_path_device);
-- 
2.26.2

[PATCH 13/35] libmultipath: constify blacklist code

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add "const" qualifiers where appropriate.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/blacklist.c | 34 +++++++++++++++++++---------------
 libmultipath/blacklist.h | 17 +++++++++++------
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index db58ccc..4964e70 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -121,7 +121,7 @@ int set_ble_device(vector blist, char * vendor, char * product, int origin)
 }
 
 static int
-match_reglist (vector blist, const char * str)
+match_reglist (const struct _vector *blist, const char *str)
 {
 	int i;
 	struct blentry * ble;
@@ -134,8 +134,8 @@ match_reglist (vector blist, const char * str)
 }
 
 static int
-match_reglist_device (const struct _vector *blist, const char * vendor,
-		    const char * product)
+match_reglist_device (const struct _vector *blist, const char *vendor,
+		      const char * product)
 {
 	int i;
 	struct blentry_device * ble;
@@ -155,8 +155,8 @@ match_reglist_device (const struct _vector *blist, const char * vendor,
 }
 
 static int
-find_blacklist_device (const struct _vector *blist, const char * vendor,
-		       const char * product)
+find_blacklist_device (const struct _vector *blist, const char *vendor,
+		       const char *product)
 {
 	int i;
 	struct blentry_device * ble;
@@ -231,8 +231,9 @@ setup_default_blist (struct config * conf)
 		condlog(lvl, "%s: %s %s", dev, (M), (S))
 
 static void
-log_filter (const char *dev, char *vendor, char *product, char *wwid,
-	    const char *env, const char *protocol, int r, int lvl)
+log_filter (const char *dev, const char *vendor, const char *product,
+	    const char *wwid, const char *env, const char *protocol,
+	    int r, int lvl)
 {
 	/*
 	 * Try to sort from most likely to least.
@@ -277,8 +278,8 @@ log_filter (const char *dev, char *vendor, char *product, char *wwid,
 }
 
 int
-filter_device (vector blist, vector elist, char * vendor, char * product,
-	       char * dev)
+filter_device (const struct _vector *blist, const struct _vector *elist,
+	       const char *vendor, const char * product, const char *dev)
 {
 	int r = MATCH_NOTHING;
 
@@ -294,7 +295,8 @@ filter_device (vector blist, vector elist, char * vendor, char * product,
 }
 
 int
-filter_devnode (vector blist, vector elist, char * dev)
+filter_devnode (const struct _vector *blist, const struct _vector *elist,
+		const char *dev)
 {
 	int r = MATCH_NOTHING;
 
@@ -310,7 +312,8 @@ filter_devnode (vector blist, vector elist, char * dev)
 }
 
 int
-filter_wwid (vector blist, vector elist, char * wwid, char * dev)
+filter_wwid (const struct _vector *blist, const struct _vector *elist,
+	     const char *wwid, const char *dev)
 {
 	int r = MATCH_NOTHING;
 
@@ -326,7 +329,8 @@ filter_wwid (vector blist, vector elist, char * wwid, char * dev)
 }
 
 int
-filter_protocol(vector blist, vector elist, struct path * pp)
+filter_protocol(const struct _vector *blist, const struct _vector *elist,
+		const struct path *pp)
 {
 	char buf[PROTOCOL_BUF_SIZE];
 	int r = MATCH_NOTHING;
@@ -345,7 +349,7 @@ filter_protocol(vector blist, vector elist, struct path * pp)
 }
 
 int
-filter_path (struct config * conf, struct path * pp)
+filter_path (const struct config *conf, const struct path *pp)
 {
 	int r;
 
@@ -367,8 +371,8 @@ filter_path (struct config * conf, struct path * pp)
 }
 
 int
-filter_property(struct config *conf, struct udev_device *udev, int lvl,
-		const char *uid_attribute)
+filter_property(const struct config *conf, struct udev_device *udev,
+		int lvl, const char *uid_attribute)
 {
 	const char *devname = udev_device_get_sysname(udev);
 	struct udev_list_entry *list_entry;
diff --git a/libmultipath/blacklist.h b/libmultipath/blacklist.h
index 4305857..ed079f3 100644
--- a/libmultipath/blacklist.h
+++ b/libmultipath/blacklist.h
@@ -36,12 +36,17 @@ struct blentry_device {
 
 int setup_default_blist (struct config *);
 int alloc_ble_device (vector);
-int filter_devnode (vector, vector, char *);
-int filter_wwid (vector, vector, char *, char *);
-int filter_device (vector, vector, char *, char *, char *);
-int filter_path (struct config *, struct path *);
-int filter_property(struct config *, struct udev_device *, int, const char*);
-int filter_protocol(vector, vector, struct path *);
+int filter_devnode (const struct _vector *, const struct _vector *,
+		    const char *);
+int filter_wwid (const struct _vector *, const struct _vector *,
+		 const char *, const char *);
+int filter_device (const struct _vector *, const struct _vector *,
+		   const char *, const char *, const char *);
+int filter_path (const struct config *, const struct path *);
+int filter_property(const struct config *, struct udev_device *,
+		    int, const char*);
+int filter_protocol(const struct _vector *, const struct _vector *,
+		    const struct path *);
 int store_ble (vector, char *, int);
 int set_ble_device (vector, char *, char *, int);
 void free_blacklist (vector);
-- 
2.26.2

[PATCH 14/35] libmultipath: rlookup_binding(): remove newline in log message

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This clutters the log. The corresponding message in lookup_binding()
doesn't have a newline, either.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c | 2 +-
 tests/alias.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 14401ca..35f1beb 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -215,7 +215,7 @@ rlookup_binding(FILE *f, char *buff, const char *map_alias)
 		}
 		if (strcmp(alias, map_alias) == 0){
 			condlog(3, "Found matching alias [%s] in bindings file."
-				"\nSetting wwid to %s", alias, wwid);
+				" Setting wwid to %s", alias, wwid);
 			strlcpy(buff, wwid, WWID_SIZE);
 			return 0;
 		}
diff --git a/tests/alias.c b/tests/alias.c
index 30414db..22ffd55 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -552,7 +552,7 @@ static void rl_match_a(void **state)
 
 	buf[0] = '\0';
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
-	expect_condlog(3, "Found matching alias [MPATHa] in bindings file.\n"
+	expect_condlog(3, "Found matching alias [MPATHa] in bindings file. "
 		       "Setting wwid to WWID0\n");
 	rc = rlookup_binding(NULL, buf, "MPATHa");
 	assert_int_equal(rc, 0);
@@ -617,7 +617,7 @@ static void rl_match_b(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, "MPATHz WWID26\n");
 	will_return(__wrap_fgets, "MPATHb WWID2\n");
-	expect_condlog(3, "Found matching alias [MPATHb] in bindings file.\n"
+	expect_condlog(3, "Found matching alias [MPATHb] in bindings file. "
 		       "Setting wwid to WWID2\n");
 	rc = rlookup_binding(NULL, buf, "MPATHb");
 	assert_int_equal(rc, 0);
-- 
2.26.2

[PATCH 15/35] libmultipath: fix missing initializer warning from clang 3.9

[mwilck]

From: Martin Wilck <mwilck@suse.com>

clang 3.9 needs designators to accept implicit struct initialization.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers/directio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/checkers/directio.c b/libmultipath/checkers/directio.c
index 503519e..f73cbe3 100644
--- a/libmultipath/checkers/directio.c
+++ b/libmultipath/checkers/directio.c
@@ -256,7 +256,7 @@ get_events(struct aio_group *aio_grp, struct timespec *timeout)
 {
 	struct io_event events[128];
 	int i, nr, got_events = 0;
-	struct timespec zero_timeout = {0};
+	struct timespec zero_timeout = { .tv_sec = 0, };
 	struct timespec *timep = timeout;
 
 	do {
-- 
2.26.2

[PATCH 16/35] libmultipath: fix gcc -Wstringop-overflow warning

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This fixes this gcc error message:

error: ‘strncat’ specified bound 1 equals source length
   [-Werror=stringop-overflow=]
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 11a6168..4f65ba1 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -374,7 +374,7 @@ set_value(vector strvec)
 			goto oom;
 		}
 		if (*alloc != '\0')
-			strncat(alloc, " ", 1);
+			strncat(alloc, " ", len - strlen(alloc));
 		strncat(alloc, str, len - strlen(alloc) - 1);
 	}
 	return alloc;
-- 
2.26.2


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[PATCH 17/35] libmultipath: remove uevent listener failback

[mwilck]

From: Martin Wilck <mwilck@suse.com>

TL;DR: this code is obsolete.

The failback code uses the udev 'RUN+="socket:"' technique, which
has been deprecated since udev 178 and removed since udev 183,
in 2012. The corresponding udev rule in multipath-tools has been removed
in ad067ac ("multipath: do not install rules file"), in 0.5.0 (2013).
Since that time, the failback code has been non-functional and basically
unmaintained.

While there's yet another fallback to the netlink socket, this 2nd fallback
would only be used if the socket(AF_LOCAL, ...) call failed, which would be
very unlikely. Even with the AF_LOCAL failback removed, leaving this 2nd
fallback in place doesn't make sense because it's just a reimplementation
of udev_monitor_new_from_netlink(); it's hard to imagine a case where the
latter would fail but our code would succeed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 245 +-----------------------------------------
 libmultipath/uevent.h |   4 -
 2 files changed, 2 insertions(+), 247 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index e0d13b1..6a3f8bd 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -450,243 +450,6 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 	return 0;
 }
 
-struct uevent *uevent_from_buffer(char *buf, ssize_t buflen)
-{
-	struct uevent *uev;
-	char *buffer;
-	size_t bufpos;
-	int i;
-	char *pos;
-
-	uev = alloc_uevent();
-	if (!uev) {
-		condlog(1, "lost uevent, oom");
-		return NULL;
-	}
-
-	if ((size_t)buflen > sizeof(buf)-1)
-		buflen = sizeof(buf)-1;
-
-	/*
-	 * Copy the shared receive buffer contents to buffer private
-	 * to this uevent so we can immediately reuse the shared buffer.
-	 */
-	memcpy(uev->buffer, buf, HOTPLUG_BUFFER_SIZE + OBJECT_SIZE);
-	buffer = uev->buffer;
-	buffer[buflen] = '\0';
-
-	/* save start of payload */
-	bufpos = strlen(buffer) + 1;
-
-	/* action string */
-	uev->action = buffer;
-	pos = strchr(buffer, '@');
-	if (!pos) {
-		condlog(3, "bad action string '%s'", buffer);
-		FREE(uev);
-		return NULL;
-	}
-	pos[0] = '\0';
-
-	/* sysfs path */
-	uev->devpath = &pos[1];
-
-	/* hotplug events have the environment attached - reconstruct envp[] */
-	for (i = 0; (bufpos < (size_t)buflen) && (i < HOTPLUG_NUM_ENVP-1); i++) {
-		int keylen;
-		char *key;
-
-		key = &buffer[bufpos];
-		keylen = strlen(key);
-		uev->envp[i] = key;
-		/* Filter out sequence number */
-		if (strncmp(key, "SEQNUM=", 7) == 0) {
-			char *eptr;
-
-			uev->seqnum = strtoul(key + 7, &eptr, 10);
-			if (eptr == key + 7)
-				uev->seqnum = -1;
-		}
-		bufpos += keylen + 1;
-	}
-	uev->envp[i] = NULL;
-
-	condlog(3, "uevent %ld '%s' from '%s'", uev->seqnum,
-		uev->action, uev->devpath);
-	uev->kernel = strrchr(uev->devpath, '/');
-	if (uev->kernel)
-		uev->kernel++;
-
-	/* print payload environment */
-	for (i = 0; uev->envp[i] != NULL; i++)
-		condlog(5, "%s", uev->envp[i]);
-
-	return uev;
-}
-
-int failback_listen(void)
-{
-	int sock;
-	struct sockaddr_nl snl;
-	struct sockaddr_un sun;
-	socklen_t addrlen;
-	int retval;
-	int rcvbufsz = 128*1024;
-	int rcvsz = 0;
-	int rcvszsz = sizeof(rcvsz);
-	unsigned int *prcvszsz = (unsigned int *)&rcvszsz;
-	const int feature_on = 1;
-	/*
-	 * First check whether we have a udev socket
-	 */
-	memset(&sun, 0x00, sizeof(struct sockaddr_un));
-	sun.sun_family = AF_LOCAL;
-	strcpy(&sun.sun_path[1], "/org/kernel/dm/multipath_event");
-	addrlen = offsetof(struct sockaddr_un, sun_path) + strlen(sun.sun_path+1) + 1;
-
-	sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
-	if (sock >= 0) {
-
-		condlog(3, "reading events from udev socket.");
-
-		/* the bind takes care of ensuring only one copy running */
-		retval = bind(sock, (struct sockaddr *) &sun, addrlen);
-		if (retval < 0) {
-			condlog(0, "bind failed, exit");
-			goto exit;
-		}
-
-		/* enable receiving of the sender credentials */
-		retval = setsockopt(sock, SOL_SOCKET, SO_PASSCRED,
-				    &feature_on, sizeof(feature_on));
-		if (retval < 0) {
-			condlog(0, "failed to enable credential passing, exit");
-			goto exit;
-		}
-
-	} else {
-		/* Fallback to read kernel netlink events */
-		memset(&snl, 0x00, sizeof(struct sockaddr_nl));
-		snl.nl_family = AF_NETLINK;
-		snl.nl_pid = getpid();
-		snl.nl_groups = 0x01;
-
-		sock = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_KOBJECT_UEVENT);
-		if (sock == -1) {
-			condlog(0, "error getting socket, exit");
-			return 1;
-		}
-
-		condlog(3, "reading events from kernel.");
-
-		/*
-		 * try to avoid dropping uevents, even so, this is not a guarantee,
-		 * but it does help to change the netlink uevent socket's
-		 * receive buffer threshold from the default value of 106,496 to
-		 * the maximum value of 262,142.
-		 */
-		retval = setsockopt(sock, SOL_SOCKET, SO_RCVBUF, &rcvbufsz,
-				    sizeof(rcvbufsz));
-
-		if (retval < 0) {
-			condlog(0, "error setting receive buffer size for socket, exit");
-			exit(1);
-		}
-		retval = getsockopt(sock, SOL_SOCKET, SO_RCVBUF, &rcvsz, prcvszsz);
-		if (retval < 0) {
-			condlog(0, "error setting receive buffer size for socket, exit");
-			exit(1);
-		}
-		condlog(3, "receive buffer size for socket is %u.", rcvsz);
-
-		/* enable receiving of the sender credentials */
-		if (setsockopt(sock, SOL_SOCKET, SO_PASSCRED,
-			       &feature_on, sizeof(feature_on)) < 0) {
-			condlog(0, "error on enabling credential passing for socket");
-			exit(1);
-		}
-
-		retval = bind(sock, (struct sockaddr *) &snl,
-			      sizeof(struct sockaddr_nl));
-		if (retval < 0) {
-			condlog(0, "bind failed, exit");
-			goto exit;
-		}
-	}
-
-	while (1) {
-		size_t bufpos;
-		ssize_t buflen;
-		struct uevent *uev;
-		struct msghdr smsg;
-		struct iovec iov;
-		char cred_msg[CMSG_SPACE(sizeof(struct ucred))];
-		struct cmsghdr *cmsg;
-		struct ucred *cred;
-		static char buf[HOTPLUG_BUFFER_SIZE + OBJECT_SIZE];
-
-		memset(buf, 0x00, sizeof(buf));
-		iov.iov_base = &buf;
-		iov.iov_len = sizeof(buf);
-		memset (&smsg, 0x00, sizeof(struct msghdr));
-		smsg.msg_iov = &iov;
-		smsg.msg_iovlen = 1;
-		smsg.msg_control = cred_msg;
-		smsg.msg_controllen = sizeof(cred_msg);
-
-		buflen = recvmsg(sock, &smsg, 0);
-		if (buflen < 0) {
-			if (errno != EINTR)
-				condlog(0, "error receiving message, errno %d", errno);
-			continue;
-		}
-
-		cmsg = CMSG_FIRSTHDR(&smsg);
-		if (cmsg == NULL || cmsg->cmsg_type != SCM_CREDENTIALS) {
-			condlog(3, "no sender credentials received, message ignored");
-			continue;
-		}
-
-		cred = (struct ucred *)CMSG_DATA(cmsg);
-		if (cred->uid != 0) {
-			condlog(3, "sender uid=%d, message ignored", cred->uid);
-			continue;
-		}
-
-		/* skip header */
-		bufpos = strlen(buf) + 1;
-		if (bufpos < sizeof("a@/d") || bufpos >= sizeof(buf)) {
-			condlog(3, "invalid message length");
-			continue;
-		}
-
-		/* check message header */
-		if (strstr(buf, "@/") == NULL) {
-			condlog(3, "unrecognized message header");
-			continue;
-		}
-		if ((size_t)buflen > sizeof(buf)-1) {
-			condlog(2, "buffer overflow for received uevent");
-			buflen = sizeof(buf)-1;
-		}
-
-		uev = uevent_from_buffer(buf, buflen);
-		if (!uev)
-			continue;
-		/*
-		 * Queue uevent and poke service pthread.
-		 */
-		pthread_mutex_lock(uevq_lockp);
-		list_add_tail(&uev->node, &uevq);
-		pthread_cond_signal(uev_condp);
-		pthread_mutex_unlock(uevq_lockp);
-	}
-
-exit:
-	close(sock);
-	return 1;
-}
-
 struct uevent *uevent_from_udev_device(struct udev_device *dev)
 {
 	struct uevent *uev;
@@ -786,7 +549,6 @@ int uevent_listen(struct udev *udev)
 	struct udev_monitor *monitor = NULL;
 	int fd, socket_flags, events;
 	struct timeval start_time;
-	int need_failback = 1;
 	int timeout = 30;
 	LIST_HEAD(uevlisten_tmp);
 
@@ -806,7 +568,7 @@ int uevent_listen(struct udev *udev)
 	monitor = udev_monitor_new_from_netlink(udev, "udev");
 	if (!monitor) {
 		condlog(2, "failed to create udev monitor");
-		goto failback;
+		goto out_udev;
 	}
 	pthread_cleanup_push(monitor_cleanup, monitor);
 #ifdef LIBUDEV_API_RECVBUF
@@ -891,12 +653,9 @@ int uevent_listen(struct udev *udev)
 		gettimeofday(&start_time, NULL);
 		timeout = 30;
 	}
-	need_failback = 0;
 out:
 	pthread_cleanup_pop(1);
-failback:
-	if (need_failback)
-		err = failback_listen();
+out_udev:
 	pthread_cleanup_pop(1);
 	return err;
 }
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 0aa8675..4956bfc 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -9,10 +9,6 @@
 #define HOTPLUG_NUM_ENVP		32
 #define OBJECT_SIZE			512
 
-#ifndef NETLINK_KOBJECT_UEVENT
-#define NETLINK_KOBJECT_UEVENT		15
-#endif
-
 struct udev;
 
 struct uevent {
-- 
2.26.2

[PATCH 18/35] libmultipath: uevent: use static linkage where possible

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Most of the symbols in uevent.c can be converted to static linkage.
alloc_uevent() and uevent_get_wwid() are called in the unit test
and added to the header file.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 45 ++++++++++++++++++++-----------------------
 libmultipath/uevent.h |  2 ++
 tests/uevent.c        |  4 ----
 3 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 6a3f8bd..d74389e 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -60,14 +60,14 @@
 
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
-LIST_HEAD(uevq);
-pthread_mutex_t uevq_lock = PTHREAD_MUTEX_INITIALIZER;
-pthread_mutex_t *uevq_lockp = &uevq_lock;
-pthread_cond_t uev_cond = PTHREAD_COND_INITIALIZER;
-pthread_cond_t *uev_condp = &uev_cond;
-uev_trigger *my_uev_trigger;
-void * my_trigger_data;
-int servicing_uev;
+static LIST_HEAD(uevq);
+static pthread_mutex_t uevq_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t *uevq_lockp = &uevq_lock;
+static pthread_cond_t uev_cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t *uev_condp = &uev_cond;
+static uev_trigger *my_uev_trigger;
+static void *my_trigger_data;
+static int servicing_uev;
 
 int is_uevent_busy(void)
 {
@@ -91,8 +91,7 @@ struct uevent * alloc_uevent (void)
 	return uev;
 }
 
-void
-uevq_cleanup(struct list_head *tmpq)
+static void uevq_cleanup(struct list_head *tmpq)
 {
 	struct uevent *uev, *tmp;
 
@@ -172,8 +171,7 @@ uevent_get_wwid(struct uevent *uev)
 		uev->wwid = val;
 }
 
-bool
-uevent_need_merge(void)
+static bool uevent_need_merge(void)
 {
 	struct config * conf;
 	bool need_merge = false;
@@ -186,8 +184,7 @@ uevent_need_merge(void)
 	return need_merge;
 }
 
-bool
-uevent_can_discard(struct uevent *uev)
+static bool uevent_can_discard(struct uevent *uev)
 {
 	int invalid = 0;
 	struct config * conf;
@@ -212,7 +209,7 @@ uevent_can_discard(struct uevent *uev)
 	return false;
 }
 
-bool
+static bool
 uevent_can_filter(struct uevent *earlier, struct uevent *later)
 {
 
@@ -246,7 +243,7 @@ uevent_can_filter(struct uevent *earlier, struct uevent *later)
 	return false;
 }
 
-bool
+static bool
 merge_need_stop(struct uevent *earlier, struct uevent *later)
 {
 	/*
@@ -283,7 +280,7 @@ merge_need_stop(struct uevent *earlier, struct uevent *later)
 	return false;
 }
 
-bool
+static bool
 uevent_can_merge(struct uevent *earlier, struct uevent *later)
 {
 	/* merge paths uevents
@@ -302,7 +299,7 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
 	return false;
 }
 
-void
+static void
 uevent_prepare(struct list_head *tmpq)
 {
 	struct uevent *uev, *tmp;
@@ -322,7 +319,7 @@ uevent_prepare(struct list_head *tmpq)
 	}
 }
 
-void
+static void
 uevent_filter(struct uevent *later, struct list_head *tmpq)
 {
 	struct uevent *earlier, *tmp;
@@ -345,7 +342,7 @@ uevent_filter(struct uevent *later, struct list_head *tmpq)
 	}
 }
 
-void
+static void
 uevent_merge(struct uevent *later, struct list_head *tmpq)
 {
 	struct uevent *earlier, *tmp;
@@ -366,7 +363,7 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 	}
 }
 
-void
+static void
 merge_uevq(struct list_head *tmpq)
 {
 	struct uevent *later;
@@ -379,7 +376,7 @@ merge_uevq(struct list_head *tmpq)
 	}
 }
 
-void
+static void
 service_uevq(struct list_head *tmpq)
 {
 	struct uevent *uev, *tmp;
@@ -450,7 +447,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 	return 0;
 }
 
-struct uevent *uevent_from_udev_device(struct udev_device *dev)
+static struct uevent *uevent_from_udev_device(struct udev_device *dev)
 {
 	struct uevent *uev;
 	int i = 0;
@@ -512,7 +509,7 @@ struct uevent *uevent_from_udev_device(struct udev_device *dev)
 	return uev;
 }
 
-bool uevent_burst(struct timeval *start_time, int events)
+static bool uevent_burst(struct timeval *start_time, int events)
 {
 	struct timeval diff_time, end_time;
 	unsigned long speed;
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 4956bfc..9a5b213 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -24,6 +24,7 @@ struct uevent {
 	char *envp[HOTPLUG_NUM_ENVP];
 };
 
+struct uevent *alloc_uevent(void);
 int is_uevent_busy(void);
 
 int uevent_listen(struct udev *udev);
@@ -36,5 +37,6 @@ char *uevent_get_dm_name(const struct uevent *uev);
 char *uevent_get_dm_path(const struct uevent *uev);
 char *uevent_get_dm_action(const struct uevent *uev);
 bool uevent_is_mpath(const struct uevent *uev);
+void uevent_get_wwid(struct uevent *uev);
 
 #endif /* _UEVENT_H */
diff --git a/tests/uevent.c b/tests/uevent.c
index f4afd9b..9ffcd2d 100644
--- a/tests/uevent.c
+++ b/tests/uevent.c
@@ -27,10 +27,6 @@
 
 #include "globals.c"
 
-/* Private prototypes missing in uevent.h */
-struct uevent * alloc_uevent(void);
-void uevent_get_wwid(struct uevent *uev);
-
 /* Stringify helpers */
 #define _str_(x) #x
 #define str(x) _str_(x)
-- 
2.26.2

[PATCH 19/35] libmultipath: uevent: inline trivial functions

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Export rather the not-quite-as-trivial getter functions, and
convert the accessors to inline wrappers.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 34 ++--------------------------------
 libmultipath/uevent.h | 41 +++++++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index d74389e..d3061bf 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -136,7 +136,7 @@ invalid:
 	return NULL;
 }
 
-static int uevent_get_env_positive_int(const struct uevent *uev,
+int uevent_get_env_positive_int(const struct uevent *uev,
 				       const char *attr)
 {
 	const char *p = uevent_get_env_var(uev, attr);
@@ -657,22 +657,7 @@ out_udev:
 	return err;
 }
 
-int uevent_get_major(const struct uevent *uev)
-{
-	return uevent_get_env_positive_int(uev, "MAJOR");
-}
-
-int uevent_get_minor(const struct uevent *uev)
-{
-	return uevent_get_env_positive_int(uev, "MINOR");
-}
-
-int uevent_get_disk_ro(const struct uevent *uev)
-{
-	return uevent_get_env_positive_int(uev, "DISK_RO");
-}
-
-static char *uevent_get_dm_str(const struct uevent *uev, char *attr)
+char *uevent_get_dm_str(const struct uevent *uev, char *attr)
 {
 	const char *tmp = uevent_get_env_var(uev, attr);
 
@@ -681,21 +666,6 @@ static char *uevent_get_dm_str(const struct uevent *uev, char *attr)
 	return strdup(tmp);
 }
 
-char *uevent_get_dm_name(const struct uevent *uev)
-{
-	return uevent_get_dm_str(uev, "DM_NAME");
-}
-
-char *uevent_get_dm_path(const struct uevent *uev)
-{
-	return uevent_get_dm_str(uev, "DM_PATH");
-}
-
-char *uevent_get_dm_action(const struct uevent *uev)
-{
-	return uevent_get_dm_str(uev, "DM_ACTION");
-}
-
 bool uevent_is_mpath(const struct uevent *uev)
 {
 	const char *uuid = uevent_get_env_var(uev, "DM_UUID");
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 9a5b213..61ca1b5 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -30,13 +30,42 @@ int is_uevent_busy(void);
 int uevent_listen(struct udev *udev);
 int uevent_dispatch(int (*store_uev)(struct uevent *, void * trigger_data),
 		    void * trigger_data);
-int uevent_get_major(const struct uevent *uev);
-int uevent_get_minor(const struct uevent *uev);
-int uevent_get_disk_ro(const struct uevent *uev);
-char *uevent_get_dm_name(const struct uevent *uev);
-char *uevent_get_dm_path(const struct uevent *uev);
-char *uevent_get_dm_action(const struct uevent *uev);
 bool uevent_is_mpath(const struct uevent *uev);
 void uevent_get_wwid(struct uevent *uev);
 
+int uevent_get_env_positive_int(const struct uevent *uev,
+				const char *attr);
+
+static inline int uevent_get_major(const struct uevent *uev)
+{
+	return uevent_get_env_positive_int(uev, "MAJOR");
+}
+
+static inline int uevent_get_minor(const struct uevent *uev)
+{
+	return uevent_get_env_positive_int(uev, "MINOR");
+}
+
+static inline int uevent_get_disk_ro(const struct uevent *uev)
+{
+	return uevent_get_env_positive_int(uev, "DISK_RO");
+}
+
+char *uevent_get_dm_str(const struct uevent *uev, char *attr);
+
+static inline char *uevent_get_dm_name(const struct uevent *uev)
+{
+	return uevent_get_dm_str(uev, "DM_NAME");
+}
+
+static inline char *uevent_get_dm_path(const struct uevent *uev)
+{
+	return uevent_get_dm_str(uev, "DM_PATH");
+}
+
+static inline char *uevent_get_dm_action(const struct uevent *uev)
+{
+	return uevent_get_dm_str(uev, "DM_ACTION");
+}
+
 #endif /* _UEVENT_H */
-- 
2.26.2

[PATCH 20/35] libmultipath: decrease log level of "SCSI target" log message

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... in sysfs_get_tgt_nodename().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index aa5942c..1039fc4 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -391,7 +391,7 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
 		tgtdev = udev_device_new_from_subsystem_sysname(udev,
 				"fc_remote_ports", value);
 		if (tgtdev) {
-			condlog(3, "SCSI target %d:%d:%d -> "
+			condlog(4, "SCSI target %d:%d:%d -> "
 				"FC rport %d:%d-%d",
 				pp->sg_id.host_no, pp->sg_id.channel,
 				pp->sg_id.scsi_id, host, channel,
-- 
2.26.2

[PATCH 21/35] libmultipath: get_udev_uid(): more appropriate error code

[mwilck]

From: Martin Wilck <mwilck@suse.com>

An uid_attribute that is not found is not necessarily invalid
input. "No data available" seems more appropriate.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 1039fc4..81c78d3 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1897,7 +1897,7 @@ get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
 	} else {
 		condlog(3, "%s: no %s attribute", pp->dev,
 			uid_attribute);
-		len = -EINVAL;
+		len = -ENODATA;
 	}
 	return len;
 }
-- 
2.26.2

[PATCH 22/35] libmultipath: get_uid(): improve log message on udev failure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If WWID determination by uid_attribute fails, and no fallback
is available, multipath(d) print two error messages:
"failed to get udev uid" and "failed to get unknown uid".
That's confusing and unnecessary. Print only the udev message.
If the fallback is available, this will result in only the
error message from the fallback being printed. But that's not
a big issue, because the udev error message will have been printed
before, and because failure of the fallback implies previous failure
of the udev method.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 81c78d3..5d4bf7d 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2014,12 +2014,9 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev,
 
 		if (udev_available) {
 			len = get_udev_uid(pp, pp->uid_attribute, udev);
-			if (len <= 0)
-				condlog(1,
-					"%s: failed to get udev uid: %s",
-					pp->dev, strerror(-len));
-			else
-				origin = "udev";
+			origin = "udev";
+			if (len == 0)
+				condlog(1, "%s: empty udev uid", pp->dev);
 		}
 		if ((!udev_available || (len <= 0 && allow_fallback))
 		    && has_uid_fallback(pp)) {
-- 
2.26.2

[PATCH 23/35] libmultipath: make sysfs_pathinfo() static and use const

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... for the hwtable argument.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5d4bf7d..c692026 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1284,7 +1284,7 @@ get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen)
 }
 
 static int
-scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
+scsi_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable)
 {
 	struct udev_device *parent;
 	const char *attr_path = NULL;
@@ -1351,7 +1351,7 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
 }
 
 static int
-nvme_sysfs_pathinfo (struct path * pp, vector hwtable)
+nvme_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable)
 {
 	struct udev_device *parent;
 	const char *attr_path = NULL;
@@ -1396,7 +1396,7 @@ nvme_sysfs_pathinfo (struct path * pp, vector hwtable)
 }
 
 static int
-ccw_sysfs_pathinfo (struct path * pp, vector hwtable)
+ccw_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable)
 {
 	struct udev_device *parent;
 	char attr_buff[NAME_SIZE];
@@ -1455,7 +1455,7 @@ ccw_sysfs_pathinfo (struct path * pp, vector hwtable)
 }
 
 static int
-cciss_sysfs_pathinfo (struct path * pp, vector hwtable)
+cciss_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable)
 {
 	const char * attr_path = NULL;
 	struct udev_device *parent;
@@ -1609,8 +1609,8 @@ path_offline (struct path * pp)
 	return PATH_DOWN;
 }
 
-int
-sysfs_pathinfo(struct path * pp, vector hwtable)
+static int
+sysfs_pathinfo(struct path *pp, const struct _vector *hwtable)
 {
 	int r = common_sysfs_pathinfo(pp);
 
-- 
2.26.2

[PATCH 24/35] libmultipath: pathinfo(): improve a log message

[mwilck]

From: Martin Wilck <mwilck@suse.com>

"node" is too generic to be understood without checking the code.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index c692026..2e559d8 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2124,7 +2124,7 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 		pp->fd = open(udev_device_get_devnode(pp->udev), O_RDONLY);
 
 	if (pp->fd < 0) {
-		condlog(4, "Couldn't open node for %s: %s",
+		condlog(4, "Couldn't open device node for %s: %s",
 			pp->dev, strerror(errno));
 		goto blank;
 	}
-- 
2.26.2

[PATCH 25/35] libmultipath: pathinfo(): don't filter emtpy devnode names

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In the update_multipath_strings() code path (disassemble_map()),
we set an empty devname if we find devices in maps that are not
in pathvec and not in sysfs. We shouldn't filter these devices
by devnode, because failure in pathinfo() causes e.g. adopt_paths()
to bail out, and this may cause a map removal even if the map
contains other, perfectly valid paths.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 2e559d8..10b5a28 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2069,7 +2069,7 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 			return PATHINFO_SKIPPED;
 	}
 
-	if (filter_devnode(conf->blist_devnode,
+	if (strlen(pp->dev) != 0 && filter_devnode(conf->blist_devnode,
 			   conf->elist_devnode,
 			   pp->dev) > 0)
 		return PATHINFO_SKIPPED;
-- 
2.26.2

[PATCH 26/35] libmultipath: io_err_stat_handle_pathfail(): less error conditions

[mwilck]

From: Martin Wilck <mwilck@suse.com>

io_err_stat_pathfail() returns an error if marginal path checking is
disabled, and on various other conditions which aren't runtime errors.
Fix that. Also, check the validity of parameters when the configuration
is read, and not on every call.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/defaults.h    |  2 ++
 libmultipath/io_err_stat.c | 25 ++++++++-----------------
 libmultipath/propsel.c     |  6 ++++++
 3 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 01a501b..0574e8f 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -31,6 +31,8 @@
 #define DEFAULT_DEFERRED_REMOVE	DEFERRED_REMOVE_OFF
 #define DEFAULT_DELAY_CHECKS	NU_NO
 #define DEFAULT_ERR_CHECKS	NU_NO
+/* half of minimum value for marginal_path_err_sample_time */
+#define IOTIMEOUT_SEC		60
 #define DEFAULT_UEVENT_STACKSIZE 256
 #define DEFAULT_RETRIGGER_DELAY	10
 #define DEFAULT_RETRIGGER_TRIES	3
diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
index 1b9cd6c..58bc1dd 100644
--- a/libmultipath/io_err_stat.c
+++ b/libmultipath/io_err_stat.c
@@ -35,7 +35,6 @@
 #include "time-util.h"
 #include "io_err_stat.h"
 
-#define IOTIMEOUT_SEC			60
 #define TIMEOUT_NO_IO_NSEC		10000000 /*10ms = 10000000ns*/
 #define FLAKY_PATHFAIL_THRESHOLD	2
 #define CONCUR_NR_EVENT			32
@@ -301,30 +300,22 @@ int io_err_stat_handle_pathfail(struct path *path)
 	struct timespec curr_time;
 
 	if (uatomic_read(&io_err_thread_running) == 0)
-		return 1;
+		return 0;
 
 	if (path->io_err_disable_reinstate) {
 		io_err_stat_log(3, "%s: reinstate is already disabled",
 				path->dev);
-		return 1;
+		return 0;
 	}
 	if (path->io_err_pathfail_cnt < 0)
-		return 1;
+		return 0;
 
 	if (!path->mpp)
-		return 1;
-	if (path->mpp->marginal_path_double_failed_time <= 0 ||
-		path->mpp->marginal_path_err_sample_time <= 0 ||
-		path->mpp->marginal_path_err_recheck_gap_time <= 0 ||
-		path->mpp->marginal_path_err_rate_threshold < 0) {
-		io_err_stat_log(4, "%s: parameter not set", path->mpp->alias);
-		return 1;
-	}
-	if (path->mpp->marginal_path_err_sample_time < (2 * IOTIMEOUT_SEC)) {
-		io_err_stat_log(2, "%s: marginal_path_err_sample_time should not less than %d",
-				path->mpp->alias, 2 * IOTIMEOUT_SEC);
-		return 1;
-	}
+		return 0;
+
+	if (!marginal_path_check_enabled(path->mpp))
+		return 0;
+
 	/*
 	 * The test should only be started for paths that have failed
 	 * repeatedly in a certain time frame, so that we have reason
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d362beb..2233527 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -1066,6 +1066,12 @@ int select_marginal_path_err_sample_time(struct config *conf, struct multipath *
 	mp_set_conf(marginal_path_err_sample_time);
 	mp_set_default(marginal_path_err_sample_time, DEFAULT_ERR_CHECKS);
 out:
+	if (mp->marginal_path_err_sample_time > 0 &&
+	    mp->marginal_path_err_sample_time < 2 * IOTIMEOUT_SEC) {
+		condlog(2, "%s: configuration error: marginal_path_err_sample_time must be >= %d",
+			mp->alias, 2 * IOTIMEOUT_SEC);
+			mp->marginal_path_err_sample_time = 2 * IOTIMEOUT_SEC;
+	}
 	if (print_off_int_undef(buff, 12, mp->marginal_path_err_sample_time)
 	    != 0)
 		condlog(3, "%s: marginal_path_err_sample_time = %s %s",
-- 
2.26.2

[PATCH 27/35] libmultipath: improve libdm logging

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Currently no libdm messages are logged at verbosity 3 and lower,
not even fatal ones. That seems wrong. Rather, we should map
our log levels (2 ~ WARN, 3 ~ NOTICE) to those of libdm
(_LOG_WARN = 4, _LOG_NOTICE = 5). Tests show that the results
are quite satisfactory for different verbosity levels.

dm_log_init_verbose() doesn't need to be called, as it only
sets the log level for libdm's internal logging function which
we don't use.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index f597ff8..4096e9d 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -12,6 +12,7 @@
 #include <ctype.h>
 #include <unistd.h>
 #include <errno.h>
+#include <syslog.h>
 #include <sys/sysmacros.h>
 #include <linux/dm-ioctl.h>
 
@@ -65,13 +66,15 @@ __attribute__((format(printf, 4, 5))) static void
 dm_write_log (int level, const char *file, int line, const char *f, ...)
 {
 	va_list ap;
-	int thres;
 
-	if (level > 6)
-		level = 6;
+	/*
+	 * libdm uses the same log levels as syslog,
+	 * except that EMERG/ALERT are not used
+	 */
+	if (level > LOG_DEBUG)
+		level = LOG_DEBUG;
 
-	thres = dm_conf_verbosity;
-	if (thres <= 3 || level > thres)
+	if (level > dm_conf_verbosity)
 		return;
 
 	va_start(ap, f);
@@ -90,8 +93,9 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 		vfprintf(stderr, f, ap);
 		fprintf(stderr, "\n");
 	} else {
-		condlog(level, "libdevmapper: %s(%i): ", file, line);
-		log_safe(level + 3, f, ap);
+		condlog(level >= LOG_ERR ? level - LOG_ERR : 0,
+			"libdevmapper: %s(%i): ", file, line);
+		log_safe(level, f, ap);
 	}
 	va_end(ap);
 
@@ -100,9 +104,12 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 
 void dm_init(int v)
 {
-	dm_conf_verbosity = v;
+	/*
+	 * This maps libdm's standard loglevel _LOG_WARN (= 4), which is rather
+	 * quiet in practice, to multipathd's default verbosity 2
+	 */
+	dm_conf_verbosity = v + 2;
 	dm_log_init(&dm_write_log);
-	dm_log_init_verbose(v + 3);
 }
 
 static int
-- 
2.26.2

[PATCH 28/35] libmultipath: snprint_devices(): use udev_enumerate

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Rather than hand-coding the device enumeration, use udev as we do elsewhere,
too. While at it, improve the overflow detection.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/print.c | 75 ++++++++++++++++++++------------------------
 libmultipath/print.h |  2 +-
 2 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 298b376..fb94f86 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -2026,65 +2026,58 @@ int snprint_status(char *buff, int len, const struct vectors *vecs)
 	return fwd;
 }
 
-int snprint_devices(struct config *conf, char * buff, int len,
+int snprint_devices(struct config *conf, char *buff, size_t len,
 		    const struct vectors *vecs)
 {
-	DIR *blkdir;
-	struct dirent *blkdev;
-	struct stat statbuf;
-	char devpath[PATH_MAX];
-	int threshold = MAX_LINE_LEN;
-	int fwd = 0;
+	size_t fwd = 0;
 	int r;
+	struct udev_enumerate *enm;
+	struct udev_list_entry *item, *first;
 
 	struct path * pp;
 
-	if (!(blkdir = opendir("/sys/block")))
+	enm = udev_enumerate_new(udev);
+	if (!enm)
 		return 1;
+	udev_enumerate_add_match_subsystem(enm, "block");
 
-	if ((len - fwd - threshold) <= 0) {
-		closedir(blkdir);
-		return len;
-	}
 	fwd += snprintf(buff + fwd, len - fwd, "available block devices:\n");
+	r = udev_enumerate_scan_devices(enm);
+	if (r < 0)
+		goto out;
 
-	while ((blkdev = readdir(blkdir)) != NULL) {
-		if ((strcmp(blkdev->d_name,".") == 0) ||
-		    (strcmp(blkdev->d_name,"..") == 0))
-			continue;
-
-		if (safe_sprintf(devpath, "/sys/block/%s", blkdev->d_name))
-			continue;
-
-		if (stat(devpath, &statbuf) < 0)
-			continue;
+	first = udev_enumerate_get_list_entry(enm);
+	udev_list_entry_foreach(item, first) {
+		const char *path, *devname, *status;
+		struct udev_device *u_dev;
 
-		if (S_ISDIR(statbuf.st_mode) == 0)
-			continue;
+		path = udev_list_entry_get_name(item);
+		u_dev = udev_device_new_from_syspath(udev, path);
+		devname = udev_device_get_sysname(u_dev);
 
-		if ((len - fwd - threshold)  <= 0) {
-			closedir(blkdir);
-			return len;
-		}
+		fwd += snprintf(buff + fwd, len - fwd, "    %s", devname);
+		if (fwd >= len)
+			break;
 
-		fwd += snprintf(buff + fwd, len - fwd, "    %s",
-				blkdev->d_name);
-		pp = find_path_by_dev(vecs->pathvec, blkdev->d_name);
+		pp = find_path_by_dev(vecs->pathvec, devname);
 		if (!pp) {
 			r = filter_devnode(conf->blist_devnode,
-					   conf->elist_devnode, blkdev->d_name);
+					   conf->elist_devnode,
+					   devname);
 			if (r > 0)
-				fwd += snprintf(buff + fwd, len - fwd,
-						" devnode blacklisted, unmonitored");
-			else if (r <= 0)
-				fwd += snprintf(buff + fwd, len - fwd,
-						" devnode whitelisted, unmonitored");
+				status = "devnode blacklisted, unmonitored";
+			else
+				status = "devnode whitelisted, unmonitored";
 		} else
-			fwd += snprintf(buff + fwd, len - fwd,
-					" devnode whitelisted, monitored");
-		fwd += snprintf(buff + fwd, len - fwd, "\n");
+			status = " devnode whitelisted, monitored";
+
+		fwd += snprintf(buff + fwd, len - fwd, " %s\n", status);
+		udev_device_unref(u_dev);
+		if (fwd >= len)
+			break;
 	}
-	closedir(blkdir);
+out:
+	udev_enumerate_unref(enm);
 
 	if (fwd >= len)
 		return len;
diff --git a/libmultipath/print.h b/libmultipath/print.h
index e8260d0..0042cef 100644
--- a/libmultipath/print.h
+++ b/libmultipath/print.h
@@ -129,7 +129,7 @@ int snprint_multipath_map_json (char * buff, int len,
 int snprint_blacklist_report (struct config *, char *, int);
 int snprint_wildcards (char *, int);
 int snprint_status (char *, int, const struct vectors *);
-int snprint_devices (struct config *, char *, int, const struct vectors *);
+int snprint_devices (struct config *, char *, size_t, const struct vectors *);
 int snprint_path_serial (char *, size_t, const struct path *);
 int snprint_host_wwnn (char *, size_t, const struct path *);
 int snprint_host_wwpn (char *, size_t, const struct path *);
-- 
2.26.2

[PATCH 29/35] libmultipath: snprint_devices(): print hidden/foreign status

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The "hidden" and "claimed by foreign" properties take precedence
over blacklisting/whitelisting in pathinfo(). We should take
this into account when printing the device list.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/print.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index fb94f86..ea95d69 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -30,6 +30,7 @@
 #include "debug.h"
 #include "discovery.h"
 #include "util.h"
+#include "foreign.h"
 
 #define MAX(x,y) (((x) > (y)) ? (x) : (y))
 #define MIN(x,y) (((x) > (y)) ? (y) : (x))
@@ -2061,13 +2062,23 @@ int snprint_devices(struct config *conf, char *buff, size_t len,
 
 		pp = find_path_by_dev(vecs->pathvec, devname);
 		if (!pp) {
-			r = filter_devnode(conf->blist_devnode,
-					   conf->elist_devnode,
-					   devname);
-			if (r > 0)
-				status = "devnode blacklisted, unmonitored";
-			else
-				status = "devnode whitelisted, unmonitored";
+			const char *hidden;
+
+			hidden = udev_device_get_sysattr_value(u_dev,
+							       "hidden");
+			if (hidden && !strcmp(hidden, "1"))
+				status = "hidden, unmonitored";
+			else if (is_claimed_by_foreign(u_dev))
+				status = "foreign, monitored";
+			else {
+				r = filter_devnode(conf->blist_devnode,
+						   conf->elist_devnode,
+						   devname);
+				if (r > 0)
+					status = "devnode blacklisted, unmonitored";
+				else
+					status = "devnode whitelisted, unmonitored";
+			}
 		} else
 			status = " devnode whitelisted, monitored";
 
-- 
2.26.2

[PATCH 30/35] libmultipath: alloc_path(): initialize pp->initialized

[mwilck]

From: Martin Wilck <mwilck@suse.com>

INIT_NEW is 0, but being explicit is better here.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 2dd378c..9407462 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -92,6 +92,7 @@ alloc_path (void)
 	pp = (struct path *)MALLOC(sizeof(struct path));
 
 	if (pp) {
+		pp->initialized = INIT_NEW;
 		pp->sg_id.host_no = -1;
 		pp->sg_id.channel = -1;
 		pp->sg_id.scsi_id = -1;
-- 
2.26.2

[PATCH 31/35] libmultipath: alloc_path_with_pathinfo(): treat devname overflow as error

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This is to be consistent with store_path_with_pathinfo(). It's not strictly
necessary; we _could_ proceed in both functions even in the unlikely case
that the device name overflows, because we use pp->dev mainly for log
messages. However, a device node name that causes an overflow for
FILE_NAME_SIZE = 256 should be considered pathologic anyway.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 10b5a28..81a3fad 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -64,6 +64,7 @@ alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
 
 	if (safe_sprintf(pp->dev, "%s", devname)) {
 		condlog(0, "pp->dev too small");
+		err = 1;
 	} else {
 		pp->udev = udev_device_ref(udevice);
 		err = pathinfo(pp, conf, flag | DI_BLACKLIST);
-- 
2.26.2

[PATCH 32/35] libmultipath: log table params in addmap()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

We currently log the table parameters with "load table" in domap()
in the success case. But it's at least as interesting to get this
information when domap() fails. Log in addmap() instead at v2 level.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 2 +-
 libmultipath/devmapper.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index fe590f4..315eb6a 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -992,7 +992,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		} else  {
 			/* multipath daemon mode */
 			mpp->stat_map_loads++;
-			condlog(2, "%s: load table [0 %llu %s %s]", mpp->alias,
+			condlog(4, "%s: load table [0 %llu %s %s]", mpp->alias,
 				mpp->size, TGT_MPATH, params);
 			/*
 			 * Required action is over, reset for the stateful daemon.
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 4096e9d..a177a54 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -395,7 +395,7 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	if (mpp->attribute_flags & (1 << ATTR_GID) &&
 	    !dm_task_set_gid(dmt, mpp->gid))
 		goto freeout;
-	condlog(4, "%s: %s [0 %llu %s %s]", mpp->alias,
+	condlog(2, "%s: %s [0 %llu %s %s]", mpp->alias,
 		task == DM_DEVICE_RELOAD ? "reload" : "addmap", mpp->size,
 		target, params);
 
-- 
2.26.2

[PATCH 33/35] multipathd: remove set_multipath_wwid()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This function was only called from one place. Making the dm_get_uuid
call explicit there makes the code more obvious.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index f014d2a..40c050b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -400,15 +400,6 @@ remove_maps_and_stop_waiters(struct vectors *vecs)
 	remove_maps(vecs);
 }
 
-static void
-set_multipath_wwid (struct multipath * mpp)
-{
-	if (strlen(mpp->wwid))
-		return;
-
-	dm_get_uuid(mpp->alias, mpp->wwid, WWID_SIZE);
-}
-
 int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		      int reset)
 {
@@ -552,7 +543,10 @@ add_map_without_path (struct vectors *vecs, const char *alias)
 		condlog(3, "%s: cannot access table", mpp->alias);
 		goto out;
 	}
-	set_multipath_wwid(mpp);
+	if (!strlen(mpp->wwid))
+		dm_get_uuid(mpp->alias, mpp->wwid, WWID_SIZE);
+	if (!strlen(mpp->wwid))
+		condlog(1, "%s: adding map with empty WWID", mpp->alias);
 	conf = get_multipath_config();
 	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
 	put_multipath_config(conf);
-- 
2.26.2

[PATCH 34/35] kpartx: print an error message if removing a partition fails

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index c24ad6d..5f59e15 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -635,6 +635,8 @@ main(int argc, char **argv){
 
 				if (!dm_simplecmd(DM_DEVICE_REMOVE,
 						  partname, 1, 0)) {
+					fprintf(stderr, "failed to remove %s",
+						partname);
 					r++;
 					continue;
 				}
-- 
2.26.2

[PATCH 35/35] kpartx: add missing newline

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 86731ea..3efd6df 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -618,7 +618,7 @@ remove_partmap(const char *name, void *data)
 
 	if (dm_get_opencount(name)) {
 		if (rd->verbose)
-			printf("%s is in use. Not removing", name);
+			printf("%s is in use. Not removing\n", name);
 		return 1;
 	}
 	if (!dm_simplecmd(DM_DEVICE_REMOVE, name, 0, 0)) {
-- 
2.26.2

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> In e32d521d ("libmultipath: coalesce_paths: fix size mismatch handling"),
> we introduced simple bitmap handling functions. We can do better. This
> patch introduces a bitfield type with overflow detection and a
> find_first_set() method.
> 
> Use this in coalesce_paths(), and adapt the unit tests. Also, add
> unit tests for "odd" bitfield sizes; so far we tested only multiples
> of 64.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c |   9 +-
>  libmultipath/util.c      |  35 ++++++
>  libmultipath/util.h      |  57 ++++++++-
>  tests/util.c             | 263 +++++++++++++++++++++++++++++++++++----
>  4 files changed, 327 insertions(+), 37 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 96c7961..fe590f4 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1092,7 +1092,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  	vector pathvec = vecs->pathvec;
>  	struct config *conf;
>  	int allow_queueing;
> -	uint64_t *size_mismatch_seen;
> +	struct bitfield *size_mismatch_seen;
>  
>  	/* ignore refwwid if it's empty */
>  	if (refwwid && !strlen(refwwid))
> @@ -1106,8 +1106,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  
>  	if (VECTOR_SIZE(pathvec) == 0)
>  		return CP_OK;
> -	size_mismatch_seen = calloc((VECTOR_SIZE(pathvec) - 1) / 64 + 1,
> -				    sizeof(uint64_t));
> +	size_mismatch_seen = alloc_bitfield(VECTOR_SIZE(pathvec));
>  	if (size_mismatch_seen == NULL)
>  		return CP_FAIL;
>  
> @@ -1131,7 +1130,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  		}
>  
>  		/* 2. if path already coalesced, or seen and discarded */
> -		if (pp1->mpp || is_bit_set_in_array(k, size_mismatch_seen))
> +		if (pp1->mpp || is_bit_set_in_bitfield(k, size_mismatch_seen))
>  			continue;
>  
>  		/* 3. if path has disappeared */
> @@ -1183,7 +1182,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  					"Discard", pp2->dev, pp2->size,
>  					mpp->size);
>  				mpp->action = ACT_REJECT;
> -				set_bit_in_array(i, size_mismatch_seen);
> +				set_bit_in_bitfield(i, size_mismatch_seen);
>  			}
>  		}
>  		verify_paths(mpp, vecs);
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index 3c43f28..46cacd4 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -404,3 +404,38 @@ void close_fd(void *arg)
>  {
>  	close((long)arg);
>  }
> +
> +struct bitfield *alloc_bitfield(unsigned int maxbit)
> +{
> +	unsigned int n;
> +	struct bitfield *bf;
> +
> +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;

What's the point in accepting 0? That's an empty bitmap.

> +	bf = calloc(1, sizeof(struct bitfield) + n * sizeof(bitfield_t));

Need to check that bf got set before dereferencing it.

> +	bf->len = maxbit;
> +	return bf;
> +}
> +
> +void _log_bitfield_overflow(const char *f, unsigned int bit, unsigned int len)
> +{
> +	condlog(0, "%s: bitfield overflow: %u >= %u", f, bit, len);
> +}
> +
> +unsigned int find_first_set(const struct bitfield *bf)
> +{
> +	unsigned int b = 0, i, n;
> +
> +	for (i = 0; i < bf->len; i+= bits_per_slot) {
> +		b = _ffs(bf->bits[i / bits_per_slot]);
> +		if (b != 0)
> +			break;
> +	};
> +	if (b == 0)
> +		return 0;
> +
> +	n = i + b;
> +	if (n <= bf->len)
> +		return n;
> +
> +	return 0;
> +}

This is neat and all, but what's it for?  I didn't see it used in the
rest of the patches.  Did I miss it, or do you have a future use for it?

> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index df75c4f..ec6de6d 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -1,6 +1,9 @@
>  #ifndef _UTIL_H
>  #define _UTIL_H
>  
> +#include <stdlib.h>
> +#include <string.h>
> +#include <limits.h>
>  #include <sys/types.h>
>  /* for rlim_t */
>  #include <sys/resource.h>
> @@ -51,19 +54,61 @@ struct scandir_result {
>  };
>  void free_scandir_result(struct scandir_result *);
>  
> -static inline bool is_bit_set_in_array(unsigned int bit, const uint64_t *arr)
> +/*
> + * ffsll() is also available on glibc < 2.27 if _GNU_SOURCE is defined.
> + * But relying on that would require that every program using this header file
> + * set _GNU_SOURCE during compilation, because otherwise the library and the
> + * program would use different types for bitfield_t, causing errors.
> + * That's too error prone, so if in doubt, use ffs().
> + */
> +#if __GLIBC_PREREQ(2, 27)
> +typedef unsigned long long int bitfield_t;
> +#define _ffs(x) ffsll(x)
> +#else
> +typedef unsigned int bitfield_t;
> +#define _ffs(x) ffs(x)
> +#endif
> +#define bits_per_slot (sizeof(bitfield_t) * CHAR_BIT)
> +
> +struct bitfield {
> +	unsigned int len;
> +	bitfield_t bits[];
> +};
> +
> +struct bitfield *alloc_bitfield(unsigned int maxbit);
> +
> +void _log_bitfield_overflow(const char *f, unsigned int bit, unsigned int len);
> +#define log_bitfield_overflow(bit, len) \
> +	_log_bitfield_overflow(__func__, bit, len)
> +
> +static inline bool is_bit_set_in_bitfield(unsigned int bit,
> +				       const struct bitfield *bf)
>  {
> -	return arr[bit / 64] & (1ULL << (bit % 64)) ? 1 : 0;
> +	if (bit >= bf->len) {
> +		log_bitfield_overflow(bit, bf->len);
> +		return false;
> +	}
> +	return !!(bf->bits[bit / bits_per_slot] &
> +		  (1ULL << (bit % bits_per_slot)));
>  }
>  
> -static inline void set_bit_in_array(unsigned int bit, uint64_t *arr)
> +static inline void set_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
>  {
> -	arr[bit / 64] |= (1ULL << (bit % 64));
> +	if (bit >= bf->len) {
> +		log_bitfield_overflow(bit, bf->len);
> +		return;
> +	}
> +	bf->bits[bit / bits_per_slot] |= (1ULL << (bit % bits_per_slot));
>  }
>  
> -static inline void clear_bit_in_array(unsigned int bit, uint64_t *arr)
> +static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
>  {
> -	arr[bit / 64] &= ~(1ULL << (bit % 64));
> +	if (bit >= bf->len) {
> +		log_bitfield_overflow(bit, bf->len);
> +		return;
> +	}
> +	bf->bits[bit / bits_per_slot] &= ~(1ULL << (bit % bits_per_slot));
>  }
>  
> +unsigned int find_first_set(const struct bitfield *bf);
>  #endif /* _UTIL_H */
> diff --git a/tests/util.c b/tests/util.c
> index 6d12fda..db7c05f 100644
> --- a/tests/util.c
> +++ b/tests/util.c
> @@ -164,19 +164,25 @@ static int test_basenamecpy(void)
>  
>  static void test_bitmask_1(void **state)
>  {
> -	uint64_t arr[BITARR_SZ];
> +	struct bitfield *bf;
> +	uint64_t *arr;
>  	int i, j, k, m, b;
>  
> -	memset(arr, 0, sizeof(arr));
> +	bf = alloc_bitfield(BITARR_SZ * 64);
> +	assert_non_null(bf);
> +	assert_int_equal(bf->len, BITARR_SZ * 64);
> +	arr = (uint64_t *)bf->bits;
>  
>  	for (j = 0; j < BITARR_SZ; j++) {
>  		for (i = 0; i < 64; i++) {
>  			b = 64 * j + i;
> -			assert(!is_bit_set_in_array(b, arr));
> -			set_bit_in_array(b, arr);
> +			assert(!is_bit_set_in_bitfield(b, bf));
> +			set_bit_in_bitfield(b, bf);
>  			for (k = 0; k < BITARR_SZ; k++) {
> +#if 0
>  				printf("b = %d j = %d k = %d a = %"PRIx64"\n",
>  				       b, j, k, arr[k]);
> +#endif
>  				if (k == j)
>  					assert_int_equal(arr[j], 1ULL << i);
>  				else
> @@ -184,39 +190,46 @@ static void test_bitmask_1(void **state)
>  			}
>  			for (m = 0; m < 64; m++)
>  				if (i == m)
> -					assert(is_bit_set_in_array(64 * j + m,
> -								   arr));
> +					assert(is_bit_set_in_bitfield(64 * j + m,
> +								      bf));
>  				else
> -					assert(!is_bit_set_in_array(64 * j + m,
> -								    arr));
> -			clear_bit_in_array(b, arr);
> -			assert(!is_bit_set_in_array(b, arr));
> +					assert(!is_bit_set_in_bitfield(64 * j + m,
> +								       bf));
> +			assert_int_equal(find_first_set(bf), b + 1);
> +			clear_bit_in_bitfield(b, bf);
> +			assert(!is_bit_set_in_bitfield(b, bf));
>  			for (k = 0; k < BITARR_SZ; k++)
>  				assert_int_equal(arr[k], 0ULL);
>  		}
>  	}
> +	free(bf);
>  }
>  
>  static void test_bitmask_2(void **state)
>  {
> -	uint64_t arr[BITARR_SZ];
> +	struct bitfield *bf;
> +	uint64_t *arr;
>  	int i, j, k, m, b;
>  
> -	memset(arr, 0, sizeof(arr));
> +	bf = alloc_bitfield(BITARR_SZ * 64);
> +	assert_non_null(bf);
> +	assert_int_equal(bf->len, BITARR_SZ * 64);
> +	arr = (uint64_t *)bf->bits;
>  
>  	for (j = 0; j < BITARR_SZ; j++) {
>  		for (i = 0; i < 64; i++) {
>  			b = 64 * j + i;
> -			assert(!is_bit_set_in_array(b, arr));
> -			set_bit_in_array(b, arr);
> +			assert(!is_bit_set_in_bitfield(b, bf));
> +			set_bit_in_bitfield(b, bf);
>  			for (m = 0; m < 64; m++)
>  				if (m <= i)
> -					assert(is_bit_set_in_array(64 * j + m,
> -								   arr));
> +					assert(is_bit_set_in_bitfield(64 * j + m,
> +								      bf));
>  				else
> -					assert(!is_bit_set_in_array(64 * j + m,
> -								    arr));
> -			assert(is_bit_set_in_array(b, arr));
> +					assert(!is_bit_set_in_bitfield(64 * j + m,
> +								       bf));
> +			assert(is_bit_set_in_bitfield(b, bf));
> +			assert_int_equal(find_first_set(bf), 1);
>  			for (k = 0; k < BITARR_SZ; k++) {
>  				if (k < j || (k == j && i == 63))
>  					assert_int_equal(arr[k], ~0ULL);
> @@ -232,16 +245,20 @@ static void test_bitmask_2(void **state)
>  	for (j = 0; j < BITARR_SZ; j++) {
>  		for (i = 0; i < 64; i++) {
>  			b = 64 * j + i;
> -			assert(is_bit_set_in_array(b, arr));
> -			clear_bit_in_array(b, arr);
> +			assert(is_bit_set_in_bitfield(b, bf));
> +			clear_bit_in_bitfield(b, bf);
>  			for (m = 0; m < 64; m++)
>  				if (m <= i)
> -					assert(!is_bit_set_in_array(64 * j + m,
> -								    arr));
> +					assert(!is_bit_set_in_bitfield(64 * j + m,
> +								       bf));
>  				else
> -					assert(is_bit_set_in_array(64 * j + m,
> -								   arr));
> -			assert(!is_bit_set_in_array(b, arr));
> +					assert(is_bit_set_in_bitfield(64 * j + m,
> +								      bf));
> +			assert(!is_bit_set_in_bitfield(b, bf));
> +			if (b == 64 * BITARR_SZ - 1)
> +				assert_int_equal(find_first_set(bf), 0);
> +			else
> +				assert_int_equal(find_first_set(bf), b + 2);
>  			for (k = 0; k < BITARR_SZ; k++) {
>  				if (k < j || (k == j && i == 63))
>  					assert_int_equal(arr[k], 0ULL);
> @@ -254,13 +271,207 @@ static void test_bitmask_2(void **state)
>  			}
>  		}
>  	}
> +	free(bf);
>  }
>  
> +/*
> + *  Test operations on a 0-length bitfield
> + */
> +static void test_bitmask_len_0(void **state)
> +{
> +	struct bitfield *bf;
> +
> +	bf = alloc_bitfield(0);
> +	assert_non_null(bf);
> +	assert_int_equal(bf->len, 0);
> +	assert_int_equal(is_bit_set_in_bitfield(0, bf), 0);
> +	assert_int_equal(is_bit_set_in_bitfield(1, bf), 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +	set_bit_in_bitfield(0, bf);
> +	assert_int_equal(is_bit_set_in_bitfield(0, bf), 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +	clear_bit_in_bitfield(0, bf);
> +	assert_int_equal(is_bit_set_in_bitfield(0, bf), 0);
> +	set_bit_in_bitfield(11, bf);
> +	assert_int_equal(find_first_set(bf), 0);
> +	assert_int_equal(is_bit_set_in_bitfield(11, bf), 0);
> +	clear_bit_in_bitfield(11, bf);
> +	assert_int_equal(is_bit_set_in_bitfield(11, bf), 0);
> +	free(bf);
> +}
> +
> +static void _test_bitmask_small(unsigned int n)
> +{
> +	struct bitfield *bf;
> +	uint64_t *arr;
> +
> +	assert(n <= 64);
> +	assert(n >= 1);
> +
> +	bf = alloc_bitfield(n);
> +	assert_non_null(bf);
> +	assert_int_equal(bf->len, n);
> +	arr = (uint64_t *)bf->bits;
> +
> +	assert_int_equal(*arr, 0);
> +
> +	set_bit_in_bitfield(n + 1, bf);
> +	assert_int_equal(*arr, 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +
> +	set_bit_in_bitfield(n, bf);
> +	assert_int_equal(*arr, 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +
> +	set_bit_in_bitfield(n - 1, bf);
> +	assert_int_equal(*arr, 1ULL << (n - 1));
> +	assert_int_equal(find_first_set(bf), n);
> +
> +	clear_bit_in_bitfield(n - 1, bf);
> +	assert_int_equal(*arr, 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +
> +	set_bit_in_bitfield(0, bf);
> +	assert_int_equal(*arr, 1);
> +	assert_int_equal(find_first_set(bf), 1);
> +
> +	free(bf);
> +}
> +
> +static void _test_bitmask_small_2(unsigned int n)
> +{
> +	struct bitfield *bf;
> +	uint64_t *arr;
> +
> +	assert(n <= 128);
> +	assert(n >= 65);
> +
> +	bf = alloc_bitfield(n);
> +	assert_non_null(bf);
> +	assert_int_equal(bf->len, n);
> +	arr = (uint64_t *)bf->bits;
> +
> +	assert_int_equal(arr[0], 0);
> +	assert_int_equal(arr[1], 0);
> +
> +	set_bit_in_bitfield(n + 1, bf);
> +	assert_int_equal(arr[0], 0);
> +	assert_int_equal(arr[1], 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +
> +	set_bit_in_bitfield(n, bf);
> +	assert_int_equal(arr[0], 0);
> +	assert_int_equal(arr[1], 0);
> +	assert_int_equal(find_first_set(bf), 0);
> +
> +	set_bit_in_bitfield(n - 1, bf);
> +	assert_int_equal(arr[0], 0);
> +	assert_int_equal(arr[1], 1ULL << (n - 65));
> +	assert_int_equal(find_first_set(bf), n);
> +
> +	set_bit_in_bitfield(0, bf);
> +	assert_int_equal(arr[0], 1);
> +	assert_int_equal(arr[1], 1ULL << (n - 65));
> +	assert_int_equal(find_first_set(bf), 1);
> +
> +	set_bit_in_bitfield(64, bf);
> +	assert_int_equal(arr[0], 1);
> +	assert_int_equal(arr[1], (1ULL << (n - 65)) | 1);
> +	assert_int_equal(find_first_set(bf), 1);
> +
> +	clear_bit_in_bitfield(0, bf);
> +	assert_int_equal(arr[0], 0);
> +	assert_int_equal(arr[1], (1ULL << (n - 65)) | 1);
> +	assert_int_equal(find_first_set(bf), 65);
> +
> +	free(bf);
> +}
> +
> +static void test_bitmask_len_1(void **state)
> +{
> +	_test_bitmask_small(1);
> +}
> +
> +static void test_bitmask_len_2(void **state)
> +{
> +	_test_bitmask_small(2);
> +}
> +
> +static void test_bitmask_len_3(void **state)
> +{
> +	_test_bitmask_small(3);
> +}
> +
> +static void test_bitmask_len_23(void **state)
> +{
> +	_test_bitmask_small(23);
> +}
> +
> +static void test_bitmask_len_63(void **state)
> +{
> +	_test_bitmask_small(63);
> +}
> +
> +static void test_bitmask_len_64(void **state)
> +{
> +	_test_bitmask_small(63);
> +}
> +
> +static void test_bitmask_len_65(void **state)
> +{
> +	_test_bitmask_small_2(65);
> +}
> +
> +static void test_bitmask_len_66(void **state)
> +{
> +	_test_bitmask_small_2(66);
> +}
> +
> +static void test_bitmask_len_67(void **state)
> +{
> +	_test_bitmask_small_2(67);
> +}
> +
> +static void test_bitmask_len_103(void **state)
> +{
> +	_test_bitmask_small_2(103);
> +}
> +
> +static void test_bitmask_len_126(void **state)
> +{
> +	_test_bitmask_small_2(126);
> +}
> +
> +static void test_bitmask_len_127(void **state)
> +{
> +	_test_bitmask_small_2(127);
> +}
> +
> +static void test_bitmask_len_128(void **state)
> +{
> +	_test_bitmask_small_2(128);
> +}
> +
> +
>  static int test_bitmasks(void)
>  {
>  	const struct CMUnitTest tests[] = {
>  		cmocka_unit_test(test_bitmask_1),
>  		cmocka_unit_test(test_bitmask_2),
> +		cmocka_unit_test(test_bitmask_len_0),
> +		cmocka_unit_test(test_bitmask_len_1),
> +		cmocka_unit_test(test_bitmask_len_2),
> +		cmocka_unit_test(test_bitmask_len_3),
> +		cmocka_unit_test(test_bitmask_len_23),
> +		cmocka_unit_test(test_bitmask_len_63),
> +		cmocka_unit_test(test_bitmask_len_64),
> +		cmocka_unit_test(test_bitmask_len_65),
> +		cmocka_unit_test(test_bitmask_len_66),
> +		cmocka_unit_test(test_bitmask_len_67),
> +		cmocka_unit_test(test_bitmask_len_103),
> +		cmocka_unit_test(test_bitmask_len_126),
> +		cmocka_unit_test(test_bitmask_len_127),
> +		cmocka_unit_test(test_bitmask_len_128),
>  	};
>  	return cmocka_run_group_tests(tests, NULL, NULL);
>  }
> -- 
> 2.26.2

Re: [PATCH 12/35] libmultipath: strlcpy()/strlcat(): use restrict qualifier

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:15:57PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Also remove the redundant local variables. It's not necessary to
> make "restrict" work, but it makes the intention more clear.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/util.c | 28 ++++++++++++----------------
>  libmultipath/util.h |  4 ++--
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index 957fb97..f965094 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -113,46 +113,42 @@ get_word (const char *sentence, char **word)
>  	return skip + len;
>  }
>  
> -size_t strlcpy(char *dst, const char *src, size_t size)
> +size_t strlcpy(char * restrict dst, const char * restrict src, size_t size)
>  {
>  	size_t bytes = 0;
> -	char *q = dst;
> -	const char *p = src;
>  	char ch;
>  
> -	while ((ch = *p++)) {
> -		if (bytes+1 < size)
> -			*q++ = ch;
> +	while ((ch = *src++)) {
> +		if (bytes + 1 < size)
> +			*dst++ = ch;
>  		bytes++;
>  	}
>  
>  	/* If size == 0 there is no space for a final null... */
>  	if (size)
> -		*q = '\0';
> +		*dst = '\0';
>  	return bytes;
>  }
>  
> -size_t strlcat(char *dst, const char *src, size_t size)
> +size_t strlcat(char * restrict dst, const char * restrict src, size_t size)
>  {
>  	size_t bytes = 0;
> -	char *q = dst;
> -	const char *p = src;
>  	char ch;
>  
> -	while (bytes < size && *q) {
> -		q++;
> +	while (bytes < size && *dst) {
> +		dst++;
>  		bytes++;
>  	}
>  	if (bytes == size)

this should return the strlen(dst) + strlen(src). It wouldn't in the
admittedly weird case where size isn't large enough to fit dst.

>  		return (bytes + strlen(src));
>  
> -	while ((ch = *p++)) {
> -		if (bytes+1 < size)
> -		*q++ = ch;
> +	while ((ch = *src++)) {
> +		if (bytes + 1 < size)
> +			*dst++ = ch;
>  		bytes++;
>  	}
>  
> -	*q = '\0';
> +	*dst = '\0';
>  	return bytes;
>  }
>  
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index ae18d8b..7e29b26 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -15,8 +15,8 @@ int basenamecpy (const char *src, char *dst, size_t size);
>  int filepresent (const char *run);
>  char *get_next_string(char **temp, const char *split_char);
>  int get_word (const char * sentence, char ** word);
> -size_t strlcpy(char *dst, const char *src, size_t size);
> -size_t strlcat(char *dst, const char *src, size_t size);
> +size_t strlcpy(char * restrict dst, const char * restrict src, size_t size);
> +size_t strlcat(char * restrict dst, const char * restrict src, size_t size);
>  int devt2devname (char *, int, const char *);
>  dev_t parse_devt(const char *dev_t);
>  char *convert_dev(char *dev, int is_path_device);
> -- 
> 2.26.2

Re: [PATCH 00/35] multipath-tools series part I: minor changes

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:15:45PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe, hi Ben,
> 
> This is part I of a larger patch series for multpath-tools I've been preparing.
> It contains self-contained fixes and cleanups, and unit test additions.
> 
> The full series will also be available here:
> https://github.com/mwilck/multipath-tools/tree/ups/submit-2007
> 
> There are tags in that repo for each part of the series.
> This part is tagged "submit-200709-1".

For the part, with the exception of patches 8 & 12
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> It's based on 0.8.4, plus the following set of previously
> submitted and reviewed patches:
> 
>  - libmultipath: add device to hwtable.c (Steve Schremmer)
>  - [PATCH v3 0/7] Fix muitpath/multipathd flush issue (v3 7-part series, Ben)
>  - [PATCH v2 0/4] misc patches (v2 4-part series, Ben)
>  - multipath: Fix compiler warnings when built without systemd. (Marius Bakke)
>  - [PATCH v2 0/3] multipath: change default devnode blacklist
>    (v2 3-part series, Ben)
>  - multipath: add "-e" option to enable foreign libraries (me)
>  - libmultipath: set "enable_foreign" to NONE by default (me)
>  - libmultipath: fix condlog NULL argument in uevent_get_env_var (Ben)
>  - fix boolean value with json-c 0.14 (Christian Hesse) 
>  - [PATCH v3 0/6] multipath: path validation library prep work
>    (v3 6-part series, me)
>  - [PATCH 0/2] More minor libmultipath fixes (2-part series, me)
>  - [PATCH 00/11] Minor fixes for multipath-tools (11-part series, me)
>  - libmpathpersist: depend on libmultipath (Christian Hesse)
>  - [PATCH v2 0/2] multipath-tools: fixes for kpartx.rules and skip_kpartx
>    (v2 2-part series, me)
>  - libmultipath: allow force reload with no active paths (Ben)
>  - libmutipath: don't close fd on dm_lib_release (Ben)
>  - libmultipath: assign variable to make gcc happy (Ben)
>  - [PATCH v2 0/4] libmpathpersist allocation size fixes (v2 4-part series, me)
> 
> You can find a full tree of the code this is based on here:
> https://github.com/openSUSE/multipath-tools/tree/upstream-queue
> 
> Regards, Martin
> 
> 
> Martin Wilck (35):
>   multipath-tools tests/util: separate group for bitmask tests
>   multipath-tools tests/directio: fix missing initializers
>   tests: __wrap_dlog: use check_expected()
>   multipath tools tests: add strchop() test
>   libmultipath: improve strchop()
>   multipath-tools tests: add test for devt2devname
>   libmultipath: devt2devname(): simplify using libudev
>   libmultipath: create bitfield abstraction
>   libmultipath: use bitfields in group_by_match()
>   libmultipath: util: constify function arguments
>   multipath-tools tests: add unit tests for strlcat
>   libmultipath: strlcpy()/strlcat(): use restrict qualifier
>   libmultipath: constify blacklist code
>   libmultipath: rlookup_binding(): remove newline in log message
>   libmultipath: fix missing initializer warning from clang 3.9
>   libmultipath: fix gcc -Wstringop-overflow warning
>   libmultipath: remove uevent listener failback
>   libmultipath: uevent: use static linkage where possible
>   libmultipath: uevent: inline trivial functions
>   libmultipath: decrease log level of "SCSI target" log message
>   libmultipath: get_udev_uid(): more appropriate error code
>   libmultipath: get_uid(): improve log message on udev failure
>   libmultipath: make sysfs_pathinfo() static and use const
>   libmultipath: pathinfo(): improve a log message
>   libmultipath: pathinfo(): don't filter emtpy devnode names
>   libmultipath: io_err_stat_handle_pathfail(): less error conditions
>   libmultipath: improve libdm logging
>   libmultipath: snprint_devices(): use udev_enumerate
>   libmultipath: snprint_devices(): print hidden/foreign status
>   libmultipath: alloc_path(): initialize pp->initialized
>   libmultipath: alloc_path_with_pathinfo(): treat devname overflow as
>     error
>   libmultipath: log table params in addmap()
>   multipathd: remove set_multipath_wwid()
>   kpartx: print an error message if removing a partition fails
>   kpartx: add missing newline
> 
>  kpartx/devmapper.c               |   2 +-
>  kpartx/kpartx.c                  |   2 +
>  libmultipath/alias.c             |   2 +-
>  libmultipath/blacklist.c         |  34 +-
>  libmultipath/blacklist.h         |  17 +-
>  libmultipath/checkers/directio.c |   2 +-
>  libmultipath/configure.c         |  11 +-
>  libmultipath/defaults.h          |   2 +
>  libmultipath/devmapper.c         |  27 +-
>  libmultipath/discovery.c         |  30 +-
>  libmultipath/dmparser.c          |   2 +-
>  libmultipath/io_err_stat.c       |  25 +-
>  libmultipath/parser.c            |   2 +-
>  libmultipath/pgpolicies.c        |  12 +-
>  libmultipath/print.c             |  90 ++---
>  libmultipath/print.h             |   2 +-
>  libmultipath/propsel.c           |   6 +
>  libmultipath/structs.c           |   1 +
>  libmultipath/uevent.c            | 324 ++---------------
>  libmultipath/uevent.h            |  47 ++-
>  libmultipath/util.c              | 168 ++++-----
>  libmultipath/util.h              |  73 +++-
>  multipathd/main.c                |  14 +-
>  tests/Makefile                   |   3 +-
>  tests/alias.c                    |   4 +-
>  tests/devt.c                     | 192 ++++++++++
>  tests/directio.c                 |  28 +-
>  tests/test-log.c                 |   5 +-
>  tests/uevent.c                   |   4 -
>  tests/util.c                     | 586 ++++++++++++++++++++++++++++---
>  30 files changed, 1081 insertions(+), 636 deletions(-)
>  create mode 100644 tests/devt.c
> 
> -- 
> 2.26.2

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Martin Wilck]

On Thu, 2020-07-16 at 16:17 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > In e32d521d ("libmultipath: coalesce_paths: fix size mismatch
> > handling"),
> > we introduced simple bitmap handling functions. We can do better.
> > This
> > patch introduces a bitfield type with overflow detection and a
> > find_first_set() method.
> > 
> > Use this in coalesce_paths(), and adapt the unit tests. Also, add
> > unit tests for "odd" bitfield sizes; so far we tested only
> > multiples
> > of 64.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/configure.c |   9 +-
> >  libmultipath/util.c      |  35 ++++++
> >  libmultipath/util.h      |  57 ++++++++-
> >  tests/util.c             | 263
> > +++++++++++++++++++++++++++++++++++----
> >  4 files changed, 327 insertions(+), 37 deletions(-)
> > 
> > ...
> > diff --git a/libmultipath/util.c b/libmultipath/util.c
> > index 3c43f28..46cacd4 100644
> > --- a/libmultipath/util.c
> > +++ b/libmultipath/util.c
> > @@ -404,3 +404,38 @@ void close_fd(void *arg)
> >  {
> >  	close((long)arg);
> >  }
> > +
> > +struct bitfield *alloc_bitfield(unsigned int maxbit)
> > +{
> > +	unsigned int n;
> > +	struct bitfield *bf;
> > +
> > +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
> 
> What's the point in accepting 0? That's an empty bitmap.
> 
> > +	bf = calloc(1, sizeof(struct bitfield) + n *
> > sizeof(bitfield_t));
> 
> Need to check that bf got set before dereferencing it.

Thanks for spotting these, I will fix them.

> 
> > +	bf->len = maxbit;
> > +	return bf;
> > +}
> > +
> > +void _log_bitfield_overflow(const char *f, unsigned int bit,
> > unsigned int len)
> > +{
> > +	condlog(0, "%s: bitfield overflow: %u >= %u", f, bit, len);
> > +}
> > +
> > +unsigned int find_first_set(const struct bitfield *bf)
> > +{
> > +	unsigned int b = 0, i, n;
> > +
> > +	for (i = 0; i < bf->len; i+= bits_per_slot) {
> > +		b = _ffs(bf->bits[i / bits_per_slot]);
> > +		if (b != 0)
> > +			break;
> > +	};
> > +	if (b == 0)
> > +		return 0;
> > +
> > +	n = i + b;
> > +	if (n <= bf->len)
> > +		return n;
> > +
> > +	return 0;
> > +}
> 
> This is neat and all, but what's it for?  I didn't see it used in the
> rest of the patches.  Did I miss it, or do you have a future use for
> it?

Got me :-) I first thought we had a use for it. I can rip it out if you
prefer, saving a local copy in case we need it in the future. 

Martin

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Martin Wilck]

On Tue, 2020-08-04 at 17:04 +0200, Martin Wilck wrote:
> On Thu, 2020-07-16 at 16:17 -0500, Benjamin Marzinski wrote:
> > On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > In e32d521d ("libmultipath: coalesce_paths: fix size mismatch
> > > handling"),
> > > we introduced simple bitmap handling functions. We can do better.
> > > This
> > > patch introduces a bitfield type with overflow detection and a
> > > find_first_set() method.
> > > 
> > > Use this in coalesce_paths(), and adapt the unit tests. Also, add
> > > unit tests for "odd" bitfield sizes; so far we tested only
> > > multiples
> > > of 64.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/configure.c |   9 +-
> > >  libmultipath/util.c      |  35 ++++++
> > >  libmultipath/util.h      |  57 ++++++++-
> > >  tests/util.c             | 263
> > > +++++++++++++++++++++++++++++++++++----
> > >  4 files changed, 327 insertions(+), 37 deletions(-)
> > > 
> > > ...
> > > diff --git a/libmultipath/util.c b/libmultipath/util.c
> > > index 3c43f28..46cacd4 100644
> > > --- a/libmultipath/util.c
> > > +++ b/libmultipath/util.c
> > > @@ -404,3 +404,38 @@ void close_fd(void *arg)
> > >  {
> > >  	close((long)arg);
> > >  }
> > > +
> > > +struct bitfield *alloc_bitfield(unsigned int maxbit)
> > > +{
> > > +	unsigned int n;
> > > +	struct bitfield *bf;
> > > +
> > > +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
> > 
> > What's the point in accepting 0? That's an empty bitmap.
> > 
> Thanks for spotting these, I will fix them.

Thinking about it once more, I believe that accepting 0 as the bitfield
length is actually the right thing. A bitfield of length 0 makes not
much less sense than one of length 1. The code makes sure that the bit
operations on the 0-length bitfield behave correctly (see
test_bitmask_len_0()). Thus callers can use bitfields without bothering
for extra NULL checks. That was the intention. Like we support 0-length 
vectors.

Regards,
Martin

Re: [PATCH 12/35] libmultipath: strlcpy()/strlcat(): use restrict qualifier

[Martin Wilck]

On Thu, 2020-07-16 at 17:18 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:15:57PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Also remove the redundant local variables. It's not necessary to
> > make "restrict" work, but it makes the intention more clear.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/util.c | 28 ++++++++++++----------------
> >  libmultipath/util.h |  4 ++--
> >  2 files changed, 14 insertions(+), 18 deletions(-)
> > 
> > diff --git a/libmultipath/util.c b/libmultipath/util.c
> > index 957fb97..f965094 100644
> > --- a/libmultipath/util.c
> > +++ b/libmultipath/util.c
> > 
> > -size_t strlcat(char *dst, const char *src, size_t size)
> > +size_t strlcat(char * restrict dst, const char * restrict src,
> > size_t size)
> >  {
> >  	size_t bytes = 0;
> > -	char *q = dst;
> > -	const char *p = src;
> >  	char ch;
> >  
> > -	while (bytes < size && *q) {
> > -		q++;
> > +	while (bytes < size && *dst) {
> > +		dst++;
> >  		bytes++;
> >  	}
> >  	if (bytes == size)
> 
> this should return the strlen(dst) + strlen(src). It wouldn't in the
> admittedly weird case where size isn't large enough to fit dst.

Are you sure?

https://linux.die.net/man/3/strlcat

"Note, however, that if strlcat() traverses size characters without
finding a NUL, the length of the string is considered to be size and
the destination string will not be NUL-terminated (since there was no
space for the NUL)."

The way I understand this is that the current code is actually correct
in returning bytes + strlen(src).

This is also consistent with what I see elsewhere

https://github.com/ffainelli/uClibc/blob/master/libc/string/strlcat.c
https://github.com/freebsd/freebsd/blob/master/crypto/heimdal/lib/roken/strlcat.c

Regards
Martin

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Benjamin Marzinski]

On Tue, Aug 04, 2020 at 05:18:18PM +0200, Martin Wilck wrote:
> On Tue, 2020-08-04 at 17:04 +0200, Martin Wilck wrote:
> > On Thu, 2020-07-16 at 16:17 -0500, Benjamin Marzinski wrote:
> > > On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com wrote:
> > > > From: Martin Wilck <mwilck@suse.com>
> > > > +struct bitfield *alloc_bitfield(unsigned int maxbit)
> > > > +{
> > > > +	unsigned int n;
> > > > +	struct bitfield *bf;
> > > > +
> > > > +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
> > > 
> > > What's the point in accepting 0? That's an empty bitmap.
> > > 
> > Thanks for spotting these, I will fix them.
> 
> Thinking about it once more, I believe that accepting 0 as the bitfield
> length is actually the right thing. A bitfield of length 0 makes not
> much less sense than one of length 1. The code makes sure that the bit
> operations on the 0-length bitfield behave correctly (see
> test_bitmask_len_0()). Thus callers can use bitfields without bothering
> for extra NULL checks. That was the intention. Like we support 0-length 
> vectors.

But the calloc call itself can return NULL, so deferencing bf (as in
bf->len = maxbit), can crash.

I'm also still fuzzy on why we want to support zero length bitfields.
Since they can't be grown like vectors can, it seem like requesting a
zero length bitfield will always be a sign of a coding error. We
would get a more useful error by having the failure happen closer to
the error in the code.  Or is there actually a use for a zero length
bitfield that can't be grown?

-Ben

> Regards,
> Martin
> 

Re: [PATCH 12/35] libmultipath: strlcpy()/strlcat(): use restrict qualifier

[Benjamin Marzinski]

On Tue, Aug 04, 2020 at 05:36:31PM +0200, Martin Wilck wrote:
> On Thu, 2020-07-16 at 17:18 -0500, Benjamin Marzinski wrote:
> > On Thu, Jul 09, 2020 at 12:15:57PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > Also remove the redundant local variables. It's not necessary to
> > > make "restrict" work, but it makes the intention more clear.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/util.c | 28 ++++++++++++----------------
> > >  libmultipath/util.h |  4 ++--
> > >  2 files changed, 14 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/libmultipath/util.c b/libmultipath/util.c
> > > index 957fb97..f965094 100644
> > > --- a/libmultipath/util.c
> > > +++ b/libmultipath/util.c
> > > 
> > > -size_t strlcat(char *dst, const char *src, size_t size)
> > > +size_t strlcat(char * restrict dst, const char * restrict src,
> > > size_t size)
> > >  {
> > >  	size_t bytes = 0;
> > > -	char *q = dst;
> > > -	const char *p = src;
> > >  	char ch;
> > >  
> > > -	while (bytes < size && *q) {
> > > -		q++;
> > > +	while (bytes < size && *dst) {
> > > +		dst++;
> > >  		bytes++;
> > >  	}
> > >  	if (bytes == size)
> > 
> > this should return the strlen(dst) + strlen(src). It wouldn't in the
> > admittedly weird case where size isn't large enough to fit dst.
> 
> Are you sure?
> 

Nope. But I might be right.

> https://linux.die.net/man/3/strlcat
> 
> "Note, however, that if strlcat() traverses size characters without
> finding a NUL, the length of the string is considered to be size and
> the destination string will not be NUL-terminated (since there was no
> space for the NUL)."
> 
> The way I understand this is that the current code is actually correct
> in returning bytes + strlen(src).
> 
> This is also consistent with what I see elsewhere
> 
> https://github.com/ffainelli/uClibc/blob/master/libc/string/strlcat.c

This returns bytes + strlen(src) like you think is correct

> https://github.com/freebsd/freebsd/blob/master/crypto/heimdal/lib/roken/strlcat.c

This returns strlen(dst) + strlen(src) like I think is correct


I would argue that the important lines from https://linux.die.net/man/3/strlcat
are

"The strlcpy() and strlcat() functions return the total length of the
string they tried to create."

and

"For strlcat() that means the initial length of dst plus the length of
src."


The alternative to strlen(dst) + strlen(src) (which follows the snprintf
convention, and I think makes the most sense) seems to be "the length of
the string is considered to be size" which I assume means it should
return bytes. According to the man page, the reason for this is to keep
"strlcat() from running off the end of a string". I admit to being kinda
confused about this. I suppose if you assume that you can't trust the
strings enough to run strlen() this makes sense.  But if you can't trust
strlen(dst), you shouldn't be able to trust strlen(src) either, which
means that strlcat() should never return more than bytes, and that is
clearly at odds with other statements in the man page.

In my mind, there is value in returning strlen(dst) + strlen(src), since
that is the size needed to hold the strings.  Returning bytes means you can
write strlcat to avoid trusting strlen(). bytes + strlen(src) is a
meaningless value, and getting that value doesn't protect you against
src not being null terminated.

Clearly this is a nitpicky corner case, and I don't think we need
protection against src not being null terminated, so I'm not strongly
against bytes + strlen(src), if you prefer that.

-Ben

> Regards
> Martin
> 

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Martin Wilck]

On Tue, 2020-08-04 at 11:26 -0500, Benjamin Marzinski wrote:
> On Tue, Aug 04, 2020 at 05:18:18PM +0200, Martin Wilck wrote:
> > On Tue, 2020-08-04 at 17:04 +0200, Martin Wilck wrote:
> > > On Thu, 2020-07-16 at 16:17 -0500, Benjamin Marzinski wrote:
> > > > On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com
> > > > wrote:
> > > > > From: Martin Wilck <mwilck@suse.com>
> > > > > +struct bitfield *alloc_bitfield(unsigned int maxbit)
> > > > > +{
> > > > > +	unsigned int n;
> > > > > +	struct bitfield *bf;
> > > > > +
> > > > > +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
> > > > 
> > > > What's the point in accepting 0? That's an empty bitmap.
> > > > 
> > > Thanks for spotting these, I will fix them.
> > 
> > Thinking about it once more, I believe that accepting 0 as the
> > bitfield
> > length is actually the right thing. A bitfield of length 0 makes
> > not
> > much less sense than one of length 1. The code makes sure that the
> > bit
> > operations on the 0-length bitfield behave correctly (see
> > test_bitmask_len_0()). Thus callers can use bitfields without
> > bothering
> > for extra NULL checks. That was the intention. Like we support 0-
> > length 
> > vectors.
> 
> But the calloc call itself can return NULL, so deferencing bf (as in
> bf->len = maxbit), can crash.

Of course, I wasn't arguing about that. I'm going to resend with this
fixed.

> I'm also still fuzzy on why we want to support zero length bitfields.
> Since they can't be grown like vectors can, it seem like requesting a
> zero length bitfield will always be a sign of a coding error. We
> would get a more useful error by having the failure happen closer to
> the error in the code.  Or is there actually a use for a zero length
> bitfield that can't be grown?

The only "use" would be to be able to work with bitmaps in situations
where zero elements are possible, without the need to catch another
exception. 0-element bitfields are technically possible although
logically they make no sense. That's robust design for a library
function, IMO. I don't see why it'd be "better" to return NULL instead.
If we are after errors in callers, we might want to print an error
message and still not return NULL.

Martin

Re: [PATCH 12/35] libmultipath: strlcpy()/strlcat(): use restrict qualifier

[Martin Wilck]

On Tue, 2020-08-04 at 12:29 -0500, Benjamin Marzinski wrote:
> On Tue, Aug 04, 2020 at 05:36:31PM +0200, Martin Wilck wrote:
> > On Thu, 2020-07-16 at 17:18 -0500, Benjamin Marzinski wrote:
> > > On Thu, Jul 09, 2020 at 12:15:57PM +0200, mwilck@suse.com wrote:
> > > > From: Martin Wilck <mwilck@suse.com>
> > > > 
> > > > Also remove the redundant local variables. It's not necessary
> > > > to
> > > > make "restrict" work, but it makes the intention more clear.
> > > > 
> > > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > > ---
> > > >  libmultipath/util.c | 28 ++++++++++++----------------
> > > >  libmultipath/util.h |  4 ++--
> > > >  2 files changed, 14 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/libmultipath/util.c b/libmultipath/util.c
> > > > index 957fb97..f965094 100644
> > > > --- a/libmultipath/util.c
> > > > +++ b/libmultipath/util.c
> > > > 
> > > > -size_t strlcat(char *dst, const char *src, size_t size)
> > > > +size_t strlcat(char * restrict dst, const char * restrict src,
> > > > size_t size)
> > > >  {
> > > >  	size_t bytes = 0;
> > > > -	char *q = dst;
> > > > -	const char *p = src;
> > > >  	char ch;
> > > >  
> > > > -	while (bytes < size && *q) {
> > > > -		q++;
> > > > +	while (bytes < size && *dst) {
> > > > +		dst++;
> > > >  		bytes++;
> > > >  	}
> > > >  	if (bytes == size)
> > > 
> > > this should return the strlen(dst) + strlen(src). It wouldn't in
> > > the
> > > admittedly weird case where size isn't large enough to fit dst.
> > 
> > Are you sure?
> > 
> 
> Nope. But I might be right.
> 
> > https://linux.die.net/man/3/strlcat
> > 
> > "Note, however, that if strlcat() traverses size characters without
> > finding a NUL, the length of the string is considered to be size
> > and
> > the destination string will not be NUL-terminated (since there was
> > no
> > space for the NUL)."
> > 
> > The way I understand this is that the current code is actually
> > correct
> > in returning bytes + strlen(src).
> > 
> > This is also consistent with what I see elsewhere
> > 
> > https://github.com/ffainelli/uClibc/blob/master/libc/string/strlcat.c
> 
> This returns bytes + strlen(src) like you think is correct
> 
> > https://github.com/freebsd/freebsd/blob/master/crypto/heimdal/lib/roken/strlcat.c
> 
> This returns strlen(dst) + strlen(src) like I think is correct

... only if strnlen() is unavailable. Otherwise it returns
dst_sz + strlen(src) (= bytes + strlen(src)), because len never exceeds
dst_sz.

This one:
https://opensource.apple.com/source/Libc/Libc-1244.30.3/string/strlcat.c.auto.html
also behaves like the current code (using strnlen()).

To be fair, https://www.sudo.ws/todd/papers/strlcpy.html
says that strlcat should return "the number of bytes needed to store
the entire string".

On https://elixir.bootlin.com/linux/latest/source/lib/string.c#L325
we see that the kernel would actually crash in the corner case we're
discussing.

> I would argue that the important lines from 
> https://linux.die.net/man/3/strlcat
> are
> 
> "The strlcpy() and strlcat() functions return the total length of the
> string they tried to create."
> 
> and
> 
> "For strlcat() that means the initial length of dst plus the length
> of
> src."
> 
> 
> The alternative to strlen(dst) + strlen(src) (which follows the
> snprintf
> convention, and I think makes the most sense) seems to be "the length
> of
> the string is considered to be size" which I assume means it should
> return bytes. According to the man page, the reason for this is to
> keep
> "strlcat() from running off the end of a string". I admit to being
> kinda
> confused about this. I suppose if you assume that you can't trust the
> strings enough to run strlen() this makes sense.  But if you can't
> trust
> strlen(dst), you shouldn't be able to trust strlen(src) either, which
> means that strlcat() should never return more than bytes, and that is
> clearly at odds with other statements in the man page.
> 
> In my mind, there is value in returning strlen(dst) + strlen(src),
> since
> that is the size needed to hold the strings.  Returning bytes means
> you can
> write strlcat to avoid trusting strlen(). bytes + strlen(src) is a
> meaningless value, and getting that value doesn't protect you against
> src not being null terminated.

I agree with everything you say. Except that I believe that the authors
thought indeed this is a security feature.
https://stackoverflow.com/questions/33154740/strlcat-is-dst-always-nul-terminated-what-are-size-and-the-returned-value
provides a hint about the rationale: 
"size is expected to be the size of the memory region containing dst,
so that dst[size] is assumed to be an invalid memory reference." With
this in mind, passing a non-nul-terminated string to strlcat is an
error for "src", but not for "dst", and calling strlen(dst) from
strlcat() would be a bug.

> Clearly this is a nitpicky corner case, and I don't think we need
> protection against src not being null terminated, so I'm not strongly
> against bytes + strlen(src), if you prefer that.
rationale
That code line wasn't touched by my patch anyway, so I propose that we
leave it as-is for now.

Best,
Martin

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Benjamin Marzinski]

On Tue, Aug 04, 2020 at 09:35:08PM +0200, Martin Wilck wrote:
> On Tue, 2020-08-04 at 11:26 -0500, Benjamin Marzinski wrote:
> > On Tue, Aug 04, 2020 at 05:18:18PM +0200, Martin Wilck wrote:
> > > On Tue, 2020-08-04 at 17:04 +0200, Martin Wilck wrote:
> > > > On Thu, 2020-07-16 at 16:17 -0500, Benjamin Marzinski wrote:
> > > > > On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com
> > > > > wrote:
> > > > > > From: Martin Wilck <mwilck@suse.com>
> > > > > > +struct bitfield *alloc_bitfield(unsigned int maxbit)
> > > > > > +{
> > > > > > +	unsigned int n;
> > > > > > +	struct bitfield *bf;
> > > > > > +
> > > > > > +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
> > > > > 
> > > > > What's the point in accepting 0? That's an empty bitmap.
> > > > > 
> > > > Thanks for spotting these, I will fix them.
> > > 
> > > Thinking about it once more, I believe that accepting 0 as the
> > > bitfield
> > > length is actually the right thing. A bitfield of length 0 makes
> > > not
> > > much less sense than one of length 1. The code makes sure that the
> > > bit
> > > operations on the 0-length bitfield behave correctly (see
> > > test_bitmask_len_0()). Thus callers can use bitfields without
> > > bothering
> > > for extra NULL checks. That was the intention. Like we support 0-
> > > length 
> > > vectors.
> > 
> > But the calloc call itself can return NULL, so deferencing bf (as in
> > bf->len = maxbit), can crash.
> 
> Of course, I wasn't arguing about that. I'm going to resend with this
> fixed.
> 
> > I'm also still fuzzy on why we want to support zero length bitfields.
> > Since they can't be grown like vectors can, it seem like requesting a
> > zero length bitfield will always be a sign of a coding error. We
> > would get a more useful error by having the failure happen closer to
> > the error in the code.  Or is there actually a use for a zero length
> > bitfield that can't be grown?
> 
> The only "use" would be to be able to work with bitmaps in situations
> where zero elements are possible, without the need to catch another
> exception. 0-element bitfields are technically possible although
> logically they make no sense. That's robust design for a library
> function, IMO. I don't see why it'd be "better" to return NULL instead.
> If we are after errors in callers, we might want to print an error
> message and still not return NULL.

Fine. It's a pretty trivial thing either way.

-Ben

> 
> Martin
> 

Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Martin Wilck]

Hi Ben,

On Mon, 2020-08-10 at 13:05 -0500, Benjamin Marzinski wrote:
> On Tue, Aug 04, 2020 at 09:35:08PM +0200, Martin Wilck wrote:
> > On Tue, 2020-08-04 at 11:26 -0500, Benjamin Marzinski wrote:
> > > 
> > > I'm also still fuzzy on why we want to support zero length
> > > bitfields.
> > > Since they can't be grown like vectors can, it seem like
> > > requesting a
> > > zero length bitfield will always be a sign of a coding error. We
> > > would get a more useful error by having the failure happen closer
> > > to
> > > the error in the code.  Or is there actually a use for a zero
> > > length
> > > bitfield that can't be grown?
> > 
> > The only "use" would be to be able to work with bitmaps in
> > situations
> > where zero elements are possible, without the need to catch another
> > exception. 0-element bitfields are technically possible although
> > logically they make no sense. That's robust design for a library
> > function, IMO. I don't see why it'd be "better" to return NULL
> > instead.
> > If we are after errors in callers, we might want to print an error
> > message and still not return NULL.
> 
> Fine. It's a pretty trivial thing either way.

It took me a while, but ...

The current typical usage e.g. in group_by_match() looks like this:

	bitmap = alloc_bitfield(VECTOR_SIZE(paths));

	if (!bitmap)
		goto out;

	for (i = 0; i < VECTOR_SIZE(paths); i++) {
	        ....

Here, VECTOR_SIZE(paths) == 0 is a valid value that just causes nothing
to happen in the function. The other callers aren't much different. I
think we rather don't want to print an error message in these cases.

Every caller needs to check the return value of alloc_bitfield()
anyway, to guard against OOM. The code above would actually be slightly
optimized by returning NULL from alloc_bitfield() for maxbit == 0, by
returning immediately. Besides, we eliminate an (albeit minimal) risk
of hitting OOM by avoiding one needless memory allocation. 

The fact that all our callers work like this convinces me. I'll change
the return value to NULL like you suggested.

Cheers,
Martin