[dm-devel] [PATCH v3 00/35] multipathd: uxlsnr overhaul

[dm-devel] [PATCH v3 00/35] multipathd: uxlsnr overhaul

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hello Christophe, hello Ben,

The current multipathd unix listener code has various deficiencies.

 - client disconnects aren't handled correctly,
 - the uxsock_timeout is applied for receiving, handling, and
   responding to the client requests separately, rather than for
   the entire operation,
 - timeouts are logged, but not acted upon, causing the timeout
   to be noticed in the client rather than in the server.
 - clients may see a timeout while "reconfigure" is running,
 - unpriviledged (non-root) client connections don't work
   correctly
 - most importantly, the code busy-loops, polls, or waits in
   various places in called subroutines, which is a no-go in a
   piece of code designed as an event handler and may lead
   to spurious timeouts and delayed reaction e.g. to signals
   or client requests.

This patch set approaches all these issues. Fixing the last one,
in particular, requires a major refactoring of the uxlsnr code.
Overall, the reliability and latency of client request handling
and signal handling by multipathd should be noticeably improved
by this patch set.

The biggest problem (waiting for the vecs lock in a client handler)
can only be fixed by moving this wait into the handlers ppoll()
loop (another possible fix would have been to handle all clients
in separate threads, but that would have required even more
complexity). The patch set achieves this by adding an eventfd-based
notification mechanism to the vecs lock, which can be passed to
ppoll() to wake up when the lock is freed.

Furthermore, client requests can't be handled in a single poll
iteration any more. Therefore the client connection becomes stateful,
and is handled by a state machine using the states RECEIVE, PARSE,
WAIT FOR LOCK, WORK, and SEND.

The refactoring is done step by step for ease (hopefully) of
review. 1/35-4/35 add utility code that will be used by the uxlsnr
refactoring. 5/35-7/35 are some independent patches that
aren't directly related to uxlnsr, but fix issues that I observed
while working on this set.

8/35-13/35 are minor fixups in the client handling code. This code is
strongly related to the uxlsnr, thus I thought I'd rather fix it
before making the other changes. In 25/35, the cli-handlers are
converted to use the strbuf API everywhere instead of separate "reply"
and "len" arguments. 15/35-18/35 are minor fixes for the
uxlsnr. 19/35-34/35 are the actual refactoring patches for the uxlsnr
code. First I move some code around unchanged, then I add the
state machine (handle_client()) and move the code into it piece
by piece. 35/35 adds a fix for the client side (multipathd -k).

CC'ing Lixiaokeng and Chongyun Wu, as they have test cases that use
the client code heavily AFAIR. Testing by 3rd parties would be
very welcome.

Cheers,
Martin

---

Changes wrt v2 (Ben Marzinski):

  - Rebased the series upon Lixiaokeng's recent patch 'remove unuseful
    MALLOC/REALLOC/STRDUP/FREE'.
  - changed indentation from spaces to tabs in multiple patches
  - 03/35: Fixed comment in libmultipath.version
  - 18/35, 30/35: Renamed CLT_WAIT_LOCK to CLT_LOCKED_WORK
  - 30/35: fatal error if idle_fd allocation fails
  - 32/35: check for POLLOUT before trying to send reply

I didn't change the switch statement in 30/35 to an if because another
switch clause is added in 32/35, as discussed during the review of the v2 series.

I don't repost Ben's reconfigure series which I added to the v2 submission;
it's unchanged and unaffected by the rebase.

While there are numerous minor changes mostly because of the rebase and the
whitespace fixes, I took the liberty to keep Ben's Reviewed-by: tags in the patches.

Changes wrt v1 (Ben Marzinski):

  03: this is a major library version change.
  07: make set_config_state() static
  12: further simplify add_handler, make it static, and use assert
        to check for multiply-defined handlers
  14: dropped in favor of Ben's "reconfigure all" set, numbering changes
        from here on
  29 (was 30): don't use fallthrough; call state machine in a loop instead.
     fix signedness of return codes. Fix double messages.
  30 (was 31): The lock handling in this patch was broken. It could happen that
     the uxlsnr was cancelled without releasing the lock. Fixed by
     simplification. 
  35 (new): Use recv() for getting the command length, as suggested by Ben.

Comments welcome, regards,
Martin

Martin Wilck (35):
  libmultipath: add timespeccmp() utility function
  libmultipath: add trylock() helper
  libmultipath: add optional wakeup functionality to lock.c
  libmultipath: print: add __snprint_config()
  libmultipath: improve cleanup of uevent queues on exit
  multipathd: fix systemd notification when stopping while reloading
  multipathd: improve delayed reconfigure
  multipathd: cli.h: formatting improvements
  multipathd: cli_del_map: fix reply for delayed action
  multipathd: add prototype for cli_handler functions
  multipathd: make all cli_handlers static
  multipathd: add and set cli_handlers in a single step
  multipathd: cli.c: use ESRCH for "command not found"
  multipathd: uxlsnr: avoid stalled clients during reconfigure
  multipathd: uxlsnr: handle client HUP
  multipathd: uxlsnr: use symbolic values for pollfd indices
  multipathd: uxlsnr: avoid using fd -1 in ppoll()
  multipathd: uxlsnr: data structure for stateful client connection
  multipathd: move uxsock_trigger() to uxlsnr.c
  multipathd: move parse_cmd() to uxlsnr.c
  multipathd: uxlsnr: remove check_timeout()
  multipathd: uxlsnr: move client handling to separate function
  multipathd: uxlsnr: use main poll loop for receiving
  multipathd: use strbuf in cli_handler functions
  multipathd: uxlsnr: check root on connection startup
  multipathd: uxlsnr: pass struct client to uxsock_trigger() and
    parse_cmd()
  multipathd: uxlsnr: move handler execution to separate function
  multipathd: uxlsnr: use parser to determine non-root commands
  multipathd: uxlsnr: merge uxsock_trigger() into state machine
  multipathd: uxlsnr: add idle notification
  multipathd: uxlsnr: add timeout handling
  multipathd: uxlsnr: use poll loop for sending, too
  multipathd: uxlsnr: drop client_lock
  multipathd: uxclt: allow client mode for non-root, too
  multipathd: uxlsnr: use recv() for command length

 libmultipath/libmultipath.version |  13 +-
 libmultipath/lock.c               |  12 +-
 libmultipath/lock.h               |  11 +-
 libmultipath/print.c              |  34 +-
 libmultipath/print.h              |   2 +
 libmultipath/structs_vec.h        |   2 +-
 libmultipath/time-util.c          |  12 +
 libmultipath/time-util.h          |   1 +
 libmultipath/uevent.c             |  49 ++-
 multipathd/cli.c                  | 180 ++--------
 multipathd/cli.h                  | 100 +++---
 multipathd/cli_handlers.c         | 553 ++++++++++++++----------------
 multipathd/cli_handlers.h         |  61 +---
 multipathd/main.c                 | 220 +++++-------
 multipathd/main.h                 |   2 +-
 multipathd/uxlsnr.c               | 528 +++++++++++++++++++++-------
 multipathd/uxlsnr.h               |   4 +-
 17 files changed, 957 insertions(+), 827 deletions(-)

-- 
2.33.1


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

[dm-devel] [PATCH v3 01/35] libmultipath: add timespeccmp() utility function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add a small utility that will be used in later patches.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/libmultipath.version |  5 +++++
 libmultipath/time-util.c          | 12 ++++++++++++
 libmultipath/time-util.h          |  1 +
 3 files changed, 18 insertions(+)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index eb5b5b5..c98cf7f 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -287,3 +287,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_9.1.0 {
+global:
+	timespeccmp;
+} LIBMULTIPATH_9.0.0;
diff --git a/libmultipath/time-util.c b/libmultipath/time-util.c
index 55f366c..2919300 100644
--- a/libmultipath/time-util.c
+++ b/libmultipath/time-util.c
@@ -49,3 +49,15 @@ void timespecsub(const struct timespec *a, const struct timespec *b,
 	res->tv_nsec = a->tv_nsec - b->tv_nsec;
 	normalize_timespec(res);
 }
+
+int timespeccmp(const struct timespec *a, const struct timespec *b)
+{
+	struct timespec tmp;
+
+	timespecsub(a, b, &tmp);
+	if (tmp.tv_sec > 0)
+		return 1;
+	if (tmp.tv_sec < 0)
+		return -1;
+	return tmp.tv_nsec > 0 ? 1 : (tmp.tv_nsec < 0 ? -1 : 0);
+}
diff --git a/libmultipath/time-util.h b/libmultipath/time-util.h
index b23d328..4a80ebd 100644
--- a/libmultipath/time-util.h
+++ b/libmultipath/time-util.h
@@ -10,5 +10,6 @@ void pthread_cond_init_mono(pthread_cond_t *cond);
 void normalize_timespec(struct timespec *ts);
 void timespecsub(const struct timespec *a, const struct timespec *b,
 		 struct timespec *res);
+int timespeccmp(const struct timespec *a, const struct timespec *b);
 
 #endif /* _TIME_UTIL_H_ */
-- 
2.33.1


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

[dm-devel] [PATCH v3 02/35] libmultipath: add trylock() helper

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add a small helper.

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

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index a170efe..d99eedb 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -12,6 +12,11 @@ static inline void lock(struct mutex_lock *a)
 	pthread_mutex_lock(&a->mutex);
 }
 
+static inline int trylock(struct mutex_lock *a)
+{
+	return pthread_mutex_trylock(&a->mutex);
+}
+
 static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
 {
 	return pthread_mutex_timedlock(&a->mutex, tmo);
-- 
2.33.1


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

[dm-devel] [PATCH v3 03/35] libmultipath: add optional wakeup functionality to lock.c

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Have struct mutex_lock take an optional wakeup function.
unlock() is renamed to __unlock() in order to prevent it from
being called by mistake.

This changes offsets in "struct vectors", requiring a major
libmultipath version bump. While the strucure is already changed,
in order to avoid this in the future, move the lock to the end
of "struct vectors".

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/libmultipath.version | 13 +++++++------
 libmultipath/lock.c               | 12 +++++++++++-
 libmultipath/lock.h               |  6 +++++-
 libmultipath/structs_vec.h        |  2 +-
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index c98cf7f..83aaa83 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_9.0.0 {
+LIBMULTIPATH_10.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -284,11 +284,12 @@ global:
 	/* added in 8.2.0 */
 	check_daemon;
 
+	/* added in 9.1.0 */
+	timespeccmp;
+
+	/* added in 10.0.0 */
+	set_wakeup_fn;
+
 local:
 	*;
 };
-
-LIBMULTIPATH_9.1.0 {
-global:
-	timespeccmp;
-} LIBMULTIPATH_9.0.0;
diff --git a/libmultipath/lock.c b/libmultipath/lock.c
index 72c70e3..93b48db 100644
--- a/libmultipath/lock.c
+++ b/libmultipath/lock.c
@@ -3,6 +3,16 @@
 void cleanup_lock (void * data)
 {
 	struct mutex_lock *lock = data;
+	wakeup_fn *fn = lock->wakeup;
 
-	unlock(lock);
+	__unlock(lock);
+	if (fn)
+		fn();
+}
+
+void set_wakeup_fn(struct mutex_lock *lck, wakeup_fn *fn)
+{
+	lock(lck);
+	lck->wakeup = fn;
+	__unlock(lck);
 }
diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index d99eedb..d7b779e 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -3,8 +3,11 @@
 
 #include <pthread.h>
 
+typedef void (wakeup_fn)(void);
+
 struct mutex_lock {
 	pthread_mutex_t mutex;
+	wakeup_fn *wakeup;
 };
 
 static inline void lock(struct mutex_lock *a)
@@ -22,7 +25,7 @@ static inline int timedlock(struct mutex_lock *a, struct timespec *tmo)
 	return pthread_mutex_timedlock(&a->mutex, tmo);
 }
 
-static inline void unlock(struct mutex_lock *a)
+static inline void __unlock(struct mutex_lock *a)
 {
 	pthread_mutex_unlock(&a->mutex);
 }
@@ -30,5 +33,6 @@ static inline void unlock(struct mutex_lock *a)
 #define lock_cleanup_pop(a) pthread_cleanup_pop(1)
 
 void cleanup_lock (void * data);
+void set_wakeup_fn(struct mutex_lock *lock, wakeup_fn *fn);
 
 #endif /* _LOCK_H */
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 29ede45..2a0cbd1 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -6,9 +6,9 @@
 #include "lock.h"
 
 struct vectors {
-	struct mutex_lock lock; /* defined in lock.h */
 	vector pathvec;
 	vector mpvec;
+	struct mutex_lock lock; /* defined in lock.h */
 };
 
 void __set_no_path_retry(struct multipath *mpp, bool check_features);
-- 
2.33.1


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

[dm-devel] [PATCH v3 04/35] libmultipath: print: add __snprint_config()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

exactly like snprint_config(), but takes a struct strbuf * as argument.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/libmultipath.version |  5 +++++
 libmultipath/print.c              | 34 +++++++++++++++++++++----------
 libmultipath/print.h              |  2 ++
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 83aaa83..547a71c 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -293,3 +293,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_10.1.0 {
+global:
+	__snprint_config;
+} LIBMULTIPATH_10.0.0;
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 2fb9f4e..d2ef010 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -1756,24 +1756,36 @@ static int snprint_blacklist_except(const struct config *conf,
 	return get_strbuf_len(buff) - initial_len;
 }
 
+int __snprint_config(const struct config *conf, struct strbuf *buff,
+		     const struct _vector *hwtable, const struct _vector *mpvec)
+{
+	int rc;
+
+	if ((rc = snprint_defaults(conf, buff)) < 0 ||
+	    (rc = snprint_blacklist(conf, buff)) < 0 ||
+	    (rc = snprint_blacklist_except(conf, buff)) < 0 ||
+	    (rc = snprint_hwtable(conf, buff,
+				  hwtable ? hwtable : conf->hwtable)) < 0 ||
+	    (rc = snprint_overrides(conf, buff, conf->overrides)) < 0)
+		return rc;
+
+	if (VECTOR_SIZE(conf->mptable) > 0 ||
+	    (mpvec != NULL && VECTOR_SIZE(mpvec) > 0))
+		if ((rc = snprint_mptable(conf, buff, mpvec)) < 0)
+			return rc;
+
+	return 0;
+}
+
 char *snprint_config(const struct config *conf, int *len,
 		     const struct _vector *hwtable, const struct _vector *mpvec)
 {
 	STRBUF_ON_STACK(buff);
 	char *reply;
-	int rc;
+	int rc = __snprint_config(conf, &buff, hwtable, mpvec);
 
-	if ((rc = snprint_defaults(conf, &buff)) < 0 ||
-	    (rc = snprint_blacklist(conf, &buff)) < 0 ||
-	    (rc = snprint_blacklist_except(conf, &buff)) < 0 ||
-	    (rc = snprint_hwtable(conf, &buff,
-				  hwtable ? hwtable : conf->hwtable)) < 0 ||
-	    (rc = snprint_overrides(conf, &buff, conf->overrides)) < 0)
+	if (rc < 0)
 		return NULL;
-	if (VECTOR_SIZE(conf->mptable) > 0 ||
-	    (mpvec != NULL && VECTOR_SIZE(mpvec) > 0))
-		if ((rc = snprint_mptable(conf, &buff, mpvec)) < 0)
-			return NULL;
 
 	if (len)
 		*len = get_strbuf_len(&buff);
diff --git a/libmultipath/print.h b/libmultipath/print.h
index c6674a5..b149275 100644
--- a/libmultipath/print.h
+++ b/libmultipath/print.h
@@ -54,6 +54,8 @@ int _snprint_multipath_topology (const struct gen_multipath *, struct strbuf *,
 #define snprint_multipath_topology(buf, mpp, v) \
 	_snprint_multipath_topology (dm_multipath_to_gen(mpp), buf, v)
 int snprint_multipath_topology_json(struct strbuf *, const struct vectors *vecs);
+int __snprint_config(const struct config *conf, struct strbuf *buff,
+		     const struct _vector *hwtable, const struct _vector *mpvec);
 char *snprint_config(const struct config *conf, int *len,
 		     const struct _vector *hwtable,
 		     const struct _vector *mpvec);
-- 
2.33.1


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

[dm-devel] [PATCH v3 05/35] libmultipath: improve cleanup of uevent queues on exit

[mwilck]

From: Martin Wilck <mwilck@suse.com>

uevents listed on merge_node must be cleaned up, too. uevents
cancelled while being serviced and temporary queues, likewise.
The global uevq must be cleaned out in the uevent listener thread,
because it might have added events after the dispatcher thread
had already finished.

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

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index fd4556c..70ad217 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -90,16 +90,25 @@ struct uevent * alloc_uevent (void)
 	return uev;
 }
 
+static void uevq_cleanup(struct list_head *tmpq);
+
+static void cleanup_uev(void *arg)
+{
+	struct uevent *uev = arg;
+
+	uevq_cleanup(&uev->merge_node);
+	if (uev->udev)
+		udev_device_unref(uev->udev);
+	free(uev);
+}
+
 static void uevq_cleanup(struct list_head *tmpq)
 {
 	struct uevent *uev, *tmp;
 
 	list_for_each_entry_safe(uev, tmp, tmpq, node) {
 		list_del_init(&uev->node);
-
-		if (uev->udev)
-			udev_device_unref(uev->udev);
-		free(uev);
+		cleanup_uev(uev);
 	}
 }
 
@@ -383,14 +392,10 @@ service_uevq(struct list_head *tmpq)
 	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");
-
-		uevq_cleanup(&uev->merge_node);
-
-		if (uev->udev)
-			udev_device_unref(uev->udev);
-		free(uev);
+		pthread_cleanup_pop(1);
 	}
 }
 
@@ -410,6 +415,18 @@ static void monitor_cleanup(void *arg)
 	udev_monitor_unref(monitor);
 }
 
+static void cleanup_uevq(void *arg)
+{
+	uevq_cleanup(arg);
+}
+
+static void cleanup_global_uevq(void *arg __attribute__((unused)))
+{
+	pthread_mutex_lock(uevq_lockp);
+	uevq_cleanup(&uevq);
+	pthread_mutex_unlock(uevq_lockp);
+}
+
 /*
  * Service the uevent queue.
  */
@@ -424,6 +441,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 	while (1) {
 		LIST_HEAD(uevq_tmp);
 
+		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
 		servicing_uev = 0;
 		/*
@@ -435,14 +453,17 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		}
 		servicing_uev = 1;
 		list_splice_init(&uevq, &uevq_tmp);
-		pthread_mutex_unlock(uevq_lockp);
+		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);
 	}
 	condlog(3, "Terminating uev service queue");
-	uevq_cleanup(&uevq);
 	return 0;
 }
 
@@ -599,6 +620,8 @@ int uevent_listen(struct udev *udev)
 
 	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;
@@ -649,6 +672,8 @@ int uevent_listen(struct udev *udev)
 		gettimeofday(&start_time, NULL);
 		timeout = 30;
 	}
+	pthread_cleanup_pop(1);
+	pthread_cleanup_pop(1);
 out:
 	pthread_cleanup_pop(1);
 out_udev:
-- 
2.33.1


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

[dm-devel] [PATCH v3 06/35] multipathd: fix systemd notification when stopping while reloading

[mwilck]

From: Martin Wilck <mwilck@suse.com>

After sending "RELOADING=1" to systemd, a service must send
"READY=1" before "STOPPING=1". Otherwise systemd will be confused
and will not regard the service as stopped. Subsequent attempts
to start multipathd via socket activation fail until systemd times
out the reload operation.

The problem can be reproduced by running "multipathd shutdown"
quickly after "multipathd reconfigure".

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 3f67513..8f2940e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -209,9 +209,12 @@ static void do_sd_notify(enum daemon_status old_state,
 	if (msg && !safe_sprintf(notify_msg, "STATUS=%s", msg))
 		sd_notify(0, notify_msg);
 
-	if (new_state == DAEMON_SHUTDOWN)
+	if (new_state == DAEMON_SHUTDOWN) {
+		/* Tell systemd that we're not RELOADING any more */
+		if (old_state == DAEMON_CONFIGURE && startup_done)
+			sd_notify(0, "READY=1");
 		sd_notify(0, "STOPPING=1");
-	else if (new_state == DAEMON_IDLE && old_state == DAEMON_CONFIGURE) {
+	} else if (new_state == DAEMON_IDLE && old_state == DAEMON_CONFIGURE) {
 		sd_notify(0, "READY=1");
 		startup_done = true;
 	} else if (new_state == DAEMON_CONFIGURE && startup_done)
-- 
2.33.1


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

[dm-devel] [PATCH v3 07/35] multipathd: improve delayed reconfigure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

When a reconfigure operation is requested, either by the admin
or by some condition multipathd encounters, the current code
attempts to set DAEMON_CONFIGURE state and gives up after a second
if it doesn't succeed. Apart from shutdown, this happens only
if multipathd is either already reconfiguring, or busy in the
path checker loop.

This patch modifies the logic as follows: rather than waiting,
we set a flag that requests a reconfigure operation asap, i.e.
when the current operation is finished and the status switched
to DAEMON_IDLE. In this case, multipathd will not switch to IDLE
but start another reconfigure cycle.

This assumes that if a reconfigure is requested while one is already
running, the admin has made some (additional) changes and wants
multipathd to pull them in. As we can't be sure that the currently
running reconfigure has seen the configuration changes, we need
to start over again.

A positive side effect is less waiting in clients and multipathd.

After this change, the only caller of set_config_state() is
checkerloop(). Waking up every second just to see that DAEMON_RUNNING
couldn't be set makes no sense. Therefore set_config_state() is
changed to wait "forever", or until shutdown is requested. Unless
multipathd completely hangs, the wait will terminate sooner or
later.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli_handlers.c | 10 +----
 multipathd/main.c         | 92 +++++++++++++++++++++++++++++----------
 multipathd/main.h         |  2 +-
 3 files changed, 70 insertions(+), 34 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index b674a14..f283e95 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1075,17 +1075,9 @@ cli_switch_group(void * v, char ** reply, int * len, void * data)
 int
 cli_reconfigure(void * v, char ** reply, int * len, void * data)
 {
-	int rc;
-
 	condlog(2, "reconfigure (operator)");
 
-	rc = set_config_state(DAEMON_CONFIGURE);
-	if (rc == ETIMEDOUT) {
-		condlog(2, "timeout starting reconfiguration");
-		return 1;
-	} else if (rc == EINVAL)
-		/* daemon shutting down */
-		return 1;
+	schedule_reconfigure();
 	return 0;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 8f2940e..6054fd5 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -220,6 +220,10 @@ static void do_sd_notify(enum daemon_status old_state,
 	} else if (new_state == DAEMON_CONFIGURE && startup_done)
 		sd_notify(0, "RELOADING=1");
 }
+#else
+static void do_sd_notify(__attribute__((unused)) enum daemon_status old_state,
+			 __attribute__((unused)) enum daemon_status new_state)
+{}
 #endif
 
 static void config_cleanup(__attribute__((unused)) void *arg)
@@ -265,19 +269,38 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 	return st;
 }
 
+/* Don't access this variable without holding config_lock */
+static bool reconfigure_pending;
+
 /* must be called with config_lock held */
 static void __post_config_state(enum daemon_status state)
 {
 	if (state != running_state && running_state != DAEMON_SHUTDOWN) {
-#ifdef USE_SYSTEMD
 		enum daemon_status old_state = running_state;
-#endif
 
+		/*
+		 * Handle a pending reconfigure request.
+		 * DAEMON_IDLE is set from child() after reconfigure(),
+		 * or from checkerloop() after completing checkers.
+		 * In either case, child() will see DAEMON_CONFIGURE
+		 * again and start another reconfigure cycle.
+		 */
+		if (reconfigure_pending && state == DAEMON_IDLE &&
+		    (old_state == DAEMON_CONFIGURE ||
+		     old_state == DAEMON_RUNNING)) {
+			/*
+			 * notify systemd of transient idle state, lest systemd
+			 * thinks the reload lasts forever.
+			 */
+			do_sd_notify(old_state, DAEMON_IDLE);
+			old_state = DAEMON_IDLE;
+			state = DAEMON_CONFIGURE;
+		}
+		if (reconfigure_pending && state == DAEMON_CONFIGURE)
+			reconfigure_pending = false;
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
-#ifdef USE_SYSTEMD
 		do_sd_notify(old_state, state);
-#endif
 	}
 }
 
@@ -289,24 +312,48 @@ void post_config_state(enum daemon_status state)
 	pthread_cleanup_pop(1);
 }
 
-int set_config_state(enum daemon_status state)
+void schedule_reconfigure(void)
+{
+	pthread_mutex_lock(&config_lock);
+	pthread_cleanup_push(config_cleanup, NULL);
+	switch (running_state)
+	{
+	case DAEMON_SHUTDOWN:
+		break;
+	case DAEMON_IDLE:
+		__post_config_state(DAEMON_CONFIGURE);
+		break;
+	case DAEMON_CONFIGURE:
+	case DAEMON_RUNNING:
+		reconfigure_pending = true;
+		break;
+	default:
+		break;
+	}
+	pthread_cleanup_pop(1);
+}
+
+static enum daemon_status set_config_state(enum daemon_status state)
 {
 	int rc = 0;
+	enum daemon_status st;
 
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
-	if (running_state != state) {
 
-		if (running_state == DAEMON_SHUTDOWN)
-			rc = EINVAL;
-		else
-			rc = __wait_for_state_change(
-				running_state != DAEMON_IDLE, 1000);
-		if (!rc)
-			__post_config_state(state);
+	while (rc == 0 &&
+	       running_state != state &&
+	       running_state != DAEMON_SHUTDOWN &&
+	       running_state != DAEMON_IDLE) {
+		rc = pthread_cond_wait(&config_cond, &config_lock);
 	}
+
+	if (rc == 0 && running_state == DAEMON_IDLE && state != DAEMON_IDLE)
+		__post_config_state(state);
+	st = running_state;
+
 	pthread_cleanup_pop(1);
-	return rc;
+	return st;
 }
 
 struct config *get_multipath_config(void)
@@ -744,7 +791,7 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
 			if (delayed_reconfig &&
 			    !need_to_delay_reconfig(vecs)) {
 				condlog(2, "reconfigure (delayed)");
-				set_config_state(DAEMON_CONFIGURE);
+				schedule_reconfigure();
 				return 0;
 			}
 		}
@@ -1855,7 +1902,7 @@ missing_uev_wait_tick(struct vectors *vecs)
 	if (timed_out && delayed_reconfig &&
 	    !need_to_delay_reconfig(vecs)) {
 		condlog(2, "reconfigure (delayed)");
-		set_config_state(DAEMON_CONFIGURE);
+		schedule_reconfigure();
 	}
 }
 
@@ -2494,6 +2541,10 @@ checkerloop (void *ap)
 		int num_paths = 0, strict_timing, rc = 0;
 		unsigned int ticks = 0;
 
+		if (set_config_state(DAEMON_RUNNING) != DAEMON_RUNNING)
+			/* daemon shutdown */
+			break;
+
 		get_monotonic_time(&start_time);
 		if (start_time.tv_sec && last_time.tv_sec) {
 			timespecsub(&start_time, &last_time, &diff_time);
@@ -2509,13 +2560,6 @@ checkerloop (void *ap)
 		if (use_watchdog)
 			sd_notify(0, "WATCHDOG=1");
 #endif
-		rc = set_config_state(DAEMON_RUNNING);
-		if (rc == ETIMEDOUT) {
-			condlog(4, "timeout waiting for DAEMON_IDLE");
-			continue;
-		} else if (rc == EINVAL)
-			/* daemon shutdown */
-			break;
 
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		lock(&vecs->lock);
@@ -2843,7 +2887,7 @@ handle_signals(bool nonfatal)
 		return;
 	if (reconfig_sig) {
 		condlog(2, "reconfigure (signal)");
-		set_config_state(DAEMON_CONFIGURE);
+		schedule_reconfigure();
 	}
 	if (log_reset_sig) {
 		condlog(2, "reset log (signal)");
diff --git a/multipathd/main.h b/multipathd/main.h
index bc1f938..2960a4d 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -37,6 +37,7 @@ void exit_daemon(void);
 const char * daemon_status(void);
 enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 					    unsigned long ms);
+void schedule_reconfigure(void);
 int need_to_delay_reconfig (struct vectors *);
 int reconfigure (struct vectors *);
 int ev_add_path (struct path *, struct vectors *, int);
@@ -44,7 +45,6 @@ int ev_remove_path (struct path *, struct vectors *, int);
 int ev_add_map (char *, const char *, struct vectors *);
 int ev_remove_map (char *, char *, int, struct vectors *);
 int flush_map(struct multipath *, struct vectors *, int);
-int set_config_state(enum daemon_status);
 void * mpath_alloc_prin_response(int prin_sa);
 int prin_do_scsi_ioctl(char *, int rq_servact, struct prin_resp * resp,
 		       int noisy);
-- 
2.33.1


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

[dm-devel] [PATCH v3 08/35] multipathd: cli.h: formatting improvements

[mwilck]

From: Martin Wilck <mwilck@suse.com>

No functional changes. Just make the code a little easier to read.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli.h | 82 ++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/multipathd/cli.h b/multipathd/cli.h
index d224a2d..6a68107 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -4,83 +4,83 @@
 #include <stdint.h>
 
 enum {
-	__LIST,
+	__LIST,			/*  0 */
 	__ADD,
 	__DEL,
 	__SWITCH,
 	__SUSPEND,
-	__RESUME,
+	__RESUME,			/*  5 */
 	__REINSTATE,
 	__FAIL,
 	__RESIZE,
 	__RESET,
-	__RELOAD,
+	__RELOAD,			/* 10 */
 	__FORCEQ,
 	__DISABLEQ,
 	__RESTOREQ,
 	__PATHS,
-	__MAPS,
+	__MAPS,			/* 15 */
 	__PATH,
 	__MAP,
 	__GROUP,
 	__RECONFIGURE,
-	__DAEMON,
+	__DAEMON,			/* 20 */
 	__STATUS,
 	__STATS,
 	__TOPOLOGY,
 	__CONFIG,
-	__BLACKLIST,
+	__BLACKLIST,			/* 25 */
 	__DEVICES,
 	__RAW,
 	__WILDCARDS,
 	__QUIT,
-	__SHUTDOWN,
+	__SHUTDOWN,			/* 30 */
 	__GETPRSTATUS,
 	__SETPRSTATUS,
 	__UNSETPRSTATUS,
 	__FMT,
-	__JSON,
+	__JSON,			/* 35 */
 	__GETPRKEY,
 	__SETPRKEY,
 	__UNSETPRKEY,
 	__KEY,
-	__LOCAL,
+	__LOCAL,			/* 40 */
 	__SETMARGINAL,
 	__UNSETMARGINAL,
 };
 
-#define LIST		(1 << __LIST)
-#define ADD		(1 << __ADD)
-#define DEL		(1 << __DEL)
-#define SWITCH		(1 << __SWITCH)
-#define SUSPEND		(1 << __SUSPEND)
-#define RESUME		(1 << __RESUME)
-#define REINSTATE	(1 << __REINSTATE)
-#define FAIL		(1 << __FAIL)
-#define RESIZE		(1 << __RESIZE)
-#define RESET		(1 << __RESET)
-#define RELOAD		(1 << __RELOAD)
-#define FORCEQ		(1 << __FORCEQ)
-#define DISABLEQ	(1 << __DISABLEQ)
-#define RESTOREQ	(1 << __RESTOREQ)
-#define PATHS		(1 << __PATHS)
-#define MAPS		(1 << __MAPS)
-#define PATH		(1 << __PATH)
-#define MAP		(1 << __MAP)
-#define GROUP		(1 << __GROUP)
-#define RECONFIGURE	(1 << __RECONFIGURE)
-#define DAEMON		(1 << __DAEMON)
-#define STATUS		(1 << __STATUS)
-#define STATS		(1 << __STATS)
-#define TOPOLOGY	(1 << __TOPOLOGY)
-#define CONFIG		(1 << __CONFIG)
-#define BLACKLIST	(1 << __BLACKLIST)
-#define DEVICES		(1 << __DEVICES)
-#define RAW		(1 << __RAW)
-#define COUNT		(1 << __COUNT)
-#define WILDCARDS	(1 << __WILDCARDS)
-#define QUIT		(1 << __QUIT)
-#define SHUTDOWN	(1 << __SHUTDOWN)
+#define LIST		(1ULL << __LIST)
+#define ADD		(1ULL << __ADD)
+#define DEL		(1ULL << __DEL)
+#define SWITCH		(1ULL << __SWITCH)
+#define SUSPEND	(1ULL << __SUSPEND)
+#define RESUME		(1ULL << __RESUME)
+#define REINSTATE	(1ULL << __REINSTATE)
+#define FAIL		(1ULL << __FAIL)
+#define RESIZE		(1ULL << __RESIZE)
+#define RESET		(1ULL << __RESET)
+#define RELOAD		(1ULL << __RELOAD)
+#define FORCEQ		(1ULL << __FORCEQ)
+#define DISABLEQ	(1ULL << __DISABLEQ)
+#define RESTOREQ	(1ULL << __RESTOREQ)
+#define PATHS		(1ULL << __PATHS)
+#define MAPS		(1ULL << __MAPS)
+#define PATH		(1ULL << __PATH)
+#define MAP		(1ULL << __MAP)
+#define GROUP		(1ULL << __GROUP)
+#define RECONFIGURE	(1ULL << __RECONFIGURE)
+#define DAEMON		(1ULL << __DAEMON)
+#define STATUS		(1ULL << __STATUS)
+#define STATS		(1ULL << __STATS)
+#define TOPOLOGY	(1ULL << __TOPOLOGY)
+#define CONFIG		(1ULL << __CONFIG)
+#define BLACKLIST	(1ULL << __BLACKLIST)
+#define DEVICES	(1ULL << __DEVICES)
+#define RAW		(1ULL << __RAW)
+#define COUNT		(1ULL << __COUNT)
+#define WILDCARDS	(1ULL << __WILDCARDS)
+#define QUIT		(1ULL << __QUIT)
+#define SHUTDOWN	(1ULL << __SHUTDOWN)
 #define GETPRSTATUS	(1ULL << __GETPRSTATUS)
 #define SETPRSTATUS	(1ULL << __SETPRSTATUS)
 #define UNSETPRSTATUS	(1ULL << __UNSETPRSTATUS)
-- 
2.33.1


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

[dm-devel] [PATCH v3 09/35] multipathd: cli_del_map: fix reply for delayed action

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Return code 2 from ev_remove_map means that a delayed remove has
been started, which is not the same as failure.

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

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index f283e95..db2708b 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -782,6 +782,9 @@ cli_del_map (void * v, char ** reply, int * len, void * data)
 		return 1;
 	}
 	rc = ev_remove_map(param, alias, minor, vecs);
+	if (rc == 2)
+		*reply = strdup("delayed");
+
 	free(alias);
 	return rc;
 }
-- 
2.33.1


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

[dm-devel] [PATCH v3 10/35] multipathd: add prototype for cli_handler functions

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Use a typedef instead of spelling out the function type everywhere.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli.c |  6 +++---
 multipathd/cli.h | 10 ++++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index bddf172..3582370 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -64,7 +64,7 @@ out:
 }
 
 int
-add_handler (uint64_t fp, int (*fn)(void *, char **, int *, void *))
+add_handler (uint64_t fp, cli_handler *fn)
 {
 	struct handler * h;
 
@@ -99,7 +99,7 @@ find_handler (uint64_t fp)
 }
 
 int
-set_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *))
+set_handler_callback (uint64_t fp, cli_handler *fn)
 {
 	struct handler * h = find_handler(fp);
 
@@ -111,7 +111,7 @@ set_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *))
 }
 
 int
-set_unlocked_handler_callback (uint64_t fp,int (*fn)(void *, char **, int *, void *))
+set_unlocked_handler_callback (uint64_t fp, cli_handler *fn)
 {
 	struct handler * h = find_handler(fp);
 
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 6a68107..84b1fbe 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -124,16 +124,18 @@ struct key {
 	int has_param;
 };
 
+typedef int (cli_handler)(void *keywords, char **reply, int *len, void *data);
+
 struct handler {
 	uint64_t fingerprint;
 	int locked;
-	int (*fn)(void *, char **, int *, void *);
+	cli_handler *fn;
 };
 
 int alloc_handlers (void);
-int add_handler (uint64_t fp, int (*fn)(void *, char **, int *, void *));
-int set_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *));
-int set_unlocked_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *));
+int add_handler (uint64_t fp, cli_handler *fn);
+int set_handler_callback (uint64_t fp, cli_handler *fn);
+int set_unlocked_handler_callback (uint64_t fp, cli_handler *fn);
 int parse_cmd (char * cmd, char ** reply, int * len, void *, int);
 int load_keys (void);
 char * get_keyparam (vector v, uint64_t code);
-- 
2.33.1


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

[dm-devel] [PATCH v3 11/35] multipathd: make all cli_handlers static

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The cli_handler functions are only called from the handler table and
need not be exported.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli_handlers.c | 214 ++++++++++++++++++++++----------------
 multipathd/cli_handlers.h |  61 ++---------
 multipathd/main.c         |  58 +----------
 3 files changed, 134 insertions(+), 199 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index db2708b..911272c 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -38,7 +38,7 @@
 		*(__len) = *(__rep) ? sizeof(string_literal) : 0;	\
 	} while (0)
 
-int
+static int
 show_paths (char ** r, int * len, struct vectors * vecs, char * style,
 	    int pretty)
 {
@@ -69,7 +69,7 @@ show_paths (char ** r, int * len, struct vectors * vecs, char * style,
 	return 0;
 }
 
-int
+static int
 show_path (char ** r, int * len, struct vectors * vecs, struct path *pp,
 	   char * style)
 {
@@ -84,7 +84,7 @@ show_path (char ** r, int * len, struct vectors * vecs, struct path *pp,
 	return 0;
 }
 
-int
+static int
 show_map_topology (char ** r, int * len, struct multipath * mpp,
 		   struct vectors * vecs)
 {
@@ -101,7 +101,7 @@ show_map_topology (char ** r, int * len, struct multipath * mpp,
 	return 0;
 }
 
-int
+static int
 show_maps_topology (char ** r, int * len, struct vectors * vecs)
 {
 	STRBUF_ON_STACK(reply);
@@ -127,7 +127,7 @@ show_maps_topology (char ** r, int * len, struct vectors * vecs)
 	return 0;
 }
 
-int
+static int
 show_maps_json (char ** r, int * len, struct vectors * vecs)
 {
 	STRBUF_ON_STACK(reply);
@@ -148,7 +148,7 @@ show_maps_json (char ** r, int * len, struct vectors * vecs)
 	return 0;
 }
 
-int
+static int
 show_map_json (char ** r, int * len, struct multipath * mpp,
 		   struct vectors * vecs)
 {
@@ -193,7 +193,7 @@ reset_stats(struct multipath * mpp)
 	mpp->stat_map_failures = 0;
 }
 
-int
+static int
 cli_list_config (void * v, char ** reply, int * len, void * data)
 {
 	condlog(3, "list config (operator)");
@@ -206,7 +206,7 @@ static void v_free(void *x)
 	vector_free(x);
 }
 
-int
+static int
 cli_list_config_local (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -222,7 +222,7 @@ cli_list_config_local (void * v, char ** reply, int * len, void * data)
 	return ret;
 }
 
-int
+static int
 cli_list_paths (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -232,7 +232,7 @@ cli_list_paths (void * v, char ** reply, int * len, void * data)
 	return show_paths(reply, len, vecs, PRINT_PATH_CHECKER, 1);
 }
 
-int
+static int
 cli_list_paths_fmt (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -243,7 +243,7 @@ cli_list_paths_fmt (void * v, char ** reply, int * len, void * data)
 	return show_paths(reply, len, vecs, fmt, 1);
 }
 
-int
+static int
 cli_list_paths_raw (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -254,7 +254,7 @@ cli_list_paths_raw (void * v, char ** reply, int * len, void * data)
 	return show_paths(reply, len, vecs, fmt, 0);
 }
 
-int
+static int
 cli_list_path (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -271,7 +271,7 @@ cli_list_path (void * v, char ** reply, int * len, void * data)
 	return show_path(reply, len, vecs, pp, "%o");
 }
 
-int
+static int
 cli_list_map_topology (void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -290,7 +290,7 @@ cli_list_map_topology (void * v, char ** reply, int * len, void * data)
 	return show_map_topology(reply, len, mpp, vecs);
 }
 
-int
+static int
 cli_list_maps_topology (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -300,7 +300,7 @@ cli_list_maps_topology (void * v, char ** reply, int * len, void * data)
 	return show_maps_topology(reply, len, vecs);
 }
 
-int
+static int
 cli_list_map_json (void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -319,7 +319,7 @@ cli_list_map_json (void * v, char ** reply, int * len, void * data)
 	return show_map_json(reply, len, mpp, vecs);
 }
 
-int
+static int
 cli_list_maps_json (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -329,7 +329,7 @@ cli_list_maps_json (void * v, char ** reply, int * len, void * data)
 	return show_maps_json(reply, len, vecs);
 }
 
-int
+static int
 cli_list_wildcards (void * v, char ** reply, int * len, void * data)
 {
 	STRBUF_ON_STACK(buf);
@@ -342,7 +342,7 @@ cli_list_wildcards (void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 show_status (char ** r, int *len, struct vectors * vecs)
 {
 	STRBUF_ON_STACK(reply);
@@ -355,7 +355,7 @@ show_status (char ** r, int *len, struct vectors * vecs)
 	return 0;
 }
 
-int
+static int
 show_daemon (char ** r, int *len)
 {
 	STRBUF_ON_STACK(reply);
@@ -369,7 +369,7 @@ show_daemon (char ** r, int *len)
 	return 0;
 }
 
-int
+static int
 show_map (char ** r, int *len, struct multipath * mpp, char * style,
 	  int pretty)
 {
@@ -383,7 +383,7 @@ show_map (char ** r, int *len, struct multipath * mpp, char * style,
 	return 0;
 }
 
-int
+static int
 show_maps (char ** r, int *len, struct vectors * vecs, char * style,
 	   int pretty)
 {
@@ -418,7 +418,7 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style,
 	return 0;
 }
 
-int
+static int
 cli_list_maps_fmt (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -429,7 +429,7 @@ cli_list_maps_fmt (void * v, char ** reply, int * len, void * data)
 	return show_maps(reply, len, vecs, fmt, 1);
 }
 
-int
+static int
 cli_list_maps_raw (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -440,7 +440,7 @@ cli_list_maps_raw (void * v, char ** reply, int * len, void * data)
 	return show_maps(reply, len, vecs, fmt, 0);
 }
 
-int
+static int
 cli_list_map_fmt (void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -460,27 +460,7 @@ cli_list_map_fmt (void * v, char ** reply, int * len, void * data)
 	return show_map(reply, len, mpp, fmt, 1);
 }
 
-int
-cli_list_map_raw (void * v, char ** reply, int * len, void * data)
-{
-	struct multipath * mpp;
-	struct vectors * vecs = (struct vectors *)data;
-	char * param = get_keyparam(v, MAP);
-	char * fmt = get_keyparam(v, FMT);
-
-	param = convert_dev(param, 0);
-	get_path_layout(vecs->pathvec, 0);
-	get_multipath_layout(vecs->mpvec, 1);
-	mpp = find_mp_by_str(vecs->mpvec, param);
-	if (!mpp)
-		return 1;
-
-	condlog(3, "list map %s fmt %s (operator)", param, fmt);
-
-	return show_map(reply, len, mpp, fmt, 0);
-}
-
-int
+static int
 cli_list_maps (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -490,7 +470,7 @@ cli_list_maps (void * v, char ** reply, int * len, void * data)
 	return show_maps(reply, len, vecs, PRINT_MAP_NAMES, 1);
 }
 
-int
+static int
 cli_list_status (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -500,7 +480,7 @@ cli_list_status (void * v, char ** reply, int * len, void * data)
 	return show_status(reply, len, vecs);
 }
 
-int
+static int
 cli_list_maps_status (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -510,7 +490,7 @@ cli_list_maps_status (void * v, char ** reply, int * len, void * data)
 	return show_maps(reply, len, vecs, PRINT_MAP_STATUS, 1);
 }
 
-int
+static int
 cli_list_maps_stats (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -520,7 +500,7 @@ cli_list_maps_stats (void * v, char ** reply, int * len, void * data)
 	return show_maps(reply, len, vecs, PRINT_MAP_STATS, 1);
 }
 
-int
+static int
 cli_list_daemon (void * v, char ** reply, int * len, void * data)
 {
 	condlog(3, "list daemon (operator)");
@@ -528,7 +508,7 @@ cli_list_daemon (void * v, char ** reply, int * len, void * data)
 	return show_daemon(reply, len);
 }
 
-int
+static int
 cli_reset_maps_stats (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -543,7 +523,7 @@ cli_reset_maps_stats (void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_reset_map_stats (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -561,7 +541,7 @@ cli_reset_map_stats (void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_add_path (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -677,7 +657,7 @@ blacklisted:
 	return 0;
 }
 
-int
+static int
 cli_del_path (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -700,7 +680,7 @@ cli_del_path (void * v, char ** reply, int * len, void * data)
 	return (ret == REMOVE_PATH_FAILURE);
 }
 
-int
+static int
 cli_add_map (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -760,7 +740,7 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 	return rc;
 }
 
-int
+static int
 cli_del_map (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -789,7 +769,7 @@ cli_del_map (void * v, char ** reply, int * len, void * data)
 	return rc;
 }
 
-int
+static int
 cli_del_maps (void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -808,7 +788,7 @@ cli_del_maps (void *v, char **reply, int *len, void *data)
 	return ret;
 }
 
-int
+static int
 cli_reload(void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -836,7 +816,7 @@ cli_reload(void *v, char **reply, int *len, void *data)
 	return reload_and_sync_map(mpp, vecs, 0);
 }
 
-int resize_map(struct multipath *mpp, unsigned long long size,
+static int resize_map(struct multipath *mpp, unsigned long long size,
 	       struct vectors * vecs)
 {
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
@@ -861,7 +841,7 @@ int resize_map(struct multipath *mpp, unsigned long long size,
 	return 0;
 }
 
-int
+static int
 cli_resize(void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -926,7 +906,7 @@ cli_resize(void *v, char **reply, int *len, void *data)
 	return 0;
 }
 
-int
+static int
 cli_force_no_daemon_q(void * v, char ** reply, int * len, void * data)
 {
 	struct config *conf;
@@ -939,7 +919,7 @@ cli_force_no_daemon_q(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_restore_no_daemon_q(void * v, char ** reply, int * len, void * data)
 {
 	struct config *conf;
@@ -952,7 +932,7 @@ cli_restore_no_daemon_q(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_restore_queueing(void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -993,7 +973,7 @@ cli_restore_queueing(void *v, char **reply, int *len, void *data)
 	return 0;
 }
 
-int
+static int
 cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1015,7 +995,7 @@ cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
 	return 0;
 }
 
-int
+static int
 cli_disable_queueing(void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1044,7 +1024,7 @@ cli_disable_queueing(void *v, char **reply, int *len, void *data)
 	return 0;
 }
 
-int
+static int
 cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1063,7 +1043,7 @@ cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
 	return 0;
 }
 
-int
+static int
 cli_switch_group(void * v, char ** reply, int * len, void * data)
 {
 	char * mapname = get_keyparam(v, MAP);
@@ -1075,7 +1055,7 @@ cli_switch_group(void * v, char ** reply, int * len, void * data)
 	return dm_switchgroup(mapname, groupnum);
 }
 
-int
+static int
 cli_reconfigure(void * v, char ** reply, int * len, void * data)
 {
 	condlog(2, "reconfigure (operator)");
@@ -1084,7 +1064,7 @@ cli_reconfigure(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_suspend(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1114,7 +1094,7 @@ cli_suspend(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_resume(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1146,7 +1126,7 @@ cli_resume(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_reinstate(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1169,7 +1149,7 @@ cli_reinstate(void * v, char ** reply, int * len, void * data)
 	return dm_reinstate_path(pp->mpp->alias, pp->dev_t);
 }
 
-int
+static int
 cli_reassign (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1193,7 +1173,7 @@ cli_reassign (void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_fail(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1222,7 +1202,7 @@ cli_fail(void * v, char ** reply, int * len, void * data)
 	return r;
 }
 
-int
+static int
 show_blacklist (char ** r, int * len)
 {
 	STRBUF_ON_STACK(reply);
@@ -1242,7 +1222,7 @@ show_blacklist (char ** r, int * len)
 	return 0;
 }
 
-int
+static int
 cli_list_blacklist (void * v, char ** reply, int * len, void * data)
 {
 	condlog(3, "list blacklist (operator)");
@@ -1250,7 +1230,7 @@ cli_list_blacklist (void * v, char ** reply, int * len, void * data)
 	return show_blacklist(reply, len);
 }
 
-int
+static int
 show_devices (char ** r, int * len, struct vectors *vecs)
 {
 	STRBUF_ON_STACK(reply);
@@ -1271,7 +1251,7 @@ show_devices (char ** r, int * len, struct vectors *vecs)
 	return 0;
 }
 
-int
+static int
 cli_list_devices (void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
@@ -1281,13 +1261,13 @@ cli_list_devices (void * v, char ** reply, int * len, void * data)
 	return show_devices(reply, len, vecs);
 }
 
-int
+static int
 cli_quit (void * v, char ** reply, int * len, void * data)
 {
 	return 0;
 }
 
-int
+static int
 cli_shutdown (void * v, char ** reply, int * len, void * data)
 {
 	condlog(3, "shutdown (operator)");
@@ -1295,7 +1275,7 @@ cli_shutdown (void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_getprstatus (void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -1320,7 +1300,7 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_setprstatus(void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -1343,7 +1323,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -1365,7 +1345,7 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_getprkey(void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -1397,7 +1377,7 @@ cli_getprkey(void * v, char ** reply, int * len, void * data)
 	return 0;
 }
 
-int
+static int
 cli_unsetprkey(void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -1421,7 +1401,7 @@ cli_unsetprkey(void * v, char ** reply, int * len, void * data)
 	return ret;
 }
 
-int
+static int
 cli_setprkey(void * v, char ** reply, int * len, void * data)
 {
 	struct multipath * mpp;
@@ -1453,7 +1433,7 @@ cli_setprkey(void * v, char ** reply, int * len, void * data)
 	return ret;
 }
 
-int cli_set_marginal(void * v, char ** reply, int * len, void * data)
+static int cli_set_marginal(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -1480,7 +1460,7 @@ int cli_set_marginal(void * v, char ** reply, int * len, void * data)
 	return reload_and_sync_map(pp->mpp, vecs, 0);
 }
 
-int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
+static int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -1507,7 +1487,7 @@ int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
 	return reload_and_sync_map(pp->mpp, vecs, 0);
 }
 
-int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
+static int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * mapname = get_keyparam(v, MAP);
@@ -1543,3 +1523,63 @@ int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
 
 	return reload_and_sync_map(mpp, vecs, 0);
 }
+
+void init_handler_callbacks(void)
+{
+	set_handler_callback(LIST+PATHS, cli_list_paths);
+	set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
+	set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
+	set_handler_callback(LIST+PATH, cli_list_path);
+	set_handler_callback(LIST+MAPS, cli_list_maps);
+	set_handler_callback(LIST+STATUS, cli_list_status);
+	set_unlocked_handler_callback(LIST+DAEMON, cli_list_daemon);
+	set_handler_callback(LIST+MAPS+STATUS, cli_list_maps_status);
+	set_handler_callback(LIST+MAPS+STATS, cli_list_maps_stats);
+	set_handler_callback(LIST+MAPS+FMT, cli_list_maps_fmt);
+	set_handler_callback(LIST+MAPS+RAW+FMT, cli_list_maps_raw);
+	set_handler_callback(LIST+MAPS+TOPOLOGY, cli_list_maps_topology);
+	set_handler_callback(LIST+TOPOLOGY, cli_list_maps_topology);
+	set_handler_callback(LIST+MAPS+JSON, cli_list_maps_json);
+	set_handler_callback(LIST+MAP+TOPOLOGY, cli_list_map_topology);
+	set_handler_callback(LIST+MAP+FMT, cli_list_map_fmt);
+	set_handler_callback(LIST+MAP+RAW+FMT, cli_list_map_fmt);
+	set_handler_callback(LIST+MAP+JSON, cli_list_map_json);
+	set_handler_callback(LIST+CONFIG+LOCAL, cli_list_config_local);
+	set_handler_callback(LIST+CONFIG, cli_list_config);
+	set_handler_callback(LIST+BLACKLIST, cli_list_blacklist);
+	set_handler_callback(LIST+DEVICES, cli_list_devices);
+	set_handler_callback(LIST+WILDCARDS, cli_list_wildcards);
+	set_handler_callback(RESET+MAPS+STATS, cli_reset_maps_stats);
+	set_handler_callback(RESET+MAP+STATS, cli_reset_map_stats);
+	set_handler_callback(ADD+PATH, cli_add_path);
+	set_handler_callback(DEL+PATH, cli_del_path);
+	set_handler_callback(ADD+MAP, cli_add_map);
+	set_handler_callback(DEL+MAP, cli_del_map);
+	set_handler_callback(DEL+MAPS, cli_del_maps);
+	set_handler_callback(SWITCH+MAP+GROUP, cli_switch_group);
+	set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
+	set_handler_callback(SUSPEND+MAP, cli_suspend);
+	set_handler_callback(RESUME+MAP, cli_resume);
+	set_handler_callback(RESIZE+MAP, cli_resize);
+	set_handler_callback(RELOAD+MAP, cli_reload);
+	set_handler_callback(RESET+MAP, cli_reassign);
+	set_handler_callback(REINSTATE+PATH, cli_reinstate);
+	set_handler_callback(FAIL+PATH, cli_fail);
+	set_handler_callback(DISABLEQ+MAP, cli_disable_queueing);
+	set_handler_callback(RESTOREQ+MAP, cli_restore_queueing);
+	set_handler_callback(DISABLEQ+MAPS, cli_disable_all_queueing);
+	set_handler_callback(RESTOREQ+MAPS, cli_restore_all_queueing);
+	set_unlocked_handler_callback(QUIT, cli_quit);
+	set_unlocked_handler_callback(SHUTDOWN, cli_shutdown);
+	set_handler_callback(GETPRSTATUS+MAP, cli_getprstatus);
+	set_handler_callback(SETPRSTATUS+MAP, cli_setprstatus);
+	set_handler_callback(UNSETPRSTATUS+MAP, cli_unsetprstatus);
+	set_handler_callback(FORCEQ+DAEMON, cli_force_no_daemon_q);
+	set_handler_callback(RESTOREQ+DAEMON, cli_restore_no_daemon_q);
+	set_handler_callback(GETPRKEY+MAP, cli_getprkey);
+	set_handler_callback(SETPRKEY+MAP+KEY, cli_setprkey);
+	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
+	set_handler_callback(SETMARGINAL+PATH, cli_set_marginal);
+	set_handler_callback(UNSETMARGINAL+PATH, cli_unset_marginal);
+	set_handler_callback(UNSETMARGINAL+MAP, cli_unset_all_marginal);
+}
diff --git a/multipathd/cli_handlers.h b/multipathd/cli_handlers.h
index 6f57b42..7eaf847 100644
--- a/multipathd/cli_handlers.h
+++ b/multipathd/cli_handlers.h
@@ -1,55 +1,6 @@
-int cli_list_paths (void * v, char ** reply, int * len, void * data);
-int cli_list_paths_fmt (void * v, char ** reply, int * len, void * data);
-int cli_list_paths_raw (void * v, char ** reply, int * len, void * data);
-int cli_list_path (void * v, char ** reply, int * len, void * data);
-int cli_list_status (void * v, char ** reply, int * len, void * data);
-int cli_list_daemon (void * v, char ** reply, int * len, void * data);
-int cli_list_maps (void * v, char ** reply, int * len, void * data);
-int cli_list_maps_fmt (void * v, char ** reply, int * len, void * data);
-int cli_list_maps_raw (void * v, char ** reply, int * len, void * data);
-int cli_list_map_fmt (void * v, char ** reply, int * len, void * data);
-int cli_list_map_raw (void * v, char ** reply, int * len, void * data);
-int cli_list_maps_status (void * v, char ** reply, int * len, void * data);
-int cli_list_maps_stats (void * v, char ** reply, int * len, void * data);
-int cli_list_map_topology (void * v, char ** reply, int * len, void * data);
-int cli_list_maps_topology (void * v, char ** reply, int * len, void * data);
-int cli_list_map_json (void * v, char ** reply, int * len, void * data);
-int cli_list_maps_json (void * v, char ** reply, int * len, void * data);
-int cli_list_config (void * v, char ** reply, int * len, void * data);
-int cli_list_config_local (void * v, char ** reply, int * len, void * data);
-int cli_list_blacklist (void * v, char ** reply, int * len, void * data);
-int cli_list_devices (void * v, char ** reply, int * len, void * data);
-int cli_list_wildcards (void * v, char ** reply, int * len, void * data);
-int cli_reset_maps_stats (void * v, char ** reply, int * len, void * data);
-int cli_reset_map_stats (void * v, char ** reply, int * len, void * data);
-int cli_add_path (void * v, char ** reply, int * len, void * data);
-int cli_del_path (void * v, char ** reply, int * len, void * data);
-int cli_add_map (void * v, char ** reply, int * len, void * data);
-int cli_del_map (void * v, char ** reply, int * len, void * data);
-int cli_del_maps (void * v, char ** reply, int * len, void * data);
-int cli_switch_group(void * v, char ** reply, int * len, void * data);
-int cli_reconfigure(void * v, char ** reply, int * len, void * data);
-int cli_resize(void * v, char ** reply, int * len, void * data);
-int cli_reload(void * v, char ** reply, int * len, void * data);
-int cli_disable_queueing(void * v, char ** reply, int * len, void * data);
-int cli_disable_all_queueing(void * v, char ** reply, int * len, void * data);
-int cli_restore_queueing(void * v, char ** reply, int * len, void * data);
-int cli_restore_all_queueing(void * v, char ** reply, int * len, void * data);
-int cli_suspend(void * v, char ** reply, int * len, void * data);
-int cli_resume(void * v, char ** reply, int * len, void * data);
-int cli_reinstate(void * v, char ** reply, int * len, void * data);
-int cli_fail(void * v, char ** reply, int * len, void * data);
-int cli_force_no_daemon_q(void * v, char ** reply, int * len, void * data);
-int cli_restore_no_daemon_q(void * v, char ** reply, int * len, void * data);
-int cli_quit(void * v, char ** reply, int * len, void * data);
-int cli_shutdown(void * v, char ** reply, int * len, void * data);
-int cli_reassign (void * v, char ** reply, int * len, void * data);
-int cli_getprstatus(void * v, char ** reply, int * len, void * data);
-int cli_setprstatus(void * v, char ** reply, int * len, void * data);
-int cli_unsetprstatus(void * v, char ** reply, int * len, void * data);
-int cli_getprkey(void * v, char ** reply, int * len, void * data);
-int cli_setprkey(void * v, char ** reply, int * len, void * data);
-int cli_unsetprkey(void * v, char ** reply, int * len, void * data);
-int cli_set_marginal(void * v, char ** reply, int * len, void * data);
-int cli_unset_marginal(void * v, char ** reply, int * len, void * data);
-int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data);
+#ifndef _CLI_HANDLERS_H
+#define _CLI_HANDLERS_H
+
+void init_handler_callbacks(void);
+
+#endif
diff --git a/multipathd/main.c b/multipathd/main.c
index 6054fd5..3062f3d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1703,63 +1703,7 @@ uxlsnrloop (void * ap)
 	/* Tell main thread that thread has started */
 	post_config_state(DAEMON_CONFIGURE);
 
-	set_handler_callback(LIST+PATHS, cli_list_paths);
-	set_handler_callback(LIST+PATHS+FMT, cli_list_paths_fmt);
-	set_handler_callback(LIST+PATHS+RAW+FMT, cli_list_paths_raw);
-	set_handler_callback(LIST+PATH, cli_list_path);
-	set_handler_callback(LIST+MAPS, cli_list_maps);
-	set_handler_callback(LIST+STATUS, cli_list_status);
-	set_unlocked_handler_callback(LIST+DAEMON, cli_list_daemon);
-	set_handler_callback(LIST+MAPS+STATUS, cli_list_maps_status);
-	set_handler_callback(LIST+MAPS+STATS, cli_list_maps_stats);
-	set_handler_callback(LIST+MAPS+FMT, cli_list_maps_fmt);
-	set_handler_callback(LIST+MAPS+RAW+FMT, cli_list_maps_raw);
-	set_handler_callback(LIST+MAPS+TOPOLOGY, cli_list_maps_topology);
-	set_handler_callback(LIST+TOPOLOGY, cli_list_maps_topology);
-	set_handler_callback(LIST+MAPS+JSON, cli_list_maps_json);
-	set_handler_callback(LIST+MAP+TOPOLOGY, cli_list_map_topology);
-	set_handler_callback(LIST+MAP+FMT, cli_list_map_fmt);
-	set_handler_callback(LIST+MAP+RAW+FMT, cli_list_map_fmt);
-	set_handler_callback(LIST+MAP+JSON, cli_list_map_json);
-	set_handler_callback(LIST+CONFIG+LOCAL, cli_list_config_local);
-	set_handler_callback(LIST+CONFIG, cli_list_config);
-	set_handler_callback(LIST+BLACKLIST, cli_list_blacklist);
-	set_handler_callback(LIST+DEVICES, cli_list_devices);
-	set_handler_callback(LIST+WILDCARDS, cli_list_wildcards);
-	set_handler_callback(RESET+MAPS+STATS, cli_reset_maps_stats);
-	set_handler_callback(RESET+MAP+STATS, cli_reset_map_stats);
-	set_handler_callback(ADD+PATH, cli_add_path);
-	set_handler_callback(DEL+PATH, cli_del_path);
-	set_handler_callback(ADD+MAP, cli_add_map);
-	set_handler_callback(DEL+MAP, cli_del_map);
-	set_handler_callback(DEL+MAPS, cli_del_maps);
-	set_handler_callback(SWITCH+MAP+GROUP, cli_switch_group);
-	set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
-	set_handler_callback(SUSPEND+MAP, cli_suspend);
-	set_handler_callback(RESUME+MAP, cli_resume);
-	set_handler_callback(RESIZE+MAP, cli_resize);
-	set_handler_callback(RELOAD+MAP, cli_reload);
-	set_handler_callback(RESET+MAP, cli_reassign);
-	set_handler_callback(REINSTATE+PATH, cli_reinstate);
-	set_handler_callback(FAIL+PATH, cli_fail);
-	set_handler_callback(DISABLEQ+MAP, cli_disable_queueing);
-	set_handler_callback(RESTOREQ+MAP, cli_restore_queueing);
-	set_handler_callback(DISABLEQ+MAPS, cli_disable_all_queueing);
-	set_handler_callback(RESTOREQ+MAPS, cli_restore_all_queueing);
-	set_unlocked_handler_callback(QUIT, cli_quit);
-	set_unlocked_handler_callback(SHUTDOWN, cli_shutdown);
-	set_handler_callback(GETPRSTATUS+MAP, cli_getprstatus);
-	set_handler_callback(SETPRSTATUS+MAP, cli_setprstatus);
-	set_handler_callback(UNSETPRSTATUS+MAP, cli_unsetprstatus);
-	set_handler_callback(FORCEQ+DAEMON, cli_force_no_daemon_q);
-	set_handler_callback(RESTOREQ+DAEMON, cli_restore_no_daemon_q);
-	set_handler_callback(GETPRKEY+MAP, cli_getprkey);
-	set_handler_callback(SETPRKEY+MAP+KEY, cli_setprkey);
-	set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
-	set_handler_callback(SETMARGINAL+PATH, cli_set_marginal);
-	set_handler_callback(UNSETMARGINAL+PATH, cli_unset_marginal);
-	set_handler_callback(UNSETMARGINAL+MAP, cli_unset_all_marginal);
-
+	init_handler_callbacks();
 	umask(077);
 	uxsock_listen(&uxsock_trigger, ux_sock, ap);
 
-- 
2.33.1


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

[dm-devel] [PATCH v3 12/35] multipathd: add and set cli_handlers in a single step

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Modify set_handler_callback() such that a missing slot is created
if no matching slot is found. This way, we can skip the initialization
with NULL handlers on startup. Assigning the same handler multiple
times would be a bug which is tested with assert().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli.c | 95 ++++++++----------------------------------------
 multipathd/cli.h |  7 ++--
 2 files changed, 19 insertions(+), 83 deletions(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index 3582370..c1fa7ce 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -4,6 +4,7 @@
 #include <sys/time.h>
 #include <errno.h>
 #include <pthread.h>
+#include <assert.h>
 #include "vector.h"
 #include "structs.h"
 #include "structs_vec.h"
@@ -63,26 +64,26 @@ out:
 	return 1;
 }
 
-int
-add_handler (uint64_t fp, cli_handler *fn)
+static struct handler *add_handler(uint64_t fp, cli_handler *fn, bool locked)
 {
 	struct handler * h;
 
 	h = alloc_handler();
 
-	if (!h)
-		return 1;
+	if (h == NULL)
+		return NULL;
 
 	if (!vector_alloc_slot(handlers)) {
 		free(h);
-		return 1;
+		return NULL;
 	}
 
 	vector_set_slot(handlers, h);
 	h->fingerprint = fp;
 	h->fn = fn;
+	h->locked = locked;
 
-	return 0;
+	return h;
 }
 
 static struct handler *
@@ -99,26 +100,17 @@ find_handler (uint64_t fp)
 }
 
 int
-set_handler_callback (uint64_t fp, cli_handler *fn)
+__set_handler_callback (uint64_t fp, cli_handler *fn, bool locked)
 {
-	struct handler * h = find_handler(fp);
+	struct handler *h;
 
-	if (!h)
+	assert(find_handler(fp) == NULL);
+        h = add_handler(fp, fn, locked);
+        if (!h) {
+		condlog(0, "%s: failed to set handler for code %"PRIu64,
+			__func__, fp);
 		return 1;
-	h->fn = fn;
-	h->locked = 1;
-	return 0;
-}
-
-int
-set_unlocked_handler_callback (uint64_t fp, cli_handler *fn)
-{
-	struct handler * h = find_handler(fp);
-
-	if (!h)
-		return 1;
-	h->fn = fn;
-	h->locked = 0;
+	}
 	return 0;
 }
 
@@ -512,63 +504,6 @@ cli_init (void) {
 	if (alloc_handlers())
 		return 1;
 
-	add_handler(LIST+PATHS, NULL);
-	add_handler(LIST+PATHS+FMT, NULL);
-	add_handler(LIST+PATHS+RAW+FMT, NULL);
-	add_handler(LIST+PATH, NULL);
-	add_handler(LIST+STATUS, NULL);
-	add_handler(LIST+DAEMON, NULL);
-	add_handler(LIST+MAPS, NULL);
-	add_handler(LIST+MAPS+STATUS, NULL);
-	add_handler(LIST+MAPS+STATS, NULL);
-	add_handler(LIST+MAPS+FMT, NULL);
-	add_handler(LIST+MAPS+RAW+FMT, NULL);
-	add_handler(LIST+MAPS+TOPOLOGY, NULL);
-	add_handler(LIST+MAPS+JSON, NULL);
-	add_handler(LIST+TOPOLOGY, NULL);
-	add_handler(LIST+MAP+TOPOLOGY, NULL);
-	add_handler(LIST+MAP+JSON, NULL);
-	add_handler(LIST+MAP+FMT, NULL);
-	add_handler(LIST+MAP+RAW+FMT, NULL);
-	add_handler(LIST+CONFIG, NULL);
-	add_handler(LIST+CONFIG+LOCAL, NULL);
-	add_handler(LIST+BLACKLIST, NULL);
-	add_handler(LIST+DEVICES, NULL);
-	add_handler(LIST+WILDCARDS, NULL);
-	add_handler(RESET+MAPS+STATS, NULL);
-	add_handler(RESET+MAP+STATS, NULL);
-	add_handler(ADD+PATH, NULL);
-	add_handler(DEL+PATH, NULL);
-	add_handler(ADD+MAP, NULL);
-	add_handler(DEL+MAP, NULL);
-	add_handler(DEL+MAPS, NULL);
-	add_handler(SWITCH+MAP+GROUP, NULL);
-	add_handler(RECONFIGURE, NULL);
-	add_handler(SUSPEND+MAP, NULL);
-	add_handler(RESUME+MAP, NULL);
-	add_handler(RESIZE+MAP, NULL);
-	add_handler(RESET+MAP, NULL);
-	add_handler(RELOAD+MAP, NULL);
-	add_handler(DISABLEQ+MAP, NULL);
-	add_handler(RESTOREQ+MAP, NULL);
-	add_handler(DISABLEQ+MAPS, NULL);
-	add_handler(RESTOREQ+MAPS, NULL);
-	add_handler(REINSTATE+PATH, NULL);
-	add_handler(FAIL+PATH, NULL);
-	add_handler(QUIT, NULL);
-	add_handler(SHUTDOWN, NULL);
-	add_handler(GETPRSTATUS+MAP, NULL);
-	add_handler(SETPRSTATUS+MAP, NULL);
-	add_handler(UNSETPRSTATUS+MAP, NULL);
-	add_handler(GETPRKEY+MAP, NULL);
-	add_handler(SETPRKEY+MAP+KEY, NULL);
-	add_handler(UNSETPRKEY+MAP, NULL);
-	add_handler(FORCEQ+DAEMON, NULL);
-	add_handler(RESTOREQ+DAEMON, NULL);
-	add_handler(SETMARGINAL+PATH, NULL);
-	add_handler(UNSETMARGINAL+PATH, NULL);
-	add_handler(UNSETMARGINAL+MAP, NULL);
-
 	return 0;
 }
 
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 84b1fbe..07fd61b 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -133,9 +133,10 @@ struct handler {
 };
 
 int alloc_handlers (void);
-int add_handler (uint64_t fp, cli_handler *fn);
-int set_handler_callback (uint64_t fp, cli_handler *fn);
-int set_unlocked_handler_callback (uint64_t fp, cli_handler *fn);
+int __set_handler_callback (uint64_t fp, cli_handler *fn, bool locked);
+#define set_handler_callback(fp, fn) __set_handler_callback(fp, fn, true)
+#define set_unlocked_handler_callback(fp, fn) __set_handler_callback(fp, fn, false)
+
 int parse_cmd (char * cmd, char ** reply, int * len, void *, int);
 int load_keys (void);
 char * get_keyparam (vector v, uint64_t code);
-- 
2.33.1


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

[dm-devel] [PATCH v3 13/35] multipathd: cli.c: use ESRCH for "command not found"

[mwilck]

From: Martin Wilck <mwilck@suse.com>

EAGAIN is too generic, and doesn't fit semantically either.
ESRCH in't used anywhere else in our code.

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

diff --git a/multipathd/cli.c b/multipathd/cli.c
index c1fa7ce..2422ff9 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -250,7 +250,7 @@ find_key (const char * str)
  *
  * returns:
  * ENOMEM: not enough memory to allocate command
- * EAGAIN: command not found
+ * ESRCH: command not found
  * EINVAL: argument missing for command
  */
 static int
@@ -285,7 +285,7 @@ get_cmdvec (char * cmd, vector *v)
 		}
 		kw = find_key(buff);
 		if (!kw) {
-			r = EAGAIN;
+			r = ESRCH;
 			goto out;
 		}
 		cmdkw = alloc_key();
@@ -375,7 +375,7 @@ do_genhelp(struct strbuf *reply, const char *cmd, int error) {
 	case ENOMEM:
 		rc = print_strbuf(reply, "%s: Not enough memory\n", cmd);
 		break;
-	case EAGAIN:
+	case ESRCH:
 		rc = print_strbuf(reply, "%s: not found\n", cmd);
 		break;
 	case EINVAL:
-- 
2.33.1


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

[dm-devel] [PATCH v3 14/35] multipathd: uxlsnr: avoid stalled clients during reconfigure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since 47cc1d3 ("multipathd: fix client response for socket
activation"), we hold back clients while reconfigure is running.
The idea of 47cc1d3 was to fix the behavior during initial
start up. When multipathd reconfigures itself during runtime,
and the reconfiguration takes a long time (a minute or more is
not unusual in big configurations), clients will time out with
no response ("timeout receiving packet"). Waiting for reconfigure
to finish breaks our timeout handling.

Therefore we should only apply the logic of 47cc1d3 during initial
configuration. In this case, the client that triggered socket
activation may still encounter a timeout, but there's not much we can
do about that.

Fixes: 47cc1d3 ("multipathd: fix client response for socket activation")
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c   |  9 +++++++++
 multipathd/uxlsnr.c | 12 ------------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 3062f3d..309c11e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1705,6 +1705,15 @@ uxlsnrloop (void * ap)
 
 	init_handler_callbacks();
 	umask(077);
+
+	/*
+	 * Wait for initial reconfiguration to finish, while
+	 * hadling signals
+	 */
+	while (wait_for_state_change_if(DAEMON_CONFIGURE, 50)
+	       == DAEMON_CONFIGURE)
+		handle_signals(false);
+
 	uxsock_listen(&uxsock_trigger, ux_sock, ap);
 
 out_sock:
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 912c35b..400375c 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -390,18 +390,6 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 			continue;
 		}
 
-		/*
-		 * Client connection. We shouldn't answer while we're
-		 * configuring - nothing may be configured yet.
-		 * But we can't wait forever either, because this thread
-		 * must handle signals. So wait a short while only.
-		 */
-		if (wait_for_state_change_if(DAEMON_CONFIGURE, 10)
-		    == DAEMON_CONFIGURE) {
-			handle_signals(false);
-			continue;
-		}
-
 		/* see if a client wants to speak to us */
 		for (i = POLLFDS_BASE; i < n_pfds; i++) {
 			if (polls[i].revents & POLLIN) {
-- 
2.33.1


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

[dm-devel] [PATCH v3 15/35] multipathd: uxlsnr: handle client HUP

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The unix socket listener thread doesn't even look at the revents
returned by poll() while the daemon is configuring. This may cause a
closed client socket to be kept open for a long time by the server,
while the listener basically performs a busy loop, as ppoll() always
returns immediately as long as the POLLHUP condition exists.

Worse, it can happen that multipathd reads data from such a closed
client socket after the client has disconnected. See the description
of POLLHUP in poll(2).

Close connections immediately if HUP is received.

Also, use the fd in log messages to identify the client rather
than the random index.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 400375c..9a6ab72 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -392,7 +392,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 
 		/* see if a client wants to speak to us */
 		for (i = POLLFDS_BASE; i < n_pfds; i++) {
-			if (polls[i].revents & POLLIN) {
+			if (polls[i].revents & (POLLIN|POLLHUP|POLLERR)) {
 				struct timespec start_time;
 
 				c = NULL;
@@ -409,6 +409,12 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 						i, polls[i].fd);
 					continue;
 				}
+				if (polls[i].revents & (POLLHUP|POLLERR)) {
+					condlog(4, "cli[%d]: Disconnected",
+						c->fd);
+					dead_client(c);
+					continue;
+				}
 				get_monotonic_time(&start_time);
 				if (recv_packet_from_client(c->fd, &inbuf,
 							    uxsock_timeout)
@@ -422,7 +428,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 					continue;
 				}
 				condlog(4, "cli[%d]: Got request [%s]",
-					i, inbuf);
+					polls[i].fd, inbuf);
 				uxsock_trigger(inbuf, &reply, &rlen,
 					       _socket_client_is_root(c->fd),
 					       trigger_data);
@@ -433,7 +439,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 					} else {
 						condlog(4, "cli[%d]: "
 							"Reply [%d bytes]",
-							i, rlen);
+							polls[i].fd, rlen);
 					}
 					free(reply);
 					reply = NULL;
-- 
2.33.1


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

[dm-devel] [PATCH v3 16/35] multipathd: uxlsnr: use symbolic values for pollfd indices

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Avoid hardcoding the indices as 0, 1, 2...

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 9a6ab72..a0653f6 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -45,8 +45,13 @@ struct client {
 	int fd;
 };
 
-/* The number of fds we poll on, other than individual client connections */
-#define POLLFDS_BASE 2
+/* Indices for array of poll fds */
+enum {
+	POLLFD_UX = 0,
+	POLLFD_NOTIFY,
+	POLLFDS_BASE,
+};
+
 #define POLLFD_CHUNK (4096 / sizeof(struct pollfd))
 /* Minimum mumber of pollfds to reserve for clients */
 #define MIN_POLLS (POLLFD_CHUNK - POLLFDS_BASE)
@@ -338,8 +343,8 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 			}
 		}
 		if (num_clients < MAX_CLIENTS) {
-			polls[0].fd = ux_sock;
-			polls[0].events = POLLIN;
+			polls[POLLFD_UX].fd = ux_sock;
+			polls[POLLFD_UX].events = POLLIN;
 		} else {
 			/*
 			 * New clients can't connect, num_clients won't grow
@@ -347,15 +352,15 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 			 */
 			condlog(1, "%s: max client connections reached, pausing polling",
 				__func__);
-			polls[0].fd = -1;
+			polls[POLLFD_UX].fd = -1;
 		}
 
 		reset_watch(notify_fd, &wds, &sequence_nr);
 		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
-			polls[1].fd = -1;
+			polls[POLLFD_NOTIFY].fd = -1;
 		else
-			polls[1].fd = notify_fd;
-		polls[1].events = POLLIN;
+			polls[POLLFD_NOTIFY].fd = notify_fd;
+		polls[POLLFD_NOTIFY].events = POLLIN;
 
 		/* setup the clients */
 		i = POLLFDS_BASE;
@@ -453,12 +458,12 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		handle_signals(true);
 
 		/* see if we got a new client */
-		if (polls[0].revents & POLLIN) {
+		if (polls[POLLFD_UX].revents & POLLIN) {
 			new_client(ux_sock);
 		}
 
 		/* handle inotify events on config files */
-		if (polls[1].revents & POLLIN)
+		if (polls[POLLFD_NOTIFY].revents & POLLIN)
 			handle_inotify(notify_fd, &wds);
 	}
 
-- 
2.33.1


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

[dm-devel] [PATCH v3 17/35] multipathd: uxlsnr: avoid using fd -1 in ppoll()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Minor edit: if notifications are off, we set the poll fd to
-1 but still use the POLLIN mask. It looks nicer if to poll
the correct fd, but reset the event mask to 0 if we're not
actually interested in it.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index a0653f6..7bbec29 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -356,11 +356,11 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		}
 
 		reset_watch(notify_fd, &wds, &sequence_nr);
+		polls[POLLFD_NOTIFY].fd = notify_fd;
 		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
-			polls[POLLFD_NOTIFY].fd = -1;
+			polls[POLLFD_NOTIFY].events = 0;
 		else
-			polls[POLLFD_NOTIFY].fd = notify_fd;
-		polls[POLLFD_NOTIFY].events = POLLIN;
+			polls[POLLFD_NOTIFY].events = POLLIN;
 
 		/* setup the clients */
 		i = POLLFDS_BASE;
-- 
2.33.1


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

[dm-devel] [PATCH v3 18/35] multipathd: uxlsnr: data structure for stateful client connection

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Currently the uxlsnr handles each client request (receive requset -
handle request - respond) in a single loop iteration. This has
severe disadvantages. In particular, the code may wait in poll()
called from read_all(), or wait for the vecs lock, while other
clients are ready to be serviced or signals to be handled.

This patch adds some fields to "struct client" which will be used
by later patches to change this into a state machine that basically
waits only in place, the ppoll() call in uxsock_listen().

For now, we just introduce and initialize the fields.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 7bbec29..38a9d97 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -39,10 +39,30 @@
 #include "main.h"
 #include "cli.h"
 #include "uxlsnr.h"
+#include "strbuf.h"
+
+/* state of client connection */
+enum {
+	CLT_RECV,
+	CLT_PARSE,
+	CLT_LOCKED_WORK,
+	CLT_WORK,
+	CLT_SEND,
+};
 
 struct client {
 	struct list_head node;
+	struct timespec expires;
+	int state;
 	int fd;
+	vector cmdvec;
+	/* NUL byte at end */
+	char cmd[_MAX_CMD_LEN + 1];
+	struct strbuf reply;
+	struct handler *handler;
+	size_t cmd_len, len;
+	int error;
+	bool is_root;
 };
 
 /* Indices for array of poll fds */
@@ -110,6 +130,7 @@ static void new_client(int ux_sock)
 	}
 	INIT_LIST_HEAD(&c->node);
 	c->fd = fd;
+	c->state = CLT_RECV;
 
 	/* put it in our linked list */
 	pthread_mutex_lock(&client_lock);
@@ -125,6 +146,9 @@ static void _dead_client(struct client *c)
 	int fd = c->fd;
 	list_del_init(&c->node);
 	c->fd = -1;
+	reset_strbuf(&c->reply);
+	if (c->cmdvec)
+		free_keys(c->cmdvec);
 	free(c);
 	close(fd);
 }
-- 
2.33.1


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

[dm-devel] [PATCH v3 19/35] multipathd: move uxsock_trigger() to uxlsnr.c

[mwilck]

From: Martin Wilck <mwilck@suse.com>

uxsock_trigger() really belongs into cli.c. I suppose that way back in
the past there were strong reasons to call this function via a
pointer. I don't think these reasons are valid any more. Moving
the function to cli.c allows restructuring the code.

No functional changes.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 309c11e..cd9a127 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1536,48 +1536,6 @@ map_discovery (struct vectors * vecs)
 	return 0;
 }
 
-int
-uxsock_trigger (char * str, char ** reply, int * len, bool is_root,
-		void * trigger_data)
-{
-	struct vectors * vecs;
-	int r;
-
-	*reply = NULL;
-	*len = 0;
-	vecs = (struct vectors *)trigger_data;
-
-	if ((str != NULL) && (is_root == false) &&
-	    (strncmp(str, "list", strlen("list")) != 0) &&
-	    (strncmp(str, "show", strlen("show")) != 0)) {
-		*reply = strdup("permission deny: need to be root");
-		if (*reply)
-			*len = strlen(*reply) + 1;
-		return 1;
-	}
-
-	r = parse_cmd(str, reply, len, vecs, uxsock_timeout / 1000);
-
-	if (r > 0) {
-		if (r == ETIMEDOUT)
-			*reply = strdup("timeout\n");
-		else
-			*reply = strdup("fail\n");
-		if (*reply)
-			*len = strlen(*reply) + 1;
-		r = 1;
-	}
-	else if (!r && *len == 0) {
-		*reply = strdup("ok\n");
-		if (*reply)
-			*len = strlen(*reply) + 1;
-		r = 0;
-	}
-	/* else if (r < 0) leave *reply alone */
-
-	return r;
-}
-
 int
 uev_trigger (struct uevent * uev, void * trigger_data)
 {
@@ -1714,7 +1672,7 @@ uxlsnrloop (void * ap)
 	       == DAEMON_CONFIGURE)
 		handle_signals(false);
 
-	uxsock_listen(&uxsock_trigger, ux_sock, ap);
+	uxsock_listen(ux_sock, ap);
 
 out_sock:
 	pthread_cleanup_pop(1); /* uxsock_cleanup */
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 38a9d97..449f149 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -311,11 +311,51 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 		condlog(1, "Multipath configuration updated.\nReload multipathd for changes to take effect");
 }
 
+static int uxsock_trigger(char *str, char **reply, int *len,
+			  bool is_root, void *trigger_data)
+{
+	struct vectors * vecs;
+	int r;
+
+	*reply = NULL;
+	*len = 0;
+	vecs = (struct vectors *)trigger_data;
+
+	if ((str != NULL) && (is_root == false) &&
+	    (strncmp(str, "list", strlen("list")) != 0) &&
+	    (strncmp(str, "show", strlen("show")) != 0)) {
+		*reply = strdup("permission deny: need to be root");
+		if (*reply)
+			*len = strlen(*reply) + 1;
+		return 1;
+	}
+
+	r = parse_cmd(str, reply, len, vecs, uxsock_timeout / 1000);
+
+	if (r > 0) {
+		if (r == ETIMEDOUT)
+			*reply = strdup("timeout\n");
+		else
+			*reply = strdup("fail\n");
+		if (*reply)
+			*len = strlen(*reply) + 1;
+		r = 1;
+	}
+	else if (!r && *len == 0) {
+		*reply = strdup("ok\n");
+		if (*reply)
+			*len = strlen(*reply) + 1;
+		r = 0;
+	}
+	/* else if (r < 0) leave *reply alone */
+
+	return r;
+}
+
 /*
  * entry point
  */
-void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
-		     void * trigger_data)
+void *uxsock_listen(long ux_sock, void *trigger_data)
 {
 	int rlen;
 	char *inbuf;
diff --git a/multipathd/uxlsnr.h b/multipathd/uxlsnr.h
index 18f008d..60c3a2c 100644
--- a/multipathd/uxlsnr.h
+++ b/multipathd/uxlsnr.h
@@ -3,10 +3,8 @@
 
 #include <stdbool.h>
 
-typedef int (uxsock_trigger_fn)(char *, char **, int *, bool, void *);
-
 void uxsock_cleanup(void *arg);
-void *uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
+void *uxsock_listen(long ux_sock,
 		    void * trigger_data);
 
 #endif
-- 
2.33.1


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

[dm-devel] [PATCH v3 20/35] multipathd: move parse_cmd() to uxlsnr.c

[mwilck]

From: Martin Wilck <mwilck@suse.com>

parse_cmd() does more than the name says - it parses, executes
handlers, and even provides reply strings for some cases. This doesn't
work well with the state machine idea. Thus move it to uxlsnr.c,
where later patches will move some functionality elsewhere.

No functional changes.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli.c    | 74 +++++----------------------------------------
 multipathd/cli.h    |  5 ++-
 multipathd/uxlsnr.c | 61 +++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 67 deletions(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index 2422ff9..912a078 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -253,8 +253,7 @@ find_key (const char * str)
  * ESRCH: command not found
  * EINVAL: argument missing for command
  */
-static int
-get_cmdvec (char * cmd, vector *v)
+int get_cmdvec (char *cmd, vector *v)
 {
 	int i;
 	int r = 0;
@@ -319,7 +318,7 @@ out:
 }
 
 static uint64_t
-fingerprint(vector vec)
+fingerprint(const struct _vector *vec)
 {
 	int i;
 	uint64_t fp = 0;
@@ -334,6 +333,11 @@ fingerprint(vector vec)
 	return fp;
 }
 
+struct handler *find_handler_for_cmdvec(const struct _vector *v)
+{
+	return find_handler(fingerprint(v));
+}
+
 int
 alloc_handlers (void)
 {
@@ -412,8 +416,7 @@ do_genhelp(struct strbuf *reply, const char *cmd, int error) {
 }
 
 
-static char *
-genhelp_handler (const char *cmd, int error)
+char *genhelp_handler(const char *cmd, int error)
 {
 	STRBUF_ON_STACK(reply);
 
@@ -422,67 +425,6 @@ genhelp_handler (const char *cmd, int error)
 	return steal_strbuf_str(&reply);
 }
 
-int
-parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
-{
-	int r;
-	struct handler * h;
-	vector cmdvec = NULL;
-	struct timespec tmo;
-
-	r = get_cmdvec(cmd, &cmdvec);
-
-	if (r) {
-		*reply = genhelp_handler(cmd, r);
-		if (*reply == NULL)
-			return EINVAL;
-		*len = strlen(*reply) + 1;
-		return 0;
-	}
-
-	h = find_handler(fingerprint(cmdvec));
-
-	if (!h || !h->fn) {
-		free_keys(cmdvec);
-		*reply = genhelp_handler(cmd, EINVAL);
-		if (*reply == NULL)
-			return EINVAL;
-		*len = strlen(*reply) + 1;
-		return 0;
-	}
-
-	/*
-	 * execute handler
-	 */
-	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
-		tmo.tv_sec += timeout;
-	} else {
-		tmo.tv_sec = 0;
-	}
-	if (h->locked) {
-		int locked = 0;
-		struct vectors * vecs = (struct vectors *)data;
-
-		pthread_cleanup_push(cleanup_lock, &vecs->lock);
-		if (tmo.tv_sec) {
-			r = timedlock(&vecs->lock, &tmo);
-		} else {
-			lock(&vecs->lock);
-			r = 0;
-		}
-		if (r == 0) {
-			locked = 1;
-			pthread_testcancel();
-			r = h->fn(cmdvec, reply, len, data);
-		}
-		pthread_cleanup_pop(locked);
-	} else
-		r = h->fn(cmdvec, reply, len, data);
-	free_keys(cmdvec);
-
-	return r;
-}
-
 char *
 get_keyparam (vector v, uint64_t code)
 {
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 07fd61b..7ca9b2f 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -137,7 +137,10 @@ int __set_handler_callback (uint64_t fp, cli_handler *fn, bool locked);
 #define set_handler_callback(fp, fn) __set_handler_callback(fp, fn, true)
 #define set_unlocked_handler_callback(fp, fn) __set_handler_callback(fp, fn, false)
 
-int parse_cmd (char * cmd, char ** reply, int * len, void *, int);
+int get_cmdvec (char *cmd, vector *v);
+struct handler *find_handler_for_cmdvec(const struct _vector *v);
+char *genhelp_handler (const char *cmd, int error);
+
 int load_keys (void);
 char * get_keyparam (vector v, uint64_t code);
 void free_keys (vector vec);
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 449f149..99fee16 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -311,6 +311,67 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 		condlog(1, "Multipath configuration updated.\nReload multipathd for changes to take effect");
 }
 
+static int parse_cmd (char *cmd, char **reply, int *len, void *data,
+		      int timeout)
+{
+	int r;
+	struct handler * h;
+	vector cmdvec = NULL;
+	struct timespec tmo;
+
+	r = get_cmdvec(cmd, &cmdvec);
+
+	if (r) {
+		*reply = genhelp_handler(cmd, r);
+		if (*reply == NULL)
+			return EINVAL;
+		*len = strlen(*reply) + 1;
+		return 0;
+	}
+
+	h = find_handler_for_cmdvec(cmdvec);
+
+	if (!h || !h->fn) {
+		free_keys(cmdvec);
+		*reply = genhelp_handler(cmd, EINVAL);
+		if (*reply == NULL)
+			return EINVAL;
+		*len = strlen(*reply) + 1;
+		return 0;
+	}
+
+	/*
+	 * execute handler
+	 */
+	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
+		tmo.tv_sec += timeout;
+	} else {
+		tmo.tv_sec = 0;
+	}
+	if (h->locked) {
+		int locked = 0;
+		struct vectors * vecs = (struct vectors *)data;
+
+		pthread_cleanup_push(cleanup_lock, &vecs->lock);
+		if (tmo.tv_sec) {
+			r = timedlock(&vecs->lock, &tmo);
+		} else {
+			lock(&vecs->lock);
+			r = 0;
+		}
+		if (r == 0) {
+			locked = 1;
+			pthread_testcancel();
+			r = h->fn(cmdvec, reply, len, data);
+		}
+		pthread_cleanup_pop(locked);
+	} else
+		r = h->fn(cmdvec, reply, len, data);
+	free_keys(cmdvec);
+
+	return r;
+}
+
 static int uxsock_trigger(char *str, char **reply, int *len,
 			  bool is_root, void *trigger_data)
 {
-- 
2.33.1


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

[dm-devel] [PATCH v3 21/35] multipathd: uxlsnr: remove check_timeout()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This function just prints a warning, anyway. If this warning
is printed, the client will see a timeout and print a warning, too.
A later patch will re-introduce this function with real functionality.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 99fee16..daa86c9 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -168,25 +168,6 @@ static void free_polls (void)
 	polls = NULL;
 }
 
-static void check_timeout(struct timespec start_time, char *inbuf,
-		   unsigned int timeout)
-{
-	struct timespec diff_time, end_time;
-
-	if (start_time.tv_sec) {
-		unsigned long msecs;
-
-		get_monotonic_time(&end_time);
-		timespecsub(&end_time, &start_time, &diff_time);
-		msecs = diff_time.tv_sec * 1000 +
-			diff_time.tv_nsec / (1000 * 1000);
-		if (msecs > timeout)
-			condlog(2, "cli cmd '%s' timeout reached "
-				"after %ld.%06lu secs", inbuf,
-				(long)diff_time.tv_sec, diff_time.tv_nsec / 1000);
-	}
-}
-
 void uxsock_cleanup(void *arg)
 {
 	struct client *client_loop;
@@ -574,8 +555,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 					free(reply);
 					reply = NULL;
 				}
-				check_timeout(start_time, inbuf,
-					      uxsock_timeout);
 				free(inbuf);
 			}
 		}
-- 
2.33.1


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

[dm-devel] [PATCH v3 22/35] multipathd: uxlsnr: move client handling to separate function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

No functional changes at this point. handle_client() will become
the state machine for handling client requests.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 67 ++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index daa86c9..bfeb30d 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -394,14 +394,42 @@ static int uxsock_trigger(char *str, char **reply, int *len,
 	return r;
 }
 
+static void handle_client(struct client *c, void *trigger_data)
+{
+	int rlen;
+	char *inbuf, *reply;
+
+	if (recv_packet_from_client(c->fd, &inbuf, uxsock_timeout) != 0) {
+		dead_client(c);
+		return;
+	}
+
+	if (!inbuf) {
+		condlog(4, "recv_packet_from_client get null request");
+		return;
+	}
+
+	condlog(4, "cli[%d]: Got request [%s]", c->fd, inbuf);
+	uxsock_trigger(inbuf, &reply, &rlen,
+		       _socket_client_is_root(c->fd),
+		       trigger_data);
+
+	if (reply) {
+		if (send_packet(c->fd, reply) != 0)
+			dead_client(c);
+		else
+			condlog(4, "cli[%d]: Reply [%d bytes]", c->fd, rlen);
+		free(reply);
+		reply = NULL;
+	}
+	free(inbuf);
+}
+
 /*
  * entry point
  */
 void *uxsock_listen(long ux_sock, void *trigger_data)
 {
-	int rlen;
-	char *inbuf;
-	char *reply;
 	sigset_t mask;
 	int max_pfds = MIN_POLLS + POLLFDS_BASE;
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
@@ -504,8 +532,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 		/* see if a client wants to speak to us */
 		for (i = POLLFDS_BASE; i < n_pfds; i++) {
 			if (polls[i].revents & (POLLIN|POLLHUP|POLLERR)) {
-				struct timespec start_time;
-
 				c = NULL;
 				pthread_mutex_lock(&client_lock);
 				list_for_each_entry(tmp, &clients, node) {
@@ -526,36 +552,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 					dead_client(c);
 					continue;
 				}
-				get_monotonic_time(&start_time);
-				if (recv_packet_from_client(c->fd, &inbuf,
-							    uxsock_timeout)
-				    != 0) {
-					dead_client(c);
-					continue;
-				}
-				if (!inbuf) {
-					condlog(4, "recv_packet_from_client "
-						"get null request");
-					continue;
-				}
-				condlog(4, "cli[%d]: Got request [%s]",
-					polls[i].fd, inbuf);
-				uxsock_trigger(inbuf, &reply, &rlen,
-					       _socket_client_is_root(c->fd),
-					       trigger_data);
-				if (reply) {
-					if (send_packet(c->fd,
-							reply) != 0) {
-						dead_client(c);
-					} else {
-						condlog(4, "cli[%d]: "
-							"Reply [%d bytes]",
-							polls[i].fd, rlen);
-					}
-					free(reply);
-					reply = NULL;
-				}
-				free(inbuf);
+				handle_client(c, trigger_data);
 			}
 		}
 		/* see if we got a non-fatal signal */
-- 
2.33.1


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

[dm-devel] [PATCH v3 23/35] multipathd: uxlsnr: use main poll loop for receiving

[mwilck]

From: Martin Wilck <mwilck@suse.com>

As a first step towards our state machine, avoid the call to
read_all() via recv_packet_from_client(). handle_client() is now
invoked twice for the same connection. The first time it reads
the command length, and later on it reads the command itself
piece-wise, as sent by the client. This will be just a single
read in most cases, but not always.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 90 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 15 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index bfeb30d..eb1c48e 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -292,6 +292,8 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 		condlog(1, "Multipath configuration updated.\nReload multipathd for changes to take effect");
 }
 
+static const struct timespec ts_zero = { .tv_sec = 0, };
+
 static int parse_cmd (char *cmd, char **reply, int *len, void *data,
 		      int timeout)
 {
@@ -394,23 +396,78 @@ static int uxsock_trigger(char *str, char **reply, int *len,
 	return r;
 }
 
+static void set_client_state(struct client *c, int state)
+{
+	switch(state)
+	{
+	case CLT_RECV:
+		reset_strbuf(&c->reply);
+		memset(c->cmd, '\0', sizeof(c->cmd));
+		c->expires = ts_zero;
+		/* fallthrough */
+	case CLT_SEND:
+		/* reuse these fields for next data transfer */
+		c->len = c->cmd_len = 0;
+		break;
+	default:
+		break;
+	}
+	c->state = state;
+}
+
 static void handle_client(struct client *c, void *trigger_data)
 {
 	int rlen;
-	char *inbuf, *reply;
+	char *reply;
+	ssize_t n;
 
-	if (recv_packet_from_client(c->fd, &inbuf, uxsock_timeout) != 0) {
-		dead_client(c);
-		return;
+	switch (c->state) {
+	case CLT_RECV:
+		if (c->cmd_len == 0) {
+			/*
+			 * We got POLLIN; assume that at least the length can
+			 * be read immediately.
+			 */
+			get_monotonic_time(&c->expires);
+			c->expires.tv_sec += uxsock_timeout / 1000;
+			c->expires.tv_nsec += (uxsock_timeout % 1000) * 1000000;
+			normalize_timespec(&c->expires);
+			n = mpath_recv_reply_len(c->fd, 0);
+			if (n == -1) {
+				condlog(1, "%s: cli[%d]: failed to receive reply len",
+					__func__, c->fd);
+				c->error = -ECONNRESET;
+			} else if (n > _MAX_CMD_LEN) {
+				condlog(1, "%s: cli[%d]: overlong command (%zd bytes)",
+					__func__, c->fd, n);
+				c->error = -ECONNRESET;
+			} else {
+				c->cmd_len = n;
+				condlog(4, "%s: cli[%d]: connected", __func__, c->fd);
+			}
+			/* poll for data */
+			return;
+		} else if (c->len < c->cmd_len) {
+			n = recv(c->fd, c->cmd + c->len, c->cmd_len - c->len, 0);
+			if (n <= 0 && errno != EINTR && errno != EAGAIN) {
+				condlog(1, "%s: cli[%d]: error in recv: %m",
+					__func__, c->fd);
+				c->error = -ECONNRESET;
+				return;
+			}
+			c->len += n;
+			if (c->len < c->cmd_len)
+				/* continue polling */
+				return;
+			set_client_state(c, CLT_PARSE);
+		}
+		break;
+	default:
+		break;
 	}
 
-	if (!inbuf) {
-		condlog(4, "recv_packet_from_client get null request");
-		return;
-	}
-
-	condlog(4, "cli[%d]: Got request [%s]", c->fd, inbuf);
-	uxsock_trigger(inbuf, &reply, &rlen,
+	condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
+	uxsock_trigger(c->cmd, &reply, &rlen,
 		       _socket_client_is_root(c->fd),
 		       trigger_data);
 
@@ -418,11 +475,12 @@ static void handle_client(struct client *c, void *trigger_data)
 		if (send_packet(c->fd, reply) != 0)
 			dead_client(c);
 		else
-			condlog(4, "cli[%d]: Reply [%d bytes]", c->fd, rlen);
-		free(reply);
-		reply = NULL;
+			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd,
+				get_strbuf_len(&c->reply) + 1);
+		reset_strbuf(&c->reply);
 	}
-	free(inbuf);
+
+	set_client_state(c, CLT_RECV);
 }
 
 /*
@@ -553,6 +611,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 					continue;
 				}
 				handle_client(c, trigger_data);
+				if (c->error == -ECONNRESET)
+					dead_client(c);
 			}
 		}
 		/* see if we got a non-fatal signal */
-- 
2.33.1


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

[dm-devel] [PATCH v3 24/35] multipathd: use strbuf in cli_handler functions

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This allows us to simplify callers by not having to track the
reply length separately.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli.c          |   7 +-
 multipathd/cli.h          |   6 +-
 multipathd/cli_handlers.c | 316 +++++++++++++++-----------------------
 multipathd/uxlsnr.c       |  49 +++---
 4 files changed, 152 insertions(+), 226 deletions(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index 912a078..cd147b3 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -416,13 +416,10 @@ do_genhelp(struct strbuf *reply, const char *cmd, int error) {
 }
 
 
-char *genhelp_handler(const char *cmd, int error)
+void genhelp_handler(const char *cmd, int error, struct strbuf *reply)
 {
-	STRBUF_ON_STACK(reply);
-
-	if (do_genhelp(&reply, cmd, error) == -1)
+	if (do_genhelp(reply, cmd, error) == -1)
 		condlog(0, "genhelp_handler: out of memory");
-	return steal_strbuf_str(&reply);
 }
 
 char *
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 7ca9b2f..4b3f617 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -124,7 +124,9 @@ struct key {
 	int has_param;
 };
 
-typedef int (cli_handler)(void *keywords, char **reply, int *len, void *data);
+struct strbuf;
+
+typedef int (cli_handler)(void *keywords, struct strbuf *reply, void *data);
 
 struct handler {
 	uint64_t fingerprint;
@@ -139,7 +141,7 @@ int __set_handler_callback (uint64_t fp, cli_handler *fn, bool locked);
 
 int get_cmdvec (char *cmd, vector *v);
 struct handler *find_handler_for_cmdvec(const struct _vector *v);
-char *genhelp_handler (const char *cmd, int error);
+void genhelp_handler (const char *cmd, int error, struct strbuf *reply);
 
 int load_keys (void);
 char * get_keyparam (vector v, uint64_t code);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 911272c..22464d6 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -32,17 +32,9 @@
 #include "strbuf.h"
 #include "cli_handlers.h"
 
-#define SET_REPLY_AND_LEN(__rep, __len, string_literal)			\
-	do {								\
-		*(__rep) = strdup(string_literal);			\
-		*(__len) = *(__rep) ? sizeof(string_literal) : 0;	\
-	} while (0)
-
 static int
-show_paths (char ** r, int * len, struct vectors * vecs, char * style,
-	    int pretty)
+show_paths (struct strbuf *reply, struct vectors *vecs, char *style, int pretty)
 {
-	STRBUF_ON_STACK(reply);
 	int i;
 	struct path * pp;
 	int hdr_len = 0;
@@ -50,61 +42,49 @@ show_paths (char ** r, int * len, struct vectors * vecs, char * style,
 	get_path_layout(vecs->pathvec, 1);
 	foreign_path_layout();
 
-	if (pretty && (hdr_len = snprint_path_header(&reply, style)) < 0)
+	if (pretty && (hdr_len = snprint_path_header(reply, style)) < 0)
 		return 1;
 
 	vector_foreach_slot(vecs->pathvec, pp, i) {
-		if (snprint_path(&reply, style, pp, pretty) < 0)
+		if (snprint_path(reply, style, pp, pretty) < 0)
 			return 1;
 	}
-	if (snprint_foreign_paths(&reply, style, pretty) < 0)
+	if (snprint_foreign_paths(reply, style, pretty) < 0)
 		return 1;
 
-	if (pretty && get_strbuf_len(&reply) == (size_t)hdr_len)
+	if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
 		/* No output - clear header */
-		truncate_strbuf(&reply, 0);
+		truncate_strbuf(reply, 0);
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_path (char ** r, int * len, struct vectors * vecs, struct path *pp,
-	   char * style)
+show_path (struct strbuf *reply, struct vectors *vecs, struct path *pp,
+	   char *style)
 {
-	STRBUF_ON_STACK(reply);
-
 	get_path_layout(vecs->pathvec, 1);
-	if (snprint_path(&reply, style, pp, 0) < 0)
+	if (snprint_path(reply, style, pp, 0) < 0)
 		return 1;
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
-
 	return 0;
 }
 
 static int
-show_map_topology (char ** r, int * len, struct multipath * mpp,
-		   struct vectors * vecs)
+show_map_topology (struct strbuf *reply, struct multipath *mpp,
+		   struct vectors *vecs)
 {
-	STRBUF_ON_STACK(reply);
-
 	if (update_multipath(vecs, mpp->alias, 0))
 		return 1;
 
-	if (snprint_multipath_topology(&reply, mpp, 2) < 0)
+	if (snprint_multipath_topology(reply, mpp, 2) < 0)
 		return 1;
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 
 	return 0;
 }
 
 static int
-show_maps_topology (char ** r, int * len, struct vectors * vecs)
+show_maps_topology (struct strbuf *reply, struct vectors * vecs)
 {
-	STRBUF_ON_STACK(reply);
 	int i;
 	struct multipath * mpp;
 
@@ -116,21 +96,18 @@ show_maps_topology (char ** r, int * len, struct vectors * vecs)
 			i--;
 			continue;
 		}
-		if (snprint_multipath_topology(&reply, mpp, 2) < 0)
+		if (snprint_multipath_topology(reply, mpp, 2) < 0)
 			return 1;
 	}
-	if (snprint_foreign_topology(&reply, 2) < 0)
+	if (snprint_foreign_topology(reply, 2) < 0)
 		return 1;
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_maps_json (char ** r, int * len, struct vectors * vecs)
+show_maps_json (struct strbuf *reply, struct vectors * vecs)
 {
-	STRBUF_ON_STACK(reply);
 	int i;
 	struct multipath * mpp;
 
@@ -140,45 +117,38 @@ show_maps_json (char ** r, int * len, struct vectors * vecs)
 		}
 	}
 
-	if (snprint_multipath_topology_json(&reply, vecs) < 0)
+	if (snprint_multipath_topology_json(reply, vecs) < 0)
 		return 1;
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_map_json (char ** r, int * len, struct multipath * mpp,
-		   struct vectors * vecs)
+show_map_json (struct strbuf *reply, struct multipath * mpp,
+	       struct vectors * vecs)
 {
-	STRBUF_ON_STACK(reply);
-
 	if (update_multipath(vecs, mpp->alias, 0))
 		return 1;
 
-	if (snprint_multipath_map_json(&reply, mpp) < 0)
+	if (snprint_multipath_map_json(reply, mpp) < 0)
 		return 1;
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_config (char ** r, int * len, const struct _vector *hwtable,
+show_config (struct strbuf *reply, const struct _vector *hwtable,
 	     const struct _vector *mpvec)
 {
 	struct config *conf;
-	char *reply;
+	int rc;
 
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
-	reply = snprint_config(conf, len, hwtable, mpvec);
+	rc = __snprint_config(conf, reply, hwtable, mpvec);
 	pthread_cleanup_pop(1);
-	if (reply == NULL)
+	if (rc < 0)
 		return 1;
-	*r = reply;
 	return 0;
 }
 
@@ -194,11 +164,11 @@ reset_stats(struct multipath * mpp)
 }
 
 static int
-cli_list_config (void * v, char ** reply, int * len, void * data)
+cli_list_config (void *v, struct strbuf *reply, void *data)
 {
 	condlog(3, "list config (operator)");
 
-	return show_config(reply, len, NULL, NULL);
+	return show_config(reply, NULL, NULL);
 }
 
 static void v_free(void *x)
@@ -207,9 +177,9 @@ static void v_free(void *x)
 }
 
 static int
-cli_list_config_local (void * v, char ** reply, int * len, void * data)
+cli_list_config_local (void *v, struct strbuf *reply, void *data)
 {
-	struct vectors * vecs = (struct vectors *)data;
+	struct vectors *vecs = (struct vectors *)data;
 	vector hwes;
 	int ret;
 
@@ -217,45 +187,45 @@ cli_list_config_local (void * v, char ** reply, int * len, void * data)
 
 	hwes = get_used_hwes(vecs->pathvec);
 	pthread_cleanup_push(v_free, hwes);
-	ret = show_config(reply, len, hwes, vecs->mpvec);
+	ret = show_config(reply, hwes, vecs->mpvec);
 	pthread_cleanup_pop(1);
 	return ret;
 }
 
 static int
-cli_list_paths (void * v, char ** reply, int * len, void * data)
+cli_list_paths (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list paths (operator)");
 
-	return show_paths(reply, len, vecs, PRINT_PATH_CHECKER, 1);
+	return show_paths(reply, vecs, PRINT_PATH_CHECKER, 1);
 }
 
 static int
-cli_list_paths_fmt (void * v, char ** reply, int * len, void * data)
+cli_list_paths_fmt (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * fmt = get_keyparam(v, FMT);
 
 	condlog(3, "list paths (operator)");
 
-	return show_paths(reply, len, vecs, fmt, 1);
+	return show_paths(reply, vecs, fmt, 1);
 }
 
 static int
-cli_list_paths_raw (void * v, char ** reply, int * len, void * data)
+cli_list_paths_raw (void *v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * fmt = get_keyparam(v, FMT);
 
 	condlog(3, "list paths (operator)");
 
-	return show_paths(reply, len, vecs, fmt, 0);
+	return show_paths(reply, vecs, fmt, 0);
 }
 
 static int
-cli_list_path (void * v, char ** reply, int * len, void * data)
+cli_list_path (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -268,11 +238,11 @@ cli_list_path (void * v, char ** reply, int * len, void * data)
 	if (!pp)
 		return 1;
 
-	return show_path(reply, len, vecs, pp, "%o");
+	return show_path(reply, vecs, pp, "%o");
 }
 
 static int
-cli_list_map_topology (void * v, char ** reply, int * len, void * data)
+cli_list_map_topology (void *v, struct strbuf *reply, void *data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -287,21 +257,21 @@ cli_list_map_topology (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "list multipath %s (operator)", param);
 
-	return show_map_topology(reply, len, mpp, vecs);
+	return show_map_topology(reply, mpp, vecs);
 }
 
 static int
-cli_list_maps_topology (void * v, char ** reply, int * len, void * data)
+cli_list_maps_topology (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list multipaths (operator)");
 
-	return show_maps_topology(reply, len, vecs);
+	return show_maps_topology(reply, vecs);
 }
 
 static int
-cli_list_map_json (void * v, char ** reply, int * len, void * data)
+cli_list_map_json (void *v, struct strbuf *reply, void *data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -316,78 +286,61 @@ cli_list_map_json (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "list multipath json %s (operator)", param);
 
-	return show_map_json(reply, len, mpp, vecs);
+	return show_map_json(reply, mpp, vecs);
 }
 
 static int
-cli_list_maps_json (void * v, char ** reply, int * len, void * data)
+cli_list_maps_json (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list multipaths json (operator)");
 
-	return show_maps_json(reply, len, vecs);
+	return show_maps_json(reply, vecs);
 }
 
 static int
-cli_list_wildcards (void * v, char ** reply, int * len, void * data)
+cli_list_wildcards (void *v, struct strbuf *reply, void *data)
 {
-	STRBUF_ON_STACK(buf);
-
-	if (snprint_wildcards(&buf) < 0)
+	if (snprint_wildcards(reply) < 0)
 		return 1;
 
-	*len = get_strbuf_len(&buf) + 1;
-	*reply = steal_strbuf_str(&buf);
 	return 0;
 }
 
 static int
-show_status (char ** r, int *len, struct vectors * vecs)
+show_status (struct strbuf *reply, struct vectors *vecs)
 {
-	STRBUF_ON_STACK(reply);
-
-	if (snprint_status(&reply, vecs) < 0)
+	if (snprint_status(reply, vecs) < 0)
 		return 1;
 
-	*len = get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_daemon (char ** r, int *len)
+show_daemon (struct strbuf *reply)
 {
-	STRBUF_ON_STACK(reply);
-
-	if (print_strbuf(&reply, "pid %d %s\n",
+	if (print_strbuf(reply, "pid %d %s\n",
 			 daemon_pid, daemon_status()) < 0)
 		return 1;
 
-	*len = get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_map (char ** r, int *len, struct multipath * mpp, char * style,
+show_map (struct strbuf *reply, struct multipath *mpp, char *style,
 	  int pretty)
 {
-	STRBUF_ON_STACK(reply);
-
-	if (snprint_multipath(&reply, style, mpp, pretty) < 0)
+	if (snprint_multipath(reply, style, mpp, pretty) < 0)
 		return 1;
 
-	*len = get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-show_maps (char ** r, int *len, struct vectors * vecs, char * style,
+show_maps (struct strbuf *reply, struct vectors *vecs, char *style,
 	   int pretty)
 {
-	STRBUF_ON_STACK(reply);
 	int i;
 	struct multipath * mpp;
 	int hdr_len = 0;
@@ -395,7 +348,7 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style,
 	get_multipath_layout(vecs->mpvec, 1);
 	foreign_multipath_layout();
 
-	if (pretty && (hdr_len = snprint_multipath_header(&reply, style)) < 0)
+	if (pretty && (hdr_len = snprint_multipath_header(reply, style)) < 0)
 		return 1;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
@@ -403,45 +356,43 @@ show_maps (char ** r, int *len, struct vectors * vecs, char * style,
 			i--;
 			continue;
 		}
-		if (snprint_multipath(&reply, style, mpp, pretty) < 0)
+		if (snprint_multipath(reply, style, mpp, pretty) < 0)
 			return 1;
 	}
-	if (snprint_foreign_multipaths(&reply, style, pretty) < 0)
+	if (snprint_foreign_multipaths(reply, style, pretty) < 0)
 		return 1;
 
-	if (pretty && get_strbuf_len(&reply) == (size_t)hdr_len)
+	if (pretty && get_strbuf_len(reply) == (size_t)hdr_len)
 		/* No output - clear header */
-		truncate_strbuf(&reply, 0);
+		truncate_strbuf(reply, 0);
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-cli_list_maps_fmt (void * v, char ** reply, int * len, void * data)
+cli_list_maps_fmt (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * fmt = get_keyparam(v, FMT);
 
 	condlog(3, "list maps (operator)");
 
-	return show_maps(reply, len, vecs, fmt, 1);
+	return show_maps(reply, vecs, fmt, 1);
 }
 
 static int
-cli_list_maps_raw (void * v, char ** reply, int * len, void * data)
+cli_list_maps_raw (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * fmt = get_keyparam(v, FMT);
 
 	condlog(3, "list maps (operator)");
 
-	return show_maps(reply, len, vecs, fmt, 0);
+	return show_maps(reply, vecs, fmt, 0);
 }
 
 static int
-cli_list_map_fmt (void * v, char ** reply, int * len, void * data)
+cli_list_map_fmt (void *v, struct strbuf *reply, void *data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -457,59 +408,59 @@ cli_list_map_fmt (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "list map %s fmt %s (operator)", param, fmt);
 
-	return show_map(reply, len, mpp, fmt, 1);
+	return show_map(reply, mpp, fmt, 1);
 }
 
 static int
-cli_list_maps (void * v, char ** reply, int * len, void * data)
+cli_list_maps (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list maps (operator)");
 
-	return show_maps(reply, len, vecs, PRINT_MAP_NAMES, 1);
+	return show_maps(reply, vecs, PRINT_MAP_NAMES, 1);
 }
 
 static int
-cli_list_status (void * v, char ** reply, int * len, void * data)
+cli_list_status (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list status (operator)");
 
-	return show_status(reply, len, vecs);
+	return show_status(reply, vecs);
 }
 
 static int
-cli_list_maps_status (void * v, char ** reply, int * len, void * data)
+cli_list_maps_status (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list maps status (operator)");
 
-	return show_maps(reply, len, vecs, PRINT_MAP_STATUS, 1);
+	return show_maps(reply, vecs, PRINT_MAP_STATUS, 1);
 }
 
 static int
-cli_list_maps_stats (void * v, char ** reply, int * len, void * data)
+cli_list_maps_stats (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list maps stats (operator)");
 
-	return show_maps(reply, len, vecs, PRINT_MAP_STATS, 1);
+	return show_maps(reply, vecs, PRINT_MAP_STATS, 1);
 }
 
 static int
-cli_list_daemon (void * v, char ** reply, int * len, void * data)
+cli_list_daemon (void *v, struct strbuf *reply, void *data)
 {
 	condlog(3, "list daemon (operator)");
 
-	return show_daemon(reply, len);
+	return show_daemon(reply);
 }
 
 static int
-cli_reset_maps_stats (void * v, char ** reply, int * len, void * data)
+cli_reset_maps_stats (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	int i;
@@ -524,7 +475,7 @@ cli_reset_maps_stats (void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_reset_map_stats (void * v, char ** reply, int * len, void * data)
+cli_reset_map_stats (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	struct multipath * mpp;
@@ -542,7 +493,7 @@ cli_reset_map_stats (void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_add_path (void * v, char ** reply, int * len, void * data)
+cli_add_path (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -652,13 +603,13 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
 	}
 	return ev_add_path(pp, vecs, 1);
 blacklisted:
-	SET_REPLY_AND_LEN(reply, len, "blacklisted\n");
+	append_strbuf_str(reply, "blacklisted\n");
 	condlog(2, "%s: path blacklisted", param);
 	return 0;
 }
 
 static int
-cli_del_path (void * v, char ** reply, int * len, void * data)
+cli_del_path (void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -674,14 +625,14 @@ cli_del_path (void * v, char ** reply, int * len, void * data)
 	}
 	ret = ev_remove_path(pp, vecs, 1);
 	if (ret == REMOVE_PATH_DELAY)
-		SET_REPLY_AND_LEN(reply, len, "delayed\n");
+		append_strbuf_str(reply, "delayed\n");
 	else if (ret == REMOVE_PATH_MAP_ERROR)
-		SET_REPLY_AND_LEN(reply, len, "map reload error. removed\n");
+		append_strbuf_str(reply, "map reload error. removed\n");
 	return (ret == REMOVE_PATH_FAILURE);
 }
 
 static int
-cli_add_map (void * v, char ** reply, int * len, void * data)
+cli_add_map (void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
@@ -701,7 +652,7 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 		invalid = 1;
 	pthread_cleanup_pop(1);
 	if (invalid) {
-		SET_REPLY_AND_LEN(reply, len, "blacklisted\n");
+		append_strbuf_str(reply, "blacklisted\n");
 		condlog(2, "%s: map blacklisted", param);
 		return 1;
 	}
@@ -741,7 +692,7 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_del_map (void * v, char ** reply, int * len, void * data)
+cli_del_map (void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
@@ -763,14 +714,14 @@ cli_del_map (void * v, char ** reply, int * len, void * data)
 	}
 	rc = ev_remove_map(param, alias, minor, vecs);
 	if (rc == 2)
-		*reply = strdup("delayed");
+		append_strbuf_str(reply, "delayed");
 
 	free(alias);
 	return rc;
 }
 
 static int
-cli_del_maps (void *v, char **reply, int *len, void *data)
+cli_del_maps (void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	struct multipath *mpp;
@@ -789,7 +740,7 @@ cli_del_maps (void *v, char **reply, int *len, void *data)
 }
 
 static int
-cli_reload(void *v, char **reply, int *len, void *data)
+cli_reload(void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * mapname = get_keyparam(v, MAP);
@@ -842,7 +793,7 @@ static int resize_map(struct multipath *mpp, unsigned long long size,
 }
 
 static int
-cli_resize(void *v, char **reply, int *len, void *data)
+cli_resize(void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * mapname = get_keyparam(v, MAP);
@@ -907,7 +858,7 @@ cli_resize(void *v, char **reply, int *len, void *data)
 }
 
 static int
-cli_force_no_daemon_q(void * v, char ** reply, int * len, void * data)
+cli_force_no_daemon_q(void * v, struct strbuf *reply, void * data)
 {
 	struct config *conf;
 
@@ -920,7 +871,7 @@ cli_force_no_daemon_q(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_restore_no_daemon_q(void * v, char ** reply, int * len, void * data)
+cli_restore_no_daemon_q(void * v, struct strbuf *reply, void * data)
 {
 	struct config *conf;
 
@@ -933,7 +884,7 @@ cli_restore_no_daemon_q(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_restore_queueing(void *v, char **reply, int *len, void *data)
+cli_restore_queueing(void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * mapname = get_keyparam(v, MAP);
@@ -974,7 +925,7 @@ cli_restore_queueing(void *v, char **reply, int *len, void *data)
 }
 
 static int
-cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
+cli_restore_all_queueing(void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	struct multipath *mpp;
@@ -996,7 +947,7 @@ cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
 }
 
 static int
-cli_disable_queueing(void *v, char **reply, int *len, void *data)
+cli_disable_queueing(void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * mapname = get_keyparam(v, MAP);
@@ -1025,7 +976,7 @@ cli_disable_queueing(void *v, char **reply, int *len, void *data)
 }
 
 static int
-cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
+cli_disable_all_queueing(void *v, struct strbuf *reply, void *data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	struct multipath *mpp;
@@ -1044,7 +995,7 @@ cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
 }
 
 static int
-cli_switch_group(void * v, char ** reply, int * len, void * data)
+cli_switch_group(void * v, struct strbuf *reply, void * data)
 {
 	char * mapname = get_keyparam(v, MAP);
 	int groupnum = atoi(get_keyparam(v, GROUP));
@@ -1056,7 +1007,7 @@ cli_switch_group(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_reconfigure(void * v, char ** reply, int * len, void * data)
+cli_reconfigure(void * v, struct strbuf *reply, void * data)
 {
 	condlog(2, "reconfigure (operator)");
 
@@ -1065,7 +1016,7 @@ cli_reconfigure(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_suspend(void * v, char ** reply, int * len, void * data)
+cli_suspend(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
@@ -1095,7 +1046,7 @@ cli_suspend(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_resume(void * v, char ** reply, int * len, void * data)
+cli_resume(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
@@ -1127,7 +1078,7 @@ cli_resume(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_reinstate(void * v, char ** reply, int * len, void * data)
+cli_reinstate(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -1150,7 +1101,7 @@ cli_reinstate(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_reassign (void * v, char ** reply, int * len, void * data)
+cli_reassign (void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, MAP);
@@ -1174,7 +1125,7 @@ cli_reassign (void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_fail(void * v, char ** reply, int * len, void * data)
+cli_fail(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -1203,72 +1154,65 @@ cli_fail(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-show_blacklist (char ** r, int * len)
+show_blacklist (struct strbuf *reply)
 {
-	STRBUF_ON_STACK(reply);
 	struct config *conf;
 	bool fail;
 
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
-	fail = snprint_blacklist_report(conf, &reply) < 0;
+	fail = snprint_blacklist_report(conf, reply) < 0;
 	pthread_cleanup_pop(1);
 
 	if (fail)
 		return 1;
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
 	return 0;
 }
 
 static int
-cli_list_blacklist (void * v, char ** reply, int * len, void * data)
+cli_list_blacklist (void * v, struct strbuf *reply, void * data)
 {
 	condlog(3, "list blacklist (operator)");
 
-	return show_blacklist(reply, len);
+	return show_blacklist(reply);
 }
 
 static int
-show_devices (char ** r, int * len, struct vectors *vecs)
+show_devices (struct strbuf *reply, struct vectors *vecs)
 {
-	STRBUF_ON_STACK(reply);
 	struct config *conf;
 	bool fail;
 
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
-	fail = snprint_devices(conf, &reply, vecs) < 0;
+	fail = snprint_devices(conf, reply, vecs) < 0;
 	pthread_cleanup_pop(1);
 
 	if (fail)
 		return 1;
 
-	*len = (int)get_strbuf_len(&reply) + 1;
-	*r = steal_strbuf_str(&reply);
-
 	return 0;
 }
 
 static int
-cli_list_devices (void * v, char ** reply, int * len, void * data)
+cli_list_devices (void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 
 	condlog(3, "list devices (operator)");
 
-	return show_devices(reply, len, vecs);
+	return show_devices(reply, vecs);
 }
 
 static int
-cli_quit (void * v, char ** reply, int * len, void * data)
+cli_quit (void * v, struct strbuf *reply, void * data)
 {
 	return 0;
 }
 
 static int
-cli_shutdown (void * v, char ** reply, int * len, void * data)
+cli_shutdown (void * v, struct strbuf *reply, void * data)
 {
 	condlog(3, "shutdown (operator)");
 	exit_daemon();
@@ -1276,7 +1220,7 @@ cli_shutdown (void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_getprstatus (void * v, char ** reply, int * len, void * data)
+cli_getprstatus (void * v, struct strbuf *reply, void * data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1291,17 +1235,16 @@ cli_getprstatus (void * v, char ** reply, int * len, void * data)
 
 	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
 
-	*len = asprintf(reply, "%d", mpp->prflag);
-	if (*len < 0)
+	if (print_strbuf(reply, "%d", mpp->prflag) < 0)
 		return 1;
 
-	condlog(3, "%s: reply = %s", param, *reply);
+	condlog(3, "%s: reply = %s", param, get_strbuf_str(reply));
 
 	return 0;
 }
 
 static int
-cli_setprstatus(void * v, char ** reply, int * len, void * data)
+cli_setprstatus(void * v, struct strbuf *reply, void * data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1324,7 +1267,7 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
+cli_unsetprstatus(void * v, struct strbuf *reply, void * data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1346,7 +1289,7 @@ cli_unsetprstatus(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_getprkey(void * v, char ** reply, int * len, void * data)
+cli_getprkey(void * v, struct strbuf *reply, void * data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1360,25 +1303,20 @@ cli_getprkey(void * v, char ** reply, int * len, void * data)
 	if (!mpp)
 		return 1;
 
-	*reply = malloc(26);
-	if (!*reply)
-		return 1;
-
 	key = get_be64(mpp->reservation_key);
 	if (!key) {
-		sprintf(*reply, "none\n");
-		*len = sizeof("none\n");
+		append_strbuf_str(reply, "none\n");
 		return 0;
 	}
 
-	/* This snprintf() can't overflow - PRIx64 needs max 16 chars */
-	*len = snprintf(*reply, 26, "0x%" PRIx64 "%s\n", key,
-			mpp->sa_flags & MPATH_F_APTPL_MASK ? ":aptpl" : "") + 1;
+	if (print_strbuf(reply, "0x%" PRIx64 "%s\n", key,
+			 mpp->sa_flags & MPATH_F_APTPL_MASK ? ":aptpl" : "") < 0)
+		return 1;
 	return 0;
 }
 
 static int
-cli_unsetprkey(void * v, char ** reply, int * len, void * data)
+cli_unsetprkey(void * v, struct strbuf *reply, void * data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1402,7 +1340,7 @@ cli_unsetprkey(void * v, char ** reply, int * len, void * data)
 }
 
 static int
-cli_setprkey(void * v, char ** reply, int * len, void * data)
+cli_setprkey(void * v, struct strbuf *reply, void * data)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1433,7 +1371,7 @@ cli_setprkey(void * v, char ** reply, int * len, void * data)
 	return ret;
 }
 
-static int cli_set_marginal(void * v, char ** reply, int * len, void * data)
+static int cli_set_marginal(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -1460,7 +1398,7 @@ static int cli_set_marginal(void * v, char ** reply, int * len, void * data)
 	return reload_and_sync_map(pp->mpp, vecs, 0);
 }
 
-static int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
+static int cli_unset_marginal(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, PATH);
@@ -1487,7 +1425,7 @@ static int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
 	return reload_and_sync_map(pp->mpp, vecs, 0);
 }
 
-static int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
+static int cli_unset_all_marginal(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * mapname = get_keyparam(v, MAP);
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index eb1c48e..3d21a3d 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -294,7 +294,7 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
 
-static int parse_cmd (char *cmd, char **reply, int *len, void *data,
+static int parse_cmd (char *cmd, struct strbuf *reply, void *data,
 		      int timeout)
 {
 	int r;
@@ -305,10 +305,9 @@ static int parse_cmd (char *cmd, char **reply, int *len, void *data,
 	r = get_cmdvec(cmd, &cmdvec);
 
 	if (r) {
-		*reply = genhelp_handler(cmd, r);
-		if (*reply == NULL)
+		genhelp_handler(cmd, r, reply);
+		if (get_strbuf_len(reply) == 0)
 			return EINVAL;
-		*len = strlen(*reply) + 1;
 		return 0;
 	}
 
@@ -316,10 +315,9 @@ static int parse_cmd (char *cmd, char **reply, int *len, void *data,
 
 	if (!h || !h->fn) {
 		free_keys(cmdvec);
-		*reply = genhelp_handler(cmd, EINVAL);
-		if (*reply == NULL)
+		genhelp_handler(cmd, EINVAL, reply);
+		if (get_strbuf_len(reply) == 0)
 			return EINVAL;
-		*len = strlen(*reply) + 1;
 		return 0;
 	}
 
@@ -345,50 +343,42 @@ static int parse_cmd (char *cmd, char **reply, int *len, void *data,
 		if (r == 0) {
 			locked = 1;
 			pthread_testcancel();
-			r = h->fn(cmdvec, reply, len, data);
+			r = h->fn(cmdvec, reply, data);
 		}
 		pthread_cleanup_pop(locked);
 	} else
-		r = h->fn(cmdvec, reply, len, data);
+		r = h->fn(cmdvec, reply, data);
 	free_keys(cmdvec);
 
 	return r;
 }
 
-static int uxsock_trigger(char *str, char **reply, int *len,
+static int uxsock_trigger(char *str, struct strbuf *reply,
 			  bool is_root, void *trigger_data)
 {
 	struct vectors * vecs;
 	int r;
 
-	*reply = NULL;
-	*len = 0;
 	vecs = (struct vectors *)trigger_data;
 
 	if ((str != NULL) && (is_root == false) &&
 	    (strncmp(str, "list", strlen("list")) != 0) &&
 	    (strncmp(str, "show", strlen("show")) != 0)) {
-		*reply = strdup("permission deny: need to be root");
-		if (*reply)
-			*len = strlen(*reply) + 1;
+		append_strbuf_str(reply, "permission deny: need to be root");
 		return 1;
 	}
 
-	r = parse_cmd(str, reply, len, vecs, uxsock_timeout / 1000);
+	r = parse_cmd(str, reply, vecs, uxsock_timeout / 1000);
 
 	if (r > 0) {
 		if (r == ETIMEDOUT)
-			*reply = strdup("timeout\n");
+			append_strbuf_str(reply, "timeout\n");
 		else
-			*reply = strdup("fail\n");
-		if (*reply)
-			*len = strlen(*reply) + 1;
+			append_strbuf_str(reply, "fail\n");
 		r = 1;
 	}
-	else if (!r && *len == 0) {
-		*reply = strdup("ok\n");
-		if (*reply)
-			*len = strlen(*reply) + 1;
+	else if (!r && get_strbuf_len(reply) == 0) {
+		append_strbuf_str(reply, "ok\n");
 		r = 0;
 	}
 	/* else if (r < 0) leave *reply alone */
@@ -417,8 +407,6 @@ static void set_client_state(struct client *c, int state)
 
 static void handle_client(struct client *c, void *trigger_data)
 {
-	int rlen;
-	char *reply;
 	ssize_t n;
 
 	switch (c->state) {
@@ -467,12 +455,13 @@ static void handle_client(struct client *c, void *trigger_data)
 	}
 
 	condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
-	uxsock_trigger(c->cmd, &reply, &rlen,
-		       _socket_client_is_root(c->fd),
+	uxsock_trigger(c->cmd, &c->reply, _socket_client_is_root(c->fd),
 		       trigger_data);
 
-	if (reply) {
-		if (send_packet(c->fd, reply) != 0)
+	if (get_strbuf_len(&c->reply) > 0) {
+		const char *buf = get_strbuf_str(&c->reply);
+
+		if (send_packet(c->fd, buf) != 0)
 			dead_client(c);
 		else
 			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd,
-- 
2.33.1


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

[dm-devel] [PATCH v3 25/35] multipathd: uxlsnr: check root on connection startup

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The SO_PEERCRED socket option returns "the credentials that were
in effect at the time of the call to connect(2)" (see unix(7)).
So we might as well fetch these credentials at that time.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 3d21a3d..a2ef9f8 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -91,8 +91,6 @@ static struct pollfd *polls;
 static int notify_fd = -1;
 static char *watch_config_dir;
 
-static bool _socket_client_is_root(int fd);
-
 static bool _socket_client_is_root(int fd)
 {
 	socklen_t len = 0;
@@ -131,6 +129,7 @@ static void new_client(int ux_sock)
 	INIT_LIST_HEAD(&c->node);
 	c->fd = fd;
 	c->state = CLT_RECV;
+	c->is_root = _socket_client_is_root(c->fd);
 
 	/* put it in our linked list */
 	pthread_mutex_lock(&client_lock);
-- 
2.33.1


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

[dm-devel] [PATCH v3 26/35] multipathd: uxlsnr: pass struct client to uxsock_trigger() and parse_cmd()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

As a next step towards the state machine, give the handler functions
access to the state of the client connection.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 61 +++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index a2ef9f8..d83d83d 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -293,31 +293,28 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
 
-static int parse_cmd (char *cmd, struct strbuf *reply, void *data,
-		      int timeout)
+static int parse_cmd (struct client *c, void *data, int timeout)
 {
 	int r;
 	struct handler * h;
-	vector cmdvec = NULL;
 	struct timespec tmo;
 
-	r = get_cmdvec(cmd, &cmdvec);
+	r = get_cmdvec(c->cmd, &c->cmdvec);
 
 	if (r) {
-		genhelp_handler(cmd, r, reply);
-		if (get_strbuf_len(reply) == 0)
+		genhelp_handler(c->cmd, r, &c->reply);
+		if (get_strbuf_len(&c->reply) == 0)
 			return EINVAL;
 		return 0;
 	}
 
-	h = find_handler_for_cmdvec(cmdvec);
+	h = find_handler_for_cmdvec(c->cmdvec);
 
 	if (!h || !h->fn) {
-		free_keys(cmdvec);
-		genhelp_handler(cmd, EINVAL, reply);
-		if (get_strbuf_len(reply) == 0)
-			return EINVAL;
-		return 0;
+		genhelp_handler(c->cmd, EINVAL, &c->reply);
+		if (get_strbuf_len(&c->reply) == 0)
+			r = EINVAL;
+		goto free_cmdvec;
 	}
 
 	/*
@@ -342,46 +339,47 @@ static int parse_cmd (char *cmd, struct strbuf *reply, void *data,
 		if (r == 0) {
 			locked = 1;
 			pthread_testcancel();
-			r = h->fn(cmdvec, reply, data);
+			r = h->fn(c->cmdvec, &c->reply, data);
 		}
 		pthread_cleanup_pop(locked);
 	} else
-		r = h->fn(cmdvec, reply, data);
-	free_keys(cmdvec);
+		r = h->fn(c->cmdvec, &c->reply, data);
+
+free_cmdvec:
+	free_keys(c->cmdvec);
+	c->cmdvec = NULL;
 
 	return r;
 }
 
-static int uxsock_trigger(char *str, struct strbuf *reply,
-			  bool is_root, void *trigger_data)
+static int uxsock_trigger(struct client *c, void *trigger_data)
 {
 	struct vectors * vecs;
-	int r;
+	int r = 1;
 
 	vecs = (struct vectors *)trigger_data;
 
-	if ((str != NULL) && (is_root == false) &&
-	    (strncmp(str, "list", strlen("list")) != 0) &&
-	    (strncmp(str, "show", strlen("show")) != 0)) {
-		append_strbuf_str(reply, "permission deny: need to be root");
-		return 1;
+
+	if (!c->is_root &&
+	    (strncmp(c->cmd, "list", strlen("list")) != 0) &&
+	    (strncmp(c->cmd, "show", strlen("show")) != 0)) {
+		append_strbuf_str(&c->reply, "permission deny: need to be root");
+		return r;
 	}
 
-	r = parse_cmd(str, reply, vecs, uxsock_timeout / 1000);
+	r = parse_cmd(c, vecs, uxsock_timeout / 1000);
 
 	if (r > 0) {
 		if (r == ETIMEDOUT)
-			append_strbuf_str(reply, "timeout\n");
+			append_strbuf_str(&c->reply, "timeout\n");
 		else
-			append_strbuf_str(reply, "fail\n");
-		r = 1;
+			append_strbuf_str(&c->reply, "fail\n");
 	}
-	else if (!r && get_strbuf_len(reply) == 0) {
-		append_strbuf_str(reply, "ok\n");
+	else if (!r && get_strbuf_len(&c->reply) == 0) {
+		append_strbuf_str(&c->reply, "ok\n");
 		r = 0;
 	}
 	/* else if (r < 0) leave *reply alone */
-
 	return r;
 }
 
@@ -454,8 +452,7 @@ static void handle_client(struct client *c, void *trigger_data)
 	}
 
 	condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
-	uxsock_trigger(c->cmd, &c->reply, _socket_client_is_root(c->fd),
-		       trigger_data);
+	uxsock_trigger(c, trigger_data);
 
 	if (get_strbuf_len(&c->reply) > 0) {
 		const char *buf = get_strbuf_str(&c->reply);
-- 
2.33.1


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

[dm-devel] [PATCH v3 27/35] multipathd: uxlsnr: move handler execution to separate function

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Move the actual execution of the handler out of parse_cmd(). For now,
we do it in uxsock_trigger().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 47 ++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index d83d83d..2c434cd 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -293,11 +293,9 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
 
-static int parse_cmd (struct client *c, void *data, int timeout)
+static int parse_cmd(struct client *c)
 {
 	int r;
-	struct handler * h;
-	struct timespec tmo;
 
 	r = get_cmdvec(c->cmd, &c->cmdvec);
 
@@ -308,26 +306,35 @@ static int parse_cmd (struct client *c, void *data, int timeout)
 		return 0;
 	}
 
-	h = find_handler_for_cmdvec(c->cmdvec);
+	c->handler = find_handler_for_cmdvec(c->cmdvec);
 
-	if (!h || !h->fn) {
+	if (!c->handler || !c->handler->fn) {
 		genhelp_handler(c->cmd, EINVAL, &c->reply);
 		if (get_strbuf_len(&c->reply) == 0)
 			r = EINVAL;
-		goto free_cmdvec;
+		else
+			r = 0;
 	}
 
-	/*
-	 * execute handler
-	 */
+	return r;
+}
+
+static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
+{
+	int r;
+	struct timespec tmo;
+
+	if (!c->handler)
+		return EINVAL;
+
 	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
 		tmo.tv_sec += timeout;
 	} else {
 		tmo.tv_sec = 0;
 	}
-	if (h->locked) {
+
+	if (c->handler->locked) {
 		int locked = 0;
-		struct vectors * vecs = (struct vectors *)data;
 
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		if (tmo.tv_sec) {
@@ -339,15 +346,11 @@ static int parse_cmd (struct client *c, void *data, int timeout)
 		if (r == 0) {
 			locked = 1;
 			pthread_testcancel();
-			r = h->fn(c->cmdvec, &c->reply, data);
+			r = c->handler->fn(c->cmdvec, &c->reply, vecs);
 		}
 		pthread_cleanup_pop(locked);
 	} else
-		r = h->fn(c->cmdvec, &c->reply, data);
-
-free_cmdvec:
-	free_keys(c->cmdvec);
-	c->cmdvec = NULL;
+		r = c->handler->fn(c->cmdvec, &c->reply, vecs);
 
 	return r;
 }
@@ -367,7 +370,15 @@ static int uxsock_trigger(struct client *c, void *trigger_data)
 		return r;
 	}
 
-	r = parse_cmd(c, vecs, uxsock_timeout / 1000);
+	r = parse_cmd(c);
+
+	if (r == 0 && c->handler)
+		r = execute_handler(c, vecs, uxsock_timeout / 1000);
+
+	if (c->cmdvec) {
+		free_keys(c->cmdvec);
+		c->cmdvec = NULL;
+	}
 
 	if (r > 0) {
 		if (r == ETIMEDOUT)
-- 
2.33.1


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

[dm-devel] [PATCH v3 28/35] multipathd: uxlsnr: use parser to determine non-root commands

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Rather than using a separate poor-man's parser for checking root
commands, use the real parser. It will return "LIST" as first verb
for the read-only commands that non-root users may execute.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 2c434cd..62b9fe5 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -362,16 +362,15 @@ static int uxsock_trigger(struct client *c, void *trigger_data)
 
 	vecs = (struct vectors *)trigger_data;
 
-
-	if (!c->is_root &&
-	    (strncmp(c->cmd, "list", strlen("list")) != 0) &&
-	    (strncmp(c->cmd, "show", strlen("show")) != 0)) {
-		append_strbuf_str(&c->reply, "permission deny: need to be root");
-		return r;
-	}
-
 	r = parse_cmd(c);
 
+	if (r == 0 && c->cmdvec && VECTOR_SIZE(c->cmdvec) > 0) {
+		struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
+
+		if (!c->is_root && kw->code != LIST)
+			r = EPERM;
+	}
+
 	if (r == 0 && c->handler)
 		r = execute_handler(c, vecs, uxsock_timeout / 1000);
 
@@ -381,10 +380,18 @@ static int uxsock_trigger(struct client *c, void *trigger_data)
 	}
 
 	if (r > 0) {
-		if (r == ETIMEDOUT)
+		switch(r) {
+		case ETIMEDOUT:
 			append_strbuf_str(&c->reply, "timeout\n");
-		else
+			break;
+		case EPERM:
+			append_strbuf_str(&c->reply,
+					  "permission deny: need to be root\n");
+			break;
+		default:
 			append_strbuf_str(&c->reply, "fail\n");
+			break;
+		}
 	}
 	else if (!r && get_strbuf_len(&c->reply) == 0) {
 		append_strbuf_str(&c->reply, "ok\n");
-- 
2.33.1


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

[dm-devel] [PATCH v3 29/35] multipathd: uxlsnr: merge uxsock_trigger() into state machine

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This patch sets up the bulk of the state machine. client_state_machine()
is called in a loop, proceeding from state to state until it needs
to poll for input or wait for a lock, in which case it returns
STM_BREAK.

While doing this, switch to negative error codes for the functions
in uxlsnr.c (e.g. parse_cmd()). Positive return codes are reserved
for the cli_handler functions themselves. This way we can clearly
distinguish the error source, and avoid confusion and misleading
error messages. No cli_handler returns negative values.

Note: with this patch applied, clients may hang and time out if
the handler fails to acquire the vecs lock. This will be fixed in the
follow-up patch "multipathd: uxlsnr: add idle notification".

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 158 ++++++++++++++++++++++++--------------------
 1 file changed, 88 insertions(+), 70 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 62b9fe5..c393477 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -299,22 +299,13 @@ static int parse_cmd(struct client *c)
 
 	r = get_cmdvec(c->cmd, &c->cmdvec);
 
-	if (r) {
-		genhelp_handler(c->cmd, r, &c->reply);
-		if (get_strbuf_len(&c->reply) == 0)
-			return EINVAL;
-		return 0;
-	}
+	if (r)
+		return -r;
 
 	c->handler = find_handler_for_cmdvec(c->cmdvec);
 
-	if (!c->handler || !c->handler->fn) {
-		genhelp_handler(c->cmd, EINVAL, &c->reply);
-		if (get_strbuf_len(&c->reply) == 0)
-			r = EINVAL;
-		else
-			r = 0;
-	}
+	if (!c->handler || !c->handler->fn)
+		return -EINVAL;
 
 	return r;
 }
@@ -325,7 +316,7 @@ static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
 	struct timespec tmo;
 
 	if (!c->handler)
-		return EINVAL;
+		return -EINVAL;
 
 	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
 		tmo.tv_sec += timeout;
@@ -355,50 +346,30 @@ static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
 	return r;
 }
 
-static int uxsock_trigger(struct client *c, void *trigger_data)
+void default_reply(struct client *c, int r)
 {
-	struct vectors * vecs;
-	int r = 1;
-
-	vecs = (struct vectors *)trigger_data;
-
-	r = parse_cmd(c);
-
-	if (r == 0 && c->cmdvec && VECTOR_SIZE(c->cmdvec) > 0) {
-		struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
-
-		if (!c->is_root && kw->code != LIST)
-			r = EPERM;
-	}
-
-	if (r == 0 && c->handler)
-		r = execute_handler(c, vecs, uxsock_timeout / 1000);
-
-	if (c->cmdvec) {
-		free_keys(c->cmdvec);
-		c->cmdvec = NULL;
-	}
-
-	if (r > 0) {
-		switch(r) {
-		case ETIMEDOUT:
-			append_strbuf_str(&c->reply, "timeout\n");
-			break;
-		case EPERM:
-			append_strbuf_str(&c->reply,
-					  "permission deny: need to be root\n");
-			break;
-		default:
-			append_strbuf_str(&c->reply, "fail\n");
-			break;
-		}
-	}
-	else if (!r && get_strbuf_len(&c->reply) == 0) {
+	switch(r) {
+	case -EINVAL:
+	case -ESRCH:
+	case -ENOMEM:
+		/* return codes from get_cmdvec() */
+		genhelp_handler(c->cmd, -r, &c->reply);
+		break;
+	case -EPERM:
+		append_strbuf_str(&c->reply,
+				  "permission deny: need to be root\n");
+		break;
+	case -ETIMEDOUT:
+		append_strbuf_str(&c->reply, "timeout\n");
+		break;
+	case 0:
 		append_strbuf_str(&c->reply, "ok\n");
-		r = 0;
+		break;
+	default:
+		/* cli_handler functions return 1 on unspecified error */
+		append_strbuf_str(&c->reply, "fail\n");
+		break;
 	}
-	/* else if (r < 0) leave *reply alone */
-	return r;
 }
 
 static void set_client_state(struct client *c, int state)
@@ -409,6 +380,7 @@ static void set_client_state(struct client *c, int state)
 		reset_strbuf(&c->reply);
 		memset(c->cmd, '\0', sizeof(c->cmd));
 		c->expires = ts_zero;
+		c->error = 0;
 		/* fallthrough */
 	case CLT_SEND:
 		/* reuse these fields for next data transfer */
@@ -420,9 +392,18 @@ static void set_client_state(struct client *c, int state)
 	c->state = state;
 }
 
-static void handle_client(struct client *c, void *trigger_data)
+enum {
+	STM_CONT,
+	STM_BREAK,
+};
+
+static int client_state_machine(struct client *c, struct vectors *vecs)
 {
 	ssize_t n;
+	const char *buf;
+
+	condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
+		c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
 
 	switch (c->state) {
 	case CLT_RECV:
@@ -449,31 +430,59 @@ static void handle_client(struct client *c, void *trigger_data)
 				condlog(4, "%s: cli[%d]: connected", __func__, c->fd);
 			}
 			/* poll for data */
-			return;
+			return STM_BREAK;
 		} else if (c->len < c->cmd_len) {
 			n = recv(c->fd, c->cmd + c->len, c->cmd_len - c->len, 0);
 			if (n <= 0 && errno != EINTR && errno != EAGAIN) {
 				condlog(1, "%s: cli[%d]: error in recv: %m",
 					__func__, c->fd);
 				c->error = -ECONNRESET;
-				return;
+				return STM_BREAK;
 			}
 			c->len += n;
 			if (c->len < c->cmd_len)
 				/* continue polling */
-				return;
-			set_client_state(c, CLT_PARSE);
+				return STM_BREAK;
 		}
-		break;
-	default:
-		break;
-	}
+		condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
+		set_client_state(c, CLT_PARSE);
+		return STM_CONT;
 
-	condlog(4, "cli[%d]: Got request [%s]", c->fd, c->cmd);
-	uxsock_trigger(c, trigger_data);
+	case CLT_PARSE:
+		c->error = parse_cmd(c);
+		if (!c->error) {
+			/* Permission check */
+			struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
 
-	if (get_strbuf_len(&c->reply) > 0) {
-		const char *buf = get_strbuf_str(&c->reply);
+			if (!c->is_root && kw->code != LIST) {
+				c->error = -EPERM;
+				condlog(0, "%s: cli[%d]: unauthorized cmd \"%s\"",
+					__func__, c->fd, c->cmd);
+			}
+		}
+		if (c->error)
+			set_client_state(c, CLT_SEND);
+		else
+			set_client_state(c, CLT_WORK);
+		return STM_CONT;
+
+	case CLT_LOCKED_WORK:
+		/* tbd */
+		set_client_state(c, CLT_WORK);
+		return STM_CONT;
+
+	case CLT_WORK:
+		c->error = execute_handler(c, vecs, uxsock_timeout / 1000);
+		free_keys(c->cmdvec);
+		c->cmdvec = NULL;
+		set_client_state(c, CLT_SEND);
+		return STM_CONT;
+
+	case CLT_SEND:
+		if (get_strbuf_len(&c->reply) == 0)
+			default_reply(c, c->error);
+
+		buf = get_strbuf_str(&c->reply);
 
 		if (send_packet(c->fd, buf) != 0)
 			dead_client(c);
@@ -481,9 +490,18 @@ static void handle_client(struct client *c, void *trigger_data)
 			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd,
 				get_strbuf_len(&c->reply) + 1);
 		reset_strbuf(&c->reply);
-	}
 
-	set_client_state(c, CLT_RECV);
+		set_client_state(c, CLT_RECV);
+		return STM_BREAK;
+
+	default:
+		return STM_BREAK;
+	}
+}
+
+static void handle_client(struct client *c, struct vectors *vecs)
+{
+	while (client_state_machine(c, vecs) == STM_CONT);
 }
 
 /*
-- 
2.33.1


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

[dm-devel] [PATCH v3 30/35] multipathd: uxlsnr: add idle notification

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The previous patches added the state machine and the timeout handling,
but there was no wakeup mechanism for the uxlsnr for cases where
client connections were waiting for the vecs lock.

This patch uses the previously introduced wakeup mechanism of
struct mutex_lock for this purpose. Processes which unlock the
"global" vecs lock send an event in an eventfd which the uxlsnr
loop is polling for.

As we are now woken up for servicing client handlers that don't
wait for input but for the lock, we need to set up the pollfds
differently, and iterate over all clients when handling events,
not only over the ones that are receiving. The hangup handling
is changed, too. We have to look at every client, even if one has
hung up. Note that I don't take client_lock for the loop in
uxsock_listen(), it's not necessary and will be removed elsewhere
in a follow-up patch.

With this in place, the lock need not be taken in execute_handler()
any more. The uxlsnr only ever calls trylock() on the vecs lock,
avoiding any waiting for other threads to finish.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 183 +++++++++++++++++++++++++++++---------------
 1 file changed, 121 insertions(+), 62 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index c393477..f559a23 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -24,6 +24,7 @@
 #include <signal.h>
 #include <stdbool.h>
 #include <sys/inotify.h>
+#include <sys/eventfd.h>
 #include "checkers.h"
 #include "debug.h"
 #include "vector.h"
@@ -69,6 +70,7 @@ struct client {
 enum {
 	POLLFD_UX = 0,
 	POLLFD_NOTIFY,
+	POLLFD_IDLE,
 	POLLFDS_BASE,
 };
 
@@ -89,6 +91,7 @@ static LIST_HEAD(clients);
 static pthread_mutex_t client_lock = PTHREAD_MUTEX_INITIALIZER;
 static struct pollfd *polls;
 static int notify_fd = -1;
+static int idle_fd = -1;
 static char *watch_config_dir;
 
 static bool _socket_client_is_root(int fd)
@@ -187,6 +190,17 @@ void uxsock_cleanup(void *arg)
 	free_polls();
 }
 
+void wakeup_cleanup(void *arg)
+{
+	struct mutex_lock *lck = arg;
+	int fd = idle_fd;
+
+	idle_fd = -1;
+	set_wakeup_fn(lck, NULL);
+	if (fd != -1)
+		close(fd);
+}
+
 struct watch_descriptors {
 	int conf_wd;
 	int dir_wd;
@@ -293,6 +307,18 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
 
+/* call with clients lock held */
+static bool __need_vecs_lock(void)
+{
+	struct client *c;
+
+	list_for_each_entry(c, &clients, node) {
+		if (c->state == CLT_LOCKED_WORK)
+			return true;
+	}
+	return false;
+}
+
 static int parse_cmd(struct client *c)
 {
 	int r;
@@ -310,40 +336,31 @@ static int parse_cmd(struct client *c)
 	return r;
 }
 
-static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
+static int execute_handler(struct client *c, struct vectors *vecs)
 {
-	int r;
-	struct timespec tmo;
 
-	if (!c->handler)
+	if (!c->handler || !c->handler->fn)
 		return -EINVAL;
 
-	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
-		tmo.tv_sec += timeout;
-	} else {
-		tmo.tv_sec = 0;
-	}
+	return c->handler->fn(c->cmdvec, &c->reply, vecs);
+}
 
-	if (c->handler->locked) {
-		int locked = 0;
+static void wakeup_listener(void)
+{
+	uint64_t one = 1;
 
-		pthread_cleanup_push(cleanup_lock, &vecs->lock);
-		if (tmo.tv_sec) {
-			r = timedlock(&vecs->lock, &tmo);
-		} else {
-			lock(&vecs->lock);
-			r = 0;
-		}
-		if (r == 0) {
-			locked = 1;
-			pthread_testcancel();
-			r = c->handler->fn(c->cmdvec, &c->reply, vecs);
-		}
-		pthread_cleanup_pop(locked);
-	} else
-		r = c->handler->fn(c->cmdvec, &c->reply, vecs);
+	if (idle_fd != -1 &&
+	    write(idle_fd, &one, sizeof(one)) != sizeof(one))
+		condlog(1, "%s: failed", __func__);
+}
 
-	return r;
+static void drain_idle_fd(int fd)
+{
+	uint64_t val;
+	int rc;
+
+	rc = read(fd, &val, sizeof(val));
+	condlog(4, "%s: %d, %"PRIu64, __func__, rc, val);
 }
 
 void default_reply(struct client *c, int r)
@@ -397,16 +414,19 @@ enum {
 	STM_BREAK,
 };
 
-static int client_state_machine(struct client *c, struct vectors *vecs)
+static int client_state_machine(struct client *c, struct vectors *vecs,
+				short revents)
 {
 	ssize_t n;
 	const char *buf;
 
-	condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
-		c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
+	condlog(4, "%s: cli[%d] poll=%x state=%d cmd=\"%s\" repl \"%s\"", __func__,
+		c->fd, revents, c->state, c->cmd, get_strbuf_str(&c->reply));
 
 	switch (c->state) {
 	case CLT_RECV:
+		if (!(revents & POLLIN))
+			return STM_BREAK;
 		if (c->cmd_len == 0) {
 			/*
 			 * We got POLLIN; assume that at least the length can
@@ -462,17 +482,30 @@ static int client_state_machine(struct client *c, struct vectors *vecs)
 		}
 		if (c->error)
 			set_client_state(c, CLT_SEND);
+		else if (c->handler->locked)
+			set_client_state(c, CLT_LOCKED_WORK);
 		else
 			set_client_state(c, CLT_WORK);
 		return STM_CONT;
 
 	case CLT_LOCKED_WORK:
-		/* tbd */
-		set_client_state(c, CLT_WORK);
-		return STM_CONT;
+                if (trylock(&vecs->lock) == 0) {
+			/* don't use cleanup_lock(), lest we wakeup ourselves */
+			pthread_cleanup_push_cast(__unlock, &vecs->lock);
+			c->error = execute_handler(c, vecs);
+			pthread_cleanup_pop(1);
+			condlog(4, "%s: cli[%d] grabbed lock", __func__, c->fd);
+			free_keys(c->cmdvec);
+			c->cmdvec = NULL;
+			set_client_state(c, CLT_SEND);
+			return STM_CONT;
+		} else {
+			condlog(4, "%s: cli[%d] waiting for lock", __func__, c->fd);
+			return STM_BREAK;
+		}
 
 	case CLT_WORK:
-		c->error = execute_handler(c, vecs, uxsock_timeout / 1000);
+		c->error = execute_handler(c, vecs);
 		free_keys(c->cmdvec);
 		c->cmdvec = NULL;
 		set_client_state(c, CLT_SEND);
@@ -499,9 +532,14 @@ static int client_state_machine(struct client *c, struct vectors *vecs)
 	}
 }
 
-static void handle_client(struct client *c, struct vectors *vecs)
+static void handle_client(struct client *c, struct vectors *vecs, short revents)
 {
-	while (client_state_machine(c, vecs) == STM_CONT);
+	if (revents & (POLLHUP|POLLERR)) {
+		c->error = -ECONNRESET;
+		return;
+	}
+
+        while (client_state_machine(c, vecs, revents) == STM_CONT);
 }
 
 /*
@@ -514,6 +552,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
 	unsigned int sequence_nr = 0;
 	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
+	bool need_lock = false;
+	struct vectors *vecs = trigger_data;
 
 	condlog(3, "uxsock: startup listener");
 	polls = calloc(1, max_pfds * sizeof(*polls));
@@ -524,6 +564,15 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	notify_fd = inotify_init1(IN_NONBLOCK);
 	if (notify_fd == -1) /* it's fine if notifications fail */
 		condlog(3, "failed to start up configuration notifications");
+
+	pthread_cleanup_push(wakeup_cleanup, &vecs->lock);
+	idle_fd = eventfd(0, EFD_NONBLOCK|EFD_CLOEXEC);
+	if (idle_fd == -1) {
+		condlog(1, "failed to create idle fd");
+		exit_daemon();
+	} else
+		set_wakeup_fn(&vecs->lock, wakeup_listener);
+
 	sigfillset(&mask);
 	sigdelset(&mask, SIGINT);
 	sigdelset(&mask, SIGTERM);
@@ -575,11 +624,25 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 		else
 			polls[POLLFD_NOTIFY].events = POLLIN;
 
+		need_lock = __need_vecs_lock();
+		polls[POLLFD_IDLE].fd = idle_fd;
+		if (need_lock)
+			polls[POLLFD_IDLE].events = POLLIN;
+		else
+			polls[POLLFD_IDLE].events = 0;
+
 		/* setup the clients */
 		i = POLLFDS_BASE;
 		list_for_each_entry(c, &clients, node) {
+			switch(c->state) {
+			case CLT_RECV:
+				polls[i].events = POLLIN;
+				break;
+			default:
+				/* don't poll for this client */
+				continue;
+			}
 			polls[i].fd = c->fd;
-			polls[i].events = POLLIN;
 			i++;
 			if (i >= max_pfds)
 				break;
@@ -607,33 +670,28 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 			handle_signals(true);
 			continue;
 		}
+		if (polls[POLLFD_IDLE].fd != -1 &&
+		    polls[POLLFD_IDLE].revents & POLLIN)
+			drain_idle_fd(idle_fd);
 
-		/* see if a client wants to speak to us */
-		for (i = POLLFDS_BASE; i < n_pfds; i++) {
-			if (polls[i].revents & (POLLIN|POLLHUP|POLLERR)) {
-				c = NULL;
-				pthread_mutex_lock(&client_lock);
-				list_for_each_entry(tmp, &clients, node) {
-					if (tmp->fd == polls[i].fd) {
-						c = tmp;
-						break;
-					}
+		/* see if a client needs handling */
+		list_for_each_entry_safe(c, tmp, &clients, node) {
+			short revents = 0;
+
+			for (i = POLLFDS_BASE; i < n_pfds; i++) {
+				if (polls[i].fd == c->fd) {
+					revents = polls[i].revents;
+					break;
 				}
-				pthread_mutex_unlock(&client_lock);
-				if (!c) {
-					condlog(4, "cli%d: new fd %d",
-						i, polls[i].fd);
-					continue;
-				}
-				if (polls[i].revents & (POLLHUP|POLLERR)) {
-					condlog(4, "cli[%d]: Disconnected",
-						c->fd);
-					dead_client(c);
-					continue;
-				}
-				handle_client(c, trigger_data);
-				if (c->error == -ECONNRESET)
-					dead_client(c);
+			}
+
+			handle_client(c, trigger_data, revents);
+
+			if (c->error == -ECONNRESET) {
+				condlog(4, "cli[%d]: disconnected", c->fd);
+				dead_client(c);
+				if (i < n_pfds)
+					polls[i].fd = -1;
 			}
 		}
 		/* see if we got a non-fatal signal */
@@ -649,5 +707,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 			handle_inotify(notify_fd, &wds);
 	}
 
+	pthread_cleanup_pop(1);
 	return NULL;
 }
-- 
2.33.1


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

[dm-devel] [PATCH v3 31/35] multipathd: uxlsnr: add timeout handling

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Our ppoll() call needs to wake up when a client request times out.
This logic can be added by determining the first client that's about
to time out. The logic in handle_client() will then cause a timeout
reply to be sent to the client. This is more client-friendly
as the client timing out without receiving a reply.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index f559a23..1ebcf10 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -306,6 +306,35 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 }
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
+static const struct timespec ts_max = { .tv_sec = LONG_MAX, .tv_nsec = 999999999 };
+
+/* call with clients lock held */
+static struct timespec *__get_soonest_timeout(struct timespec *ts)
+{
+	struct timespec ts_min = ts_max, now;
+	bool any = false;
+	struct client *c;
+
+	list_for_each_entry(c, &clients, node) {
+		if (timespeccmp(&c->expires, &ts_zero) != 0 &&
+		    timespeccmp(&c->expires, &ts_min) < 0) {
+			ts_min = c->expires;
+			any = true;
+		}
+	}
+
+	if (!any)
+		return NULL;
+
+	get_monotonic_time(&now);
+	timespecsub(&ts_min, &now, ts);
+	if (timespeccmp(ts, &ts_zero) < 0)
+		*ts = ts_zero;
+
+	condlog(4, "%s: next client expires in %ld.%03lds", __func__,
+		(long)ts->tv_sec, ts->tv_nsec / 1000000);
+	return ts;
+}
 
 /* call with clients lock held */
 static bool __need_vecs_lock(void)
@@ -532,6 +561,24 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 	}
 }
 
+static void check_timeout(struct client *c)
+{
+	struct timespec now;
+
+	if (timespeccmp(&c->expires, &ts_zero) == 0)
+		return;
+
+	get_monotonic_time(&now);
+	if (timespeccmp(&c->expires, &now) > 0)
+		return;
+
+	condlog(2, "%s: cli[%d]: timed out at %ld.%03ld", __func__,
+		c->fd, (long)c->expires.tv_sec, c->expires.tv_nsec / 1000000);
+
+	c->error = -ETIMEDOUT;
+	set_client_state(c, CLT_SEND);
+}
+
 static void handle_client(struct client *c, struct vectors *vecs, short revents)
 {
 	if (revents & (POLLHUP|POLLERR)) {
@@ -539,6 +586,7 @@ static void handle_client(struct client *c, struct vectors *vecs, short revents)
 		return;
 	}
 
+	check_timeout(c);
         while (client_state_machine(c, vecs, revents) == STM_CONT);
 }
 
@@ -581,6 +629,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	while (1) {
 		struct client *c, *tmp;
 		int i, n_pfds, poll_count, num_clients;
+		struct timespec __timeout, *timeout;
 
 		/* setup for a poll */
 		pthread_mutex_lock(&client_lock);
@@ -648,10 +697,11 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 				break;
 		}
 		n_pfds = i;
+		timeout = __get_soonest_timeout(&__timeout);
 		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, n_pfds, NULL, &mask);
+		poll_count = ppoll(polls, n_pfds, timeout, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -666,10 +716,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 			break;
 		}
 
-		if (poll_count == 0) {
-			handle_signals(true);
-			continue;
-		}
 		if (polls[POLLFD_IDLE].fd != -1 &&
 		    polls[POLLFD_IDLE].revents & POLLIN)
 			drain_idle_fd(idle_fd);
-- 
2.33.1


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

[dm-devel] [PATCH v3 31/35] rmultipathd: uxlsnr: add timeout handling

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Our ppoll() call needs to wake up when a client request times out.
This logic can be added by determining the first client that's about
to time out. The logic in handle_client() will then cause a timeout
reply to be sent to the client. This is more client-friendly
as the client timing out without receiving a reply.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index f559a23..1ebcf10 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -306,6 +306,35 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 }
 
 static const struct timespec ts_zero = { .tv_sec = 0, };
+static const struct timespec ts_max = { .tv_sec = LONG_MAX, .tv_nsec = 999999999 };
+
+/* call with clients lock held */
+static struct timespec *__get_soonest_timeout(struct timespec *ts)
+{
+	struct timespec ts_min = ts_max, now;
+	bool any = false;
+	struct client *c;
+
+	list_for_each_entry(c, &clients, node) {
+		if (timespeccmp(&c->expires, &ts_zero) != 0 &&
+		    timespeccmp(&c->expires, &ts_min) < 0) {
+			ts_min = c->expires;
+			any = true;
+		}
+	}
+
+	if (!any)
+		return NULL;
+
+	get_monotonic_time(&now);
+	timespecsub(&ts_min, &now, ts);
+	if (timespeccmp(ts, &ts_zero) < 0)
+		*ts = ts_zero;
+
+	condlog(4, "%s: next client expires in %ld.%03lds", __func__,
+		(long)ts->tv_sec, ts->tv_nsec / 1000000);
+	return ts;
+}
 
 /* call with clients lock held */
 static bool __need_vecs_lock(void)
@@ -532,6 +561,24 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 	}
 }
 
+static void check_timeout(struct client *c)
+{
+	struct timespec now;
+
+	if (timespeccmp(&c->expires, &ts_zero) == 0)
+		return;
+
+	get_monotonic_time(&now);
+	if (timespeccmp(&c->expires, &now) > 0)
+		return;
+
+	condlog(2, "%s: cli[%d]: timed out at %ld.%03ld", __func__,
+		c->fd, (long)c->expires.tv_sec, c->expires.tv_nsec / 1000000);
+
+	c->error = -ETIMEDOUT;
+	set_client_state(c, CLT_SEND);
+}
+
 static void handle_client(struct client *c, struct vectors *vecs, short revents)
 {
 	if (revents & (POLLHUP|POLLERR)) {
@@ -539,6 +586,7 @@ static void handle_client(struct client *c, struct vectors *vecs, short revents)
 		return;
 	}
 
+	check_timeout(c);
         while (client_state_machine(c, vecs, revents) == STM_CONT);
 }
 
@@ -581,6 +629,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	while (1) {
 		struct client *c, *tmp;
 		int i, n_pfds, poll_count, num_clients;
+		struct timespec __timeout, *timeout;
 
 		/* setup for a poll */
 		pthread_mutex_lock(&client_lock);
@@ -648,10 +697,11 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 				break;
 		}
 		n_pfds = i;
+		timeout = __get_soonest_timeout(&__timeout);
 		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, n_pfds, NULL, &mask);
+		poll_count = ppoll(polls, n_pfds, timeout, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -666,10 +716,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 			break;
 		}
 
-		if (poll_count == 0) {
-			handle_signals(true);
-			continue;
-		}
 		if (polls[POLLFD_IDLE].fd != -1 &&
 		    polls[POLLFD_IDLE].revents & POLLIN)
 			drain_idle_fd(idle_fd);
-- 
2.33.1


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

[dm-devel] [PATCH v3 32/35] multipathd: uxlsnr: use poll loop for sending, too

[mwilck]

From: Martin Wilck <mwilck@suse.com>

send_packet() may busy-loop. By polling for POLLOUT, we can
avoid that, even if it's very unlikely in practice.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 1ebcf10..c5da569 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -447,7 +447,6 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 				short revents)
 {
 	ssize_t n;
-	const char *buf;
 
 	condlog(4, "%s: cli[%d] poll=%x state=%d cmd=\"%s\" repl \"%s\"", __func__,
 		c->fd, revents, c->state, c->cmd, get_strbuf_str(&c->reply));
@@ -527,7 +526,8 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 			free_keys(c->cmdvec);
 			c->cmdvec = NULL;
 			set_client_state(c, CLT_SEND);
-			return STM_CONT;
+			/* Wait for POLLOUT */
+			return STM_BREAK;
 		} else {
 			condlog(4, "%s: cli[%d] waiting for lock", __func__, c->fd);
 			return STM_BREAK;
@@ -538,22 +538,38 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 		free_keys(c->cmdvec);
 		c->cmdvec = NULL;
 		set_client_state(c, CLT_SEND);
-		return STM_CONT;
+		/* Wait for POLLOUT */
+		return STM_BREAK;
 
 	case CLT_SEND:
 		if (get_strbuf_len(&c->reply) == 0)
 			default_reply(c, c->error);
 
-		buf = get_strbuf_str(&c->reply);
+		if (c->cmd_len == 0) {
+			size_t len = get_strbuf_len(&c->reply) + 1;
 
-		if (send_packet(c->fd, buf) != 0)
-			dead_client(c);
-		else
-			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd,
-				get_strbuf_len(&c->reply) + 1);
-		reset_strbuf(&c->reply);
+			if (send(c->fd, &len, sizeof(len), MSG_NOSIGNAL)
+			    != sizeof(len))
+				c->error = -ECONNRESET;
+			c->cmd_len = len;
+			return STM_BREAK;
+		}
 
-		set_client_state(c, CLT_RECV);
+		if (c->len < c->cmd_len) {
+			const char *buf = get_strbuf_str(&c->reply);
+
+			n = send(c->fd, buf + c->len, c->cmd_len, MSG_NOSIGNAL);
+			if (n == -1) {
+				if (!(errno == EAGAIN || errno == EINTR))
+					c->error = -ECONNRESET;
+			} else
+				c->len += n;
+		}
+
+                if (c->len >= c->cmd_len) {
+			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd, c->cmd_len);
+			set_client_state(c, CLT_RECV);
+		}
 		return STM_BREAK;
 
 	default:
@@ -687,6 +703,9 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 			case CLT_RECV:
 				polls[i].events = POLLIN;
 				break;
+			case CLT_SEND:
+				polls[i].events = POLLOUT;
+				break;
 			default:
 				/* don't poll for this client */
 				continue;
-- 
2.33.1


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

[dm-devel] [PATCH v3 33/35] multipathd: uxlsnr: drop client_lock

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The list of clients is never changed anywhere except in
uxsock_listen(). No need to lock.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index c5da569..07a484c 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -88,7 +88,6 @@ enum {
 static __attribute__((unused)) char ___a[-(MIN_POLLS <= 0)];
 
 static LIST_HEAD(clients);
-static pthread_mutex_t client_lock = PTHREAD_MUTEX_INITIALIZER;
 static struct pollfd *polls;
 static int notify_fd = -1;
 static int idle_fd = -1;
@@ -135,15 +134,13 @@ static void new_client(int ux_sock)
 	c->is_root = _socket_client_is_root(c->fd);
 
 	/* put it in our linked list */
-	pthread_mutex_lock(&client_lock);
 	list_add_tail(&c->node, &clients);
-	pthread_mutex_unlock(&client_lock);
 }
 
 /*
  * kill off a dead client
  */
-static void _dead_client(struct client *c)
+static void dead_client(struct client *c)
 {
 	int fd = c->fd;
 	list_del_init(&c->node);
@@ -155,14 +152,6 @@ static void _dead_client(struct client *c)
 	close(fd);
 }
 
-static void dead_client(struct client *c)
-{
-	pthread_cleanup_push(cleanup_mutex, &client_lock);
-	pthread_mutex_lock(&client_lock);
-	_dead_client(c);
-	pthread_cleanup_pop(1);
-}
-
 static void free_polls (void)
 {
 	if (polls)
@@ -180,11 +169,9 @@ void uxsock_cleanup(void *arg)
 	close(notify_fd);
 	free(watch_config_dir);
 
-	pthread_mutex_lock(&client_lock);
 	list_for_each_entry_safe(client_loop, client_tmp, &clients, node) {
-		_dead_client(client_loop);
+		dead_client(client_loop);
 	}
-	pthread_mutex_unlock(&client_lock);
 
 	cli_exit();
 	free_polls();
@@ -308,8 +295,7 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 static const struct timespec ts_zero = { .tv_sec = 0, };
 static const struct timespec ts_max = { .tv_sec = LONG_MAX, .tv_nsec = 999999999 };
 
-/* call with clients lock held */
-static struct timespec *__get_soonest_timeout(struct timespec *ts)
+static struct timespec *get_soonest_timeout(struct timespec *ts)
 {
 	struct timespec ts_min = ts_max, now;
 	bool any = false;
@@ -336,8 +322,7 @@ static struct timespec *__get_soonest_timeout(struct timespec *ts)
 	return ts;
 }
 
-/* call with clients lock held */
-static bool __need_vecs_lock(void)
+static bool need_vecs_lock(void)
 {
 	struct client *c;
 
@@ -616,7 +601,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
 	unsigned int sequence_nr = 0;
 	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
-	bool need_lock = false;
 	struct vectors *vecs = trigger_data;
 
 	condlog(3, "uxsock: startup listener");
@@ -648,8 +632,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 		struct timespec __timeout, *timeout;
 
 		/* setup for a poll */
-		pthread_mutex_lock(&client_lock);
-		pthread_cleanup_push(cleanup_mutex, &client_lock);
 		num_clients = 0;
 		list_for_each_entry(c, &clients, node) {
 			num_clients++;
@@ -689,9 +671,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 		else
 			polls[POLLFD_NOTIFY].events = POLLIN;
 
-		need_lock = __need_vecs_lock();
 		polls[POLLFD_IDLE].fd = idle_fd;
-		if (need_lock)
+		if (need_vecs_lock())
 			polls[POLLFD_IDLE].events = POLLIN;
 		else
 			polls[POLLFD_IDLE].events = 0;
@@ -716,8 +697,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 				break;
 		}
 		n_pfds = i;
-		timeout = __get_soonest_timeout(&__timeout);
-		pthread_cleanup_pop(1);
+		timeout = get_soonest_timeout(&__timeout);
 
 		/* most of our life is spent in this call */
 		poll_count = ppoll(polls, n_pfds, timeout, &mask);
-- 
2.33.1


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

[dm-devel] [PATCH v3 34/35] multipathd: uxclt: allow client mode for non-root, too

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The server checks for root permissions anyway. "multipathd -k"
should work for ordinary users as long as no priviledged commands
are executed.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index cd9a127..1888710 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3344,11 +3344,6 @@ main (int argc, char *argv[])
 
 	logsink = LOGSINK_SYSLOG;
 
-	if (getuid() != 0) {
-		fprintf(stderr, "need to be root\n");
-		exit(1);
-	}
-
 	/* make sure we don't lock any path */
 	if (chdir("/") < 0)
 		fprintf(stderr, "can't chdir to root directory : %s\n",
@@ -3435,6 +3430,11 @@ main (int argc, char *argv[])
 		return err;
 	}
 
+	if (getuid() != 0) {
+		fprintf(stderr, "need to be root\n");
+		exit(1);
+	}
+
 	if (foreground) {
 		if (!isatty(fileno(stdout)))
 			setbuf(stdout, NULL);
-- 
2.33.1


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

[dm-devel] [PATCH v3 35/35] multipathd: uxlsnr: use recv() for command length

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If the peer uses libmpathcmd, we can be certain that the first
8 bytes are being sent in a single chunk of data. It's overkill
to try and receive the command length byte-by-byte.

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

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 07a484c..1b3aa95 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -441,6 +441,7 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 		if (!(revents & POLLIN))
 			return STM_BREAK;
 		if (c->cmd_len == 0) {
+			size_t len;
 			/*
 			 * We got POLLIN; assume that at least the length can
 			 * be read immediately.
@@ -449,17 +450,17 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
 			c->expires.tv_sec += uxsock_timeout / 1000;
 			c->expires.tv_nsec += (uxsock_timeout % 1000) * 1000000;
 			normalize_timespec(&c->expires);
-			n = mpath_recv_reply_len(c->fd, 0);
-			if (n == -1) {
-				condlog(1, "%s: cli[%d]: failed to receive reply len",
-					__func__, c->fd);
-				c->error = -ECONNRESET;
-			} else if (n > _MAX_CMD_LEN) {
-				condlog(1, "%s: cli[%d]: overlong command (%zd bytes)",
+			n = recv(c->fd, &len, sizeof(len), 0);
+			if (n < (ssize_t)sizeof(len)) {
+				condlog(1, "%s: cli[%d]: failed to receive reply len: %zd",
 					__func__, c->fd, n);
 				c->error = -ECONNRESET;
+			} else if (len <= 0 || len > _MAX_CMD_LEN) {
+				condlog(1, "%s: cli[%d]: invalid command length (%zu bytes)",
+					__func__, c->fd, len);
+				c->error = -ECONNRESET;
 			} else {
-				c->cmd_len = n;
+				c->cmd_len = len;
 				condlog(4, "%s: cli[%d]: connected", __func__, c->fd);
 			}
 			/* poll for data */
-- 
2.33.1


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

Re: [dm-devel] [PATCH v3 30/35] multipathd: uxlsnr: add idle notification

[Benjamin Marzinski]

On Sat, Nov 27, 2021 at 04:19:23PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The previous patches added the state machine and the timeout handling,
> but there was no wakeup mechanism for the uxlsnr for cases where
> client connections were waiting for the vecs lock.
> 
> This patch uses the previously introduced wakeup mechanism of
> struct mutex_lock for this purpose. Processes which unlock the
> "global" vecs lock send an event in an eventfd which the uxlsnr
> loop is polling for.
> 
> As we are now woken up for servicing client handlers that don't
> wait for input but for the lock, we need to set up the pollfds
> differently, and iterate over all clients when handling events,
> not only over the ones that are receiving. The hangup handling
> is changed, too. We have to look at every client, even if one has
> hung up. Note that I don't take client_lock for the loop in
> uxsock_listen(), it's not necessary and will be removed elsewhere
> in a follow-up patch.
> 
> With this in place, the lock need not be taken in execute_handler()
> any more. The uxlsnr only ever calls trylock() on the vecs lock,
> avoiding any waiting for other threads to finish.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/uxlsnr.c | 183 +++++++++++++++++++++++++++++---------------
>  1 file changed, 121 insertions(+), 62 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index c393477..f559a23 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -24,6 +24,7 @@
>  #include <signal.h>
>  #include <stdbool.h>
>  #include <sys/inotify.h>
> +#include <sys/eventfd.h>
>  #include "checkers.h"
>  #include "debug.h"
>  #include "vector.h"
> @@ -69,6 +70,7 @@ struct client {
>  enum {
>  	POLLFD_UX = 0,
>  	POLLFD_NOTIFY,
> +	POLLFD_IDLE,
>  	POLLFDS_BASE,
>  };
>  
> @@ -89,6 +91,7 @@ static LIST_HEAD(clients);
>  static pthread_mutex_t client_lock = PTHREAD_MUTEX_INITIALIZER;
>  static struct pollfd *polls;
>  static int notify_fd = -1;
> +static int idle_fd = -1;
>  static char *watch_config_dir;
>  
>  static bool _socket_client_is_root(int fd)
> @@ -187,6 +190,17 @@ void uxsock_cleanup(void *arg)
>  	free_polls();
>  }
>  
> +void wakeup_cleanup(void *arg)
> +{
> +	struct mutex_lock *lck = arg;
> +	int fd = idle_fd;
> +
> +	idle_fd = -1;
> +	set_wakeup_fn(lck, NULL);
> +	if (fd != -1)
> +		close(fd);
> +}
> +
>  struct watch_descriptors {
>  	int conf_wd;
>  	int dir_wd;
> @@ -293,6 +307,18 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
>  
>  static const struct timespec ts_zero = { .tv_sec = 0, };
>  
> +/* call with clients lock held */
> +static bool __need_vecs_lock(void)
> +{
> +	struct client *c;
> +
> +	list_for_each_entry(c, &clients, node) {
> +		if (c->state == CLT_LOCKED_WORK)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static int parse_cmd(struct client *c)
>  {
>  	int r;
> @@ -310,40 +336,31 @@ static int parse_cmd(struct client *c)
>  	return r;
>  }
>  
> -static int execute_handler(struct client *c, struct vectors *vecs, int timeout)
> +static int execute_handler(struct client *c, struct vectors *vecs)
>  {
> -	int r;
> -	struct timespec tmo;
>  
> -	if (!c->handler)
> +	if (!c->handler || !c->handler->fn)
>  		return -EINVAL;
>  
> -	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
> -		tmo.tv_sec += timeout;
> -	} else {
> -		tmo.tv_sec = 0;
> -	}
> +	return c->handler->fn(c->cmdvec, &c->reply, vecs);
> +}
>  
> -	if (c->handler->locked) {
> -		int locked = 0;
> +static void wakeup_listener(void)
> +{
> +	uint64_t one = 1;
>  
> -		pthread_cleanup_push(cleanup_lock, &vecs->lock);
> -		if (tmo.tv_sec) {
> -			r = timedlock(&vecs->lock, &tmo);
> -		} else {
> -			lock(&vecs->lock);
> -			r = 0;
> -		}
> -		if (r == 0) {
> -			locked = 1;
> -			pthread_testcancel();
> -			r = c->handler->fn(c->cmdvec, &c->reply, vecs);
> -		}
> -		pthread_cleanup_pop(locked);
> -	} else
> -		r = c->handler->fn(c->cmdvec, &c->reply, vecs);
> +	if (idle_fd != -1 &&
> +	    write(idle_fd, &one, sizeof(one)) != sizeof(one))
> +		condlog(1, "%s: failed", __func__);
> +}
>  
> -	return r;
> +static void drain_idle_fd(int fd)
> +{
> +	uint64_t val;
> +	int rc;
> +
> +	rc = read(fd, &val, sizeof(val));
> +	condlog(4, "%s: %d, %"PRIu64, __func__, rc, val);
>  }
>  
>  void default_reply(struct client *c, int r)
> @@ -397,16 +414,19 @@ enum {
>  	STM_BREAK,
>  };
>  
> -static int client_state_machine(struct client *c, struct vectors *vecs)
> +static int client_state_machine(struct client *c, struct vectors *vecs,
> +				short revents)
>  {
>  	ssize_t n;
>  	const char *buf;
>  
> -	condlog(4, "%s: cli[%d] state=%d cmd=\"%s\" repl \"%s\"", __func__,
> -		c->fd, c->state, c->cmd, get_strbuf_str(&c->reply));
> +	condlog(4, "%s: cli[%d] poll=%x state=%d cmd=\"%s\" repl \"%s\"", __func__,
> +		c->fd, revents, c->state, c->cmd, get_strbuf_str(&c->reply));
>  
>  	switch (c->state) {
>  	case CLT_RECV:
> +		if (!(revents & POLLIN))
> +			return STM_BREAK;
>  		if (c->cmd_len == 0) {
>  			/*
>  			 * We got POLLIN; assume that at least the length can
> @@ -462,17 +482,30 @@ static int client_state_machine(struct client *c, struct vectors *vecs)
>  		}
>  		if (c->error)
>  			set_client_state(c, CLT_SEND);
> +		else if (c->handler->locked)
> +			set_client_state(c, CLT_LOCKED_WORK);
>  		else
>  			set_client_state(c, CLT_WORK);
>  		return STM_CONT;
>  
>  	case CLT_LOCKED_WORK:
> -		/* tbd */
> -		set_client_state(c, CLT_WORK);
> -		return STM_CONT;
> +                if (trylock(&vecs->lock) == 0) {
> +			/* don't use cleanup_lock(), lest we wakeup ourselves */
> +			pthread_cleanup_push_cast(__unlock, &vecs->lock);
> +			c->error = execute_handler(c, vecs);
> +			pthread_cleanup_pop(1);
> +			condlog(4, "%s: cli[%d] grabbed lock", __func__, c->fd);
> +			free_keys(c->cmdvec);
> +			c->cmdvec = NULL;
> +			set_client_state(c, CLT_SEND);
> +			return STM_CONT;
> +		} else {
> +			condlog(4, "%s: cli[%d] waiting for lock", __func__, c->fd);
> +			return STM_BREAK;
> +		}
>  
>  	case CLT_WORK:
> -		c->error = execute_handler(c, vecs, uxsock_timeout / 1000);
> +		c->error = execute_handler(c, vecs);
>  		free_keys(c->cmdvec);
>  		c->cmdvec = NULL;
>  		set_client_state(c, CLT_SEND);
> @@ -499,9 +532,14 @@ static int client_state_machine(struct client *c, struct vectors *vecs)
>  	}
>  }
>  
> -static void handle_client(struct client *c, struct vectors *vecs)
> +static void handle_client(struct client *c, struct vectors *vecs, short revents)
>  {
> -	while (client_state_machine(c, vecs) == STM_CONT);
> +	if (revents & (POLLHUP|POLLERR)) {
> +		c->error = -ECONNRESET;
> +		return;
> +	}
> +
> +        while (client_state_machine(c, vecs, revents) == STM_CONT);
>  }
>  
>  /*
> @@ -514,6 +552,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
>  	unsigned int sequence_nr = 0;
>  	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
> +	bool need_lock = false;
> +	struct vectors *vecs = trigger_data;
>  
>  	condlog(3, "uxsock: startup listener");
>  	polls = calloc(1, max_pfds * sizeof(*polls));
> @@ -524,6 +564,15 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  	notify_fd = inotify_init1(IN_NONBLOCK);
>  	if (notify_fd == -1) /* it's fine if notifications fail */
>  		condlog(3, "failed to start up configuration notifications");
> +
> +	pthread_cleanup_push(wakeup_cleanup, &vecs->lock);
> +	idle_fd = eventfd(0, EFD_NONBLOCK|EFD_CLOEXEC);
> +	if (idle_fd == -1) {
> +		condlog(1, "failed to create idle fd");
> +		exit_daemon();
> +	} else
> +		set_wakeup_fn(&vecs->lock, wakeup_listener);
> +
>  	sigfillset(&mask);
>  	sigdelset(&mask, SIGINT);
>  	sigdelset(&mask, SIGTERM);
> @@ -575,11 +624,25 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  		else
>  			polls[POLLFD_NOTIFY].events = POLLIN;
>  
> +		need_lock = __need_vecs_lock();
> +		polls[POLLFD_IDLE].fd = idle_fd;
> +		if (need_lock)
> +			polls[POLLFD_IDLE].events = POLLIN;
> +		else
> +			polls[POLLFD_IDLE].events = 0;
> +
>  		/* setup the clients */
>  		i = POLLFDS_BASE;
>  		list_for_each_entry(c, &clients, node) {
> +			switch(c->state) {
> +			case CLT_RECV:
> +				polls[i].events = POLLIN;
> +				break;
> +			default:
> +				/* don't poll for this client */
> +				continue;
> +			}
>  			polls[i].fd = c->fd;
> -			polls[i].events = POLLIN;
>  			i++;
>  			if (i >= max_pfds)
>  				break;
> @@ -607,33 +670,28 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  			handle_signals(true);
>  			continue;
>  		}
> +		if (polls[POLLFD_IDLE].fd != -1 &&
> +		    polls[POLLFD_IDLE].revents & POLLIN)
> +			drain_idle_fd(idle_fd);
>  
> -		/* see if a client wants to speak to us */
> -		for (i = POLLFDS_BASE; i < n_pfds; i++) {
> -			if (polls[i].revents & (POLLIN|POLLHUP|POLLERR)) {
> -				c = NULL;
> -				pthread_mutex_lock(&client_lock);
> -				list_for_each_entry(tmp, &clients, node) {
> -					if (tmp->fd == polls[i].fd) {
> -						c = tmp;
> -						break;
> -					}
> +		/* see if a client needs handling */
> +		list_for_each_entry_safe(c, tmp, &clients, node) {
> +			short revents = 0;
> +
> +			for (i = POLLFDS_BASE; i < n_pfds; i++) {
> +				if (polls[i].fd == c->fd) {
> +					revents = polls[i].revents;
> +					break;
>  				}
> -				pthread_mutex_unlock(&client_lock);
> -				if (!c) {
> -					condlog(4, "cli%d: new fd %d",
> -						i, polls[i].fd);
> -					continue;
> -				}
> -				if (polls[i].revents & (POLLHUP|POLLERR)) {
> -					condlog(4, "cli[%d]: Disconnected",
> -						c->fd);
> -					dead_client(c);
> -					continue;
> -				}
> -				handle_client(c, trigger_data);
> -				if (c->error == -ECONNRESET)
> -					dead_client(c);
> +			}
> +
> +			handle_client(c, trigger_data, revents);
> +
> +			if (c->error == -ECONNRESET) {
> +				condlog(4, "cli[%d]: disconnected", c->fd);
> +				dead_client(c);
> +				if (i < n_pfds)
> +					polls[i].fd = -1;
>  			}
>  		}
>  		/* see if we got a non-fatal signal */
> @@ -649,5 +707,6 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  			handle_inotify(notify_fd, &wds);
>  	}
>  
> +	pthread_cleanup_pop(1);
>  	return NULL;
>  }
> -- 
> 2.33.1

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

Re: [dm-devel] [PATCH v3 32/35] multipathd: uxlsnr: use poll loop for sending, too

[Benjamin Marzinski]

On Sat, Nov 27, 2021 at 04:19:26PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> send_packet() may busy-loop. By polling for POLLOUT, we can
> avoid that, even if it's very unlikely in practice.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/uxlsnr.c | 41 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 1ebcf10..c5da569 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -447,7 +447,6 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
>  				short revents)
>  {
>  	ssize_t n;
> -	const char *buf;
>  
>  	condlog(4, "%s: cli[%d] poll=%x state=%d cmd=\"%s\" repl \"%s\"", __func__,
>  		c->fd, revents, c->state, c->cmd, get_strbuf_str(&c->reply));
> @@ -527,7 +526,8 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
>  			free_keys(c->cmdvec);
>  			c->cmdvec = NULL;
>  			set_client_state(c, CLT_SEND);
> -			return STM_CONT;
> +			/* Wait for POLLOUT */
> +			return STM_BREAK;
>  		} else {
>  			condlog(4, "%s: cli[%d] waiting for lock", __func__, c->fd);
>  			return STM_BREAK;
> @@ -538,22 +538,38 @@ static int client_state_machine(struct client *c, struct vectors *vecs,
>  		free_keys(c->cmdvec);
>  		c->cmdvec = NULL;
>  		set_client_state(c, CLT_SEND);
> -		return STM_CONT;
> +		/* Wait for POLLOUT */
> +		return STM_BREAK;
>  
>  	case CLT_SEND:
>  		if (get_strbuf_len(&c->reply) == 0)
>  			default_reply(c, c->error);
>  
> -		buf = get_strbuf_str(&c->reply);
> +		if (c->cmd_len == 0) {
> +			size_t len = get_strbuf_len(&c->reply) + 1;
>  
> -		if (send_packet(c->fd, buf) != 0)
> -			dead_client(c);
> -		else
> -			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd,
> -				get_strbuf_len(&c->reply) + 1);
> -		reset_strbuf(&c->reply);
> +			if (send(c->fd, &len, sizeof(len), MSG_NOSIGNAL)
> +			    != sizeof(len))
> +				c->error = -ECONNRESET;
> +			c->cmd_len = len;
> +			return STM_BREAK;
> +		}
>  
> -		set_client_state(c, CLT_RECV);
> +		if (c->len < c->cmd_len) {
> +			const char *buf = get_strbuf_str(&c->reply);
> +
> +			n = send(c->fd, buf + c->len, c->cmd_len, MSG_NOSIGNAL);
> +			if (n == -1) {
> +				if (!(errno == EAGAIN || errno == EINTR))
> +					c->error = -ECONNRESET;
> +			} else
> +				c->len += n;
> +		}
> +
> +                if (c->len >= c->cmd_len) {
> +			condlog(4, "cli[%d]: Reply [%zu bytes]", c->fd, c->cmd_len);
> +			set_client_state(c, CLT_RECV);
> +		}
>  		return STM_BREAK;
>  
>  	default:
> @@ -687,6 +703,9 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  			case CLT_RECV:
>  				polls[i].events = POLLIN;
>  				break;
> +			case CLT_SEND:
> +				polls[i].events = POLLOUT;
> +				break;
>  			default:
>  				/* don't poll for this client */
>  				continue;
> -- 
> 2.33.1

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

Re: [dm-devel] [PATCH v3 32/35] multipathd: uxlsnr: use poll loop for sending, too

[Martin Wilck]

Hi Ben,

On Mon, 2021-11-29 at 14:29 -0600, Benjamin Marzinski wrote:
> On Sat, Nov 27, 2021 at 04:19:26PM +0100, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > send_packet() may busy-loop. By polling for POLLOUT, we can
> > avoid that, even if it's very unlikely in practice.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

Thanks. I think we're good for a submission to Christophe. I'd like
you to review 12/13 and 13/13 of my latest series though, as they fix
actual bugs in the current code. You're of course welcome to review 
more, too ;-), but if these two are good, I believe we should move
forward.

Regards
Martin


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