| Message ID | 20240712171458.77611-20-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: devmapper API refactored | expand |
On Fri, Jul 12, 2024 at 07:14:27PM +0200, Martin Wilck wrote: > 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 | 192 +++++++++++++++++++++++++++++- > libmultipath/devmapper.h | 70 +++++++++++ > libmultipath/libmultipath.version | 3 +- > 3 files changed, 263 insertions(+), 2 deletions(-) > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 3abdc26..4e6b5b2 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,197 @@ 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) > +{ > + 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", __func__, 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) { I wonder if we should wrap ibmp_mapinfo in a macro that grabs the function name of the calling function and passes it down to libmp_mapinfo__() otherwise there will be a lot of messages like this or condlog(2, "%s: map %s doesn't exist", __func__, map_id); that use "libmp_mapinfo__" as the function, which isn't super useful. > + condlog(2, "%s: map %s not found", __func__, map_id); > + return DMP_NOT_FOUND; > + } else > + return DMP_ERR; > + } > + > + condlog(4, "%s: DM ioctl %d succeeded for %s", > + __func__, ioctl_nr, map_id); > + > + if (!dm_task_get_info(dmt, &dmi)) { > + condlog(2, "%s: dm_task_get_info() failed for %s ", __func__, map_id); > + return DMP_ERR; > + } else if(!dmi.exists) { > + condlog(2, "%s: map %s doesn't exist", __func__, 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", __func__, map_id); > + return DMP_NOT_FOUND; > + } > + if (!params) { > + condlog(2, "%s: map %s has no targets", __func__, 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\"", > + __func__, 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\"", __func__, map_id, info.name); > + } > + if (info.uuid) { > + strlcpy(info.uuid, uuid, DM_UUID_LEN); > + condlog(4, "%s: %s: uuid: \"%s\"", __func__, map_id, info.uuid); > + } > + > + if (info.size) { > + *info.size = length; > + condlog(4, "%s: %s: size: %lld", __func__, 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", > + __func__, 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\"", __func__, map_id, *info.target); > + } > + > + if (info.status) { > + *info.status = steal_ptr(tmp_status); > + condlog(4, "%s: %s: status: \"%s\"", __func__, 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..725889b 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, I mentioned tihs before, but the DM_MAP_BY_* identifiers only take up 3 bytes so __DM_MAP_BY_MASK can just be 3 or (1 << 2) - 1, unless you are reserving space for more identifiers which is fine but probably unnecessary, since libmultipath isn't a stable API. Or is there some other reason I'm missing. > + __DM_MAP_BY_MASK = (1 << 3) - 1, > + /* Fail if target type is not multipath */ > + MAPINFO_MPATH_ONLY = (1 << 3), > + /* Fail if target type is not "partition" (linear) */ > + MAPINFO_PART_ONLY = (1 << 4), > + __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. Will be ignored if @target is NULL */ AFAICT this isn't true. You can just request the device size, and libmp_mapinfo() will give you the size. -Ben > + 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
On Mon, 2024-07-15 at 18:41 -0400, Benjamin Marzinski wrote: > On Fri, Jul 12, 2024 at 07:14:27PM +0200, Martin Wilck wrote: > > 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 | 192 > > +++++++++++++++++++++++++++++- > > libmultipath/devmapper.h | 70 +++++++++++ > > libmultipath/libmultipath.version | 3 +- > > 3 files changed, 263 insertions(+), 2 deletions(-) > > > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > > index 3abdc26..4e6b5b2 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,197 @@ 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) > > +{ > > + 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", __func__, 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) { > > I wonder if we should wrap ibmp_mapinfo in a macro that grabs the > function name of the calling function and passes it down to > libmp_mapinfo__() otherwise there will be a lot of messages like this > or > > condlog(2, "%s: map %s doesn't exist", __func__, map_id); > > that use "libmp_mapinfo__" as the function, which isn't super useful. Right. I think what I'll do is just hardcode "libmp_mapinfo" in these messages instead of using __func__. > > > + condlog(2, "%s: map %s not found", > > __func__, map_id); > > + return DMP_NOT_FOUND; > > + } else > > + return DMP_ERR; > > + } > > + > > + condlog(4, "%s: DM ioctl %d succeeded for %s", > > + __func__, ioctl_nr, map_id); > > + > > + if (!dm_task_get_info(dmt, &dmi)) { > > + condlog(2, "%s: dm_task_get_info() failed for %s > > ", __func__, map_id); > > + return DMP_ERR; > > + } else if(!dmi.exists) { > > + condlog(2, "%s: map %s doesn't exist", __func__, > > 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", __func__, map_id); > > + return DMP_NOT_FOUND; > > + } > > + if (!params) { > > + condlog(2, "%s: map %s has no targets", > > __func__, 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\"", > > + __func__, 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\"", __func__, > > map_id, info.name); > > + } > > + if (info.uuid) { > > + strlcpy(info.uuid, uuid, DM_UUID_LEN); > > + condlog(4, "%s: %s: uuid: \"%s\"", __func__, > > map_id, info.uuid); > > + } > > + > > + if (info.size) { > > + *info.size = length; > > + condlog(4, "%s: %s: size: %lld", __func__, 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", > > + __func__, 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\"", > > __func__, map_id, *info.target); > > + } > > + > > + if (info.status) { > > + *info.status = steal_ptr(tmp_status); > > + condlog(4, "%s: %s: status: \"%s\"", __func__, > > 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..725889b 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, > > I mentioned tihs before, but the DM_MAP_BY_* identifiers only take up > 3 > bytes so __DM_MAP_BY_MASK can just be 3 or (1 << 2) - 1, unless you > are > reserving space for more identifiers which is fine but probably > unnecessary, since libmultipath isn't a stable API. Or is there some > other reason I'm missing. Is it important that this bit field is "dense"? Indeed I wanted to leave some space, even though your're right that it most probably won't be necessary. I thought it might be helpful during debugging, for example. Perhaps I should have used the entire low byte for the map_by part. That might even provide optimization opportunities for the compiler. Unless you object, this is what I'll do. > > > + __DM_MAP_BY_MASK = (1 << 3) - 1, > > + /* Fail if target type is not multipath */ > > + MAPINFO_MPATH_ONLY = (1 << 3), > > + /* Fail if target type is not "partition" (linear) */ > > + MAPINFO_PART_ONLY = (1 << 4), > > + __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. Will be ignored if @target is NULL > > */ > > AFAICT this isn't true. You can just request the device size, and > libmp_mapinfo() will give you the size. Right. Thanks Martin > > -Ben > > > + 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 >
On Tue, Jul 16, 2024 at 05:51:21PM +0200, Martin Wilck wrote: > On Mon, 2024-07-15 at 18:41 -0400, Benjamin Marzinski wrote: > > On Fri, Jul 12, 2024 at 07:14:27PM +0200, Martin Wilck wrote: > > I wonder if we should wrap ibmp_mapinfo in a macro that grabs the > > function name of the calling function and passes it down to > > libmp_mapinfo__() otherwise there will be a lot of messages like this > > or > > > > condlog(2, "%s: map %s doesn't exist", __func__, map_id); > > > > that use "libmp_mapinfo__" as the function, which isn't super useful. > > Right. I think what I'll do is just hardcode "libmp_mapinfo" in these > messages instead of using __func__. Actually, I was thinking more of something like: int do_libmp_mapinfo(int flags, mapid_t id, mapinfo_t info, const char *func) { char idbuf[BLK_DEV_SIZE]; return libmp_mapinfo__(flags, id, info, libmp_map_identifier(flags, id, idbuf)); } #define libmp_mapinfo(flags, id, info) \ do_libmp_mapinfo(flags, id, info, __func__) So we would retain the caller info, but I don't feel that strongly about it. > > > > > > + condlog(2, "%s: map %s not found", > > > __func__, map_id); > > > + return DMP_NOT_FOUND; > > > + } else > > > + return DMP_ERR; > > > + } > > > + > > > > I mentioned tihs before, but the DM_MAP_BY_* identifiers only take up > > 3 > > bytes so __DM_MAP_BY_MASK can just be 3 or (1 << 2) - 1, unless you > > are > > reserving space for more identifiers which is fine but probably > > unnecessary, since libmultipath isn't a stable API. Or is there some > > other reason I'm missing. > > Is it important that this bit field is "dense"? Indeed I wanted to > leave some space, even though your're right that it most probably won't > be necessary. I thought it might be helpful during debugging, for > example. > > Perhaps I should have used the entire low byte for the map_by part. > That might even provide optimization opportunities for the compiler. > Unless you object, this is what I'll do. That's fine. Like I said, it's not a stable API. If it turns out we don't like it later, we can always change it then. -Ben
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 3abdc26..4e6b5b2 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,197 @@ 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) +{ + 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", __func__, 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", __func__, map_id); + return DMP_NOT_FOUND; + } else + return DMP_ERR; + } + + condlog(4, "%s: DM ioctl %d succeeded for %s", + __func__, ioctl_nr, map_id); + + if (!dm_task_get_info(dmt, &dmi)) { + condlog(2, "%s: dm_task_get_info() failed for %s ", __func__, map_id); + return DMP_ERR; + } else if(!dmi.exists) { + condlog(2, "%s: map %s doesn't exist", __func__, 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", __func__, map_id); + return DMP_NOT_FOUND; + } + if (!params) { + condlog(2, "%s: map %s has no targets", __func__, 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\"", + __func__, 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\"", __func__, map_id, info.name); + } + if (info.uuid) { + strlcpy(info.uuid, uuid, DM_UUID_LEN); + condlog(4, "%s: %s: uuid: \"%s\"", __func__, map_id, info.uuid); + } + + if (info.size) { + *info.size = length; + condlog(4, "%s: %s: size: %lld", __func__, 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", + __func__, 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\"", __func__, map_id, *info.target); + } + + if (info.status) { + *info.status = steal_ptr(tmp_status); + condlog(4, "%s: %s: status: \"%s\"", __func__, 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..725889b 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 << 3) - 1, + /* Fail if target type is not multipath */ + MAPINFO_MPATH_ONLY = (1 << 3), + /* Fail if target type is not "partition" (linear) */ + MAPINFO_PART_ONLY = (1 << 4), + __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. Will be ignored if @target is NULL */ + 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;
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 | 192 +++++++++++++++++++++++++++++- libmultipath/devmapper.h | 70 +++++++++++ libmultipath/libmultipath.version | 3 +- 3 files changed, 263 insertions(+), 2 deletions(-)