Re: [PATCH v2 14/26] multipathd: add_map_without_path(): orphan paths instead of freeing them

[Benjamin Marzinski <bmarzins@redhat.com>]

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 <bmarzins@suse.com>
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  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").
 
> 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.

> > Perhaps the easiest way to
> > solve this is to make cleanup_multipath_and_paths() loop through all
> > the
> > path in mpp->pg and orphan them before calling free_multipath().
> 
> Like I said above, that would have no effect in the case of
> cancellation.
> 
> I can add pthread_cleanup_push()/pop() pair if you insist, but so far I
> don't see the benefit.

Sure. I drop my objection.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Regards
> Martin