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?
> 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
