[dm-devel] [PATCH v2 00/15] Rework uevent filtering and merging

[dm-devel] [PATCH v2 00/15] Rework uevent filtering and merging

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 2974 bytes --]

From: Martin Wilck <mwilck@suse.com>

Hi Ben, hi Christophe,

the bulk of this patch set (3-7) is a rework of the uevent filtering and
merging logic introduced in commit ee8888f ("multipath-tools: improve 
processing efficiency for addition and deletion of multipath devices"),
by Tang Junhui.

The rationale is explained in detail in the commit message in patch 08/14.
TL;DR: The previous approach delayed uevent handling, possibly a lot, which
is often undesirable. The new logic passes events to the dispatcher
immediately, but if they queue up (because the dispatcher can't keep
up with the rate at which events arrive, or is blocked e.g. by the
path checker), the dispatcher will apply filtering and merging
between servicing individual events. This worked well in my own testing,
but I'd appreciate if ZTE could give it a shot in their special test
environment. Unfortunately, Tang Junhui isn't reachable at ZTE any more.

Patch 8-13 add some more improvements to the uevent handling code, and
improve logging. The first 2 patches are unrelated fixes.

Changes v1->v2:

 04: merged patch 04 and 05 of v1 series into 04. Numbering changes accordingly.
     (Benjamin Marzinski)
 04: added function for uevent node deletion in simple cases (no tail deletion,
     no merge_node) (Benjamin Marzinski)
 05 (was 06 in v1): avoid memory leak
 14: new, found by coverity when testing the patched code.
 15: new, from discussion about 05 (Benjamin Marzinski)

Martin Wilck (15):
  multipathd: allow adding git rev as version suffix
  multipathd: don't switch to DAEMON_IDLE during startup
  uevent_dispatch(): use while in wait loop
  libmultipath: uevent_dispatch(): process uevents one by one
  multipathd: reconfigure: disallow changing uid_attrs
  libmultipath: microoptimize uevent filtering and merging
  libmultipath: uevent_listen(): don't delay uevents
  libmultipath: uevent: use struct to pass parameters around
  libmultipath: uevent: log statistics about filtering and merging
  libmultipath: merge_uevq(): filter first, then merge
  libmultipath: uevent_filter(): filter previously merged events
  libmultipath: uevent: improve log messages
  libmultipath: uevent: add debug messages for event queue
  libmultipath: apply_format(): prevent buffer overflow
  libmultipath: avoid memory leak with uid_attrs

 Makefile.inc               |   3 +-
 libmultipath/callout.c     |   2 +-
 libmultipath/config.c      |  11 +-
 libmultipath/config.h      |   4 +-
 libmultipath/dict.c        |   5 +
 libmultipath/discovery.c   |   2 +-
 libmultipath/list.h        |  53 +++++
 libmultipath/structs.h     |   2 +-
 libmultipath/uevent.c      | 422 ++++++++++++++++++++++---------------
 libmultipath/uevent.h      |   3 +-
 multipath/multipath.conf.5 |   2 +
 multipathd/main.c          |  65 ++++--
 tests/uevent.c             |   2 +-
 13 files changed, 386 insertions(+), 190 deletions(-)

-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 01/15] multipathd: allow adding git rev as version suffix

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1834 bytes --]

From: Martin Wilck <mwilck@suse.com>

Use the make variable EXTRAVERSION to hold a version suffix,
and print it on multipathd startup. By default, EXTRAVERSION is
the 7-digit git rev in the format "-g1234567" if git is available,
and empty otherwise. Build systems can pass in alternative
EXTRAVERSION strings as they prefer.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 Makefile.inc      | 3 ++-
 multipathd/main.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Makefile.inc b/Makefile.inc
index d24da43..120f671 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -13,6 +13,7 @@
 # SCSI_DH_MODULES_PRELOAD := scsi_dh_alua scsi_dh_rdac
 SCSI_DH_MODULES_PRELOAD :=
 
+EXTRAVERSION := $(shell rev=$$(git rev-parse --short=7 HEAD 2>/dev/null); echo $${rev:+-g$$rev})
 
 PKGCONFIG	?= pkg-config
 
@@ -126,7 +127,7 @@ WARNFLAGS	:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) -Werror=implici
 CPPFLAGS	:= -Wp,-D_FORTIFY_SOURCE=2
 CFLAGS		:= --std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
 		   -DBIN_DIR=\"$(bindir)\" -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" \
-		   -MMD -MP
+		   -DEXTRAVERSION=\"$(EXTRAVERSION)\" -MMD -MP
 BIN_CFLAGS	= -fPIE -DPIE
 LIB_CFLAGS	= -fPIC
 SHARED_FLAGS	= -shared
diff --git a/multipathd/main.c b/multipathd/main.c
index 1406251..3a31154 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3247,7 +3247,8 @@ child (__attribute__((unused)) void *param)
 
 	post_config_state(DAEMON_START);
 
-	condlog(2, "multipathd v%d.%d.%d: start up", MULTIPATH_VERSION(VERSION_CODE));
+	condlog(2, "multipathd v%d.%d.%d%s: start up",
+		MULTIPATH_VERSION(VERSION_CODE), EXTRAVERSION);
 	condlog(3, "read " DEFAULT_CONFIGFILE);
 
 	if (verbosity)
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 02/15] multipathd: don't switch to DAEMON_IDLE during startup

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1046 bytes --]

From: Martin Wilck <mwilck@suse.com>

By switching to DAEMON_IDLE early during startup, we may signal
systemd READY=1 prematurely. Don't do this.

Fixes: c95a739 ("multipathd: avoid sending "READY=1" to systemd on early shutdown")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 3a31154..13b1948 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3307,11 +3307,10 @@ child (__attribute__((unused)) void *param)
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
 
-	__post_config_state(DAEMON_IDLE);
 	rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
 	if (!rc) {
 		/* Wait for uxlsnr startup */
-		while (running_state == DAEMON_IDLE)
+		while (running_state == DAEMON_START)
 			pthread_cond_wait(&config_cond, &config_lock);
 		state = running_state;
 	}
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 03/15] uevent_dispatch(): use while in wait loop

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1153 bytes --]

From: Martin Wilck <mwilck@suse.com>

Callers of pthread_cond_wait() should generally use a while loop
to test the condition. Also, remove the misleading comment.
Condition variables aren't unreliable, they're just not strictly
tied to the condition tested.

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

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 29e2557..fe60ae3 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -443,13 +443,10 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
 		servicing_uev = 0;
-		/*
-		 * Condition signals are unreliable,
-		 * so make sure we only wait if we have to.
-		 */
-		if (list_empty(&uevq)) {
+
+		while (list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
-		}
+
 		servicing_uev = 1;
 		list_splice_init(&uevq, &uevq_tmp);
 		pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 04/15] libmultipath: uevent_dispatch(): process uevents one by one

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 10407 bytes --]

From: Martin Wilck <mwilck@suse.com>

The main rationale for delaying uevents is that the
uevent dispatcher may have to wait for other threads to release the
vecs lock, may the vecs lock for an extended amount of time, and
even sleep occasionally. By delaying them, we have the chance
to accumulate events for the same path device ("filtering") or
WWID ("merging"), thus avoiding duplicate work if we merge these
into one.

A similar effect can be obtained in the uevent dispatcher itself
by looking for new uevents after each dispatched event, and trying
to merge the newly arrived events with those that remained
in the queue.

When uevq_work is non-empty and we append a list of new events,
we don't need to check the entire list for filterable and mergeable
uevents. uevq_work had been filtered and merged already. So we just
need to check the newly appended events. These must of course be
checked for merges with earlier events, too.

We must deal with some special cases here, like previously merged
uevents being filtered later.

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

diff --git a/libmultipath/list.h b/libmultipath/list.h
index ced021f..248f72b 100644
--- a/libmultipath/list.h
+++ b/libmultipath/list.h
@@ -246,6 +246,35 @@ static inline void list_splice_tail_init(struct list_head *list,
 #define list_entry(ptr, type, member) \
 	container_of(ptr, type, member)
 
+
+/**
+ * list_pop - unlink and return the first list element
+ * @head:	the &struct list_head pointer.
+ */
+static inline struct list_head *list_pop(struct list_head *head)
+{
+	struct list_head *tmp;
+
+	if (list_empty(head))
+		return NULL;
+	tmp = head->next;
+	list_del_init(tmp);
+	return tmp;
+}
+
+/**
+ * list_pop_entry - unlink and return the entry of the first list element
+ * @head:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_pop_entry(head, type, member)		\
+({							\
+	struct list_head *__h = list_pop(head);		\
+							\
+	(__h ? container_of(__h, type, member) : NULL);	\
+})
+
 /**
  * list_for_each	-	iterate over a list
  * @pos:	the &struct list_head to use as a loop counter.
@@ -334,6 +363,30 @@ static inline void list_splice_tail_init(struct list_head *list,
 	     &pos->member != (head);                                    \
 	     pos = n, n = list_entry(n->member.prev, typeof(*n), member))
 
+/**
+ * list_for_some_entry - iterate list from the given begin node to the given end node
+ * @pos:	the type * to use as a loop counter.
+ * @from:	the begin node of the iteration.
+ * @to:		the end node of the iteration.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_some_entry(pos, from, to, member)                      \
+	for (pos = list_entry((from)->next, typeof(*pos), member);      \
+	     &pos->member != (to);                                      \
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_some_entry_reverse - iterate backwards list from the given begin node to the given end node
+ * @pos:	the type * to use as a loop counter.
+ * @from:	the begin node of the iteration.
+ * @to:		the end node of the iteration.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_some_entry_reverse(pos, from, to, member)		\
+	for (pos = list_entry((from)->prev, typeof(*pos), member);      \
+	     &pos->member != (to);                                      \
+	     pos = list_entry(pos->member.prev, typeof(*pos), member))
+
 /**
  * list_for_some_entry_safe - iterate list from the given begin node to the given end node safe against removal of list entry
  * @pos:	the type * to use as a loop counter.
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index fe60ae3..66acd6c 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -306,17 +306,64 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
 	return false;
 }
 
+static void uevent_delete_from_list(struct uevent *to_delete,
+				    struct uevent **previous,
+				    struct list_head **old_tail)
+{
+	/*
+	 * "old_tail" is the list_head before the last list element to which
+	 * the caller iterates (the list anchor if the caller iterates over
+	 * the entire list). If this element is removed (which can't happen
+	 * for the anchor), "old_tail" must be moved. It can happen that
+	 * "old_tail" ends up pointing at the anchor.
+	 */
+	if (*old_tail == &to_delete->node)
+		*old_tail = to_delete->node.prev;
+
+	list_del_init(&to_delete->node);
+
+	/*
+	 * The "to_delete" uevent has been merged with other uevents
+	 * previously. Re-insert them into the list, at the point we're
+	 * currently at. This must be done after the list_del_init() above,
+	 * otherwise previous->next would still point to to_delete.
+	 */
+	if (!list_empty(&to_delete->merge_node)) {
+		struct uevent *last = list_entry(to_delete->merge_node.prev,
+						 typeof(*last), node);
+
+		list_splice(&to_delete->merge_node, &(*previous)->node);
+		*previous = last;
+	}
+	if (to_delete->udev)
+		udev_device_unref(to_delete->udev);
+
+	free(to_delete);
+}
+
+/*
+ * Use this function to delete events that are known not to
+ * be equal to old_tail, and have an empty merge_node list.
+ * For others, use uevent_delete_from_list().
+ */
+static void uevent_delete_simple(struct uevent *to_delete)
+{
+	list_del_init(&to_delete->node);
+
+	if (to_delete->udev)
+		udev_device_unref(to_delete->udev);
+
+	free(to_delete);
+}
+
 static void
-uevent_prepare(struct list_head *tmpq)
+uevent_prepare(struct list_head *tmpq, const struct list_head *stop)
 {
 	struct uevent *uev, *tmp;
 
-	list_for_each_entry_reverse_safe(uev, tmp, tmpq, node) {
+	list_for_some_entry_reverse_safe(uev, tmp, tmpq, stop, node) {
 		if (uevent_can_discard(uev)) {
-			list_del_init(&uev->node);
-			if (uev->udev)
-				udev_device_unref(uev->udev);
-			free(uev);
+			uevent_delete_simple(uev);
 			continue;
 		}
 
@@ -327,7 +374,7 @@ uevent_prepare(struct list_head *tmpq)
 }
 
 static void
-uevent_filter(struct uevent *later, struct list_head *tmpq)
+uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *earlier, *tmp;
 
@@ -341,16 +388,13 @@ uevent_filter(struct uevent *later, struct list_head *tmpq)
 				earlier->kernel, earlier->action,
 				later->kernel, later->action);
 
-			list_del_init(&earlier->node);
-			if (earlier->udev)
-				udev_device_unref(earlier->udev);
-			free(earlier);
+			uevent_delete_from_list(earlier, &tmp, stop);
 		}
 	}
 }
 
 static void
-uevent_merge(struct uevent *later, struct list_head *tmpq)
+uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *earlier, *tmp;
 
@@ -365,37 +409,42 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 				earlier->action, earlier->kernel, earlier->wwid,
 				later->action, later->kernel, later->wwid);
 
+			/* See comment in uevent_delete_from_list() */
+			if (&earlier->node == *stop)
+				*stop = earlier->node.prev;
+
 			list_move(&earlier->node, &later->merge_node);
+			list_splice_init(&earlier->merge_node,
+					 &later->merge_node);
 		}
 	}
 }
 
 static void
-merge_uevq(struct list_head *tmpq)
+merge_uevq(struct list_head *tmpq, struct list_head *stop)
 {
 	struct uevent *later;
 
-	uevent_prepare(tmpq);
-	list_for_each_entry_reverse(later, tmpq, node) {
-		uevent_filter(later, tmpq);
+	uevent_prepare(tmpq, stop);
+	list_for_some_entry_reverse(later, tmpq, stop, node) {
+		uevent_filter(later, tmpq, &stop);
 		if(uevent_need_merge())
-			uevent_merge(later, tmpq);
+			uevent_merge(later, tmpq, &stop);
 	}
 }
 
 static void
 service_uevq(struct list_head *tmpq)
 {
-	struct uevent *uev, *tmp;
+	struct uevent *uev = list_pop_entry(tmpq, typeof(*uev), node);
 
-	list_for_each_entry_safe(uev, tmp, tmpq, node) {
-		list_del_init(&uev->node);
-
-		pthread_cleanup_push(cleanup_uev, uev);
-		if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
-			condlog(0, "uevent trigger error");
-		pthread_cleanup_pop(1);
-	}
+	if (uev == NULL)
+		return;
+	condlog(4, "servicing uevent '%s %s'", uev->action, uev->kernel);
+	pthread_cleanup_push(cleanup_uev, uev);
+	if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
+		condlog(0, "uevent trigger error");
+	pthread_cleanup_pop(1);
 }
 
 static void uevent_cleanup(void *arg)
@@ -432,33 +481,43 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		    void * trigger_data)
 {
+	LIST_HEAD(uevq_work);
+
 	my_uev_trigger = uev_trigger;
 	my_trigger_data = trigger_data;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
+	pthread_cleanup_push(cleanup_uevq, &uevq_work);
 	while (1) {
-		LIST_HEAD(uevq_tmp);
+		struct list_head *stop;
 
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
-		servicing_uev = 0;
 
-		while (list_empty(&uevq))
+		servicing_uev = !list_empty(&uevq_work);
+
+		while (list_empty(&uevq_work) && list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
 
 		servicing_uev = 1;
-		list_splice_init(&uevq, &uevq_tmp);
+		/*
+		 * "stop" is the list element towards which merge_uevq()
+		 * will iterate: the last element of uevq_work before
+		 * appending new uevents. If uveq_is empty, uevq_work.prev
+		 * equals &uevq_work, which is what we need.
+		 */
+		stop = uevq_work.prev;
+		list_splice_tail_init(&uevq, &uevq_work);
 		pthread_cleanup_pop(1);
 
 		if (!my_uev_trigger)
 			break;
 
-		pthread_cleanup_push(cleanup_uevq, &uevq_tmp);
-		merge_uevq(&uevq_tmp);
-		service_uevq(&uevq_tmp);
-		pthread_cleanup_pop(1);
+		merge_uevq(&uevq_work, stop);
+		service_uevq(&uevq_work);
 	}
+	pthread_cleanup_pop(1);
 	condlog(3, "Terminating uev service queue");
 	return 0;
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 05/15] multipathd: reconfigure: disallow changing uid_attrs

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 3613 bytes --]

From: Martin Wilck <mwilck@suse.com>

uevent merging by WWID relies on the uid_attrs config option. As we
drop struct config between calls to uevent_merge(), we must be sure
that the WWID is not changed in reconfigure().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.conf.5 |  2 ++
 multipathd/main.c          | 59 ++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 605b46e..a9cd776 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -264,6 +264,8 @@ If this option is configured and matches the device
 node name of a device, it overrides any other configured  methods for
 determining the WWID for this device.
 .PP
+This option cannot be changed during runtime with the multipathd \fBreconfigure\fR command.
+.PP
 The default is: \fB<unset>\fR. To enable uevent merging, set it e.g. to
 \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
 .RE
diff --git a/multipathd/main.c b/multipathd/main.c
index 13b1948..6808ad1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2835,11 +2835,58 @@ void rcu_free_config(struct rcu_head *head)
 	free_config(conf);
 }
 
+static bool reconfigure_check_uid_attrs(const struct _vector *old_attrs,
+					const struct _vector *new_attrs)
+{
+	int i;
+	char *old;
+
+	if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
+		return true;
+
+	vector_foreach_slot(old_attrs, old, i) {
+		char *new = VECTOR_SLOT(new_attrs, i);
+
+		if (strcmp(old, new))
+			return true;
+	}
+
+	return false;
+}
+
+static void reconfigure_check(struct config *old, struct config *new)
+{
+	int old_marginal_pathgroups;
+
+	old_marginal_pathgroups = old->marginal_pathgroups;
+	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
+	    (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
+		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
+			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
+			"off" : "on");
+		new->marginal_pathgroups = old_marginal_pathgroups;
+	}
+
+	if (reconfigure_check_uid_attrs(&old->uid_attrs, &new->uid_attrs)) {
+		int i;
+		void *ptr;
+
+		condlog(1, "multipathd must be restarted to change uid_attrs, keeping old values");
+		vector_foreach_slot(&new->uid_attrs, ptr, i)
+			free(ptr);
+		vector_reset(&new->uid_attrs);
+		new->uid_attrs = old->uid_attrs;
+
+		/* avoid uid_attrs being freed in rcu_free_config() */
+		old->uid_attrs.allocated = 0;
+		old->uid_attrs.slot = NULL;
+	}
+}
+
 static int
 reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 {
 	struct config * old, *conf;
-	int old_marginal_pathgroups;
 
 	conf = load_config(DEFAULT_CONFIGFILE);
 	if (!conf)
@@ -2870,14 +2917,8 @@ reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 	uxsock_timeout = conf->uxsock_timeout;
 
 	old = rcu_dereference(multipath_conf);
-	old_marginal_pathgroups = old->marginal_pathgroups;
-	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
-	    (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
-		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
-			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
-			"off" : "on");
-		conf->marginal_pathgroups = old_marginal_pathgroups;
-	}
+	reconfigure_check(old, conf);
+
 	conf->sequence_nr = old->sequence_nr + 1;
 	rcu_assign_pointer(multipath_conf, conf);
 	call_rcu(&old->rcu, rcu_free_config);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 06/15] libmultipath: microoptimize uevent filtering and merging

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 2887 bytes --]

From: Martin Wilck <mwilck@suse.com>

Simplify and microoptimize the filtering and merging tests.
E.g. do the WWID comparison last.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 66acd6c..d39f388 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -220,6 +220,10 @@ static bool
 uevent_can_filter(struct uevent *earlier, struct uevent *later)
 {
 
+	if (!strncmp(later->kernel, "dm-", 3) ||
+	    strcmp(earlier->kernel, later->kernel))
+		return false;
+
 	/*
 	 * filter earlier uvents if path has removed later. Eg:
 	 * "add path1 |chang path1 |add path2 |remove path1"
@@ -227,11 +231,8 @@ uevent_can_filter(struct uevent *earlier, struct uevent *later)
 	 * "add path2 |remove path1"
 	 * uevents "add path1" and "chang path1" are filtered out
 	 */
-	if (!strcmp(earlier->kernel, later->kernel) &&
-		!strcmp(later->action, "remove") &&
-		strncmp(later->kernel, "dm-", 3)) {
+	if (!strcmp(later->action, "remove"))
 		return true;
-	}
 
 	/*
 	 * filter change uvents if add uevents exist. Eg:
@@ -240,12 +241,9 @@ uevent_can_filter(struct uevent *earlier, struct uevent *later)
 	 * "add path1 |add path2"
 	 * uevent "chang path1" is filtered out
 	 */
-	if (!strcmp(earlier->kernel, later->kernel) &&
-		!strcmp(earlier->action, "change") &&
-		!strcmp(later->action, "add") &&
-		strncmp(later->kernel, "dm-", 3)) {
+	if (!strcmp(earlier->action, "change") &&
+	    !strcmp(later->action, "add"))
 		return true;
-	}
 
 	return false;
 }
@@ -278,10 +276,10 @@ merge_need_stop(struct uevent *earlier, struct uevent *later)
 	 * with the same wwid and different action
 	 * it would be better to stop merging.
 	 */
-	if (!strcmp(earlier->wwid, later->wwid) &&
-	    strcmp(earlier->action, later->action) &&
+	if (strcmp(earlier->action, later->action) &&
 	    strcmp(earlier->action, "change") &&
-	    strcmp(later->action, "change"))
+	    strcmp(later->action, "change") &&
+	    !strcmp(earlier->wwid, later->wwid))
 		return true;
 
 	return false;
@@ -296,12 +294,12 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
 	 * and actions are addition or deletion
 	 */
 	if (earlier->wwid && later->wwid &&
-	    !strcmp(earlier->wwid, later->wwid) &&
+	    strncmp(earlier->kernel, "dm-", 3) &&
 	    !strcmp(earlier->action, later->action) &&
-	    strncmp(earlier->action, "change", 6) &&
-	    strncmp(earlier->kernel, "dm-", 3)) {
+	    (!strcmp(earlier->action, "add") ||
+	     !strcmp(earlier->action, "remove")) &&
+	    !strcmp(earlier->wwid, later->wwid))
 		return true;
-	}
 
 	return false;
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 07/15] libmultipath: uevent_listen(): don't delay uevents

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 6675 bytes --]

From: Martin Wilck <mwilck@suse.com>

When multipathd starts up early, basically all devices are added
through uevent processing. This takes much more time than necessary
because of the artificial delays introduced for passing uevents
between the listener and the receiver thread in ee8888f
("multipath-tools: improve processing efficiency for addition and deletion of
multipath devices"). This delay could be up to 30s.

It's generally not a good idea to delay uevent processing in multipathd.
ADD events must normally be handled ASAP in order to avoid maps entering
queueing mode or eventually failing. Handling REMOVE events quickly is
also important to make multipathd aware of deleted devices and keep
kernel and multipathd state in sync.

If uevents arrive quickly, the assumption is that the dispatcher will process
them more slowly than the listener. This was the idea of commit ee8888f,
AFAIU: if a queue of unprocessed events piles up because the dispatcher is
too slow, use filtering and merging to reduce the length of the queue, and
thus the work to be done for the uevent dispatcher, especially the work
that needs to be done while holding the vecs lock. In ee8888f, the
queue was created by allowing uevents to accumulate in the listener.

This patch changes the logic of ee8888f, while keeping the uevent
filtering and discarding features. The idea is that the uevent dispatcher
shouldn't be idle if there are uevents to process. Therefore uevents
are passed to it immediately. But it now checks for new uevents after
processing every individual event, before processing the entire queue,
and it applies filtering and merging to the queue as it grows.

This patch set avoids any delay when the uevent dispatcher is idle or
able to keep up with the rate of incoming uevents, while applying an
increasing amount of filtering and merging as pressure on the uevent
dispatcher increases. It's reasonable to assume that filtering and
merging get more efficient with increasing queue length, because the
probability of finding matching events will increase.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 108 +++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 71 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index d39f388..58760e8 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -52,10 +52,6 @@
 #include "blacklist.h"
 #include "devmapper.h"
 
-#define MAX_ACCUMULATION_COUNT 2048
-#define MAX_ACCUMULATION_TIME 30*1000
-#define MIN_BURST_SPEED 10
-
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
 static LIST_HEAD(uevq);
@@ -582,44 +578,43 @@ static struct uevent *uevent_from_udev_device(struct udev_device *dev)
 	return uev;
 }
 
-static bool uevent_burst(struct timeval *start_time, int events)
+#define MAX_UEVENTS 1000
+static int uevent_receive_events(int fd, struct list_head *tmpq,
+				 struct udev_monitor *monitor)
 {
-	struct timeval diff_time, end_time;
-	unsigned long speed;
-	unsigned long eclipse_ms;
+	struct pollfd ev_poll;
+	int n = 0;
 
-	if(events > MAX_ACCUMULATION_COUNT) {
-		condlog(2, "burst got %u uevents, too much uevents, stopped", events);
-		return false;
-	}
+	do {
+		struct uevent *uev;
+		struct udev_device *dev;
 
-	gettimeofday(&end_time, NULL);
-	timersub(&end_time, start_time, &diff_time);
+		dev = udev_monitor_receive_device(monitor);
+		if (!dev) {
+			condlog(0, "failed getting udev device");
+			break;
+		}
+		uev = uevent_from_udev_device(dev);
+		if (!uev)
+			break;
 
-	eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec / 1000;
+		list_add_tail(&uev->node, tmpq);
+		n++;
+		condlog(4, "received uevent \"%s %s\"", uev->action, uev->kernel);
 
-	if (eclipse_ms == 0)
-		return true;
+		ev_poll.fd = fd;
+		ev_poll.events = POLLIN;
 
-	if (eclipse_ms > MAX_ACCUMULATION_TIME) {
-		condlog(2, "burst continued %lu ms, too long time, stopped", eclipse_ms);
-		return false;
-	}
+	} while (n < MAX_UEVENTS && poll(&ev_poll, 1, 0) > 0);
 
-	speed = (events * 1000) / eclipse_ms;
-	if (speed > MIN_BURST_SPEED)
-		return true;
-
-	return false;
+	return n;
 }
 
 int uevent_listen(struct udev *udev)
 {
 	int err = 2;
 	struct udev_monitor *monitor = NULL;
-	int fd, socket_flags, events;
-	struct timeval start_time;
-	int timeout = 30;
+	int fd, socket_flags;
 	LIST_HEAD(uevlisten_tmp);
 
 	/*
@@ -671,59 +666,30 @@ int uevent_listen(struct udev *udev)
 		goto out;
 	}
 
-	events = 0;
-	gettimeofday(&start_time, NULL);
 	pthread_cleanup_push(cleanup_global_uevq, NULL);
 	pthread_cleanup_push(cleanup_uevq, &uevlisten_tmp);
 	while (1) {
-		struct uevent *uev;
-		struct udev_device *dev;
-		struct pollfd ev_poll;
-		int poll_timeout;
-		int fdcount;
+		int fdcount, events;
+		struct pollfd ev_poll = { .fd = fd, .events = POLLIN, };
 
-		memset(&ev_poll, 0, sizeof(struct pollfd));
-		ev_poll.fd = fd;
-		ev_poll.events = POLLIN;
-		poll_timeout = timeout * 1000;
-		errno = 0;
-		fdcount = poll(&ev_poll, 1, poll_timeout);
-		if (fdcount > 0 && ev_poll.revents & POLLIN) {
-			timeout = uevent_burst(&start_time, events + 1) ? 1 : 0;
-			dev = udev_monitor_receive_device(monitor);
-			if (!dev) {
-				condlog(0, "failed getting udev device");
-				continue;
-			}
-			uev = uevent_from_udev_device(dev);
-			if (!uev)
-				continue;
-			list_add_tail(&uev->node, &uevlisten_tmp);
-			events++;
-			continue;
-		}
+		fdcount = poll(&ev_poll, 1, -1);
 		if (fdcount < 0) {
 			if (errno == EINTR)
 				continue;
 
-			condlog(0, "error receiving "
-				"uevent message: %m");
+			condlog(0, "error receiving uevent message: %m");
 			err = -errno;
 			break;
 		}
-		if (!list_empty(&uevlisten_tmp)) {
-			/*
-			 * Queue uevents and poke service pthread.
-			 */
-			condlog(3, "Forwarding %d uevents", events);
-			pthread_mutex_lock(uevq_lockp);
-			list_splice_tail_init(&uevlisten_tmp, &uevq);
-			pthread_cond_signal(uev_condp);
-			pthread_mutex_unlock(uevq_lockp);
-			events = 0;
-		}
-		gettimeofday(&start_time, NULL);
-		timeout = 30;
+		events = uevent_receive_events(fd, &uevlisten_tmp, monitor);
+		if (events <= 0)
+			continue;
+
+		condlog(4, "Forwarding %d uevents", events);
+		pthread_mutex_lock(uevq_lockp);
+		list_splice_tail_init(&uevlisten_tmp, &uevq);
+		pthread_cond_signal(uev_condp);
+		pthread_mutex_unlock(uevq_lockp);
 	}
 	pthread_cleanup_pop(1);
 	pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 08/15] libmultipath: uevent: use struct to pass parameters around

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 11128 bytes --]

From: Martin Wilck <mwilck@suse.com>

libmultipath: uevent_dispatch(): just grab config once

Introduce struct uevent_filter_state to pass parameters around.
This simplifies the function signatures and allows for easy extension
later.

Instead of grabbing multipath config repeatedly, do it just
once per dispatcher iteration, and pass the pointer around in
struct uevent_filter_state. We shouldn't use different configs
for different paths in a single iteration, anyway.

Also, properly constify get_uid_attribute_by_attrs() and
pp->uid_attribute.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c    |   6 +--
 libmultipath/config.h    |   4 +-
 libmultipath/discovery.c |   2 +-
 libmultipath/structs.h   |   2 +-
 libmultipath/uevent.c    | 110 +++++++++++++++++----------------------
 libmultipath/uevent.h    |   3 +-
 tests/uevent.c           |   2 +-
 7 files changed, 58 insertions(+), 71 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index c595e76..7346c37 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -1018,10 +1018,10 @@ out:
 	return 1;
 }
 
-char *get_uid_attribute_by_attrs(struct config *conf,
-				 const char *path_dev)
+const char *get_uid_attribute_by_attrs(const struct config *conf,
+				       const char *path_dev)
 {
-	vector uid_attrs = &conf->uid_attrs;
+	const struct _vector *uid_attrs = &conf->uid_attrs;
 	int j;
 	char *att, *col;
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index c73389b..5351ecd 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -314,7 +314,7 @@ void libmp_put_multipath_config(void *);
 void put_multipath_config(void *);
 
 int parse_uid_attrs(char *uid_attrs, struct config *conf);
-char *get_uid_attribute_by_attrs(struct config *conf,
-				 const char *path_dev);
+const char *get_uid_attribute_by_attrs(const struct config *conf,
+				       const char *path_dev);
 
 #endif
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b969fba..589eca1 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2044,7 +2044,7 @@ fix_broken_nvme_wwid(struct path *pp, const char *value, size_t size)
 }
 
 static int
-get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
+get_udev_uid(struct path * pp, const char *uid_attribute, struct udev_device *udev)
 {
 	ssize_t len;
 	const char *value;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d94f93a..afd4674 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -327,7 +327,7 @@ struct path {
 	int detect_prio;
 	int detect_checker;
 	int tpgs;
-	char * uid_attribute;
+	const char *uid_attribute;
 	char * getuid;
 	struct prio prio;
 	struct checker checker;
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 58760e8..8809f5c 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -63,6 +63,12 @@ static uev_trigger *my_uev_trigger;
 static void *my_trigger_data;
 static int servicing_uev;
 
+struct uevent_filter_state {
+	struct list_head uevq;
+	struct list_head *old_tail;
+	struct config *conf;
+};
+
 int is_uevent_busy(void)
 {
 	int empty;
@@ -158,40 +164,24 @@ int uevent_get_env_positive_int(const struct uevent *uev,
 }
 
 void
-uevent_get_wwid(struct uevent *uev)
+uevent_get_wwid(struct uevent *uev, const struct config *conf)
 {
-	char *uid_attribute;
+	const char *uid_attribute;
 	const char *val;
-	struct config * conf;
 
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
 	uid_attribute = get_uid_attribute_by_attrs(conf, uev->kernel);
-	pthread_cleanup_pop(1);
-
 	val = uevent_get_env_var(uev, uid_attribute);
 	if (val)
 		uev->wwid = val;
 }
 
-static bool uevent_need_merge(void)
+static bool uevent_need_merge(const struct config *conf)
 {
-	struct config * conf;
-	bool need_merge = false;
-
-	conf = get_multipath_config();
-	if (VECTOR_SIZE(&conf->uid_attrs) > 0)
-		need_merge = true;
-	put_multipath_config(conf);
-
-	return need_merge;
+	return VECTOR_SIZE(&conf->uid_attrs) > 0;
 }
 
-static bool uevent_can_discard(struct uevent *uev)
+static bool uevent_can_discard(struct uevent *uev, const struct config *conf)
 {
-	int invalid = 0;
-	struct config * conf;
-
 	/*
 	 * do not filter dm devices by devnode
 	 */
@@ -200,15 +190,10 @@ static bool uevent_can_discard(struct uevent *uev)
 	/*
 	 * filter paths devices by devnode
 	 */
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
 	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
 			   uev->kernel) > 0)
-		invalid = 1;
-	pthread_cleanup_pop(1);
-
-	if (invalid)
 		return true;
+
 	return false;
 }
 
@@ -350,29 +335,28 @@ static void uevent_delete_simple(struct uevent *to_delete)
 	free(to_delete);
 }
 
-static void
-uevent_prepare(struct list_head *tmpq, const struct list_head *stop)
+static void uevent_prepare(struct uevent_filter_state *st)
 {
 	struct uevent *uev, *tmp;
 
-	list_for_some_entry_reverse_safe(uev, tmp, tmpq, stop, node) {
-		if (uevent_can_discard(uev)) {
+	list_for_some_entry_reverse_safe(uev, tmp, &st->uevq, st->old_tail, node) {
+		if (uevent_can_discard(uev, st->conf)) {
 			uevent_delete_simple(uev);
 			continue;
 		}
 
 		if (strncmp(uev->kernel, "dm-", 3) &&
-		    uevent_need_merge())
-			uevent_get_wwid(uev);
+		    uevent_need_merge(st->conf))
+			uevent_get_wwid(uev, st->conf);
 	}
 }
 
 static void
-uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
+uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 {
 	struct uevent *earlier, *tmp;
 
-	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, tmpq, node) {
+	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, &st->uevq, node) {
 		/*
 		 * filter unnessary earlier uevents
 		 * by the later uevent
@@ -382,17 +366,16 @@ uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **s
 				earlier->kernel, earlier->action,
 				later->kernel, later->action);
 
-			uevent_delete_from_list(earlier, &tmp, stop);
+			uevent_delete_from_list(earlier, &tmp, &st->old_tail);
 		}
 	}
 }
 
-static void
-uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
+static void uevent_merge(struct uevent *later, struct uevent_filter_state *st)
 {
 	struct uevent *earlier, *tmp;
 
-	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, tmpq, node) {
+	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, &st->uevq, node) {
 		if (merge_need_stop(earlier, later))
 			break;
 		/*
@@ -404,8 +387,8 @@ uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **st
 				later->action, later->kernel, later->wwid);
 
 			/* See comment in uevent_delete_from_list() */
-			if (&earlier->node == *stop)
-				*stop = earlier->node.prev;
+			if (&earlier->node == st->old_tail)
+				st->old_tail = earlier->node.prev;
 
 			list_move(&earlier->node, &later->merge_node);
 			list_splice_init(&earlier->merge_node,
@@ -414,16 +397,15 @@ uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **st
 	}
 }
 
-static void
-merge_uevq(struct list_head *tmpq, struct list_head *stop)
+static void merge_uevq(struct uevent_filter_state *st)
 {
 	struct uevent *later;
 
-	uevent_prepare(tmpq, stop);
-	list_for_some_entry_reverse(later, tmpq, stop, node) {
-		uevent_filter(later, tmpq, &stop);
-		if(uevent_need_merge())
-			uevent_merge(later, tmpq, &stop);
+	uevent_prepare(st);
+	list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node) {
+		uevent_filter(later, st);
+		if(uevent_need_merge(st->conf))
+			uevent_merge(later, st);
 	}
 }
 
@@ -475,41 +457,45 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		    void * trigger_data)
 {
-	LIST_HEAD(uevq_work);
+	struct uevent_filter_state filter_state;
 
+	INIT_LIST_HEAD(&filter_state.uevq);
 	my_uev_trigger = uev_trigger;
 	my_trigger_data = trigger_data;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
-	pthread_cleanup_push(cleanup_uevq, &uevq_work);
+	pthread_cleanup_push(cleanup_uevq, &filter_state.uevq);
 	while (1) {
-		struct list_head *stop;
-
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
 
-		servicing_uev = !list_empty(&uevq_work);
+		servicing_uev = !list_empty(&filter_state.uevq);
 
-		while (list_empty(&uevq_work) && list_empty(&uevq))
+		while (list_empty(&filter_state.uevq) && list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
 
 		servicing_uev = 1;
 		/*
-		 * "stop" is the list element towards which merge_uevq()
-		 * will iterate: the last element of uevq_work before
-		 * appending new uevents. If uveq_is empty, uevq_work.prev
-		 * equals &uevq_work, which is what we need.
+		 * "old_tail" is the list element towards which merge_uevq()
+		 * will iterate: the last element of uevq before
+		 * appending new uevents. If uveq  empty, uevq.prev
+		 * equals &uevq, which is what we need.
 		 */
-		stop = uevq_work.prev;
-		list_splice_tail_init(&uevq, &uevq_work);
+		filter_state.old_tail = filter_state.uevq.prev;
+		list_splice_tail_init(&uevq, &filter_state.uevq);
 		pthread_cleanup_pop(1);
 
 		if (!my_uev_trigger)
 			break;
 
-		merge_uevq(&uevq_work, stop);
-		service_uevq(&uevq_work);
+
+		pthread_cleanup_push(put_multipath_config, filter_state.conf);
+		filter_state.conf = get_multipath_config();
+		merge_uevq(&filter_state);
+		pthread_cleanup_pop(1);
+
+		service_uevq(&filter_state.uevq);
 	}
 	pthread_cleanup_pop(1);
 	condlog(3, "Terminating uev service queue");
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 61ca1b5..53a7ca2 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -10,6 +10,7 @@
 #define OBJECT_SIZE			512
 
 struct udev;
+struct config;
 
 struct uevent {
 	struct list_head node;
@@ -31,7 +32,7 @@ int uevent_listen(struct udev *udev);
 int uevent_dispatch(int (*store_uev)(struct uevent *, void * trigger_data),
 		    void * trigger_data);
 bool uevent_is_mpath(const struct uevent *uev);
-void uevent_get_wwid(struct uevent *uev);
+void uevent_get_wwid(struct uevent *uev, const struct config *conf);
 
 int uevent_get_env_positive_int(const struct uevent *uev,
 				const char *attr);
diff --git a/tests/uevent.c b/tests/uevent.c
index 7523fec..6a010ab 100644
--- a/tests/uevent.c
+++ b/tests/uevent.c
@@ -111,7 +111,7 @@ static void test_uid_attrs(void **state)
 static void test_wwid(void **state)
 {
 	struct uevent *uev = *state;
-	uevent_get_wwid(uev);
+	uevent_get_wwid(uev, &conf);
 
 	assert_string_equal(uev->wwid, WWID);
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 09/15] libmultipath: uevent: log statistics about filtering and merging

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 3489 bytes --]

From: Martin Wilck <mwilck@suse.com>

also, log sleep/wakeup in uevent_dispatch().

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 8809f5c..a3dd752 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -67,8 +67,17 @@ struct uevent_filter_state {
 	struct list_head uevq;
 	struct list_head *old_tail;
 	struct config *conf;
+	unsigned long added;
+	unsigned long discarded;
+	unsigned long filtered;
+	unsigned long merged;
 };
 
+static void reset_filter_state(struct uevent_filter_state *st)
+{
+	st->added = st->discarded = st->filtered = st->merged = 0;
+}
+
 int is_uevent_busy(void)
 {
 	int empty;
@@ -311,6 +320,8 @@ static void uevent_delete_from_list(struct uevent *to_delete,
 		struct uevent *last = list_entry(to_delete->merge_node.prev,
 						 typeof(*last), node);
 
+		condlog(3, "%s: deleted uevent \"%s %s\" with merged uevents",
+			__func__, to_delete->action, to_delete->kernel);
 		list_splice(&to_delete->merge_node, &(*previous)->node);
 		*previous = last;
 	}
@@ -340,8 +351,11 @@ static void uevent_prepare(struct uevent_filter_state *st)
 	struct uevent *uev, *tmp;
 
 	list_for_some_entry_reverse_safe(uev, tmp, &st->uevq, st->old_tail, node) {
+
+		st->added++;
 		if (uevent_can_discard(uev, st->conf)) {
 			uevent_delete_simple(uev);
+			st->discarded++;
 			continue;
 		}
 
@@ -367,6 +381,7 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 				later->kernel, later->action);
 
 			uevent_delete_from_list(earlier, &tmp, &st->old_tail);
+			st->filtered++;
 		}
 	}
 }
@@ -393,6 +408,7 @@ static void uevent_merge(struct uevent *later, struct uevent_filter_state *st)
 			list_move(&earlier->node, &later->merge_node);
 			list_splice_init(&earlier->merge_node,
 					 &later->merge_node);
+			st->merged++;
 		}
 	}
 }
@@ -451,6 +467,15 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 	pthread_mutex_unlock(uevq_lockp);
 }
 
+static void log_filter_state(const struct uevent_filter_state *st)
+{
+	if (st->added == 0 && st->filtered == 0 && st->merged == 0)
+		return;
+
+	condlog(3, "uevents: %lu added, %lu discarded, %lu filtered, %lu merged",
+		st->added, st->discarded, st->filtered, st->merged);
+}
+
 /*
  * Service the uevent queue.
  */
@@ -472,8 +497,11 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 
 		servicing_uev = !list_empty(&filter_state.uevq);
 
-		while (list_empty(&filter_state.uevq) && list_empty(&uevq))
+		while (list_empty(&filter_state.uevq) && list_empty(&uevq)) {
+			condlog(4, "%s: waiting for events", __func__);
 			pthread_cond_wait(uev_condp, uevq_lockp);
+			condlog(4, "%s: waking up", __func__);
+		}
 
 		servicing_uev = 1;
 		/*
@@ -489,11 +517,12 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		if (!my_uev_trigger)
 			break;
 
-
+		reset_filter_state(&filter_state);
 		pthread_cleanup_push(put_multipath_config, filter_state.conf);
 		filter_state.conf = get_multipath_config();
 		merge_uevq(&filter_state);
 		pthread_cleanup_pop(1);
+		log_filter_state(&filter_state);
 
 		service_uevq(&filter_state.uevq);
 	}
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 10/15] libmultipath: merge_uevq(): filter first, then merge

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1161 bytes --]

From: Martin Wilck <mwilck@suse.com>

Run filtering over the entire list before merging.
Otherwise it can happen that the filter (which is more efficient
at saving work) doesn't find events to drop, because they have been
merged. Also, the the merging logic needs to be run for less events.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index a3dd752..b00ae3c 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -418,11 +418,13 @@ static void merge_uevq(struct uevent_filter_state *st)
 	struct uevent *later;
 
 	uevent_prepare(st);
-	list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node) {
+
+	list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node)
 		uevent_filter(later, st);
-		if(uevent_need_merge(st->conf))
+
+	if(uevent_need_merge(st->conf))
+		list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node)
 			uevent_merge(later, st);
-	}
 }
 
 static void
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 11/15] libmultipath: uevent_filter(): filter previously merged events

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1284 bytes --]

From: Martin Wilck <mwilck@suse.com>

With the new list-appending logic, it can happen that previously
merged events can now be filtered. Do it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index b00ae3c..6e8c443 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -375,6 +375,20 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 		 * filter unnessary earlier uevents
 		 * by the later uevent
 		 */
+		if (!list_empty(&earlier->merge_node)) {
+			struct uevent *mn, *t;
+
+			list_for_each_entry_reverse_safe(mn, t, &earlier->merge_node, node) {
+				if (uevent_can_filter(mn, later)) {
+					condlog(4, "uevent: \"%s %s\" (merged into \"%s %s\") filtered by \"%s %s\"",
+						mn->action, mn->kernel,
+						earlier->action, earlier->kernel,
+						later->action, later->kernel);
+					uevent_delete_simple(mn);
+					st->filtered++;
+				}
+			}
+		}
 		if (uevent_can_filter(earlier, later)) {
 			condlog(3, "uevent: %s-%s has filtered by uevent: %s-%s",
 				earlier->kernel, earlier->action,
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 12/15] libmultipath: uevent: improve log messages

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1523 bytes --]

From: Martin Wilck <mwilck@suse.com>

Use common format "\add sdl\", and log this detail only at
verbosity 4.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 6e8c443..0dccb0b 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -390,9 +390,9 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 			}
 		}
 		if (uevent_can_filter(earlier, later)) {
-			condlog(3, "uevent: %s-%s has filtered by uevent: %s-%s",
-				earlier->kernel, earlier->action,
-				later->kernel, later->action);
+			condlog(4, "uevent: \"%s %s\" filtered by \"%s %s\"",
+				earlier->action, earlier->kernel,
+				later->action, later->kernel);
 
 			uevent_delete_from_list(earlier, &tmp, &st->old_tail);
 			st->filtered++;
@@ -411,8 +411,8 @@ static void uevent_merge(struct uevent *later, struct uevent_filter_state *st)
 		 * merge earlier uevents to the later uevent
 		 */
 		if (uevent_can_merge(earlier, later)) {
-			condlog(3, "merged uevent: %s-%s-%s with uevent: %s-%s-%s",
-				earlier->action, earlier->kernel, earlier->wwid,
+			condlog(4, "uevent: \"%s %s\" merged with \"%s %s\" for WWID %s",
+				earlier->action, earlier->kernel,
 				later->action, later->kernel, later->wwid);
 
 			/* See comment in uevent_delete_from_list() */
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 13/15] libmultipath: uevent: add debug messages for event queue

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 2281 bytes --]

From: Martin Wilck <mwilck@suse.com>

For debugging, it's useful to be able to track the event queue
in detail. Enable this at verbosity level 4.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/uevent.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 0dccb0b..5793af9 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -51,6 +51,7 @@
 #include "config.h"
 #include "blacklist.h"
 #include "devmapper.h"
+#include "strbuf.h"
 
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
@@ -441,6 +442,40 @@ static void merge_uevq(struct uevent_filter_state *st)
 			uevent_merge(later, st);
 }
 
+static void print_uev(struct strbuf *buf, struct uevent *uev)
+{
+	print_strbuf(buf, "\"%s %s\"", uev->action, uev->kernel);
+	if (!list_empty(&uev->merge_node)) {
+		struct uevent *u;
+
+		append_strbuf_str(buf, "[");
+		list_for_each_entry(u, &uev->merge_node, node)
+			print_strbuf(buf, "\"%s %s \"", u->action, u->kernel);
+		append_strbuf_str(buf, "]");
+	}
+	append_strbuf_str(buf, " ");
+}
+
+static void print_uevq(const char *msg, struct list_head *uevq)
+{
+	struct uevent *uev;
+	int i = 0;
+	STRBUF_ON_STACK(buf);
+
+	if (4 > MAX_VERBOSITY || 4 > libmp_verbosity)
+		return;
+
+	if (list_empty(uevq))
+		append_strbuf_str(&buf, "*empty*");
+	else
+		list_for_each_entry(uev, uevq, node) {
+			print_strbuf(&buf, "%d:", i++);
+			print_uev(&buf, uev);
+		}
+
+	condlog(4, "uevent queue (%s): %s", msg, steal_strbuf_str(&buf));
+}
+
 static void
 service_uevq(struct list_head *tmpq)
 {
@@ -535,11 +570,13 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 
 		reset_filter_state(&filter_state);
 		pthread_cleanup_push(put_multipath_config, filter_state.conf);
+		print_uevq("append", &filter_state.uevq);
 		filter_state.conf = get_multipath_config();
 		merge_uevq(&filter_state);
 		pthread_cleanup_pop(1);
 		log_filter_state(&filter_state);
 
+		print_uevq("merge", &filter_state.uevq);
 		service_uevq(&filter_state.uevq);
 	}
 	pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 14/15] libmultipath: apply_format(): prevent buffer overflow

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 569 bytes --]

From: Martin Wilck <mwilck@suse.com>

Potential overflow found by coverity (CID 376918).
---
 libmultipath/callout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/callout.c b/libmultipath/callout.c
index dac088c..57c3481 100644
--- a/libmultipath/callout.c
+++ b/libmultipath/callout.c
@@ -160,7 +160,7 @@ int apply_format(char * string, char * cmd, struct path * pp)
 	myfree = CALLOUT_MAX_SIZE;
 
 	if (!pos) {
-		strcpy(dst, string);
+		strlcpy(dst, string, CALLOUT_MAX_SIZE);
 		return 0;
 	}
 
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH v2 15/15] libmultipath: avoid memory leak with uid_attrs

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1334 bytes --]

From: Martin Wilck <mwilck@suse.com>

Free all vector elements when freeing uid_attrs.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c | 5 +++++
 libmultipath/dict.c   | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 7346c37..bfaf041 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -656,6 +656,9 @@ static struct config *alloc_config (void)
 
 static void _uninit_config(struct config *conf)
 {
+	void *ptr;
+	int i;
+
 	if (!conf)
 		conf = &__internal_config;
 
@@ -668,6 +671,8 @@ static void _uninit_config(struct config *conf)
 	if (conf->uid_attribute)
 		free(conf->uid_attribute);
 
+	vector_foreach_slot(&conf->uid_attrs, ptr, i)
+		free(ptr);
 	vector_reset(&conf->uid_attrs);
 
 	if (conf->getuid)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 26cbe78..3b99296 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -597,8 +597,13 @@ static int uid_attrs_handler(struct config *conf, vector strvec,
 			     const char *file, int line_nr)
 {
 	char *val;
+	void *ptr;
+	int i;
 
+	vector_foreach_slot(&conf->uid_attrs, ptr, i)
+		free(ptr);
 	vector_reset(&conf->uid_attrs);
+
 	val = set_value(strvec);
 	if (!val)
 		return 1;
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

Re: [dm-devel] [PATCH v2 00/15] Rework uevent filtering and merging

[Benjamin Marzinski]

On Mon, Apr 04, 2022 at 07:04:42PM +0200, mwilck@suse.com wrote:

FYI, these are still not coming through as plain-text messages. See

https://listman.redhat.com/archives/dm-devel/2022-April/050155.html

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

Re: [dm-devel] [PATCH v2 05/15] multipathd: reconfigure: disallow changing uid_attrs

[Benjamin Marzinski]

 On Mon, Apr 04, 2022 at 07:04:47PM +0200, mwilck@suse.com wrote:
> uevent merging by WWID relies on the uid_attrs config option. As we
> drop struct config between calls to uevent_merge(), we must be sure
> that the WWID is not changed in reconfigure().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  multipath/multipath.conf.5 |  2 ++
>  multipathd/main.c          | 59 ++++++++++++++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 605b46e..a9cd776 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -264,6 +264,8 @@ If this option is configured and matches the device
>  node name of a device, it overrides any other configured  methods for
>  determining the WWID for this device.
>  .PP
> +This option cannot be changed during runtime with the multipathd \fBreconfigure\fR command.
> +.PP
>  The default is: \fB<unset>\fR. To enable uevent merging, set it e.g. to
>  \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
>  .RE
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 13b1948..6808ad1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2835,11 +2835,58 @@ void rcu_free_config(struct rcu_head *head)
>  	free_config(conf);
>  }
>  
> +static bool reconfigure_check_uid_attrs(const struct _vector *old_attrs,
> +					const struct _vector *new_attrs)
> +{
> +	int i;
> +	char *old;
> +
> +	if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
> +		return true;
> +
> +	vector_foreach_slot(old_attrs, old, i) {
> +		char *new = VECTOR_SLOT(new_attrs, i);
> +
> +		if (strcmp(old, new))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void reconfigure_check(struct config *old, struct config *new)
> +{
> +	int old_marginal_pathgroups;
> +
> +	old_marginal_pathgroups = old->marginal_pathgroups;
> +	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> +	    (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
> +		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
> +			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
> +			"off" : "on");
> +		new->marginal_pathgroups = old_marginal_pathgroups;
> +	}
> +
> +	if (reconfigure_check_uid_attrs(&old->uid_attrs, &new->uid_attrs)) {
> +		int i;
> +		void *ptr;
> +
> +		condlog(1, "multipathd must be restarted to change uid_attrs, keeping old values");
> +		vector_foreach_slot(&new->uid_attrs, ptr, i)
> +			free(ptr);
> +		vector_reset(&new->uid_attrs);
> +		new->uid_attrs = old->uid_attrs;
> +
> +		/* avoid uid_attrs being freed in rcu_free_config() */
> +		old->uid_attrs.allocated = 0;
> +		old->uid_attrs.slot = NULL;
> +	}
> +}
> +
>  static int
>  reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
>  {
>  	struct config * old, *conf;
> -	int old_marginal_pathgroups;
>  
>  	conf = load_config(DEFAULT_CONFIGFILE);
>  	if (!conf)
> @@ -2870,14 +2917,8 @@ reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
>  	uxsock_timeout = conf->uxsock_timeout;
>  
>  	old = rcu_dereference(multipath_conf);
> -	old_marginal_pathgroups = old->marginal_pathgroups;
> -	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> -	    (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
> -		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
> -			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
> -			"off" : "on");
> -		conf->marginal_pathgroups = old_marginal_pathgroups;
> -	}
> +	reconfigure_check(old, conf);
> +
>  	conf->sequence_nr = old->sequence_nr + 1;
>  	rcu_assign_pointer(multipath_conf, conf);
>  	call_rcu(&old->rcu, rcu_free_config);
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 14/15] libmultipath: apply_format(): prevent buffer overflow

[Benjamin Marzinski]

 On Mon, Apr 04, 2022 at 07:04:56PM +0200, mwilck@suse.com wrote:
> Potential overflow found by coverity (CID 376918).

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmultipath/callout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libmultipath/callout.c b/libmultipath/callout.c
> index dac088c..57c3481 100644
> --- a/libmultipath/callout.c
> +++ b/libmultipath/callout.c
> @@ -160,7 +160,7 @@ int apply_format(char * string, char * cmd, struct path * pp)
>  	myfree = CALLOUT_MAX_SIZE;
>  
>  	if (!pos) {
> -		strcpy(dst, string);
> +		strlcpy(dst, string, CALLOUT_MAX_SIZE);
>  		return 0;
>  	}
>  
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 15/15] libmultipath: avoid memory leak with uid_attrs

[Benjamin Marzinski]

On Mon, Apr 04, 2022 at 07:04:57PM +0200, mwilck@suse.com wrote:
> Free all vector elements when freeing uid_attrs.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@rehdat.com>

> ---
>  libmultipath/config.c | 5 +++++
>  libmultipath/dict.c   | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 7346c37..bfaf041 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -656,6 +656,9 @@ static struct config *alloc_config (void)
>  
>  static void _uninit_config(struct config *conf)
>  {
> +	void *ptr;
> +	int i;
> +
>  	if (!conf)
>  		conf = &__internal_config;
>  
> @@ -668,6 +671,8 @@ static void _uninit_config(struct config *conf)
>  	if (conf->uid_attribute)
>  		free(conf->uid_attribute);
>  
> +	vector_foreach_slot(&conf->uid_attrs, ptr, i)
> +		free(ptr);
>  	vector_reset(&conf->uid_attrs);
>  
>  	if (conf->getuid)
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 26cbe78..3b99296 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -597,8 +597,13 @@ static int uid_attrs_handler(struct config *conf, vector strvec,
>  			     const char *file, int line_nr)
>  {
>  	char *val;
> +	void *ptr;
> +	int i;
>  
> +	vector_foreach_slot(&conf->uid_attrs, ptr, i)
> +		free(ptr);
>  	vector_reset(&conf->uid_attrs);
> +
>  	val = set_value(strvec);
>  	if (!val)
>  		return 1;
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel