| Message ID | 20241115232256.627933-1-bmarzins@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | multipath-tools: Handle tableless DM devices | expand |
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? Thanks Martin
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. -Ben > > Thanks > Martin
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. Martin
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. Martin > > Martin >
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 > >
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
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