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@xxxxxxxx>
> ---
> 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 = ¶ms,
> + .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
