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 <[email protected]>
> > ---
> > 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
>
