On Fri, Dec 19, 2025 at 03:40:59PM +0100, Martin Wilck wrote: > Paths are freed in check_removed_paths() quite deeply in the call > stack. Annotate this function and some of its callers, lest we forget > about it.
This looks fine. I just feel like I should point out that if you're doing this to satisfy my objection, I don't think that it's necessary to immediately remove these paths on every map reload. I was fine with check_remove_paths() being removed from sync_paths(). All I was envisioning was a check in ev_remove_path() to see if the path had been removed from mpp->pg, and removing it there if it had. It actually seems a little strange that we immeditately remove paths on reloads, but not when the whole map is deleted. But I get that it's designed to avoid freeing the paths in coalesce_paths(), and like I said, I'm fine with it, so: Reviewed-by: Benjamin Marzinski <[email protected]> > > Signed-off-by: Martin Wilck <[email protected]> > --- > libmultipath/structs_vec.c | 33 +++++++++++++++++++++++++++++++-- > multipathd/main.c | 10 ++++++---- > 2 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index 62ee450..9905ff2 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -563,6 +563,34 @@ static struct path *find_devt_in_pathgroups(const struct > multipath *mpp, > return NULL; > } > > +/* > + * check_removed_paths() > + * > + * This function removes paths from the pathvec and frees them if they have > + * been marked for removal (INIT_REMOVED, INIT_PARTIAL). > + * This is important because some callers (e.g. > uev_add_path->ev_remove_path()) > + * rely on the paths being actually gone when the call stack returns. > + * Be sure not to call it while these paths are still referenced elsewhere > + * (e.g. from coalesce_paths(), where curmp may still reference them). > + * > + * Most important call stacks in multipath-tools 0.13.0: > + * > + * checker_finished() > + * sync_mpp() > + * do_sync_mpp() > + * update_multipath_strings() > + * sync_paths() > + * check_removed_paths() > + * > + * [multiple callers including update_map(), ev_remove_path(), ...] > + * setup_multipath() > + * refresh_multipath() > + * update_multipath_strings() > + * sync_paths() > + * check_removed_paths() > + * > + * refresh_multipath() is also called from a couple of CLI handlers. > + */ > static void check_removed_paths(const struct multipath *mpp, vector pathvec) > { > struct path *pp; > @@ -583,6 +611,7 @@ static void check_removed_paths(const struct multipath > *mpp, vector pathvec) > } > } > > +/* This function may free paths. See check_removed_paths(). */ > void sync_paths(struct multipath *mpp, vector pathvec) > { > struct path *pp; > @@ -611,8 +640,8 @@ void sync_paths(struct multipath *mpp, vector pathvec) > pp->mpp = mpp; > } > > -int > -update_multipath_strings(struct multipath *mpp, vector pathvec) > +/* This function may free paths. See check_removed_paths(). */ > +int update_multipath_strings(struct multipath *mpp, vector pathvec) > { > struct pathgroup *pgp; > int i, r = DMP_ERR; > diff --git a/multipathd/main.c b/multipathd/main.c > index 7b522e8..697e269 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -503,6 +503,7 @@ remove_maps_and_stop_waiters(struct vectors *vecs) > remove_maps(vecs); > } > > +/* This function may free paths. See check_removed_paths(). */ > int refresh_multipath(struct vectors *vecs, struct multipath *mpp) > { > if (update_multipath_strings(mpp, vecs->pathvec) != DMP_OK) { > @@ -513,6 +514,7 @@ int refresh_multipath(struct vectors *vecs, struct > multipath *mpp) > return 0; > } > > +/* This function may free paths. See check_removed_paths(). */ > int setup_multipath(struct vectors *vecs, struct multipath *mpp) > { > if (refresh_multipath(vecs, mpp) != 0) > @@ -2519,8 +2521,8 @@ get_new_state(struct path *pp) > return newstate; > } > > -static int > -do_sync_mpp(struct vectors * vecs, struct multipath *mpp) > +/* This function may free paths. See check_removed_paths(). */ > +static int do_sync_mpp(struct vectors *vecs, struct multipath *mpp) > { > int i, ret; > struct path *pp; > @@ -2538,8 +2540,8 @@ do_sync_mpp(struct vectors * vecs, struct multipath > *mpp) > return ret; > } > > -static int > -sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks) > +/* This function may free paths. See check_removed_paths(). */ > +static int sync_mpp(struct vectors *vecs, struct multipath *mpp, unsigned > int ticks) > { > if (mpp->sync_tick) > mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks : > -- > 2.52.0
