On Mon, 2026-01-05 at 18:17 -0500, Benjamin Marzinski wrote: > On Mon, Jan 05, 2026 at 04:15:37PM +0100, Martin Wilck wrote: > > On Fri, 2025-12-19 at 23:23 -0500, Benjamin Marzinski wrote: > > > On Fri, Dec 19, 2025 at 03:40:56PM +0100, Martin Wilck wrote: > > > > If add_map_without_path() fails to set up the multipath data > > > > structures, > > > > don't free the paths associated with the map as > > > > cleanup_multipath_and_paths() would; just orphan the paths. > > > > update_pathvec_from_dm() will handle the paths and see if they > > > > need to be removed. > > > > > > > > Suggested-by: Benjamin Marzinski <[email protected]> > > > > Signed-off-by: Martin Wilck <[email protected]> > > > > --- > > > > multipathd/main.c | 12 +++++++----- > > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > > index c03546b..7b522e8 100644 > > > > --- a/multipathd/main.c > > > > +++ b/multipathd/main.c > > > > @@ -771,8 +771,8 @@ fail: > > > > > > > > static int add_map_without_path (struct vectors *vecs, const > > > > char > > > > *alias) > > > > { > > > > - struct multipath > > > > __attribute__((cleanup(cleanup_multipath_and_paths))) > > > > - *mpp = alloc_multipath(); > > > > + struct multipath > > > > __attribute__((cleanup(cleanup_multipath))) *mpp = > > > > + alloc_multipath(); > > > > char __attribute__((cleanup(cleanup_charp))) *params = > > > > NULL; > > > > char __attribute__((cleanup(cleanup_charp))) *status = > > > > NULL; > > > > struct config *conf; > > > > @@ -802,11 +802,13 @@ static int add_map_without_path (struct > > > > vectors *vecs, const char *alias) > > > > mpp->mpe = find_mpe(conf->mptable, mpp->wwid); > > > > put_multipath_config(conf); > > > > > > > > - if ((rc = update_multipath_table__(mpp, vecs->pathvec, > > > > 0, > > > > params, status)) != DMP_OK) > > > > + if (update_multipath_table__(mpp, vecs->pathvec, 0, > > > > params, status) != DMP_OK || > > > > + !vector_alloc_slot(vecs->mpvec)) { > > > > + orphan_paths(vecs->pathvec, mpp, "failed to > > > > add > > > > map"); > > > > return DMP_ERR; > > > > > > I'm not sure we should just call orphan_paths() after we call > > > update_multipath_table__(). If we hit a cancellation point in the > > > middle > > > of update_multipath_table__(), we want to fully orphan any paths > > > that > > > got added to the device before we cancelled. > > > > Sorry, I don't understand. > > > > The __attribute__((cleanup())) functions don't protect against > > thread > > cancellation. If we want to achieve what you describe, we must add > > a > > pthread_cleanup_push()/pop() pair around the > > update_multipath_table__() > > call. > > I'm pretty sure they do run during cancellation after I added > ca957f2a > ("multipath-tools: Makefile.inc: compile with -fexceptions").
Right, I forgot about that, sorry. Re-reading the docs, I'd like to double-check if this is really guaranteed. I think we should get rid of cancellation for our long-running threads. Later ;-) > > But would that buy us anything? update_multipath_table__() only > > manipulates multipathd's internal data structures, which, if a > > cancellation happens, will almost immediately all be freed anyway. > > What > > harm would be done if these paths weren't orphaned in such a case? > > Yes, > > the some paths might now link to a non-existing struct multipath. > > I'd > > see your point if we accessed pp->mpp in the path-freeing code > > path, > > but we don't do this any more. > > We check whether pp->mpp is set in lots of places in the code to > decide > what to do with the paths. But you are correct that we don't ever > call > add_map_without_path() from a thread that can be cancelled and have > multipathd continue running, and we aren't likely to do so. Yes, this was my thinking. I didn't write it explicitly. The function is only called from the uevent_dispatch thread, which is cancelled if and only if multipathd exits. However, you are right that this isn't clean. What actually need here is remove_map(), which does exactly what we want, AFAICS. But I'll give the struct-multipath-freeing code another review. Regards Martin
