Re: Coding stye (was Re: [PATCH 1/2] libmutipath: handle blacklisted paths on map_discovery)

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Apr 30, 2025 at 09:55:43AM +0200, Martin Wilck wrote:
> On Tue, 2025-04-29 at 16:27 -0400, Benjamin Marzinski wrote:
> > On Tue, Apr 29, 2025 at 09:59:40PM +0200, Martin Wilck wrote:
> > > On Mon, 2025-04-28 at 17:45 -0400, Benjamin Marzinski wrote:
> > > > If the multipath configuration is changed to blacklist existing
> > > > devices,
> > > > and multipathd is reloaded but the blacklisted multipaths device
> > > > can't
> > > > be removed, multipathd was marking the paths as INIT_PARTIAL,
> > > > causing
> > > > them to stay in the multipath device, at least until the
> > > > partial_retrigger_delay timeout elapsed. Instead, mark them as
> > > > INIT_REMOVED and set mpp->need_reload, so the device is reloaded
> > > > and
> > > > the
> > > > paths are removed. To make sure the blacklisted paths are deleted
> > > > when
> > > > the multipath device is removed in coalesce_maps(), set their pp-
> > > > >mpp
> > > > to point to map before removing it.
> > > > 
> > > > Fixes d9c61332 ("multipathd: trigger uevents for blacklisted
> > > > paths in
> > > > reconfigure")
> > > 
> > > The patch looks good to me, but I only vaguely understand why the
> > > bug
> > > is introduced in d9c61332. Are you positive that before d9c61332,
> > > the
> > > hang didn't occur?
> > 
> > Well, I'm pretty sure these blacklisted paths stopped getting deleted
> > during reconfigure by d9c61332. Before d9c61332, blacklisted paths
> > were
> > immediately deleted in update_pathvec_from_dm().
> 
> Right, thanks.
> 
> I've taken the liberty to fix the typo ("libmutipath") and make some
> minor coding style adjustments in my queue branch. You can inspect the
> result there.
> 
> The coding style stuff is work in progress, I've added a clang-format
> based workflow on my tip branch.
> 
> I'd like to avoid major reformatting of past code while enforcing
> consistent formatting for present and future submissions which is more-
> or-less in line with what we used to do "sub-conciously" during the
> last years. clang-format has a lot of options [1]. I'm still struggling
> to find style settings that match ours [2].
> 
> Let me know if you dislike this, or if you have suggestions for
> improving the settings.

I'm fine with your changes and with using clang-format in general. And I
do prefer just formatting our new changes, like you mentioned in your
tip commit, but if someone wants to make the case for a mass reformat,
I'm willing to consider it. 

-Ben

> 
> Regards
> Martin
> 
> [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html
> [2] https://github.com/openSUSE/multipath-tools/blob/refs/heads/tip/.clang-format
> 
> >  After this change
> > 
> > @@ -193,7 +196,8 @@ static void update_pathvec_from_dm(vector
> > pathvec, struct multipath *mpp,
> >  		rc = pathinfo(pp, conf,
> >  		             
> > DI_SYSFS|DI_WWID|DI_BLACKLIST|DI_NOFALLBACK|pathinfo_flags);
> >  		pthread_cleanup_pop(1);
> > -		if (rc != PATHINFO_OK) {
> > +		if (rc == PATHINFO_FAILED ||
> > +		    (rc == PATHINFO_SKIPPED && !map_discovery)) {
> >  		        condlog(1, "%s: error %d in pathinfo,
> > discarding path",
> >  		                pp->dev, rc);
> >  		        vector_del_slot(pgp->paths, j--);
> > 
> > they started hanging around, so that uevents could be generated for
> > them
> > by trigger_paths_udev_change(). However, since coalesce_paths() will
> > usually clear pp->mpp, they won't get removed when orphan_paths() is
> > later called by remove_maps() to remove the old maps. I admit I
> > didn't
> > test if the paths got removed before that commit, but the commit
> > message
> > says that they did.
> > 
> > -Ben
> > 
> > > 
> > > > 
> > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > 
> > > Reviewed-by: Martin Wilck <mwilck@suse.com>