Re: [PATCH 40/44] multipathd: implement add_map_without_path() with libmp_mapinfo()

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Jul 09, 2024 at 11:39:31PM +0200, Martin Wilck wrote:
> Also, change the return value to int, as this is more expressive and
> the returned struct multipath isn't used by the caller.
> 
> 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 | 69 ++++++++++++++++++++++++-----------------------
>  1 file changed, 36 insertions(+), 33 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 442a154..9e82188 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -707,48 +707,53 @@ 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);

This again doesn't set mpp->size, when it previously did. Am I missing
something about why this is unnecessary? I get that it can be set by
a path, but if it is already set, it can be used the verity that the
path belongs.

> +	if ((rc = libmp_mapinfo(DM_MAP_BY_NAME,
> +				(mapid_t) { .str = mpp->alias },
> +				(mapinfo_t) {
> +					.uuid = uuid,
> +					.tgt_type = TGT_MPATH,
> +					.dmi = &mpp->dmi,
> +					.target = &params,
> +					.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);
>  	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;
> -
> -	vector_set_slot(vecs->mpvec, mpp);
> +		return DMP_ERR;
> +	vector_set_slot(vecs->mpvec, steal_ptr(mpp));
>  
>  	if (update_map(mpp, vecs, 1) != 0) /* map removed */
> -		return NULL;
> +		return DMP_ERR;
>  
> -	return mpp;
> -out:
> -	remove_map(mpp, vecs->pathvec, vecs->mpvec);
> -	return NULL;
> +	return DMP_OK;
>  }
>  
>  static int
> @@ -862,14 +867,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) {
> @@ -907,10 +907,13 @@ 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))) {
> +	if ((rc = add_map_without_path(vecs, alias)) == DMP_OK) {

sync_map_state() is called with an uninitialized mpp, however the call is
unnecssary, since add_map_without_path() will call update_map() which will
call sync_map_state(). 

-Ben

>  		sync_map_state(mpp);
>  		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