Re: [PATCH v2 18/49] libmultipath: Use symbolic return values for dm_is_mpath()

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, Jul 12, 2024 at 07:14:26PM +0200, Martin Wilck wrote:
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c |  2 +-
>  libmultipath/devmapper.c            | 22 ++++++++--------------
>  libmultipath/devmapper.h            |  6 ++++++
>  multipath/main.c                    |  4 ++--
>  multipathd/dmevents.c               | 19 +++++++++++++------
>  multipathd/main.c                   |  2 +-
>  6 files changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
> index 178c2f5..6da0401 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -185,7 +185,7 @@ static int mpath_get_map(vector curmp, vector pathvec, int fd, char **palias,
>  
>  	condlog(3, "alias = %s", alias);
>  
> -	if (dm_map_present(alias) && dm_is_mpath(alias) != 1){
> +	if (dm_map_present(alias) && dm_is_mpath(alias) != DM_IS_MPATH_YES) {
>  		condlog(3, "%s: not a multipath device.", alias);
>  		goto out;
>  	}
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index a63154f..3abdc26 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -846,15 +846,9 @@ static int dm_type_match(const char *name, char *type)
>  		return DM_TYPE_NOMATCH;
>  }
>  
> -/*
> - * returns:
> - * 1  : is multipath device
> - * 0  : is not multipath device
> - * -1 : error
> - */
>  int dm_is_mpath(const char *name)
>  {
> -	int r = -1;
> +	int r = DM_IS_MPATH_ERR;
>  	struct dm_task __attribute__((cleanup(cleanup_dm_task))) *dmt = NULL;
>  	struct dm_info info;
>  	uint64_t start, length;
> @@ -876,7 +870,7 @@ int dm_is_mpath(const char *name)
>  	if (!dm_task_get_info(dmt, &info))
>  		goto out;
>  
> -	r = 0;
> +	r = DM_IS_MPATH_NO;
>  
>  	if (!info.exists)
>  		goto out;
> @@ -895,10 +889,10 @@ int dm_is_mpath(const char *name)
>  	if (!target_type || strcmp(target_type, TGT_MPATH) != 0)
>  		goto out;
>  
> -	r = 1;
> +	r = DM_IS_MPATH_YES;
>  out:
> -	if (r < 0)
> -		condlog(3, "%s: dm command failed in %s: %s", name, __FUNCTION__, strerror(errno));
> +	if (r == DM_IS_MPATH_ERR)
> +		condlog(3, "%s: dm command failed in %s: %s", name, __func__, strerror(errno));
>  	return r;
>  }
>  
> @@ -1039,7 +1033,7 @@ int _dm_flush_map (const char *mapname, int flags, int retries)
>  	unsigned long long mapsize;
>  	char *params = NULL;
>  
> -	if (dm_is_mpath(mapname) != 1)
> +	if (dm_is_mpath(mapname) != DM_IS_MPATH_YES)
>  		return DM_FLUSH_OK; /* nothing to do */
>  
>  	/* if the device currently has no partitions, do not
> @@ -1086,7 +1080,7 @@ int _dm_flush_map (const char *mapname, int flags, int retries)
>  			}
>  			condlog(4, "multipath map %s removed", mapname);
>  			return DM_FLUSH_OK;
> -		} else if (dm_is_mpath(mapname) != 1) {
> +		} else if (dm_is_mpath(mapname) != DM_IS_MPATH_YES) {
>  			condlog(4, "multipath map %s removed externally",
>  				mapname);
>  			return DM_FLUSH_OK; /* raced. someone else removed it */
> @@ -1316,7 +1310,7 @@ int dm_get_maps(vector mp)
>  	}
>  
>  	do {
> -		if (dm_is_mpath(names->name) != 1)
> +		if (dm_is_mpath(names->name) != DM_IS_MPATH_YES)
>  			goto next;
>  
>  		mpp = dm_get_multipath(names->name);
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index ff28575..9438c2d 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -46,6 +46,12 @@ int dm_map_present (const char *name);
>  int dm_map_present_by_uuid(const char *uuid);
>  int dm_get_map(const char *name, unsigned long long *size, char **outparams);
>  int dm_get_status(const char *name, char **outstatus);
> +
> +enum {
> +	DM_IS_MPATH_NO,
> +	DM_IS_MPATH_YES,
> +	DM_IS_MPATH_ERR,
> +};
>  int dm_is_mpath(const char *name);
>  
>  enum {
> diff --git a/multipath/main.c b/multipath/main.c
> index ce702e7..c82bc86 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -247,7 +247,7 @@ static int check_usable_paths(struct config *conf,
>  		goto out;
>  	}
>  
> -	if (dm_is_mpath(mapname) != 1) {
> +	if (dm_is_mpath(mapname) != DM_IS_MPATH_YES) {
>  		condlog(1, "%s is not a multipath map", devpath);
>  		goto free;
>  	}
> @@ -1080,7 +1080,7 @@ main (int argc, char *argv[])
>  		goto out;
>  	}
>  	if (cmd == CMD_FLUSH_ONE) {
> -		if (dm_is_mpath(dev) != 1) {
> +		if (dm_is_mpath(dev) != DM_IS_MPATH_YES) {
>  			condlog(0, "%s is not a multipath device", dev);
>  			r = RTVL_FAIL;
>  			goto out;
> diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
> index 3fbdc55..af1e12e 100644
> --- a/multipathd/dmevents.c
> +++ b/multipathd/dmevents.c
> @@ -168,9 +168,13 @@ static int dm_get_events(void)
>  	while (names->dev) {
>  		uint32_t event_nr;
>  
> -		/* Don't delete device if dm_is_mpath() fails without
> -		 * checking the device type */
> -		if (dm_is_mpath(names->name) == 0)
> +		/*
> +		 * Don't delete device if dm_is_mpath() fails without
> +		 * checking the device type.
> +		 * IOW, only delete devices from the event list for which
> +		 *  we positively know that they aren't multipath devices.
> +		 */
> +		if (dm_is_mpath(names->name) == DM_IS_MPATH_NO)
>  			goto next;
>  
>  		event_nr = dm_event_nr(names);
> @@ -206,9 +210,12 @@ int watch_dmevents(char *name)
>  	struct dev_event *dev_evt, *old_dev_evt;
>  	int i;
>  
> -	/* We know that this is a multipath device, so only fail if
> -	 * device-mapper tells us that we're wrong */
> -	if (dm_is_mpath(name) == 0) {
> +	/*
> +	 * We know that this is a multipath device, so only fail if
> +	 * device-mapper tells us that we're wrong
> +	 * IOW, don't fail for DM generic errors.
> +	 */
> +	if (dm_is_mpath(name) == DM_IS_MPATH_NO) {
>  		condlog(0, "%s: not a multipath device. can't watch events",
>  			name);
>  		return -1;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 58afe14..132bb2e 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -865,7 +865,7 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
>  	int reassign_maps;
>  	struct config *conf;
>  
> -	if (dm_is_mpath(alias) != 1) {
> +	if (dm_is_mpath(alias) != DM_IS_MPATH_YES) {
>  		condlog(4, "%s: not a multipath map", alias);
>  		return 0;
>  	}
> -- 
> 2.45.2