Re: [PATCH v2 19/49] libmultipath: add libmp_mapinfo()

[Benjamin Marzinski <bmarzins@redhat.com>]

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, &params) != 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