* [PATCH v3 16/49] libmultipath: add cleanup_dm_task(), and use it in devmapper.c
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
@ 2024-07-16 20:53 ` Martin Wilck
2024-07-16 20:53 ` [PATCH v3 19/49] libmultipath: add libmp_mapinfo() Martin Wilck
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Martin Wilck @ 2024-07-16 20:53 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
This allows us to get rid of a lot of goto statements, and generally
obtain cleaner code.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/devmapper.c | 261 ++++++++++++++++-----------------------
1 file changed, 108 insertions(+), 153 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 8996c1d..7dac9fa 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -91,6 +91,12 @@ int libmp_dm_task_run(struct dm_task *dmt)
return r;
}
+static void cleanup_dm_task(struct dm_task **pdmt)
+{
+ if (*pdmt)
+ dm_task_destroy(*pdmt);
+}
+
__attribute__((format(printf, 4, 5))) static void
dm_write_log (int level, const char *file, int line, const char *f, ...)
{
@@ -203,8 +209,8 @@ static void init_dm_drv_version(void)
static int dm_tgt_version (unsigned int *version, char *str)
{
- int r = 2;
- struct dm_task *dmt;
+ bool found = false;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
struct dm_versions *target;
struct dm_versions *last_target;
unsigned int *v;
@@ -220,31 +226,28 @@ static int dm_tgt_version (unsigned int *version, char *str)
if (!libmp_dm_task_run(dmt)) {
dm_log_error(2, DM_DEVICE_LIST_VERSIONS, dmt);
condlog(0, "Cannot communicate with kernel DM");
- goto out;
+ return 1;
}
target = dm_task_get_versions(dmt);
do {
last_target = target;
if (!strncmp(str, target->name, strlen(str))) {
- r = 1;
+ found = true;
break;
}
target = (void *) target + target->next;
} while (last_target != target);
- if (r == 2) {
+ if (!found) {
condlog(0, "DM %s kernel driver not loaded", str);
- goto out;
+ return 1;
}
v = target->version;
version[0] = v[0];
version[1] = v[1];
version[2] = v[2];
- r = 0;
-out:
- dm_task_destroy(dmt);
- return r;
+ return 0;
}
static void init_dm_mpath_version(void)
@@ -383,18 +386,18 @@ libmp_dm_task_create(int task)
static int
dm_simplecmd (int task, const char *name, int flags, uint16_t udev_flags) {
- int r = 0;
+ int r;
int udev_wait_flag = (((flags & DMFL_NEED_SYNC) || udev_flags) &&
(task == DM_DEVICE_RESUME ||
task == DM_DEVICE_REMOVE));
uint32_t cookie = 0;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
if (!(dmt = libmp_dm_task_create (task)))
return 0;
if (!dm_task_set_name (dmt, name))
- goto out;
+ return 0;
dm_task_skip_lockfs(dmt); /* for DM_DEVICE_RESUME */
#ifdef LIBDM_API_FLUSH
@@ -408,7 +411,7 @@ dm_simplecmd (int task, const char *name, int flags, uint16_t udev_flags) {
if (udev_wait_flag &&
!dm_task_set_cookie(dmt, &cookie,
DM_UDEV_DISABLE_LIBRARY_FALLBACK | udev_flags))
- goto out;
+ return 0;
r = libmp_dm_task_run (dmt);
if (!r)
@@ -416,8 +419,6 @@ dm_simplecmd (int task, const char *name, int flags, uint16_t udev_flags) {
if (udev_wait_flag)
libmp_udev_wait(cookie);
-out:
- dm_task_destroy (dmt);
return r;
}
@@ -440,8 +441,9 @@ static int
dm_addmap (int task, const char *target, struct multipath *mpp,
char * params, int ro, uint16_t udev_flags) {
int r = 0;
- struct dm_task *dmt;
- char *prefixed_uuid = NULL;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
+ char __attribute__((cleanup(cleanup_charp))) *prefixed_uuid = NULL;
+
uint32_t cookie = 0;
if (task == DM_DEVICE_CREATE && strlen(mpp->wwid) == 0) {
@@ -457,10 +459,10 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
return 0;
if (!dm_task_set_name (dmt, mpp->alias))
- goto addout;
+ return 0;
if (!dm_task_add_target (dmt, 0, mpp->size, target, params))
- goto addout;
+ return 0;
if (ro)
dm_task_set_ro(dmt);
@@ -469,10 +471,10 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
if (asprintf(&prefixed_uuid, UUID_PREFIX "%s", mpp->wwid) < 0) {
condlog(0, "cannot create prefixed uuid : %s",
strerror(errno));
- goto addout;
+ return 0;
}
if (!dm_task_set_uuid(dmt, prefixed_uuid))
- goto freeout;
+ return 0;
dm_task_skip_lockfs(dmt);
#ifdef LIBDM_API_FLUSH
dm_task_no_flush(dmt);
@@ -481,33 +483,28 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
if (mpp->attribute_flags & (1 << ATTR_MODE) &&
!dm_task_set_mode(dmt, mpp->mode))
- goto freeout;
+ return 0;
if (mpp->attribute_flags & (1 << ATTR_UID) &&
!dm_task_set_uid(dmt, mpp->uid))
- goto freeout;
+ return 0;
if (mpp->attribute_flags & (1 << ATTR_GID) &&
!dm_task_set_gid(dmt, mpp->gid))
- goto freeout;
+ return 0;
+
condlog(2, "%s: %s [0 %llu %s %s]", mpp->alias,
task == DM_DEVICE_RELOAD ? "reload" : "addmap", mpp->size,
target, params);
if (task == DM_DEVICE_CREATE &&
!dm_task_set_cookie(dmt, &cookie, udev_flags))
- goto freeout;
+ return 0;
r = libmp_dm_task_run (dmt);
if (!r)
dm_log_error(2, task, dmt);
if (task == DM_DEVICE_CREATE)
- libmp_udev_wait(cookie);
-freeout:
- if (prefixed_uuid)
- free(prefixed_uuid);
-
-addout:
- dm_task_destroy (dmt);
+ libmp_udev_wait(cookie);
if (r)
mpp->need_reload = false;
@@ -648,46 +645,41 @@ int dm_map_present(const char * str)
int dm_get_map(const char *name, unsigned long long *size, char **outparams)
{
- int r = DMP_ERR;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
uint64_t start, length;
char *target_type = NULL;
char *params = NULL;
if (!(dmt = libmp_dm_task_create(DM_DEVICE_TABLE)))
- return r;
+ return DMP_ERR;
if (!dm_task_set_name(dmt, name))
- goto out;
+ return DMP_ERR;
errno = 0;
if (!libmp_dm_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
if (dm_task_get_errno(dmt) == ENXIO)
- r = DMP_NOT_FOUND;
- goto out;
+ return DMP_NOT_FOUND;
+ else
+ return DMP_ERR;
}
- r = DMP_NOT_FOUND;
/* Fetch 1st target */
if (dm_get_next_target(dmt, NULL, &start, &length,
&target_type, ¶ms) != NULL || !params)
/* more than one target or not found target */
- goto out;
+ return DMP_NOT_FOUND;
if (size)
*size = length;
if (!outparams)
- r = DMP_OK;
+ return DMP_OK;
else {
*outparams = strdup(params);
- r = *outparams ? DMP_OK : DMP_ERR;
+ return *outparams ? DMP_OK : DMP_ERR;
}
-
-out:
- dm_task_destroy(dmt);
- return r;
}
static int
@@ -767,7 +759,7 @@ is_mpath_part(const char *part_name, const char *map_name)
int dm_get_status(const char *name, char **outstatus)
{
int r = DMP_ERR;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
uint64_t start, length;
char *target_type = NULL;
char *status = NULL;
@@ -796,7 +788,7 @@ int dm_get_status(const char *name, char **outstatus)
goto out;
if (!status) {
- condlog(2, "get null status.");
+ condlog(2, "got null status.");
goto out;
}
@@ -808,9 +800,9 @@ int dm_get_status(const char *name, char **outstatus)
}
out:
if (r != DMP_OK)
- condlog(0, "%s: error getting map status string", name);
+ condlog(0, "%s: %s: error getting map status string: %d",
+ __func__, name, r);
- dm_task_destroy(dmt);
return r;
}
@@ -823,7 +815,7 @@ out:
int dm_type(const char *name, char *type)
{
int r = 0;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
uint64_t start, length;
char *target_type = NULL;
char *params;
@@ -850,7 +842,6 @@ int dm_type(const char *name, char *type)
r = 1;
out:
- dm_task_destroy(dmt);
return r;
}
@@ -863,7 +854,7 @@ out:
int dm_is_mpath(const char *name)
{
int r = -1;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
struct dm_info info;
uint64_t start, length;
char *target_type = NULL;
@@ -874,38 +865,36 @@ int dm_is_mpath(const char *name)
goto out;
if (!dm_task_set_name(dmt, name))
- goto out_task;
+ goto out;
if (!libmp_dm_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
- goto out_task;
+ goto out;
}
if (!dm_task_get_info(dmt, &info))
- goto out_task;
+ goto out;
r = 0;
if (!info.exists)
- goto out_task;
+ goto out;
uuid = dm_task_get_uuid(dmt);
if (!uuid || strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN) != 0)
- goto out_task;
+ goto out;
/* Fetch 1st target */
if (dm_get_next_target(dmt, NULL, &start, &length, &target_type,
¶ms) != NULL)
/* multiple targets */
- goto out_task;
+ goto out;
if (!target_type || strcmp(target_type, TGT_MPATH) != 0)
- goto out_task;
+ goto out;
r = 1;
-out_task:
- dm_task_destroy(dmt);
out:
if (r < 0)
condlog(3, "%s: dm command failed in %s: %s", name, __FUNCTION__, strerror(errno));
@@ -1131,10 +1120,10 @@ dm_flush_map_nopaths(const char *mapname, int deferred_remove __DR_UNUSED__)
return _dm_flush_map(mapname, flags, 0);
}
-int dm_flush_maps (int retries)
+int dm_flush_maps(int retries)
{
int r = DM_FLUSH_FAIL;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
struct dm_names *names;
unsigned next = 0;
@@ -1143,15 +1132,15 @@ int dm_flush_maps (int retries)
if (!libmp_dm_task_run (dmt)) {
dm_log_error(3, DM_DEVICE_LIST, dmt);
- goto out;
+ return r;
}
if (!(names = dm_task_get_names (dmt)))
- goto out;
+ return r;
r = DM_FLUSH_OK;
if (!names->dev)
- goto out;
+ return r;
do {
int ret;
@@ -1163,16 +1152,13 @@ int dm_flush_maps (int retries)
names = (void *) names + next;
} while (next);
-out:
- dm_task_destroy (dmt);
return r;
}
int
dm_message(const char * mapname, char * message)
{
- int r = 1;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
if (!(dmt = libmp_dm_task_create(DM_DEVICE_TARGET_MSG)))
return 1;
@@ -1191,13 +1177,10 @@ dm_message(const char * mapname, char * message)
goto out;
}
- r = 0;
+ return 0;
out:
- if (r)
- condlog(0, "DM message failed [%s]", message);
-
- dm_task_destroy(dmt);
- return r;
+ condlog(0, "DM message failed [%s]", message);
+ return 1;
}
int
@@ -1305,12 +1288,10 @@ out:
return NULL;
}
-int
-dm_get_maps (vector mp)
+int dm_get_maps(vector mp)
{
struct multipath * mpp;
- int r = 1;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
struct dm_names *names;
unsigned next = 0;
@@ -1322,15 +1303,15 @@ dm_get_maps (vector mp)
if (!libmp_dm_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_LIST, dmt);
- goto out;
+ return 1;
}
if (!(names = dm_task_get_names(dmt)))
- goto out;
+ return 1;
if (!names->dev) {
- r = 0; /* this is perfectly valid */
- goto out;
+ /* this is perfectly valid */
+ return 0;
}
do {
@@ -1339,11 +1320,11 @@ dm_get_maps (vector mp)
mpp = dm_get_multipath(names->name);
if (!mpp)
- goto out;
+ return 1;
if (!vector_alloc_slot(mp)) {
free_multipath(mpp, KEEP_PATHS);
- goto out;
+ return 1;
}
vector_set_slot(mp, mpp);
@@ -1352,11 +1333,7 @@ next:
names = (void *) names + next;
} while (next);
- r = 0;
- goto out;
-out:
- dm_task_destroy (dmt);
- return r;
+ return 0;
}
int
@@ -1419,33 +1396,29 @@ do_foreach_partmaps (const char * mapname,
int (*partmap_func)(const char *, void *),
void *data)
{
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
+ char __attribute__((cleanup(cleanup_charp))) *params = NULL;
struct dm_names *names;
unsigned next = 0;
- char *params = NULL;
unsigned long long size;
char dev_t[32];
- int r = 1;
char *p;
if (!(dmt = libmp_dm_task_create(DM_DEVICE_LIST)))
return 1;
- if (!libmp_dm_task_run(dmt)) {
- dm_log_error(3, DM_DEVICE_LIST, dmt);
- goto out;
- }
+ if (!libmp_dm_task_run(dmt))
+ return 1;
if (!(names = dm_task_get_names(dmt)))
- goto out;
+ return 1;
- if (!names->dev) {
- r = 0; /* this is perfectly valid */
- goto out;
- }
+ if (!names->dev)
+ /* this is perfectly valid */
+ return 0;
if (dm_dev_t(mapname, &dev_t[0], 32))
- goto out;
+ return 1;
do {
if (
@@ -1471,8 +1444,8 @@ do_foreach_partmaps (const char * mapname,
(p = strstr(params, dev_t)) &&
!isdigit(*(p + strlen(dev_t)))
) {
- if ((r = partmap_func(names->name, data)) != 0)
- goto out;
+ if (partmap_func(names->name, data) != 0)
+ return 1;
}
free(params);
@@ -1481,11 +1454,7 @@ do_foreach_partmaps (const char * mapname,
names = (void *) names + next;
} while (next);
- r = 0;
-out:
- free(params);
- dm_task_destroy (dmt);
- return r;
+ return 0;
}
struct remove_data {
@@ -1623,7 +1592,7 @@ int
dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
{
int r = 0;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
uint32_t cookie = 0;
uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
@@ -1634,22 +1603,19 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
return r;
if (!dm_task_set_name(dmt, old))
- goto out;
+ return r;
if (!dm_task_set_newname(dmt, new))
- goto out;
+ return r;
if (!dm_task_set_cookie(dmt, &cookie, udev_flags))
- goto out;
+ return r;
+
r = libmp_dm_task_run(dmt);
if (!r)
dm_log_error(2, DM_DEVICE_RENAME, dmt);
libmp_udev_wait(cookie);
-
-out:
- dm_task_destroy(dmt);
-
return r;
}
@@ -1672,9 +1638,10 @@ void dm_reassign_deps(char *table, const char *dep, const char *newdep)
int dm_reassign_table(const char *name, char *old, char *new)
{
- int r = 0, modified = 0;
+ int modified = 0;
uint64_t start, length;
- struct dm_task *dmt, *reload_dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *reload_dmt = NULL;
char *target, *params = NULL;
char *buff;
void *next = NULL;
@@ -1683,16 +1650,16 @@ int dm_reassign_table(const char *name, char *old, char *new)
return 0;
if (!dm_task_set_name(dmt, name))
- goto out;
+ return 0;
if (!libmp_dm_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_TABLE, dmt);
- goto out;
+ return 0;
}
if (!(reload_dmt = libmp_dm_task_create(DM_DEVICE_RELOAD)))
- goto out;
+ return 0;
if (!dm_task_set_name(reload_dmt, name))
- goto out_reload;
+ return 0;
do {
next = dm_get_next_target(dmt, next, &start, &length,
@@ -1705,13 +1672,13 @@ int dm_reassign_table(const char *name, char *old, char *new)
*/
condlog(1, "%s: invalid target found in map %s",
__func__, name);
- goto out_reload;
+ return 0;
}
buff = strdup(params);
if (!buff) {
condlog(3, "%s: failed to replace target %s, "
"out of memory", name, target);
- goto out_reload;
+ return 0;
}
if (strcmp(target, TGT_MPATH) && strstr(params, old)) {
condlog(3, "%s: replace target %s %s",
@@ -1729,18 +1696,12 @@ int dm_reassign_table(const char *name, char *old, char *new)
if (!libmp_dm_task_run(reload_dmt)) {
dm_log_error(3, DM_DEVICE_RELOAD, reload_dmt);
condlog(3, "%s: failed to reassign targets", name);
- goto out_reload;
+ return 0;
}
dm_simplecmd_noflush(DM_DEVICE_RESUME, name,
MPATH_UDEV_RELOAD_FLAG);
}
- r = 1;
-
-out_reload:
- dm_task_destroy(reload_dmt);
-out:
- dm_task_destroy(dmt);
- return r;
+ return 1;
}
@@ -1752,10 +1713,9 @@ out:
int dm_reassign(const char *mapname)
{
struct dm_deps *deps;
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
struct dm_info info;
char dev_t[32], dm_dep[32];
- int r = 0;
unsigned int i;
if (dm_dev_t(mapname, &dev_t[0], 32)) {
@@ -1769,21 +1729,21 @@ int dm_reassign(const char *mapname)
}
if (!dm_task_set_name(dmt, mapname))
- goto out;
+ return 0;
if (!libmp_dm_task_run(dmt)) {
dm_log_error(3, DM_DEVICE_DEPS, dmt);
- goto out;
+ return 0;
}
if (!dm_task_get_info(dmt, &info))
- goto out;
+ return 0;
if (!(deps = dm_task_get_deps(dmt)))
- goto out;
+ return 0;
if (!info.exists)
- goto out;
+ return 0;
for (i = 0; i < deps->count; i++) {
sprintf(dm_dep, "%d:%d",
@@ -1792,15 +1752,12 @@ int dm_reassign(const char *mapname)
sysfs_check_holders(dm_dep, dev_t);
}
- r = 1;
-out:
- dm_task_destroy (dmt);
- return r;
+ return 1;
}
int dm_setgeometry(struct multipath *mpp)
{
- struct dm_task *dmt;
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
struct path *pp;
char heads[4], sectors[4];
char cylinders[10], start[32];
@@ -1825,7 +1782,7 @@ int dm_setgeometry(struct multipath *mpp)
return 0;
if (!dm_task_set_name(dmt, mpp->alias))
- goto out;
+ return 0;
/* What a sick interface ... */
snprintf(heads, 4, "%u", pp->geom.heads);
@@ -1834,14 +1791,12 @@ int dm_setgeometry(struct multipath *mpp)
snprintf(start, 32, "%lu", pp->geom.start);
if (!dm_task_set_geometry(dmt, cylinders, heads, sectors, start)) {
condlog(3, "%s: Failed to set geometry", mpp->alias);
- goto out;
+ return 0;
}
r = libmp_dm_task_run(dmt);
if (!r)
dm_log_error(3, DM_DEVICE_SET_GEOMETRY, dmt);
-out:
- dm_task_destroy(dmt);
return r;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 19/49] libmultipath: add libmp_mapinfo()
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
2024-07-16 20:53 ` [PATCH v3 16/49] libmultipath: add cleanup_dm_task(), and use it in devmapper.c Martin Wilck
@ 2024-07-16 20:53 ` Martin Wilck
2024-07-16 20:53 ` [PATCH v3 36/49] libmultipath: use libmp_mapinfo() in do_foreach_partmaps() Martin Wilck
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Martin Wilck @ 2024-07-16 20:53 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
libmp_mapinfo() is intended as a generic abstraction for retrieving information from
the kernel device-mapper driver. It retrieves the information that the caller
needs, with a minimal set of DM ioctls, and never more then 2 ioctl calls.
libdm's DM_DEVICE_TABLE and DM_DEVICE_STATUS calls map to the kernel's
DM_TABLE_STATUS ioctl, with or without the DM_STATUS_TABLE_FLAG set,
respectively. DM_TABLE_STATUS always retrieves the basic map status (struct
dm_info) and the map UUID and name, too.
Note: I'd prefer to use an unnamed struct instead of _u in
union libmp_map_identifer. But doing using an unnamed struct and and
initializing the union like this in a function argument:
func((mapid_t) { .major = major, .minor = minor })
is not part of C99, and not supported in gcc 4.8, which we still support.
Likewise, the following syntax for initializing an empty struct:
(mapinfo_t) { 0 }
is not supported on all architectures we support (notably clang 3.5 under
Debian Jessie).
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/devmapper.c | 194 +++++++++++++++++++++++++++++-
libmultipath/devmapper.h | 70 +++++++++++
libmultipath/libmultipath.version | 3 +-
3 files changed, 265 insertions(+), 2 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 5492a86..f90a8df 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -14,7 +14,6 @@
#include <errno.h>
#include <syslog.h>
#include <sys/sysmacros.h>
-#include <linux/dm-ioctl.h>
#include "util.h"
#include "vector.h"
@@ -604,6 +603,199 @@ has_dm_info(const struct multipath *mpp)
return (mpp && mpp->dmi.exists != 0);
}
+static int libmp_set_map_identifier(int flags, mapid_t id, struct dm_task *dmt)
+{
+ switch (flags & __DM_MAP_BY_MASK) {
+ case DM_MAP_BY_UUID:
+ if (!id.str || !(*id.str))
+ return 0;
+ return dm_task_set_uuid(dmt, id.str);
+ case DM_MAP_BY_NAME:
+ if (!id.str || !(*id.str))
+ return 0;
+ return dm_task_set_name(dmt, id.str);
+ case DM_MAP_BY_DEV:
+ if (!dm_task_set_major(dmt, id._u.major))
+ return 0;
+ return dm_task_set_minor(dmt, id._u.minor);
+ case DM_MAP_BY_DEVT:
+ if (!dm_task_set_major(dmt, major(id.devt)))
+ return 0;
+ return dm_task_set_minor(dmt, minor(id.devt));
+ default:
+ condlog(0, "%s: invalid by_id", __func__);
+ return 0;
+ }
+}
+
+static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *map_id)
+{
+ /* avoid libmp_mapinfo__ in log messages */
+ static const char fname__[] = "libmp_mapinfo";
+ struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
+ struct dm_info dmi;
+ int rc, ioctl_nr;
+ uint64_t start, length = 0;
+ char *target_type = NULL, *params = NULL;
+ const char *name = NULL, *uuid = NULL;
+ char __attribute__((cleanup(cleanup_charp))) *tmp_target = NULL;
+ char __attribute__((cleanup(cleanup_charp))) *tmp_status = NULL;
+ bool tgt_set = false;
+
+ /*
+ * If both info.target and info.status are set, we need two
+ * ioctls. Call this function recursively.
+ * If successful, tmp_target will be non-NULL.
+ */
+ if (info.target && info.status) {
+ rc = libmp_mapinfo__(flags, id,
+ (mapinfo_t) { .target = &tmp_target },
+ map_id);
+ if (rc != DMP_OK)
+ return rc;
+ tgt_set = true;
+ }
+
+ /*
+ * The DM_DEVICE_TABLE and DM_DEVICE_STATUS ioctls both fetch the basic
+ * information from DM_DEVICE_INFO, too.
+ * Choose the most lightweight ioctl to fetch all requested info.
+ */
+ if (info.target && !info.status)
+ ioctl_nr = DM_DEVICE_TABLE;
+ else if (info.status || info.size || flags & __MAPINFO_TGT_TYPE)
+ ioctl_nr = DM_DEVICE_STATUS;
+ else
+ ioctl_nr = DM_DEVICE_INFO;
+
+ if (!(dmt = libmp_dm_task_create(ioctl_nr)))
+ return DMP_ERR;
+
+ if (!libmp_set_map_identifier(flags, id, dmt)) {
+ condlog(2, "%s: failed to set map identifier to %s", fname__, map_id);
+ return DMP_ERR;
+ }
+
+ if (!libmp_dm_task_run(dmt)) {
+ dm_log_error(3, ioctl_nr, dmt);
+ if (dm_task_get_errno(dmt) == ENXIO) {
+ condlog(2, "%s: map %s not found", fname__, map_id);
+ return DMP_NOT_FOUND;
+ } else
+ return DMP_ERR;
+ }
+
+ condlog(4, "%s: DM ioctl %d succeeded for %s",
+ fname__, ioctl_nr, map_id);
+
+ if (!dm_task_get_info(dmt, &dmi)) {
+ condlog(2, "%s: dm_task_get_info() failed for %s ", fname__, map_id);
+ return DMP_ERR;
+ } else if(!dmi.exists) {
+ condlog(2, "%s: map %s doesn't exist", fname__, map_id);
+ return DMP_NOT_FOUND;
+ }
+
+ if (info.target || info.status || info.size || flags & __MAPINFO_TGT_TYPE) {
+ if (dm_get_next_target(dmt, NULL, &start, &length,
+ &target_type, ¶ms) != NULL) {
+ condlog(2, "%s: map %s has multiple targets", fname__, map_id);
+ return DMP_NOT_FOUND;
+ }
+ if (!params) {
+ condlog(2, "%s: map %s has no targets", fname__, map_id);
+ return DMP_NOT_FOUND;
+ }
+ if (flags & __MAPINFO_TGT_TYPE) {
+ const char *tgt_type = flags & MAPINFO_MPATH_ONLY ? TGT_MPATH : TGT_PART;
+
+ if (strcmp(target_type, tgt_type)) {
+ condlog(3, "%s: target type mismatch: \"%s\" != \"%s\"",
+ fname__, tgt_type, target_type);
+ return DMP_NO_MATCH;
+ }
+ }
+ }
+
+ /*
+ * Check possible error conditions.
+ * If error is returned, don't touch any output parameters.
+ */
+ if ((info.name && !(name = dm_task_get_name(dmt)))
+ || (info.uuid && !(uuid = dm_task_get_uuid(dmt)))
+ || (info.status && !(tmp_status = strdup(params)))
+ || (info.target && !tmp_target && !(tmp_target = strdup(params))))
+ return DMP_ERR;
+
+ if (info.name) {
+ strlcpy(info.name, name, WWID_SIZE);
+ condlog(4, "%s: %s: name: \"%s\"", fname__, map_id, info.name);
+ }
+ if (info.uuid) {
+ strlcpy(info.uuid, uuid, DM_UUID_LEN);
+ condlog(4, "%s: %s: uuid: \"%s\"", fname__, map_id, info.uuid);
+ }
+
+ if (info.size) {
+ *info.size = length;
+ condlog(4, "%s: %s: size: %lld", fname__, map_id, *info.size);
+ }
+
+ if (info.dmi) {
+ memcpy(info.dmi, &dmi, sizeof(*info.dmi));
+ condlog(4, "%s: %s %d:%d, %d targets, %s table, %s, %s, %d opened, %u events",
+ fname__, map_id,
+ info.dmi->major, info.dmi->minor,
+ info.dmi->target_count,
+ info.dmi->live_table ? "live" :
+ info.dmi->inactive_table ? "inactive" : "no",
+ info.dmi->suspended ? "suspended" : "active",
+ info.dmi->read_only ? "ro" : "rw",
+ info.dmi->open_count,
+ info.dmi->event_nr);
+ }
+
+ if (info.target) {
+ *info.target = steal_ptr(tmp_target);
+ if (!tgt_set)
+ condlog(4, "%s: %s: target: \"%s\"", fname__, map_id, *info.target);
+ }
+
+ if (info.status) {
+ *info.status = steal_ptr(tmp_status);
+ condlog(4, "%s: %s: status: \"%s\"", fname__, map_id, *info.status);
+ }
+
+ return DMP_OK;
+}
+
+/* Helper: format a string describing the map for log messages */
+static const char* libmp_map_identifier(int flags, mapid_t id, char buf[BLK_DEV_SIZE])
+{
+ switch (flags & __DM_MAP_BY_MASK) {
+ case DM_MAP_BY_NAME:
+ case DM_MAP_BY_UUID:
+ return id.str;
+ case DM_MAP_BY_DEV:
+ safe_snprintf(buf, BLK_DEV_SIZE, "%d:%d", id._u.major, id._u.minor);
+ return buf;
+ case DM_MAP_BY_DEVT:
+ safe_snprintf(buf, BLK_DEV_SIZE, "%d:%d", major(id.devt), minor(id.devt));
+ return buf;
+ default:
+ safe_snprintf(buf, BLK_DEV_SIZE, "*invalid*");
+ return buf;
+ }
+}
+
+int libmp_mapinfo(int flags, mapid_t id, mapinfo_t info)
+{
+ char idbuf[BLK_DEV_SIZE];
+
+ return libmp_mapinfo__(flags, id, info,
+ libmp_map_identifier(flags, id, idbuf));
+}
+
int
dm_get_info(const char *name, struct dm_info *info)
{
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 9438c2d..4ccaaa7 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -1,5 +1,6 @@
#ifndef _DEVMAPPER_H
#define _DEVMAPPER_H
+#include <linux/dm-ioctl.h>
#include "autoconfig.h"
#include "structs.h"
@@ -31,8 +32,77 @@ enum {
DMP_ERR,
DMP_OK,
DMP_NOT_FOUND,
+ DMP_NO_MATCH,
};
+/**
+ * enum mapinfo_flags: input flags for libmp_mapinfo()
+ */
+enum __mapinfo_flags {
+ /** DM_MAP_BY_NAME: identify map by device-mapper name from @name */
+ DM_MAP_BY_NAME = 0,
+ /** DM_MAP_BY_UUID: identify map by device-mapper UUID from @uuid */
+ DM_MAP_BY_UUID,
+ /** DM_MAP_BY_DEV: identify map by major/minor number from @dmi */
+ DM_MAP_BY_DEV,
+ /** DM_MAP_BY_DEVT: identify map by a dev_t */
+ DM_MAP_BY_DEVT,
+ __DM_MAP_BY_MASK = (1 << 8) - 1,
+ /* Fail if target type is not multipath */
+ MAPINFO_MPATH_ONLY = (1 << 8),
+ /* Fail if target type is not "partition" (linear) */
+ MAPINFO_PART_ONLY = (1 << 9),
+ __MAPINFO_TGT_TYPE = (MAPINFO_MPATH_ONLY | MAPINFO_PART_ONLY),
+};
+
+typedef union libmp_map_identifier {
+ const char *str;
+ struct {
+ int major;
+ int minor;
+ } _u;
+ dev_t devt;
+} mapid_t;
+
+typedef struct libmp_map_info {
+ /** @name: name of the map.
+ * If non-NULL, it must point to an array of WWID_SIZE bytes
+ */
+ char *name;
+ /** @uuid: UUID of the map.
+ * If non-NULL it must point to an array of DM_UUID_LEN bytes
+ */
+ char *uuid;
+ /** @dmi: Basic info, must point to a valid dm_info buffer if non-NULL */
+ struct dm_info *dmi;
+ /** @target: target params, *@target will be allocated if @target is non-NULL*/
+ char **target;
+ /** @size: target size. */
+ unsigned long long *size;
+ /** @status: target status, *@status will be allocated if @status is non-NULL */
+ char **status;
+} mapinfo_t;
+
+/**
+ * libmp_mapinfo(): obtain information about a map from the kernel
+ * @param flags: see __mapinfo_flags above.
+ * Exactly one of DM_MAP_BY_NAME, DM_MAP_BY_UUID, and DM_MAP_BY_DEV must be set.
+ * @param id: string or major/minor to identify the map to query
+ * @param info: output parameters, see above. Non-NULL elements will be filled in.
+ * @returns:
+ * DMP_OK if successful.
+ * DMP_NOT_FOUND if the map wasn't found, or has no or multiple targets.
+ * DMP_NO_MATCH if the map didn't match @tgt_type (see above).
+ * DMP_ERR if some other error occurred.
+ *
+ * This function obtains the requested information for the device-mapper map
+ * identified by the input parameters.
+ * Output parameters are only filled in if the return value is DMP_OK.
+ * For target / status / size information, the map's table should contain
+ * only one target (usually multipath or linear).
+ */
+int libmp_mapinfo(int flags, mapid_t id, mapinfo_t info);
+
int dm_prereq(unsigned int *v);
void skip_libmp_dm_init(void);
void libmp_dm_exit(void);
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index f58cb1d..48c2b67 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -43,7 +43,7 @@ LIBMPATHCOMMON_1.0.0 {
put_multipath_config;
};
-LIBMULTIPATH_24.0.0 {
+LIBMULTIPATH_25.0.0 {
global:
/* symbols referenced by multipath and multipathd */
add_foreign;
@@ -134,6 +134,7 @@ global:
libmp_get_version;
libmp_get_multipath_config;
libmp_dm_task_run;
+ libmp_mapinfo;
libmp_put_multipath_config;
libmp_udev_set_sync_support;
libmultipath_exit;
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 36/49] libmultipath: use libmp_mapinfo() in do_foreach_partmaps()
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
2024-07-16 20:53 ` [PATCH v3 16/49] libmultipath: add cleanup_dm_task(), and use it in devmapper.c Martin Wilck
2024-07-16 20:53 ` [PATCH v3 19/49] libmultipath: add libmp_mapinfo() Martin Wilck
@ 2024-07-16 20:53 ` Martin Wilck
2024-07-16 20:53 ` [PATCH v3 42/49] multipathd: implement add_map_without_path() with libmp_mapinfo() Martin Wilck
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Martin Wilck @ 2024-07-16 20:53 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
Also, simplify the if statement a bit.
drop is_mpath_part() and dm_type_match, because do_foreach_partmaps() was the
last user of these functions.
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/devmapper.c | 100 ++++++++++-----------------------------
1 file changed, 26 insertions(+), 74 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 1d41207..37d2578 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -948,60 +948,6 @@ out:
return r;
}
-enum {
- DM_TYPE_NOMATCH = 0,
- DM_TYPE_MATCH,
- /* more than 1 target */
- DM_TYPE_MULTI,
- /* empty map */
- DM_TYPE_EMPTY,
- DM_TYPE_ERR,
-};
-static int dm_type_match(const char *name, char *type)
-{
- struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
- uint64_t start, length;
- char *target_type = NULL;
- char *params;
-
- if (!(dmt = libmp_dm_task_create(DM_DEVICE_TABLE)))
- return DM_TYPE_ERR;
-
- if (!dm_task_set_name(dmt, name))
- return DM_TYPE_ERR;
-
- if (!libmp_dm_task_run(dmt)) {
- dm_log_error(3, DM_DEVICE_TABLE, dmt);
- return DM_TYPE_ERR;
- }
-
- /* Fetch 1st target */
- if (dm_get_next_target(dmt, NULL, &start, &length,
- &target_type, ¶ms) != NULL)
- /* multiple targets */
- return DM_TYPE_MULTI;
- else if (!target_type)
- return DM_TYPE_EMPTY;
- else if (!strcmp(target_type, type))
- return DM_TYPE_MATCH;
- else
- return DM_TYPE_NOMATCH;
-}
-
-static bool is_mpath_part(const char *part_name, const char *map_name)
-{
- char part_uuid[DM_UUID_LEN], map_uuid[DM_UUID_LEN];
-
- if (dm_get_dm_uuid(map_name, map_uuid) != DMP_OK
- || !is_mpath_uuid(map_uuid))
- return false;
-
- if (dm_get_dm_uuid(part_name, part_uuid) != DMP_OK)
- return false;
-
- return is_mpath_part_uuid(part_uuid, map_uuid);
-}
-
int dm_is_mpath(const char *name)
{
char uuid[DM_UUID_LEN];
@@ -1443,7 +1389,7 @@ char *dm_mapname(int major, int minor)
}
static int
-do_foreach_partmaps (const char * mapname,
+do_foreach_partmaps (const char *mapname,
int (*partmap_func)(const char *, void *),
void *data)
{
@@ -1451,9 +1397,18 @@ do_foreach_partmaps (const char * mapname,
char __attribute__((cleanup(cleanup_charp))) *params = NULL;
struct dm_names *names;
unsigned next = 0;
- unsigned long long size;
- char dev_t[32];
+ char dev_t[BLK_DEV_SIZE];
char *p;
+ char map_uuid[DM_UUID_LEN];
+ struct dm_info info;
+
+ if (libmp_mapinfo(DM_MAP_BY_NAME,
+ (mapid_t) { .str = mapname },
+ (mapinfo_t) { .uuid = map_uuid, .dmi = &info }) != DMP_OK)
+ return 1;
+
+ if (safe_sprintf(dev_t, "%i:%i", info.major, info.minor))
+ return 1;
if (!(dmt = libmp_dm_task_create(DM_DEVICE_LIST)))
return 1;
@@ -1468,41 +1423,38 @@ do_foreach_partmaps (const char * mapname,
/* this is perfectly valid */
return 0;
- if (dm_dev_t(mapname, &dev_t[0], 32))
- return 1;
-
do {
+ char part_uuid[DM_UUID_LEN];
+
if (
/*
* if there is only a single "linear" target
*/
- (dm_type_match(names->name, TGT_PART) == DM_TYPE_MATCH) &&
-
+ libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_PART_ONLY,
+ (mapid_t) { .str = names->name },
+ (mapinfo_t) {
+ .uuid = part_uuid,
+ .target = ¶ms,
+ }) == DMP_OK &&
/*
* and the uuid of the target is a partition of the
* uuid of the multipath device
*/
- is_mpath_part(names->name, mapname) &&
-
- /*
- * and we can fetch the map table from the kernel
- */
- dm_get_map(names->name, &size, ¶ms) == DMP_OK &&
+ is_mpath_part_uuid(part_uuid, map_uuid) &&
/*
* and the table maps over the multipath map
*/
(p = strstr(params, dev_t)) &&
- !isdigit(*(p + strlen(dev_t)))
- ) {
- if (partmap_func(names->name, data) != 0)
- return 1;
- }
+ !isdigit(*(p + strlen(dev_t))) &&
+
+ (partmap_func(names->name, data) != 0))
+ return 1;
free(params);
params = NULL;
next = names->next;
- names = (void *) names + next;
+ names = (void*) names + next;
} while (next);
return 0;
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 42/49] multipathd: implement add_map_without_path() with libmp_mapinfo()
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
` (2 preceding siblings ...)
2024-07-16 20:53 ` [PATCH v3 36/49] libmultipath: use libmp_mapinfo() in do_foreach_partmaps() Martin Wilck
@ 2024-07-16 20:53 ` Martin Wilck
2024-07-16 20:53 ` [PATCH v3 47/49] libmultipath: Move UUID check into libmp_pathinfo__() Martin Wilck
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Martin Wilck @ 2024-07-16 20:53 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
Also, change the return value to int, as this is more expressive and
the returned struct multipath isn't used by the caller.
Also remove the call to sync_map_state() in ev_add_map(), which is
redundant because add_map_without_path() would have called update_map()
and thus sync_map_state() already.
Note: this removes the call to remove_map() at the end of the function,
which doesn't make sense anyway, because update_multipath_table()
would not return error unless the table disassembly failed, in which
case nothing would have been added the the mpvec or pathvec yet.
It should be sufficient to just cleanup the local data structures when
add_map_without_path() fails.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 79 ++++++++++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 38 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 1e7a6ac..32011c9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -707,51 +707,57 @@ fail:
return 0;
}
-static struct multipath *
-add_map_without_path (struct vectors *vecs, const char *alias)
+static int add_map_without_path (struct vectors *vecs, const char *alias)
{
- struct multipath * mpp = alloc_multipath();
+ struct multipath __attribute__((cleanup(cleanup_multipath_and_paths)))
+ *mpp = alloc_multipath();
+ char __attribute__((cleanup(cleanup_charp))) *params = NULL;
+ char __attribute__((cleanup(cleanup_charp))) *status = NULL;
struct config *conf;
+ char uuid[DM_UUID_LEN];
+ int rc = DMP_ERR;
- if (!mpp)
- return NULL;
- if (!alias) {
- free(mpp);
- return NULL;
- }
+ if (!mpp || !(mpp->alias = strdup(alias)))
+ return DMP_ERR;
- mpp->alias = strdup(alias);
+ if ((rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
+ (mapid_t) { .str = mpp->alias },
+ (mapinfo_t) {
+ .uuid = uuid,
+ .dmi = &mpp->dmi,
+ .size = &mpp->size,
+ .target = ¶ms,
+ .status = &status,
+ })) != DMP_OK)
+ return rc;
+
+ if (!is_mpath_uuid(uuid))
+ return DMP_NO_MATCH;
+ else
+ strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp->wwid));
- if (dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
- condlog(3, "%s: cannot access table", mpp->alias);
- goto out;
- }
- if (!strlen(mpp->wwid) &&
- dm_get_wwid(mpp->alias, mpp->wwid, WWID_SIZE) != DMP_OK) {
- condlog(3, "%s: cannot obtain WWID", mpp->alias);
- goto out;
- }
if (!strlen(mpp->wwid))
condlog(1, "%s: adding map with empty WWID", mpp->alias);
+
conf = get_multipath_config();
mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
put_multipath_config(conf);
- if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK)
- goto out;
+ if ((rc = update_multipath_table__(mpp, vecs->pathvec, 0, params, status)) != DMP_OK)
+ return DMP_ERR;
if (!vector_alloc_slot(vecs->mpvec))
- goto out;
+ return DMP_ERR;
+ vector_set_slot(vecs->mpvec, steal_ptr(mpp));
- vector_set_slot(vecs->mpvec, mpp);
+ /*
+ * We can't pass mpp here, steal_ptr() has just nullified it.
+ * vector_set_slot() just set the last slot, use that.
+ */
+ if (update_map(VECTOR_LAST_SLOT(vecs->mpvec), vecs, 1) != 0) /* map removed */
+ return DMP_ERR;
- if (update_map(mpp, vecs, 1) != 0) /* map removed */
- return NULL;
-
- return mpp;
-out:
- remove_map(mpp, vecs->pathvec, vecs->mpvec);
- return NULL;
+ return DMP_OK;
}
static int
@@ -865,14 +871,9 @@ int
ev_add_map (char * dev, const char * alias, struct vectors * vecs)
{
struct multipath * mpp;
- int reassign_maps;
+ int reassign_maps, rc;
struct config *conf;
- if (dm_is_mpath(alias) != DM_IS_MPATH_YES) {
- condlog(4, "%s: not a multipath map", alias);
- return 0;
- }
-
mpp = find_mp_by_alias(vecs->mpvec, alias);
if (mpp) {
@@ -910,10 +911,12 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
/*
* now we can register the map
*/
- if ((mpp = add_map_without_path(vecs, alias))) {
- sync_map_state(mpp);
+ if ((rc = add_map_without_path(vecs, alias)) == DMP_OK) {
condlog(2, "%s: devmap %s registered", alias, dev);
return 0;
+ } else if (rc == DMP_NO_MATCH) {
+ condlog(4, "%s: not a multipath map", alias);
+ return 0;
} else {
condlog(2, "%s: ev_add_map failed", dev);
return 1;
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 47/49] libmultipath: Move UUID check into libmp_pathinfo__()
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
` (3 preceding siblings ...)
2024-07-16 20:53 ` [PATCH v3 42/49] multipathd: implement add_map_without_path() with libmp_mapinfo() Martin Wilck
@ 2024-07-16 20:53 ` Martin Wilck
2024-07-16 20:53 ` [PATCH v3 48/49] libmultipath: don't call do_foreach_partmaps() recursively Martin Wilck
2024-07-17 19:55 ` [PATCH v3 00/49] multipath-tools: devmapper API refactored Benjamin Marzinski
6 siblings, 0 replies; 8+ messages in thread
From: Martin Wilck @ 2024-07-16 20:53 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
We have a couple of callers that check whether the map UUID conforms
to the multipath convention ("mpath-xyz"). Move this check into
libmp_mapinfo__(). Add another flag MAPINFO_CHECK_UUID for this
purpose. This allows to simplify some callers, which only fetched
the UUID in order to check it.
Note that the UUID check is orthogonal to MAPINFO_MPATH_ONLY, which
tests the target type. We shold make sure that both tests are in
agreement, but this is postponed to a later patch set.
is_mpath_uuid() can now be converted to a static function.
Also add some unit tests for the WWID check.
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmpathpersist/mpath_persist_int.c | 11 +-
libmultipath/devmapper.c | 52 +++----
libmultipath/devmapper.h | 3 +-
libmultipath/libmultipath.version | 1 -
multipath/main.c | 5 +-
multipathd/main.c | 7 +-
tests/mapinfo.c | 226 ++++++++++++++++++++++++++++
7 files changed, 256 insertions(+), 49 deletions(-)
diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 807415f..f4d9e7c 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -158,7 +158,7 @@ static int mpath_get_map(vector curmp, vector pathvec, int fd, struct multipath
{
int rc;
struct stat info;
- char alias[WWID_SIZE], uuid[DM_UUID_LEN];
+ char alias[WWID_SIZE];
struct multipath *mpp;
if (fstat(fd, &info) != 0){
@@ -171,14 +171,11 @@ static int mpath_get_map(vector curmp, vector pathvec, int fd, struct multipath
}
/* get alias from major:minor*/
- rc = libmp_mapinfo(DM_MAP_BY_DEVT | MAPINFO_MPATH_ONLY,
+ rc = libmp_mapinfo(DM_MAP_BY_DEVT | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
(mapid_t) { .devt = info.st_rdev },
- (mapinfo_t) {
- .name = alias,
- .uuid = uuid,
- });
+ (mapinfo_t) { .name = alias });
- if (rc == DMP_NO_MATCH || !is_mpath_uuid(uuid)) {
+ if (rc == DMP_NO_MATCH) {
condlog(3, "%s: not a multipath device.", alias);
return MPATH_PR_DMMP_ERROR;
} else if (rc != DMP_OK) {
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e96b83e..455905a 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -611,6 +611,11 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
return 0;
}
+static bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
+{
+ return !strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN);
+}
+
bool
has_dm_info(const struct multipath *mpp)
{
@@ -736,11 +741,17 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
* If error is returned, don't touch any output parameters.
*/
if ((info.name && !(name = dm_task_get_name(dmt)))
- || (info.uuid && !(uuid = dm_task_get_uuid(dmt)))
+ || ((info.uuid || flags & MAPINFO_CHECK_UUID)
+ && !(uuid = dm_task_get_uuid(dmt)))
|| (info.status && !(tmp_status = strdup(params)))
|| (info.target && !tmp_target && !(tmp_target = strdup(params))))
return DMP_ERR;
+ if (flags & MAPINFO_CHECK_UUID && !is_mpath_uuid(uuid)) {
+ condlog(3, "%s: UUID mismatch: %s", fname__, uuid);
+ return DMP_NO_MATCH;
+ }
+
if (info.name) {
strlcpy(info.name, name, WWID_SIZE);
condlog(4, "%s: %s: name: \"%s\"", fname__, map_id, info.name);
@@ -810,18 +821,6 @@ int libmp_mapinfo(int flags, mapid_t id, mapinfo_t info)
libmp_map_identifier(flags, id, idbuf));
}
-static int dm_get_dm_uuid(const char *mapname, char uuid[DM_UUID_LEN])
-{
- return libmp_mapinfo(DM_MAP_BY_NAME,
- (mapid_t) { .str = mapname },
- (mapinfo_t) { .uuid = uuid });
-}
-
-bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
-{
- return !strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN);
-}
-
/**
* dm_get_wwid(): return WWID for a multipath map
* @returns:
@@ -834,16 +833,14 @@ bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
int dm_get_wwid(const char *name, char *uuid, int uuid_len)
{
char tmp[DM_UUID_LEN];
- int rc = dm_get_dm_uuid(name, tmp);
+ int rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = name },
+ (mapinfo_t) { .uuid = tmp });
if (rc != DMP_OK)
return rc;
- if (is_mpath_uuid(tmp))
- strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
- else
- return DMP_NO_MATCH;
-
+ strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
return DMP_OK;
}
@@ -861,16 +858,13 @@ static bool is_mpath_part_uuid(const char part_uuid[DM_UUID_LEN],
int dm_is_mpath(const char *name)
{
- char uuid[DM_UUID_LEN];
- int rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
+ int rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
(mapid_t) { .str = name },
- (mapinfo_t) { .uuid = uuid });
+ (mapinfo_t) { .uuid = NULL });
switch (rc) {
case DMP_OK:
- if (is_mpath_uuid(uuid))
- return DM_IS_MPATH_YES;
- /* fallthrough */
+ return DM_IS_MPATH_YES;
case DMP_NOT_FOUND:
case DMP_NO_MATCH:
return DM_IS_MPATH_NO;
@@ -961,14 +955,10 @@ int _dm_flush_map (const char *mapname, int flags, int retries)
int queue_if_no_path = 0;
int udev_flags = 0;
char *params __attribute__((cleanup(cleanup_charp))) = NULL;
- char uuid[DM_UUID_LEN];
- if (libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
+ if (libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
(mapid_t) { .str = mapname },
- (mapinfo_t) {
- .uuid = uuid,
- .target = ¶ms }) != DMP_OK
- || !is_mpath_uuid(uuid))
+ (mapinfo_t) { .target = ¶ms }) != DMP_OK)
return DM_FLUSH_OK; /* nothing to do */
/* if the device currently has no partitions, do not
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index d0e20bf..7a551d9 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -57,6 +57,8 @@ enum __mapinfo_flags {
/* Fail if target type is not "partition" (linear) */
MAPINFO_PART_ONLY = (1 << 9),
__MAPINFO_TGT_TYPE = (MAPINFO_MPATH_ONLY | MAPINFO_PART_ONLY),
+ /* Fail if the UUID doesn't match the multipath UUID format */
+ MAPINFO_CHECK_UUID = (1 << 10),
};
typedef union libmp_map_identifier {
@@ -138,7 +140,6 @@ enum {
DM_IS_MPATH_ERR,
};
-bool is_mpath_uuid(const char uuid[DM_UUID_LEN]);
int dm_is_mpath(const char *name);
enum {
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 54b5a23..292a330 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -127,7 +127,6 @@ global:
init_foreign;
init_prio;
io_err_stat_handle_pathfail;
- is_mpath_uuid;
is_path_valid;
libmp_dm_task_create;
libmp_get_version;
diff --git a/multipath/main.c b/multipath/main.c
index 0d989dc..4b19d2e 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -254,7 +254,7 @@ static int check_usable_paths(struct config *conf,
if (pathvec == NULL)
return r;
- if (libmp_mapinfo(DM_MAP_BY_DEVT | MAPINFO_MPATH_ONLY,
+ if (libmp_mapinfo(DM_MAP_BY_DEVT | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
(mapid_t) { .devt = devt },
(mapinfo_t) {
.name = mpp->alias,
@@ -266,9 +266,6 @@ static int check_usable_paths(struct config *conf,
}) != DMP_OK)
return r;
- if (!is_mpath_uuid(uuid))
- return r;
-
strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp->wwid));
if (update_multipath_table__(mpp, pathvec, 0, params, status) != DMP_OK)
diff --git a/multipathd/main.c b/multipathd/main.c
index 32011c9..833c1e2 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -720,7 +720,7 @@ static int add_map_without_path (struct vectors *vecs, const char *alias)
if (!mpp || !(mpp->alias = strdup(alias)))
return DMP_ERR;
- if ((rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
+ if ((rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
(mapid_t) { .str = mpp->alias },
(mapinfo_t) {
.uuid = uuid,
@@ -731,10 +731,7 @@ static int add_map_without_path (struct vectors *vecs, const char *alias)
})) != DMP_OK)
return rc;
- if (!is_mpath_uuid(uuid))
- return DMP_NO_MATCH;
- else
- strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp->wwid));
+ strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp->wwid));
if (!strlen(mpp->wwid))
condlog(1, "%s: adding map with empty WWID", mpp->alias);
diff --git a/tests/mapinfo.c b/tests/mapinfo.c
index f3a8440..f7ad868 100644
--- a/tests/mapinfo.c
+++ b/tests/mapinfo.c
@@ -54,6 +54,16 @@ static const char MPATH_STATUS_01[] =
"A 0 3 2 65:32 A 0 0 1 67:64 A 0 0 1 69:96 A 0 0 1 "
"E 0 3 2 8:16 A 0 0 1 66:48 A 0 0 1 68:80 A 0 0 1 ";
+static const char BAD_UUID_01[] = "";
+static const char BAD_UUID_02[] = "mpath3600a098038302d414b2b4d4453474f62";
+static const char BAD_UUID_03[] = " mpath-3600a098038302d414b2b4d4453474f62";
+static const char BAD_UUID_04[] = "-mpath-3600a098038302d414b2b4d4453474f62";
+static const char BAD_UUID_05[] = "mpth-3600a098038302d414b2b4d4453474f62";
+static const char BAD_UUID_06[] = "part1-mpath-3600a098038302d414b2b4d4453474f62";
+static const char BAD_UUID_07[] = "mpath 3600a098038302d414b2b4d4453474f62";
+static const char BAD_UUID_08[] = "mpath";
+static const char BAD_UUID_09[] = "mpath-";
+
char *__real_strdup(const char *str);
char *__wrap_strdup(const char *str)
{
@@ -413,6 +423,208 @@ static void test_mapinfo_good_exists(void **state)
assert_int_equal(rc, DMP_OK);
}
+static void test_mapinfo_bad_check_uuid_00(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, NULL);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_ERR);
+}
+
+static void test_mapinfo_bad_check_uuid_01(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_01);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_02(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_02);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_03(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_03);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_04(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_04);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_05(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_05);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_06(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_06);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_07(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_07);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_08(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_08);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_NO_MATCH);
+}
+
+static void test_mapinfo_bad_check_uuid_09(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, BAD_UUID_09);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_OK);
+}
+
+static void test_mapinfo_good_check_uuid_01(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_OK);
+}
+
+static void test_mapinfo_good_check_uuid_02(void **state)
+{
+ int rc;
+ char uuid[DM_UUID_LEN];
+
+ mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .uuid = uuid });
+ assert_int_equal(rc, DMP_OK);
+}
+
+static void test_mapinfo_good_check_uuid_03(void **state)
+{
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ mock_dm_get_next_target(12345, TGT_MPATH, MPATH_STATUS_01, NULL);
+ will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .name = NULL });
+ assert_int_equal(rc, DMP_OK);
+}
+
+static void test_mapinfo_good_check_uuid_04(void **state)
+{
+ char __attribute__((cleanup(cleanup_charp))) *target = NULL;
+ int rc;
+
+ mock_mapinfo_name_1(DM_DEVICE_TABLE, 1, "foo", 1, 1, 0);
+ WRAP_DM_TASK_GET_INFO(1);
+ WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+ mock_dm_get_next_target(12345, TGT_MPATH, MPATH_STATUS_01, NULL);
+ will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+ will_return(__wrap_strdup, 1);
+
+ rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
+ (mapid_t) { .str = "foo", },
+ (mapinfo_t) { .target = &target });
+ assert_int_equal(rc, DMP_OK);
+}
+
static void test_mapinfo_bad_set_uuid(void **state)
{
int rc;
@@ -1126,6 +1338,20 @@ static int test_mapinfo(void)
cmocka_unit_test(test_mapinfo_bad_get_info_03),
cmocka_unit_test(test_mapinfo_bad_get_info_04),
cmocka_unit_test(test_mapinfo_good_exists),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_00),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_01),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_02),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_03),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_04),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_05),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_06),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_07),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_08),
+ cmocka_unit_test(test_mapinfo_bad_check_uuid_09),
+ cmocka_unit_test(test_mapinfo_good_check_uuid_01),
+ cmocka_unit_test(test_mapinfo_good_check_uuid_02),
+ cmocka_unit_test(test_mapinfo_good_check_uuid_03),
+ cmocka_unit_test(test_mapinfo_good_check_uuid_04),
cmocka_unit_test(test_mapinfo_bad_set_uuid),
cmocka_unit_test(test_mapinfo_bad_set_dev_01),
cmocka_unit_test(test_mapinfo_bad_set_dev_02),
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 48/49] libmultipath: don't call do_foreach_partmaps() recursively
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
` (4 preceding siblings ...)
2024-07-16 20:53 ` [PATCH v3 47/49] libmultipath: Move UUID check into libmp_pathinfo__() Martin Wilck
@ 2024-07-16 20:53 ` Martin Wilck
2024-07-17 19:55 ` [PATCH v3 00/49] multipath-tools: devmapper API refactored Benjamin Marzinski
6 siblings, 0 replies; 8+ messages in thread
From: Martin Wilck @ 2024-07-16 20:53 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
We've removed partition mappings recursively since 83fb936 ("Correctly remove
logical partition maps"). This was wrong, because kpartx doesn't create
logical partitions as mappings onto the extended partition. Rather, logical
partitions are created by kpartx as mappings to the multipath device, and
afaics, this has always been the case. Therefore, the loop in
do_foreach_partmaps() will detect all partition mappings (primary, extended,
and logical) without recursion. At least since 4059e42 ("libmultipath: fix
partition detection"), the recursion has actually been pointless, because
is_mpath_part() would never have returned "true" for a pair of two partition
mappings (one representing an extended partition and one a logical partition).
Avoiding the recursion has the additional benefit that the complexity of
removing maps scales with N, where N is the number of dm devices, rather than
with N^2. Also, it simplifies the code.
Split partmap_in_use() into two separate functions, mpath_in_use() (to be called
for multipath maps) and count_partitions(), which is called from
do_foreach_partmaps().
Because do_foreach_partmaps() is now only legitimately called for multipath
maps, quit early if called for another map type.
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/devmapper.c | 48 +++++++++++++++++++------------
libmultipath/devmapper.h | 2 +-
libmultipath/libmultipath.version | 2 +-
multipathd/main.c | 2 +-
4 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 455905a..3b2e8ac 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -929,22 +929,37 @@ has_partmap(const char *name __attribute__((unused)),
return 1;
}
-int
-partmap_in_use(const char *name, void *data)
+/*
+ * This will be called from mpath_in_use, for each partition.
+ * If the partition itself in use, returns 1 immediately, causing
+ * do_foreach_partmaps() to stop iterating and return 1.
+ * Otherwise, increases the partition count.
+ */
+static int count_partitions(const char *name, void *data)
+{
+ int *ret_count = (int *)data;
+ int open_count = dm_get_opencount(name);
+
+ if (open_count)
+ return 1;
+ (*ret_count)++;
+ return 0;
+}
+
+int mpath_in_use(const char *name)
{
- int part_count, *ret_count = (int *)data;
int open_count = dm_get_opencount(name);
- if (ret_count)
- (*ret_count)++;
- part_count = 0;
if (open_count) {
- if (do_foreach_partmaps(name, partmap_in_use, &part_count))
- return 1;
- if (open_count != part_count) {
- condlog(2, "%s: map in use", name);
+ int part_count = 0;
+
+ if (do_foreach_partmaps(name, count_partitions, &part_count)) {
+ condlog(4, "%s: %s has open partitions", __func__, name);
return 1;
}
+ condlog(4, "%s: %s: %d openers, %d partitions", __func__, name,
+ open_count, part_count);
+ return open_count > part_count;
}
return 0;
}
@@ -968,7 +983,7 @@ int _dm_flush_map (const char *mapname, int flags, int retries)
/* If you aren't doing a deferred remove, make sure that no
* devices are in use */
- if (!(flags & DMFL_DEFERRED) && partmap_in_use(mapname, NULL))
+ if (!(flags & DMFL_DEFERRED) && mpath_in_use(mapname))
return DM_FLUSH_BUSY;
if ((flags & DMFL_SUSPEND) &&
@@ -1305,7 +1320,7 @@ do_foreach_partmaps (const char *mapname,
char map_uuid[DM_UUID_LEN];
struct dm_info info;
- if (libmp_mapinfo(DM_MAP_BY_NAME,
+ if (libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
(mapid_t) { .str = mapname },
(mapinfo_t) { .uuid = map_uuid, .dmi = &info }) != DMP_OK)
return 1;
@@ -1372,12 +1387,9 @@ 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->flags);
- if (!(rd->flags & DMFL_DEFERRED) && dm_get_opencount(name)) {
- condlog(2, "%s: map in use", name);
- return DM_FLUSH_BUSY;
- }
+ 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->flags);
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 7a551d9..f6d0017 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -150,7 +150,7 @@ enum {
DM_FLUSH_BUSY,
};
-int partmap_in_use(const char *name, void *data);
+int mpath_in_use(const char *name);
enum {
DMFL_NONE = 0,
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 292a330..959f675 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -138,10 +138,10 @@ global:
libmultipath_exit;
libmultipath_init;
load_config;
+ mpath_in_use;
need_io_err_check;
orphan_path;
parse_prkey_flags;
- partmap_in_use;
pathcount;
path_discovery;
path_get_tpgs;
diff --git a/multipathd/main.c b/multipathd/main.c
index 833c1e2..13ed6d0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -597,7 +597,7 @@ flush_map_nopaths(struct multipath *mpp, struct vectors *vecs) {
return false;
}
if (mpp->flush_on_last_del == FLUSH_UNUSED &&
- partmap_in_use(mpp->alias, NULL) && is_queueing) {
+ mpath_in_use(mpp->alias) && is_queueing) {
condlog(2, "%s: map in use and queueing, can't remove",
mpp->alias);
return false;
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 00/49] multipath-tools: devmapper API refactored
2024-07-16 20:53 [PATCH v3 00/49] multipath-tools: devmapper API refactored Martin Wilck
` (5 preceding siblings ...)
2024-07-16 20:53 ` [PATCH v3 48/49] libmultipath: don't call do_foreach_partmaps() recursively Martin Wilck
@ 2024-07-17 19:55 ` Benjamin Marzinski
6 siblings, 0 replies; 8+ messages in thread
From: Benjamin Marzinski @ 2024-07-17 19:55 UTC (permalink / raw)
To: Martin Wilck; +Cc: Christophe Varoqui, dm-devel
On Tue, Jul 16, 2024 at 10:53:38PM +0200, Martin Wilck wrote:
For the set:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Patch 1-9 are generic cleanups, partly targeting the previously reported
> issues with multipath-tools installation on NixOS by removing hard-coded
> paths as much as possible.
>
> Patch 10-18 are basic cleanups for the devmapper code.
>
> Patch 19 is the heart of this patch set, introducing the function
> libmp_mapinfo(), which is supposed to serve as general API for fetching
> information about maps from libdevmapper. It can work with arbitrary
> input for identifying a map, and produce any combination of desired
> outputs, with the most lightweight (combination of) ioctls possible.
> This part of the set removes some code that had been changed
> in patch 10-16. This is sort of suboptimal, but IMO it improves
> the readability of the set as a whole.
>
> Patch 20 adds unit tests for this function.
>
> Patch 21-45 change the libmultipath code to use the new API everywhere,
> and add some additional minor improvements and fixes.
>
> Patch 46-49 have been added in v2.
>
> * 46: fix a long-standing bug I accidentally discovered during testing
> * 47: implement Ben's idea from the comment of #31 in the old series.
> * 48: simplify and fix the logic for determining whether a map is in use
> * 49: unrelated, fixes for the directio test
>
> The code is pushed to https://github.com/openSUSE/multipath-tools/tree/tip.
>
> Comments and reviews welcome.
>
> # Changes v2->v3 (all suggestions by Ben Marzinski)
>
> Just posting the patches that have changed wrt v2.
>
> * 16: remove variable r in do_foreach_partmaps
> * 19: - print "libmp_mapinfo" rather than "libmp_mapinfo__" in log messages
> - use entire low byte of flags parameter for mapid type.
> * 36: adapt to 11
> * 42: Fix comment about steal_ptr
> * 47: exchanged with 48, adapt to 14
> * 48: exchanged with 47
>
> # Changes v1->v2:
>
> I'm posting the entire series, because some of those patches that Ben had
> already reviewed are changed.
>
> * 1: add fix for libaio
> * 6: make sure kpartx.rules is built (Ben)
> * 16: split this large patch between the changes that are just related
> to cleanup_dm_task(), and the other code modifications
> * 17: new, used to be part of #16
> * 18: new, used to be part of #17. Changed the treatment of DMP_ERR /
> DM_IS_MPATH_ERR (Ben)
>
> (From here on, patch number n maps to (n-2) in the v1 set).
>
> * 19 (was 17):
> - Allow passing a dev_t in mapid_t (Ben).
> - Remove the tgt_type field in mapinfo_t. Instead, introduce
> flags MAPINFO_MPATH_ONLY and MAPINFO_PART_ONLY to make
> libmp_mapinfo__() filter either multipath or partition devices,
> inspired by Ben's comment on patch #31 of the v1 series.
> See also #48.
> These changes require changes in users of the API in patch 30ff.
> I have not removed the Reviewed-by: tags in the followup patches.
> * 20 (18): Adapt tests to #19.
> * 25 (23):
> - dm_get_wwid() won't touch the output unless it returns DMP_OK
> - adjusted callers to make sure the uuid is not accessed unless
> DMP_OK was returned
> - distinguish between "multipath map is present" and "other map
> is present" in domap() (Ben)
> - Change handling of DMP_ERR in alias_already_taken() (Ben)
> * 28 (26): Fixed bug in the return code handling in dm_dev_t()
> * 30 (28):
> - Change return code handling in dm_type_match(), to distinguish
> between "not multipath" and "generic error" (Ben)
> - adapt to #19
> * 31 (29): adapt to #19
> * 32 (30): adapt to #19
> * 33 (31): adapt to #18. is_mpath_uuid() will be unexported in #48
> * 36 (34):
> - don't check the UUID in do_foreach_partmaps(). This would break
> the partmap_in_use detection (but see #47 below).
> - adapt to #19
> * 37 (35): fetch mpp->size (Ben). Print a warning if the size has changed.
> * 41 (39):
> - fetch mpp->size (Ben).
> - adapt to #19
> * 42 (40): - remove redundant sync_map_state() call (Ben)
> - fetch mpp->size (Ben)
> - check return code of dm_get_wwid()
> - fix segfault caused by passing NULL to update_map()
> * n/a (42): dropped (Ben)
>
> (From here on, n maps to (n - 1) in the v1 set).
>
> * 44 (43): adapt to #19
>
> Patch 46-49 are new.
>
> Martin Wilck (49):
> multipath-tools CI: more fixes for arm/v7
> multipath-tools CI: fix dmevents test for Debian Sid, arm/v7
> create-config.mk: use printf instead of /bin/echo
> multipathd.service.in: use @BINDIR@ instead of /sbin
> Makefile.inc: replace @BINDIR@ with $(TGTDIR)/$(bindir)
> kpartx.rules: use @BINDIR@ to locate kpartx
> multipath-tools: Remove hard-coded paths to executables
> multipath-tools: compile_commands.json fixes
> multipath-tools: .gitignore: ignore o.wrap files for CI helpers
> libmultipath: remove unused includes in devmapper.h
> libmultipath: use DM_DEVICE_INFO in dm_mapname()
> multipath-tools: don't call dm_task_no_open_count()
> libmpathutil: export cleanup_udev_device()
> libmpathutil: add cleanup_vector()
> libmultipath: add cleanup helpers for struct multipath
> libmultipath: add cleanup_dm_task(), and use it in devmapper.c
> libmultipath: rename dm_type()->dm_type_match() and use symbolic
> values
> libmultipath: Use symbolic return values for dm_is_mpath()
> libmultipath: add libmp_mapinfo()
> libmultipath tests: add tests for libmp_mapinfo()
> libmultipath: implement dm_get_info() and dm_map_present() with new
> API
> libmultipath: remove dm_get_prefixed_uuid()
> libmultipath: is_mpath_part(): improve parsing
> libmultipath: rename dm_get_uuid() -> dm_get_wwid()
> libmultipath: improve dm_get_wwid() return value logic
> libmultipath: reimplement dm_map_name() with new API
> libmultipath: reimplement dm_map_present_by_uuid()
> libmultipath: reimplement dm_get_opencount() with new API
> libmpathpersist: skip redundant dm_map_present() call
> libmultipath: implement dm_is_mpath() with new API
> libmultipath: implement dm_get_multipath() with new API
> libmultipath: use libmp_mapinfo() in _dm_flush_map()
> libmultipath: add is_mpath_uuid() helper
> libmultipath: add is_mpath_part_uuid() helper
> libmultipath: add dmp_errstr() helper
> libmultipath: use libmp_mapinfo() in do_foreach_partmaps()
> libmultipath: use libmp_pathinfo() in update_multipath_table()
> libmultipath: update mpp->dmi in update_multipath_table()
> libmultipath: drop extra call to dm_map_present() in domap()
> libmultipath: split off update_multipath_table__()
> multipath: implement check_usable_paths() with libmp_pathinfo()
> multipathd: implement add_map_without_path() with libmp_mapinfo()
> libmultipath: simplify dm_get_maps()
> libmpathpersist: use libmp_mapinfo() in mpath_get_map()
> libmpathpersist: use mpp->alias in do_mpath_persistent_reserve_out()
> libmultipath: fix deferred_remove logic in remove_partmap()
> libmultipath: Move UUID check into libmp_pathinfo__()
> libmultipath: don't call do_foreach_partmaps() recursively
> multipath-tools tests: fix directio test with real device
>
> .gitignore | 4 +
> Makefile | 4 +-
> Makefile.inc | 29 +-
> create-config.mk | 4 +-
> kpartx/Makefile | 6 +-
> kpartx/devmapper.c | 15 -
> kpartx/{kpartx.rules => kpartx.rules.in} | 2 +-
> kpartx/kpartx_id | 8 +-
> libmpathpersist/mpath_persist_int.c | 81 +-
> libmpathutil/libmpathutil.version | 5 +
> libmpathutil/util.c | 6 +
> libmpathutil/util.h | 3 +
> libmpathutil/vector.c | 6 +
> libmpathutil/vector.h | 1 +
> libmultipath/alias.c | 11 +-
> libmultipath/configure.c | 31 +-
> libmultipath/devmapper.c | 1055 +++++++---------
> libmultipath/devmapper.h | 109 +-
> libmultipath/libmultipath.version | 11 +-
> libmultipath/print.c | 6 -
> libmultipath/structs.c | 12 +
> libmultipath/structs.h | 2 +
> libmultipath/structs_vec.c | 52 +-
> libmultipath/structs_vec.h | 2 +
> libmultipath/valid.c | 2 +-
> libmultipath/wwids.c | 2 +-
> multipath/11-dm-mpath.rules.in | 4 +-
> multipath/main.c | 57 +-
> multipath/multipath.rules.in | 6 +-
> multipathd/dmevents.c | 21 +-
> multipathd/main.c | 77 +-
> multipathd/multipathd.service.in | 4 +-
> multipathd/waiter.c | 2 -
> tests/Makefile | 13 +-
> tests/README.md | 29 +-
> tests/alias.c | 32 +-
> tests/directio.c | 182 +--
> tests/dmevents.c | 34 +-
> tests/mapinfo.c | 1401 ++++++++++++++++++++++
> tests/valid.c | 10 +-
> tests/wrap64.h | 11 +-
> 41 files changed, 2372 insertions(+), 980 deletions(-)
> rename kpartx/{kpartx.rules => kpartx.rules.in} (96%)
> create mode 100644 tests/mapinfo.c
>
> --
> 2.45.2
^ permalink raw reply [flat|nested] 8+ messages in thread