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.
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.
-Ben
> Martin
>
>
>
>
>
> >
> > Martin
> >
