Re: [PATCH 0/6] multipath-tools: Handle tableless DM devices

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Nov 20, 2024 at 09:51:16AM +0100, Martin Wilck wrote:
> On Tue, 2024-11-19 at 14:13 -0500, Benjamin Marzinski wrote:
> > On Tue, Nov 19, 2024 at 05:40:19PM +0100, Martin Wilck wrote:
> > > On Tue, 2024-11-19 at 13:20 +0100, Martin Wilck wrote:
> > > > On Mon, 2024-11-18 at 16:14 -0500, Benjamin Marzinski wrote:
> > > > > On Mon, Nov 18, 2024 at 12:18:20PM +0100, Martin Wilck wrote:
> > > > > > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> > > > > > > 
> > > > > > > I'm not completely happy with the MAPINFO_ID_IF_FOUND flag.
> > > > > > > An
> > > > > > > alternative would be to run a second libmp_mapinfo() call
> > > > > > > without
> > > > > > > MAPINFO_MPATH_ONLY to grab the name, if the first failed
> > > > > > > with
> > > > > > > DMP_EMPTY.
> > > > > > > If people think that's a better way to solve this, I can
> > > > > > > rework
> > > > > > > those
> > > > > > > patches.
> > > > > > 
> > > > > > We could simply choose to always fill in this information if
> > > > > > the
> > > > > > the caller has requested it, without an additional input
> > > > > > flag.
> > > > > > It's
> > > > > > not
> > > > > > an expensive operation. Is there a reason not to do this?
> > > > > 
> > > > > Your comments in the code said that libmp_mapinfo() will not
> > > > > touch
> > > > > any
> > > > > of the output parameters if it doesn't succeed. I didn't audit
> > > > > the
> > > > > code,
> > > > > but I can certainly imagine a situation where you passed in
> > > > > pointers
> > > > > to
> > > > > some varaibles that already had values and you didn't want
> > > > > those
> > > > > values
> > > > > overwritten unless libmp_mapinfo() returned DMP_OK.
> > > > > 
> > > > > But I can go look and see if any callers would get messed up if
> > > > > name
> > > > > or
> > > > > uuid got set, even when the found device didn't match.
> > > > 
> > > > I am pretty sure that that shouldn't happen. The memory must be
> > > > allocated anyway, and callers can't ignore the return value. But
> > > > double-checking is a good idea, of course, and we should adapt
> > > > the
> > > > documentation.
> > > 
> > > I just looked through the code. Except for the unit tests, I only
> > > found
> > > one call where this matters:
> > > 
> > > dm_find_map_by_wwid() fills in the name if non-NULL. This is only
> > > used
> > > by cli_add_map(), where it doesn't cause an issue.
> > 
> > Yep. In my patch that adds MAPINFO_ID_IF_FOUND to
> > dm_find_map_by_wwid(),
> > I even use the returned name to print out a better error message.
> > Which,
> > BTW I noticed that mpath_get_map() also does, even though we haven't
> > been setting name on DMP_NO_MATCH returns, so this has been broken. 
> > 
> > I was also wondering if we could do the same thing with info.dmi,
> > since
> > that will also be set whenever we find a device, even if it doesn't
> > match the type of device we're looking for. The only problem with
> > that
> > is that update_multipath_table() would then set mpp->dmi even on
> > DMP_NO_MATCH or DMP_EMPTY. Now, if update_multipath_table() returns
> > one of those values, something is already very wrong. But even still,
> > I think we should not overwrite the existing dmi value. Of course
> > it's
> > simple to just create a tmp dmi variable, and only overwrite the
> > mpp->dmi on DMP_OK.
> 
> Absolutely, yes. Do we have use for the dmi information in cases where
> there's no match?

There's definitely information we could use. I'm not sure if it would
actually cut down on any current function calls. But, for instance, you
get the major:minor of the device that you did find. If the device is
empty, you can see if it has an inactive table or any openers. So I
certainly can see ways it would either cut down on calls or make the
code be able to act more intelligently.
 
> > I should note that just removing the MAPINFO_ID_IF_FOUND flag won't
> > remove most of the complexity of ("libmultipath: Add flag to always
> > return device ID when found"), since most of the code exists to make
> > sure that the other output variables aren't updated if allocating
> > space
> > for the status and target outputs fail. This means that no outputs
> > are
> > ever touched when we return DMP_ERR. 
> > 
> > If we don't care about that, and just say that the uuid, name, and
> > dmi
> > output parameters may be overwritten regardless of the return status
> > of libmp_mapinfo(), then we can set those values as soon as we know
> > them, and most of that patch becomes very small. I'm not sure if it
> > really matters one way or the other.
> > 
> 
> I like this idea.
> 
> Martin