On Fri, Jul 12, 2024 at 07:14:50PM +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.
>
> Also remove the call to sync_map_state() in ev_add_map(), which is
> redundant because add_map_without_path() would have called update_map()
> and thus sync_map_state() already.
>
> 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 | 79 ++++++++++++++++++++++++-----------------------
> 1 file changed, 41 insertions(+), 38 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1e7a6ac..536974c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -707,51 +707,57 @@ 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);
> + if ((rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
> + (mapid_t) { .str = mpp->alias },
> + (mapinfo_t) {
> + .uuid = uuid,
> + .dmi = &mpp->dmi,
> + .size = &mpp->size,
> + .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) != DMP_OK) {
> - condlog(3, "%s: cannot obtain WWID", mpp->alias);
> - goto out;
> - }
> 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;
> + return DMP_ERR;
> + vector_set_slot(vecs->mpvec, steal_ptr(mpp));
>
> - vector_set_slot(vecs->mpvec, mpp);
> + /*
> + * We can pass mpp here, steal_ptr() has just nullified it.
Nitpick: "We *can't* pass mpp here, ..."
-Ben
> + * vector_set_slot() just set the last slot, use that.
> + */
> + if (update_map(VECTOR_LAST_SLOT(vecs->mpvec), vecs, 1) != 0) /* map removed */
> + return DMP_ERR;
>
> - if (update_map(mpp, vecs, 1) != 0) /* map removed */
> - return NULL;
> -
> - return mpp;
> -out:
> - remove_map(mpp, vecs->pathvec, vecs->mpvec);
> - return NULL;
> + return DMP_OK;
> }
>
> static int
> @@ -865,14 +871,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) {
> @@ -910,10 +911,12 @@ 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))) {
> - sync_map_state(mpp);
> + if ((rc = add_map_without_path(vecs, alias)) == DMP_OK) {
> 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
