[PATCH 0/3] libmultipath: use bitwise flags in devmapper API

[PATCH 0/3] libmultipath: use bitwise flags in devmapper API

[Martin Wilck]

This series goes on top of Benjamin Marzinksi's late "multipath: fix hang
in flush_map_nopaths" series. It introduces no functional changes. It
just combines the multiple boolean arguments to _dm_flush_map() and
dm_simplecmd() into a flags variable. This reduces the number of
function arguments, but that's not the main intention. The symbolic
flags improve the readability of the code by making it obvious which
flags are passed to the respective functions in their callers.

Martin Wilck (3):
  libmultipath: use bitwise flags for map flushing API
  libmultipath: use bitwise flags for dm_simplecmd API
  libmultipath: add argument names to some prototypes

 libmultipath/devmapper.c | 94 ++++++++++++++++++----------------------
 libmultipath/devmapper.h | 33 +++++++++-----
 2 files changed, 62 insertions(+), 65 deletions(-)

-- 
2.44.0

[PATCH 1/3] libmultipath: use bitwise flags for map flushing API

[Martin Wilck]

Rather than passing 3 separate bool variables to _dm_flush_map(),
define bitwise flags to modify the function's behavior, and pass
these flags to called functions accordingly.

This improves the readability of the code, function calls are
more expressive.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 2e7b2c6..a6a9c2b 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -53,9 +53,8 @@ static int dm_cancel_remove_partmaps(const char * mapname);
 #define __DR_UNUSED__ __attribute__((unused))
 #endif
 
-static int dm_remove_partmaps (const char * mapname, int need_sync,
-			       int deferred_remove);
-static int do_foreach_partmaps(const char * mapname,
+static int dm_remove_partmaps (const char *mapname, int flags);
+static int do_foreach_partmaps(const char *mapname,
 			       int (*partmap_func)(const char *, void *),
 			       void *data);
 static int _dm_queue_if_no_path(const char *mapname, int enable);
@@ -439,9 +438,9 @@ int dm_simplecmd_noflush (int task, const char *name, uint16_t udev_flags)
 }
 
 static int
-dm_device_remove (const char *name, int needsync, int deferred_remove) {
-	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, needsync, 0,
-			    deferred_remove);
+dm_device_remove (const char *name, int flags) {
+	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, flags & DMFL_NEED_SYNC, 0,
+			    flags & DMFL_DEFERRED);
 }
 
 static int
@@ -1061,8 +1060,7 @@ partmap_in_use(const char *name, void *data)
 	return 0;
 }
 
-int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
-		   int need_suspend, int retries)
+int _dm_flush_map (const char *mapname, int flags, int retries)
 {
 	int r;
 	int queue_if_no_path = 0;
@@ -1080,10 +1078,10 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 
 	/* If you aren't doing a deferred remove, make sure that no
 	 * devices are in use */
-	if (!deferred_remove && partmap_in_use(mapname, NULL))
+	if (!(flags & DMFL_DEFERRED) && partmap_in_use(mapname, NULL))
 			return DM_FLUSH_BUSY;
 
-	if (need_suspend &&
+	if ((flags & DMFL_SUSPEND) &&
 	    dm_get_map(mapname, &mapsize, &params) == DMP_OK &&
 	    strstr(params, "queue_if_no_path")) {
 		if (!_dm_queue_if_no_path(mapname, 0))
@@ -1095,22 +1093,22 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 	free(params);
 	params = NULL;
 
-	if ((r = dm_remove_partmaps(mapname, need_sync, deferred_remove)))
+	if ((r = dm_remove_partmaps(mapname, flags)))
 		return r;
 
-	if (!deferred_remove && dm_get_opencount(mapname)) {
+	if (!(flags & DMFL_DEFERRED) && dm_get_opencount(mapname)) {
 		condlog(2, "%s: map in use", mapname);
 		return DM_FLUSH_BUSY;
 	}
 
 	do {
-		if (need_suspend && queue_if_no_path != -1)
+		if ((flags & DMFL_SUSPEND) && queue_if_no_path != -1)
 			dm_simplecmd_flush(DM_DEVICE_SUSPEND, mapname, 0);
 
-		r = dm_device_remove(mapname, need_sync, deferred_remove);
+		r = dm_device_remove(mapname, flags);
 
 		if (r) {
-			if (deferred_remove && dm_map_present(mapname)) {
+			if ((flags & DMFL_DEFERRED) && dm_map_present(mapname)) {
 				condlog(4, "multipath map %s remove deferred",
 					mapname);
 				return DM_FLUSH_DEFERRED;
@@ -1124,7 +1122,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 		} else {
 			condlog(2, "failed to remove multipath map %s",
 				mapname);
-			if (need_suspend && queue_if_no_path != -1) {
+			if ((flags & DMFL_SUSPEND) && queue_if_no_path != -1) {
 				dm_simplecmd_noflush(DM_DEVICE_RESUME,
 						     mapname, udev_flags);
 			}
@@ -1139,27 +1137,18 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 	return DM_FLUSH_FAIL;
 }
 
+int
+dm_flush_map_nopaths(const char *mapname, int deferred_remove __DR_UNUSED__)
+{
+	int flags = DMFL_NEED_SYNC;
+
 #ifdef LIBDM_API_DEFERRED
-
-int
-dm_flush_map_nopaths(const char * mapname, int deferred_remove)
-{
-	return _dm_flush_map(mapname, 1,
-			     (deferred_remove == DEFERRED_REMOVE_ON ||
-			      deferred_remove == DEFERRED_REMOVE_IN_PROGRESS),
-			     0, 0);
-}
-
-#else
-
-int
-dm_flush_map_nopaths(const char * mapname,
-		     int deferred_remove __attribute__((unused)))
-{
-	return _dm_flush_map(mapname, 1, 0, 0, 0);
-}
-
+	flags |= ((deferred_remove == DEFERRED_REMOVE_ON ||
+		   deferred_remove == DEFERRED_REMOVE_IN_PROGRESS) ?
+		  DMFL_DEFERRED : 0);
 #endif
+	return _dm_flush_map(mapname, flags, 0);
+}
 
 int dm_flush_maps (int retries)
 {
@@ -1528,8 +1517,7 @@ out:
 }
 
 struct remove_data {
-	int need_sync;
-	int deferred_remove;
+	int flags;
 };
 
 static int
@@ -1538,21 +1526,21 @@ remove_partmap(const char *name, void *data)
 	struct remove_data *rd = (struct remove_data *)data;
 
 	if (dm_get_opencount(name)) {
-		dm_remove_partmaps(name, rd->need_sync, rd->deferred_remove);
-		if (rd->deferred_remove && dm_get_opencount(name)) {
+		dm_remove_partmaps(name, rd->flags);
+		if ((rd->flags & DMFL_DEFERRED) && dm_get_opencount(name)) {
 			condlog(2, "%s: map in use", name);
 			return DM_FLUSH_BUSY;
 		}
 	}
 	condlog(4, "partition map %s removed", name);
-	dm_device_remove(name, rd->need_sync, rd->deferred_remove);
+	dm_device_remove(name, rd->flags);
 	return DM_FLUSH_OK;
 }
 
 static int
-dm_remove_partmaps (const char * mapname, int need_sync, int deferred_remove)
+dm_remove_partmaps (const char * mapname, int flags)
 {
-	struct remove_data rd = { need_sync, deferred_remove };
+	struct remove_data rd = { flags };
 	return do_foreach_partmaps(mapname, remove_partmap, &rd);
 }
 
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 93caa2a..bb4a55a 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -58,12 +58,20 @@ enum {
 };
 
 int partmap_in_use(const char *name, void *data);
-int _dm_flush_map (const char *, int, int, int, int);
-int dm_flush_map_nopaths(const char * mapname, int deferred_remove);
-#define dm_flush_map(mapname) _dm_flush_map(mapname, 1, 0, 0, 0)
-#define dm_flush_map_nosync(mapname) _dm_flush_map(mapname, 0, 0, 0, 0)
+
+enum {
+	DMFL_NONE      = 0,
+	DMFL_NEED_SYNC = 1 << 0,
+	DMFL_DEFERRED  = 1 << 1,
+	DMFL_SUSPEND   = 1 << 2,
+};
+
+int _dm_flush_map (const char *mapname, int flags, int retries);
+#define dm_flush_map(mapname) _dm_flush_map(mapname, DMFL_NEED_SYNC, 0)
+#define dm_flush_map_nosync(mapname) _dm_flush_map(mapname, DMFL_NONE, 0)
 #define dm_suspend_and_flush_map(mapname, retries) \
-	_dm_flush_map(mapname, 1, 0, 1, retries)
+	_dm_flush_map(mapname, DMFL_NEED_SYNC|DMFL_SUSPEND, retries)
+int dm_flush_map_nopaths(const char * mapname, int deferred_remove);
 int dm_cancel_deferred_remove(struct multipath *mpp);
 int dm_flush_maps (int retries);
 int dm_fail_path(const char * mapname, char * path);
-- 
2.44.0

[PATCH 2/3] libmultipath: use bitwise flags for dm_simplecmd API

[Martin Wilck]

Also use bitwise flags for dm_simplecmd() and its relatives. Again,
this makes the code more expressive and more readable, while
simplifying the function calls.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index a6a9c2b..08bb3c5 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -386,10 +386,9 @@ libmp_dm_task_create(int task)
 }
 
 static int
-dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
-	      uint16_t udev_flags, int deferred_remove __DR_UNUSED__) {
+dm_simplecmd (int task, const char *name, int flags, uint16_t udev_flags) {
 	int r = 0;
-	int udev_wait_flag = ((need_sync || udev_flags) &&
+	int udev_wait_flag = (((flags & DMFL_NEED_SYNC) || udev_flags) &&
 			      (task == DM_DEVICE_RESUME ||
 			       task == DM_DEVICE_REMOVE));
 	uint32_t cookie = 0;
@@ -404,11 +403,11 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync,
 	dm_task_no_open_count(dmt);
 	dm_task_skip_lockfs(dmt);	/* for DM_DEVICE_RESUME */
 #ifdef LIBDM_API_FLUSH
-	if (no_flush)
+	if (flags & DMFL_NO_FLUSH)
 		dm_task_no_flush(dmt);		/* for DM_DEVICE_SUSPEND/RESUME */
 #endif
 #ifdef LIBDM_API_DEFERRED
-	if (deferred_remove)
+	if (flags & DMFL_DEFERRED)
 		dm_task_deferred_remove(dmt);
 #endif
 	if (udev_wait_flag &&
@@ -429,18 +428,17 @@ out:
 
 int dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags)
 {
-	return dm_simplecmd(task, name, 0, 1, udev_flags, 0);
+	return dm_simplecmd(task, name, DMFL_NEED_SYNC, udev_flags);
 }
 
 int dm_simplecmd_noflush (int task, const char *name, uint16_t udev_flags)
 {
-	return dm_simplecmd(task, name, 1, 1, udev_flags, 0);
+	return dm_simplecmd(task, name, DMFL_NO_FLUSH|DMFL_NEED_SYNC, udev_flags);
 }
 
 static int
 dm_device_remove (const char *name, int flags) {
-	return dm_simplecmd(DM_DEVICE_REMOVE, name, 0, flags & DMFL_NEED_SYNC, 0,
-			    flags & DMFL_DEFERRED);
+	return dm_simplecmd(DM_DEVICE_REMOVE, name, flags, 0);
 }
 
 static int
@@ -594,8 +592,9 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 			      params, ADDMAP_RO, 0);
 	}
 	if (r)
-		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, !flush,
-				 1, udev_flags, 0);
+		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
+				 DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
+				 udev_flags);
 	if (r)
 		return r;
 
@@ -603,8 +602,9 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 	 * drop the new table, so doing a second resume will try using
 	 * the original table */
 	if (dm_is_suspended(mpp->alias))
-		dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, !flush, 1,
-			     udev_flags, 0);
+		dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
+			     DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
+			     udev_flags);
 	return 0;
 }
 
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index bb4a55a..a242381 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -38,8 +38,8 @@ void skip_libmp_dm_init(void);
 void libmp_dm_exit(void);
 void libmp_udev_set_sync_support(int on);
 struct dm_task *libmp_dm_task_create(int task);
-int dm_simplecmd_flush (int, const char *, uint16_t);
-int dm_simplecmd_noflush (int, const char *, uint16_t);
+int dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags);
+int dm_simplecmd_noflush (int task, const char *name, uint16_t udev_flags);
 int dm_addmap_create (struct multipath *mpp, char *params);
 int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
 int dm_map_present (const char *);
@@ -64,6 +64,7 @@ enum {
 	DMFL_NEED_SYNC = 1 << 0,
 	DMFL_DEFERRED  = 1 << 1,
 	DMFL_SUSPEND   = 1 << 2,
+	DMFL_NO_FLUSH  = 1 << 3,
 };
 
 int _dm_flush_map (const char *mapname, int flags, int retries);
-- 
2.44.0

[PATCH 3/3] libmultipath: add argument names to some prototypes

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index a242381..19b79c5 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -42,12 +42,12 @@ int dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags);
 int dm_simplecmd_noflush (int task, const char *name, uint16_t udev_flags);
 int dm_addmap_create (struct multipath *mpp, char *params);
 int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
-int dm_map_present (const char *);
+int dm_map_present (const char *name);
 int dm_map_present_by_uuid(const char *uuid);
-int dm_get_map(const char *, unsigned long long *, char **);
-int dm_get_status(const char *, char **);
-int dm_type(const char *, char *);
-int dm_is_mpath(const char *);
+int dm_get_map(const char *name, unsigned long long *size, char **outparams);
+int dm_get_status(const char *name, char **outstatus);
+int dm_type(const char *name, char *type);
+int dm_is_mpath(const char *name);
 
 enum {
 	DM_FLUSH_OK = 0,
-- 
2.44.0

Re: [PATCH 0/3] libmultipath: use bitwise flags in devmapper API

[Benjamin Marzinski]

On Thu, May 02, 2024 at 08:59:43PM +0200, Martin Wilck wrote:
> This series goes on top of Benjamin Marzinksi's late "multipath: fix hang
> in flush_map_nopaths" series. It introduces no functional changes. It
> just combines the multiple boolean arguments to _dm_flush_map() and
> dm_simplecmd() into a flags variable. This reduces the number of
> function arguments, but that's not the main intention. The symbolic
> flags improve the readability of the code by making it obvious which
> flags are passed to the respective functions in their callers.

Nice cleanup. For the set:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Martin Wilck (3):
>   libmultipath: use bitwise flags for map flushing API
>   libmultipath: use bitwise flags for dm_simplecmd API
>   libmultipath: add argument names to some prototypes
> 
>  libmultipath/devmapper.c | 94 ++++++++++++++++++----------------------
>  libmultipath/devmapper.h | 33 +++++++++-----
>  2 files changed, 62 insertions(+), 65 deletions(-)
> 
> -- 
> 2.44.0