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

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

[Benjamin Marzinski]

This is another stab at handling tableless multipath devices. The first
two patches are reworkings of two patches from my last patchset based
on feedback from Martin. The rest are new.

libmp_mapinfo now has a new return value, DMP_EMPTY, for devices that
don't have any table. The creation failure path is fixed to check for
this value before removing a device after a failed create, instead of
just removing any existing DM device with the same name. 

libmp_mapinfo now also accepts a new flag, MAPINFO_ID_IF_FOUND, which
causes it to populate info.name and info.uuid if requested, whenever a
device is found, even if it returns DMP_NO_MATCH or DMP_EMPTY.
dm_find_map_by_wwid() uses this new flag, as well as MAPINFO_MPATH_ONLY,
to only return DM_OK for valid multipath devices, but to return the
alias of any device that matches the uuid, even if it's not a valid
multipath device.

domap() has been updated to use dm_find_map_by_wwid() to check for any
DM device that matches the WWID of the device to be created. This lets
it handle tableless devices that share the WWID of the device to be
created but not its alias. Previously, multipath would fail attempting
to create these devices. Now it correctly renames and reloads them when
running multipath, starting or reloading multipathd, or running
"multipathd add map".

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.

Benjamin Marzinski (5):
  libmultipath: signal device with no table in libmp_mapinfo
  libmultipath: fix removing device after failed creation
  libmultipath: Add flag to always return device ID when found
  libmultipath: check table type in dm_find_map_by_wwid
  libmultipath: handle a create corner case for empty devices

Martin Wilck (1):
  multipath-tools tests: fix mapinfo tests

 libmultipath/configure.c  |  21 +++++-
 libmultipath/devmapper.c  |  48 ++++++++----
 libmultipath/devmapper.h  |  13 +++-
 multipathd/cli_handlers.c |   8 +-
 tests/mapinfo.c           | 149 ++++++++++++++++++++++++++++++++++++--
 5 files changed, 216 insertions(+), 23 deletions(-)

-- 
2.46.2

[PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Benjamin Marzinski]

if libmp_mapinfo() is run on a device that has no active table,
it will now return with a new exit code, DMP_EMPTY, to signal this.
Currently all code paths treat this identically to DMP_NOT_FOUND.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 6 ++++--
 libmultipath/devmapper.h | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 9714270d..b718dccf 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -86,6 +86,7 @@ const char *dmp_errstr(int rc)
 		[DMP_OK] = "success",
 		[DMP_NOT_FOUND] = "not found",
 		[DMP_NO_MATCH] = "target type mismatch",
+		[DMP_EMPTY] = "no target",
 		[DMP_LAST__] = "**invalid**",
 	};
 	if (rc < 0 || rc > DMP_LAST__)
@@ -747,9 +748,9 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 			condlog(lvl, "%s: map %s has multiple targets", fname__, map_id);
 			return DMP_NO_MATCH;
 		}
-		if (!params) {
+		if (!params || !target_type) {
 			condlog(lvl, "%s: map %s has no targets", fname__, map_id);
-			return DMP_NOT_FOUND;
+			return DMP_EMPTY;
 		}
 		if (flags & MAPINFO_TGT_TYPE__) {
 			const char *tgt_type = flags & MAPINFO_MPATH_ONLY ? TGT_MPATH : TGT_PART;
@@ -873,6 +874,7 @@ int dm_is_mpath(const char *name)
 		return DM_IS_MPATH_YES;
 	case DMP_NOT_FOUND:
 	case DMP_NO_MATCH:
+	case DMP_EMPTY:
 		return DM_IS_MPATH_NO;
 	case DMP_ERR:
 	default:
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 6b3bbad9..23926e3f 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -35,6 +35,7 @@ enum {
 	DMP_OK,
 	DMP_NOT_FOUND,
 	DMP_NO_MATCH,
+	DMP_EMPTY,
 	DMP_LAST__,
 };
 
@@ -105,7 +106,11 @@ typedef struct libmp_map_info {
  * @returns:
  *     DMP_OK if successful.
  *     DMP_NOT_FOUND if the map wasn't found, or has no or multiple targets.
- *     DMP_NO_MATCH if the map didn't match @tgt_type (see above).
+ *     DMP_NO_MATCH if the map didn't match @tgt_type (see above) or didn't
+ *                  have a multipath uuid prefix.
+ *     DMP_EMPTY if the map has no table. Note. The check for matching uuid
+ *               prefix will happen first, but the check for matching
+ *               tgt_type will happen afterwards.
  *     DMP_ERR if some other error occurred.
  *
  * This function obtains the requested information for the device-mapper map
-- 
2.46.2

Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Martin Wilck]

On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> if libmp_mapinfo() is run on a device that has no active table,
> it will now return with a new exit code, DMP_EMPTY, to signal this.
> Currently all code paths treat this identically to DMP_NOT_FOUND.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

I just looked through the code again. I think with this change, we need
to modify dm_flush_map__() and do_foreach_partmaps(). They should
remove / act on empty maps. What do you think?

Also, we can skip the call to is_mpath_part_uuid() in
do_foreach_partmaps() after my "make MAPINFO_CHECK_UUID work with
partitions" patch. I can submit a patch myself, but as you're at
already, maybe you want to do that?

Martin

Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Benjamin Marzinski]

On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote:
> On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> > if libmp_mapinfo() is run on a device that has no active table,
> > it will now return with a new exit code, DMP_EMPTY, to signal this.
> > Currently all code paths treat this identically to DMP_NOT_FOUND.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> I just looked through the code again. I think with this change, we need
> to modify dm_flush_map__() and do_foreach_partmaps(). They should
> remove / act on empty maps. What do you think?

I'm not sure that we need to add extra code to handle the possiblity
that an empty device could appear at any time, since like I said,
this is a corner case that I've never actually seen in the wild. So if
a device was previously a valid multipath device, I don't think we need
to worry about the possibility that it suddenly became empty. 

But I can see the value in running something like

# multipath -F

and having it clean up any empty multipath devices. As for
do_foreach_partmaps(), are you thinking about cleaning up empty
partition devices or non-empty partition devices on top of empty
multipath devices?

Non-empty partition devices on top of empty multipath devices would
imply that a multipath device was valid at one point, and then became
empty, which I don't see an easy way of happening.

The problem with empty partition devices is that partition devices are
created by kpartx completely asynchronously to us. That empty partition
device could be in the process of being created.

So I'm not against checking for DMP_EMPTY in dm_flush_map__() and
removing the empty device if its opencount is 0. But I'd rather not
try messing with partmaps. Do you have a specific case you are worried
about?

> Also, we can skip the call to is_mpath_part_uuid() in
> do_foreach_partmaps() after my "make MAPINFO_CHECK_UUID work with
> partitions" patch. I can submit a patch myself, but as you're at
> already, maybe you want to do that?

Yep. I can do that.

-Ben

> Martin

Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Martin Wilck]

On Tue, 2024-11-19 at 15:33 -0500, Benjamin Marzinski wrote:
> On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote:
> > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> > > if libmp_mapinfo() is run on a device that has no active table,
> > > it will now return with a new exit code, DMP_EMPTY, to signal
> > > this.
> > > Currently all code paths treat this identically to DMP_NOT_FOUND.
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > 
> > I just looked through the code again. I think with this change, we
> > need
> > to modify dm_flush_map__() and do_foreach_partmaps(). They should
> > remove / act on empty maps. What do you think?
> 
> I'm not sure that we need to add extra code to handle the possiblity
> that an empty device could appear at any time, since like I said,
> this is a corner case that I've never actually seen in the wild. So
> if
> a device was previously a valid multipath device, I don't think we
> need
> to worry about the possibility that it suddenly became empty. 
> 
> But I can see the value in running something like
> 
> # multipath -F
> 
> and having it clean up any empty multipath devices. As for
> do_foreach_partmaps(), are you thinking about cleaning up empty
> partition devices or non-empty partition devices on top of empty
> multipath devices?
> 
> Non-empty partition devices on top of empty multipath devices would
> imply that a multipath device was valid at one point, and then became
> empty, which I don't see an easy way of happening.
> 
> The problem with empty partition devices is that partition devices
> are
> created by kpartx completely asynchronously to us. That empty
> partition
> device could be in the process of being created.

Right, but multipathd is in the process of deleting the map. If there's
actually a race and kpartx finishes creating the partition map,
multipathd will fail to remove the multipath map. The likely outcome
will be a multipath map with just one partition device. If multipathd
comes first, kpartx will fail, but there's a good chance that
multipathd will succeed in flushing the multipath map, so we'll end up
with a consistent state.

If kpartx had run a little earlier and had finished setting up the map,
multipathd would have removed it, and if kpartx had run a little later,
it would have failed because of a missing target.

> So I'm not against checking for DMP_EMPTY in dm_flush_map__() and
> removing the empty device if its opencount is 0. But I'd rather not
> try messing with partmaps. Do you have a specific case you are
> worried
> about?

From the argument above, I'd say that flushing empty partition maps is
the right thing to do, for consistency reasons.

But now I may be overlooking something.

Martin

Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Benjamin Marzinski]

On Wed, Nov 20, 2024 at 09:49:17AM +0100, Martin Wilck wrote:
> On Tue, 2024-11-19 at 15:33 -0500, Benjamin Marzinski wrote:
> > On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote:
> > > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> > > > if libmp_mapinfo() is run on a device that has no active table,
> > > > it will now return with a new exit code, DMP_EMPTY, to signal
> > > > this.
> > > > Currently all code paths treat this identically to DMP_NOT_FOUND.
> > > > 
> > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > 
> > > I just looked through the code again. I think with this change, we
> > > need
> > > to modify dm_flush_map__() and do_foreach_partmaps(). They should
> > > remove / act on empty maps. What do you think?
> > 
> > I'm not sure that we need to add extra code to handle the possiblity
> > that an empty device could appear at any time, since like I said,
> > this is a corner case that I've never actually seen in the wild. So
> > if
> > a device was previously a valid multipath device, I don't think we
> > need
> > to worry about the possibility that it suddenly became empty. 
> > 
> > But I can see the value in running something like
> > 
> > # multipath -F
> > 
> > and having it clean up any empty multipath devices. As for
> > do_foreach_partmaps(), are you thinking about cleaning up empty
> > partition devices or non-empty partition devices on top of empty
> > multipath devices?
> > 
> > Non-empty partition devices on top of empty multipath devices would
> > imply that a multipath device was valid at one point, and then became
> > empty, which I don't see an easy way of happening.
> > 
> > The problem with empty partition devices is that partition devices
> > are
> > created by kpartx completely asynchronously to us. That empty
> > partition
> > device could be in the process of being created.
> 
> Right, but multipathd is in the process of deleting the map. If there's
> actually a race and kpartx finishes creating the partition map,
> multipathd will fail to remove the multipath map. The likely outcome
> will be a multipath map with just one partition device. If multipathd
> comes first, kpartx will fail, but there's a good chance that
> multipathd will succeed in flushing the multipath map, so we'll end up
> with a consistent state.
> 
> If kpartx had run a little earlier and had finished setting up the map,
> multipathd would have removed it, and if kpartx had run a little later,
> it would have failed because of a missing target.
> 

If we make do_foreach_partmaps() remove empty partition maps, it has
keep the is_mpath_part_uuid() call. Otherwise we would remove any empty
partition device, including ones that aren't for this multipath device,
which breaks your argument above.

> > So I'm not against checking for DMP_EMPTY in dm_flush_map__() and
> > removing the empty device if its opencount is 0. But I'd rather not
> > try messing with partmaps. Do you have a specific case you are
> > worried
> > about?
> 
> >From the argument above, I'd say that flushing empty partition maps is
> the right thing to do, for consistency reasons.
> 
> But now I may be overlooking something.

How would you feel about adding a parameter to do_foreach_partmaps() to
say whether or not it should remove empty partmaps (or possibly just
checking if the partmap_func is remove_partmaps).  Your argument makes
sense when you are removing a device. But what about functions like
dm_cancel_remove_partmaps() and dm_rename_partmaps()? I'm not sure that
these should automatically empty (a possibly being created) partition
devices.

-Ben
 
> Martin

Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Martin Wilck]

On Wed, 2024-11-20 at 16:59 -0500, Benjamin Marzinski wrote:
> On Wed, Nov 20, 2024 at 09:49:17AM +0100, Martin Wilck wrote:
> > On Tue, 2024-11-19 at 15:33 -0500, Benjamin Marzinski wrote:
> > > On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote:
> > > > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> > > > > if libmp_mapinfo() is run on a device that has no active
> > > > > table,
> > > > > it will now return with a new exit code, DMP_EMPTY, to signal
> > > > > this.
> > > > > Currently all code paths treat this identically to
> > > > > DMP_NOT_FOUND.
> > > > > 
> > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > > 
> > > > I just looked through the code again. I think with this change,
> > > > we
> > > > need
> > > > to modify dm_flush_map__() and do_foreach_partmaps(). They
> > > > should
> > > > remove / act on empty maps. What do you think?
> > > 
> > > I'm not sure that we need to add extra code to handle the
> > > possiblity
> > > that an empty device could appear at any time, since like I said,
> > > this is a corner case that I've never actually seen in the wild.
> > > So
> > > if
> > > a device was previously a valid multipath device, I don't think
> > > we
> > > need
> > > to worry about the possibility that it suddenly became empty. 
> > > 
> > > But I can see the value in running something like
> > > 
> > > # multipath -F
> > > 
> > > and having it clean up any empty multipath devices. As for
> > > do_foreach_partmaps(), are you thinking about cleaning up empty
> > > partition devices or non-empty partition devices on top of empty
> > > multipath devices?
> > > 
> > > Non-empty partition devices on top of empty multipath devices
> > > would
> > > imply that a multipath device was valid at one point, and then
> > > became
> > > empty, which I don't see an easy way of happening.
> > > 
> > > The problem with empty partition devices is that partition
> > > devices
> > > are
> > > created by kpartx completely asynchronously to us. That empty
> > > partition
> > > device could be in the process of being created.
> > 
> > Right, but multipathd is in the process of deleting the map. If
> > there's
> > actually a race and kpartx finishes creating the partition map,
> > multipathd will fail to remove the multipath map. The likely
> > outcome
> > will be a multipath map with just one partition device. If
> > multipathd
> > comes first, kpartx will fail, but there's a good chance that
> > multipathd will succeed in flushing the multipath map, so we'll end
> > up
> > with a consistent state.
> > 
> > If kpartx had run a little earlier and had finished setting up the
> > map,
> > multipathd would have removed it, and if kpartx had run a little
> > later,
> > it would have failed because of a missing target.
> > 
> 
> If we make do_foreach_partmaps() remove empty partition maps, it has
> keep the is_mpath_part_uuid() call. Otherwise we would remove any
> empty
> partition device, including ones that aren't for this multipath
> device,
> which breaks your argument above.

Yes, my bad. There's of course a difference between "is this a
partition mapping" and "is this a partition mapped to the current mpath
device". Thanks for pointing it out.

> > > So I'm not against checking for DMP_EMPTY in dm_flush_map__() and
> > > removing the empty device if its opencount is 0. But I'd rather
> > > not
> > > try messing with partmaps. Do you have a specific case you are
> > > worried
> > > about?
> > 
> > > From the argument above, I'd say that flushing empty partition
> > > maps is
> > the right thing to do, for consistency reasons.
> > 
> > But now I may be overlooking something.
> 
> How would you feel about adding a parameter to do_foreach_partmaps()
> to
> say whether or not it should remove empty partmaps (or possibly just
> checking if the partmap_func is remove_partmaps).  Your argument
> makes
> sense when you are removing a device. But what about functions like
> dm_cancel_remove_partmaps() and dm_rename_partmaps()? I'm not sure
> that
> these should automatically empty (a possibly being created) partition
> devices.

We're moving far into corner case land here :-) 

I think we should keep it as simple as possible. For me, that means
that an empty partition map (with UUID matching the current mpath map)
should be treated like any other partition map with matching UUID, no
matter what the current operation is.

There will be some situations where the outcome will be suboptimal. By
keeping it simple, we'll at least be able to understand the outcome.

Martin

Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Benjamin Marzinski]

On Thu, Nov 21, 2024 at 09:57:28AM +0100, Martin Wilck wrote:
> On Wed, 2024-11-20 at 16:59 -0500, Benjamin Marzinski wrote:
> > 
> > How would you feel about adding a parameter to do_foreach_partmaps()
> > to
> > say whether or not it should remove empty partmaps (or possibly just
> > checking if the partmap_func is remove_partmaps).  Your argument
> > makes
> > sense when you are removing a device. But what about functions like
> > dm_cancel_remove_partmaps() and dm_rename_partmaps()? I'm not sure
> > that
> > these should automatically empty (a possibly being created) partition
> > devices.
> 
> We're moving far into corner case land here :-) 
> 
> I think we should keep it as simple as possible. For me, that means
> that an empty partition map (with UUID matching the current mpath map)
> should be treated like any other partition map with matching UUID, no
> matter what the current operation is.
> 
> There will be some situations where the outcome will be suboptimal. By
> keeping it simple, we'll at least be able to understand the outcome.

That's reasonable. I misunderstood your suggestion as asking to always
remove empty partition devices in do_foreach_partmaps(). We'd need to
make sure all the partmap_func()s work with empty devices. But that
shouldn't be too bad.

-Ben 
 
> Martin

[PATCH 2/6] multipath-tools tests: fix mapinfo tests

[Benjamin Marzinski]

From: Martin Wilck <mwilck@suse.com>

Add tests for the check for empty table introduced in ("libmultipath:
signal device with no table in libmp_mapinfo").

Signed-off-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/mapinfo.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/tests/mapinfo.c b/tests/mapinfo.c
index 4362cdb0..ee487989 100644
--- a/tests/mapinfo.c
+++ b/tests/mapinfo.c
@@ -47,11 +47,13 @@ static const struct dm_info __attribute__((unused)) MPATH_DMI_02 = {
 	.exists = 1,
 	.live_table = 0,
 	.open_count = 1,
-	.target_count = 1,
+	.target_count = 0,
 	.major = 254,
 	.minor = 123,
 };
 
+#define TGT_ANY "any"
+
 static const char MPATH_NAME_01[] = "mpathx";
 static const char MPATH_UUID_01[] = "mpath-3600a098038302d414b2b4d4453474f62";
 static const char MPATH_TARGET_01[] =
@@ -858,7 +860,7 @@ static void test_mapinfo_good_size(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
-	mock_dm_get_next_target(12345, NULL, MPATH_TARGET_01, NULL);
+	mock_dm_get_next_target(12345, TGT_ANY, MPATH_TARGET_01, NULL);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
 			   (mapid_t) { .str = "foo", },
 			   (mapinfo_t) { .size = &size });
@@ -889,13 +891,13 @@ static void test_mapinfo_bad_next_target_02(void **state)
 
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
-	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_02);
 	/* no targets */
-	mock_dm_get_next_target(12345, NULL, NULL, NULL);
+	mock_dm_get_next_target(0, NULL, NULL, NULL);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
 			   (mapid_t) { .str = "foo", },
 			   (mapinfo_t) { .size = &size });
-	assert_int_equal(rc, DMP_NOT_FOUND);
+	assert_int_equal(rc, DMP_EMPTY);
 }
 
 /* libmp_mapinfo needs to do a DM_DEVICE_STATUS ioctl */
@@ -1049,6 +1051,39 @@ static void test_mapinfo_good_target_type_04(void **state)
 	assert_true(!strcmp(uuid, MPATH_UUID_01));
 }
 
+static void test_mapinfo_no_table_01(void **state)
+{
+	int rc;
+	struct dm_info dmi = { .suspended = 0 };
+
+	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
+	WRAP_DM_TASK_GET_INFO(1);
+	/* DMI with no live table, MAPINFO_CHECK_UUID not set */
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_02);
+	mock_dm_get_next_target(0, NULL, NULL, NULL);
+	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
+			   (mapid_t) { .str = "foo", },
+			   (mapinfo_t) { .dmi = &dmi });
+	assert_int_equal(rc, DMP_EMPTY);
+}
+
+static void test_mapinfo_no_table_02(void **state)
+{
+	int rc;
+	struct dm_info dmi = { .suspended = 0 };
+
+	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
+	WRAP_DM_TASK_GET_INFO(1);
+	/* DMI with no live table, MAPINFO_CHECK_UUID set */
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_02);
+	mock_dm_get_next_target(0, NULL, NULL, NULL);
+	will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID | MAPINFO_MPATH_ONLY,
+			   (mapid_t) { .str = "foo", },
+			   (mapinfo_t) { .dmi = &dmi });
+	assert_int_equal(rc, DMP_EMPTY);
+}
+
 static void test_mapinfo_good_status_01(void **state)
 {
 	int rc;
@@ -1142,7 +1177,7 @@ static void test_mapinfo_bad_task_run_11(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_TABLE, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
-	mock_dm_get_next_target(12345, NULL, MPATH_TARGET_01, NULL);
+	mock_dm_get_next_target(12345, TGT_ANY, MPATH_TARGET_01, NULL);
 	will_return(__wrap_strdup, 1);
 	/* error in 2nd dm_task_run */
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 0, EINVAL);
@@ -1384,6 +1419,8 @@ static int test_mapinfo(void)
 		cmocka_unit_test(test_mapinfo_good_target_type_02),
 		cmocka_unit_test(test_mapinfo_good_target_type_03),
 		cmocka_unit_test(test_mapinfo_good_target_type_04),
+		cmocka_unit_test(test_mapinfo_no_table_01),
+		cmocka_unit_test(test_mapinfo_no_table_02),
 		cmocka_unit_test(test_mapinfo_good_status_01),
 		cmocka_unit_test(test_mapinfo_bad_get_name_01),
 		cmocka_unit_test(test_mapinfo_bad_get_uuid_01),
-- 
2.46.2

[PATCH 3/6] libmultipath: fix removing device after failed creation

[Benjamin Marzinski]

dm_flush_nap_nosync() only removes a device if it has a multipath table.
On failed removes, there is no table, so this function does nothing.
Instead, if libmp_mapinfo() returns DMP_EMPTY, then there is an empty map,
and it is removed using dm_device_remove().

Also, since there are no longer any callers of dm_flush_map_nosync(),
remove it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 7 +++++--
 libmultipath/devmapper.h | 1 -
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index b718dccf..842e7c80 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -557,9 +557,12 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 		 * Failing the second part leaves an empty map. Clean it up.
 		 */
 		err = errno;
-		if (dm_map_present(mpp->alias)) {
+		if (libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY |
+				  MAPINFO_CHECK_UUID,
+				(mapid_t) { .str = mpp->alias },
+				(mapinfo_t) { .uuid = NULL }) == DMP_EMPTY) {
 			condlog(3, "%s: failed to load map (a path might be in use)", mpp->alias);
-			dm_flush_map_nosync(mpp->alias);
+			dm_device_remove(mpp->alias, 0);
 		}
 		if (err != EROFS) {
 			condlog(3, "%s: failed to load map, error %d",
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 23926e3f..0bd2b907 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -174,7 +174,6 @@ enum {
 
 int dm_flush_map__ (const char *mapname, int flags, int retries);
 #define dm_flush_map(mapname) dm_flush_map__(mapname, DMFL_NEED_SYNC, 0)
-#define dm_flush_map_nosync(mapname) dm_flush_map__(mapname, DMFL_NONE, 0)
 #define dm_suspend_and_flush_map(mapname, retries) \
 	dm_flush_map__(mapname, DMFL_NEED_SYNC|DMFL_SUSPEND, retries)
 int dm_flush_map_nopaths(const char * mapname, int deferred_remove);
-- 
2.46.2

Re: [PATCH 3/6] libmultipath: fix removing device after failed creation

[Martin Wilck]

On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> dm_flush_nap_nosync() only removes a device if it has a multipath
> table.
> On failed removes, there is no table, so this function does nothing.
> Instead, if libmp_mapinfo() returns DMP_EMPTY, then there is an empty
> map,
> and it is removed using dm_device_remove().
> 
> Also, since there are no longer any callers of dm_flush_map_nosync(),
> remove it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/devmapper.c | 7 +++++--
>  libmultipath/devmapper.h | 1 -
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index b718dccf..842e7c80 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -557,9 +557,12 @@ int dm_addmap_create (struct multipath *mpp,
> char * params)
>  		 * Failing the second part leaves an empty map.
> Clean it up.
>  		 */
>  		err = errno;
> -		if (dm_map_present(mpp->alias)) {
> +		if (libmp_mapinfo(DM_MAP_BY_NAME |
> MAPINFO_MPATH_ONLY |
> +				  MAPINFO_CHECK_UUID,
> +				(mapid_t) { .str = mpp->alias },
> +				(mapinfo_t) { .uuid = NULL }) ==
> DMP_EMPTY) {
>  			condlog(3, "%s: failed to load map (a path
> might be in use)", mpp->alias);

This error message seems wrong for emtpy tables.

Martin

Re: [PATCH 3/6] libmultipath: fix removing device after failed creation

[Benjamin Marzinski]

On Mon, Nov 18, 2024 at 12:21:01PM +0100, Martin Wilck wrote:
> On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote:
> > dm_flush_nap_nosync() only removes a device if it has a multipath
> > table.
> > On failed removes, there is no table, so this function does nothing.
> > Instead, if libmp_mapinfo() returns DMP_EMPTY, then there is an empty
> > map,
> > and it is removed using dm_device_remove().
> > 
> > Also, since there are no longer any callers of dm_flush_map_nosync(),
> > remove it.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/devmapper.c | 7 +++++--
> >  libmultipath/devmapper.h | 1 -
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index b718dccf..842e7c80 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -557,9 +557,12 @@ int dm_addmap_create (struct multipath *mpp,
> > char * params)
> >  		 * Failing the second part leaves an empty map.
> > Clean it up.
> >  		 */
> >  		err = errno;
> > -		if (dm_map_present(mpp->alias)) {
> > +		if (libmp_mapinfo(DM_MAP_BY_NAME |
> > MAPINFO_MPATH_ONLY |
> > +				  MAPINFO_CHECK_UUID,
> > +				(mapid_t) { .str = mpp->alias },
> > +				(mapinfo_t) { .uuid = NULL }) ==
> > DMP_EMPTY) {
> >  			condlog(3, "%s: failed to load map (a path
> > might be in use)", mpp->alias);
> 
> This error message seems wrong for emtpy tables.

This code exists because if you fail to fully create a DM device, you
are (or were, I'm not sure if this issue still exists) sometimes left
with an empty device that needs cleaning up. Previously multipath would
just delete any device that existed with the name of the device you were
trying to create. The new code only deletes empty devices, to avoid
accidentally deleting a valid device that got set up at the same time as
you were doing the create. But these empty devices should only exist if
you failed to load the table for the new device, perhaps because a path
was in use.

So the message still makes sense here, as much as it ever did.

-Ben

> 
> Martin
> 

[PATCH 4/6] libmultipath: Add flag to always return device ID when found

[Benjamin Marzinski]

Normally, libmp_mapinfo will only return the name and uuid of a device
if the device matches all the check requirements. Add a flag,
MAPINFO_ID_IF_FOUND, to make it return the name and uuid as long as
the result isn't DMP_ERR or DMP_NOT_FOUND. This will be used by
a future patch.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c |  33 +++++++++----
 libmultipath/devmapper.h |   5 ++
 tests/mapinfo.c          | 100 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 8 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 842e7c80..42dcbc77 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -663,6 +663,19 @@ static int libmp_set_map_identifier(int flags, mapid_t id, struct dm_task *dmt)
 	}
 }
 
+static void write_ids(char *dst_name, const char *src_name,
+		      char *dst_uuid, const char *src_uuid, const char *map_id)
+{
+	if (dst_name) {
+		strlcpy(dst_name, src_name, WWID_SIZE);
+		condlog(4, "libmp_mapinfo: %s: name: \"%s\"", map_id, dst_name);
+	}
+	if (dst_uuid) {
+		strlcpy(dst_uuid, src_uuid, DM_UUID_LEN);
+		condlog(4, "libmp_mapinfo: %s: uuid: \"%s\"", map_id, dst_uuid);
+	}
+}
+
 static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *map_id)
 {
 	/* avoid libmp_mapinfo__ in log messages */
@@ -740,6 +753,8 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 	    ((flags & MAPINFO_PART_ONLY && !is_mpath_part_uuid(uuid, NULL)) ||
 	     !is_mpath_uuid(uuid))) {
 		condlog(4, "%s: UUID mismatch: %s", fname__, uuid);
+		if (flags & MAPINFO_ID_IF_FOUND)
+			write_ids(info.name, name, info.uuid, uuid, map_id);
 		return DMP_NO_MATCH;
 	}
 
@@ -749,10 +764,16 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 		if (dm_get_next_target(dmt, NULL, &start, &length,
 				       &target_type, &params) != NULL) {
 			condlog(lvl, "%s: map %s has multiple targets", fname__, map_id);
+			if (flags & MAPINFO_ID_IF_FOUND)
+				write_ids(info.name, name, info.uuid, uuid,
+					  map_id);
 			return DMP_NO_MATCH;
 		}
 		if (!params || !target_type) {
 			condlog(lvl, "%s: map %s has no targets", fname__, map_id);
+			if (flags & MAPINFO_ID_IF_FOUND)
+				write_ids(info.name, name, info.uuid, uuid,
+					  map_id);
 			return DMP_EMPTY;
 		}
 		if (flags & MAPINFO_TGT_TYPE__) {
@@ -761,6 +782,9 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 			if (strcmp(target_type, tgt_type)) {
 				condlog(lvl, "%s: target type mismatch: \"%s\" != \"%s\"",
 					fname__, tgt_type, target_type);
+				if (flags & MAPINFO_ID_IF_FOUND)
+					write_ids(info.name, name, info.uuid,
+						  uuid, map_id);
 				return DMP_NO_MATCH;
 			}
 		}
@@ -774,14 +798,7 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 	    || (info.target && !tmp_target && !(tmp_target = strdup(params))))
 		return DMP_ERR;
 
-	if (info.name) {
-		strlcpy(info.name, name, WWID_SIZE);
-		condlog(4, "%s: %s: name: \"%s\"", fname__, map_id, info.name);
-	}
-	if (info.uuid) {
-		strlcpy(info.uuid, uuid, DM_UUID_LEN);
-		condlog(4, "%s: %s: uuid: \"%s\"", fname__, map_id, info.uuid);
-	}
+	write_ids(info.name, name, info.uuid, uuid, map_id);
 
 	if (info.size) {
 		*info.size = length;
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 0bd2b907..630dd7b6 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -67,6 +67,11 @@ enum {
 	 * multipath UUID format ("mpath-xyz").
 	 */
 	MAPINFO_CHECK_UUID  = (1 << 10),
+	/*
+	 * If a device is found and info.name or info.uuid are non-NULL
+	 * they will be set, even if the device doesn't match
+	 */
+	MAPINFO_ID_IF_FOUND = (1 << 11),
 };
 
 typedef union libmp_map_identifier {
diff --git a/tests/mapinfo.c b/tests/mapinfo.c
index ee487989..1bfa1201 100644
--- a/tests/mapinfo.c
+++ b/tests/mapinfo.c
@@ -381,6 +381,23 @@ static void test_mapinfo_bad_get_info_01(void **state)
 	assert_int_equal(rc, DMP_ERR);
 }
 
+static void test_mapinfo_bad_get_with_ids(void **state)
+{
+	int rc;
+	char name[WWID_SIZE] = { 0 };
+	struct dm_info dmi = { .suspended = 0 };
+
+	mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+	WRAP_DM_TASK_GET_INFO(1);
+	WRAP_DM_TASK_GET_INFO(&dmi);
+	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_ID_IF_FOUND,
+			   (mapid_t) { .str = "foo", },
+			   (mapinfo_t) { .name = name });
+	assert_int_equal(rc, DMP_NOT_FOUND);
+	/* verify that name isn't set */
+	assert_memory_equal(&name, &((char[WWID_SIZE]) { 0 }), WWID_SIZE);
+}
+
 static void test_mapinfo_bad_get_info_02(void **state)
 {
 	int rc;
@@ -574,6 +591,22 @@ static void test_mapinfo_bad_check_uuid_09(void **state)
 	assert_int_equal(rc, DMP_OK);
 }
 
+static void test_mapinfo_bad_check_uuid_with_ids(void **state)
+{
+	int rc;
+	char uuid[DM_UUID_LEN] = { 0 };
+
+	mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
+	WRAP_DM_TASK_GET_INFO(1);
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+	will_return(__wrap_dm_task_get_uuid, BAD_UUID_01);
+	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID | MAPINFO_ID_IF_FOUND,
+			   (mapid_t) { .str = "foo", },
+			   (mapinfo_t) { .uuid = uuid });
+	assert_int_equal(rc, DMP_NO_MATCH);
+	assert_true(!strcmp(uuid, BAD_UUID_01));
+}
+
 static void test_mapinfo_good_check_uuid_01(void **state)
 {
 	int rc;
@@ -884,6 +917,25 @@ static void test_mapinfo_bad_next_target_01(void **state)
 	assert_int_equal(rc, DMP_NO_MATCH);
 }
 
+static void test_mapinfo_bad_next_target_with_ids(void **state)
+{
+	int rc;
+	unsigned long long size;
+	char uuid[DM_UUID_LEN] = { 0 };
+
+	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
+	WRAP_DM_TASK_GET_INFO(1);
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+	will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+	/* multiple targets */
+	mock_dm_get_next_target(12345, NULL, MPATH_STATUS_01, (void *)1);
+	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_ID_IF_FOUND,
+			   (mapid_t) { .str = "foo", },
+			   (mapinfo_t) { .size = &size, .uuid = uuid });
+	assert_int_equal(rc, DMP_NO_MATCH);
+	assert_true(!strcmp(uuid, MPATH_UUID_01));
+}
+
 static void test_mapinfo_bad_next_target_02(void **state)
 {
 	int rc;
@@ -900,6 +952,29 @@ static void test_mapinfo_bad_next_target_02(void **state)
 	assert_int_equal(rc, DMP_EMPTY);
 }
 
+static void test_mapinfo_no_target_with_ids(void **state)
+{
+	int rc;
+	unsigned long long size;
+	char name[WWID_SIZE] = { 0 };
+
+	expect_value(__wrap_dm_task_create, task, DM_DEVICE_STATUS);
+	will_return(__wrap_dm_task_create, 1);
+	expect_value(__wrap_dm_task_set_uuid, uuid, MPATH_UUID_01);
+	will_return(__wrap_dm_task_set_uuid, 1);
+	will_return(__wrap_dm_task_run, 1);
+	WRAP_DM_TASK_GET_INFO(1);
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_02);
+	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
+	/* no targets */
+	mock_dm_get_next_target(0, NULL, NULL, NULL);
+	rc = libmp_mapinfo(DM_MAP_BY_UUID | MAPINFO_ID_IF_FOUND,
+			   (mapid_t) { .str = MPATH_UUID_01, },
+			   (mapinfo_t) { .size = &size, .name = name });
+	assert_int_equal(rc, DMP_EMPTY);
+	assert_true(!strcmp(name, MPATH_NAME_01));
+}
+
 /* libmp_mapinfo needs to do a DM_DEVICE_STATUS ioctl */
 static void test_mapinfo_bad_target_type_01(void **state)
 {
@@ -915,6 +990,26 @@ static void test_mapinfo_bad_target_type_01(void **state)
 	assert_int_equal(rc, DMP_NO_MATCH);
 }
 
+static void test_mapinfo_bad_target_with_ids(void **state)
+{
+	int rc;
+	char uuid[DM_UUID_LEN] = { 0 };
+	struct dm_info dmi = { .suspended = 0 };
+
+	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
+	WRAP_DM_TASK_GET_INFO(1);
+	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+	will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
+	mock_dm_get_next_target(12345, "linear", MPATH_STATUS_01, NULL);
+	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_ID_IF_FOUND,
+			   (mapid_t) { .str = "foo", },
+			   (mapinfo_t) { .uuid = uuid, .dmi = &dmi });
+	assert_int_equal(rc, DMP_NO_MATCH);
+	assert_true(!strcmp(uuid, MPATH_UUID_01));
+	/* Verify the dmi isn't geting set */
+	assert_memory_equal(&dmi, &((struct dm_info) { .exists = 0 }), sizeof(dmi));
+}
+
 static void test_mapinfo_bad_target_type_02(void **state)
 {
 	int rc;
@@ -1376,6 +1471,7 @@ static int test_mapinfo(void)
 		cmocka_unit_test(test_mapinfo_bad_task_run_10),
 		cmocka_unit_test(test_mapinfo_bad_task_run_11),
 		cmocka_unit_test(test_mapinfo_bad_get_info_01),
+		cmocka_unit_test(test_mapinfo_bad_get_with_ids),
 		cmocka_unit_test(test_mapinfo_bad_get_info_02),
 		cmocka_unit_test(test_mapinfo_bad_get_info_03),
 		cmocka_unit_test(test_mapinfo_bad_get_info_04),
@@ -1390,6 +1486,7 @@ static int test_mapinfo(void)
 		cmocka_unit_test(test_mapinfo_bad_check_uuid_07),
 		cmocka_unit_test(test_mapinfo_bad_check_uuid_08),
 		cmocka_unit_test(test_mapinfo_bad_check_uuid_09),
+		cmocka_unit_test(test_mapinfo_bad_check_uuid_with_ids),
 		cmocka_unit_test(test_mapinfo_good_check_uuid_01),
 		cmocka_unit_test(test_mapinfo_good_check_uuid_02),
 		cmocka_unit_test(test_mapinfo_good_check_uuid_03),
@@ -1409,8 +1506,11 @@ static int test_mapinfo(void)
 		cmocka_unit_test(test_mapinfo_good_uuid),
 		cmocka_unit_test(test_mapinfo_good_size),
 		cmocka_unit_test(test_mapinfo_bad_next_target_01),
+		cmocka_unit_test(test_mapinfo_bad_next_target_with_ids),
 		cmocka_unit_test(test_mapinfo_bad_next_target_02),
+		cmocka_unit_test(test_mapinfo_no_target_with_ids),
 		cmocka_unit_test(test_mapinfo_bad_target_type_01),
+		cmocka_unit_test(test_mapinfo_bad_target_with_ids),
 		cmocka_unit_test(test_mapinfo_bad_target_type_02),
 		cmocka_unit_test(test_mapinfo_bad_target_type_03),
 		cmocka_unit_test(test_mapinfo_bad_target_type_04),
-- 
2.46.2

[PATCH 5/6] libmultipath: check table type in dm_find_map_by_wwid

[Benjamin Marzinski]

This function is only supposed to work on multipath devices (it adds the
multipath uuid prefix to the passed in wwid). Check this, and adapt
cli_add_map() to handle the corner case where there is a non-multipath
device with a multipath uuid.

Also, make dm_find_map_by_wwid() returned the alias of any found device,
even if it's not a valid multipath device.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c  | 2 +-
 multipathd/cli_handlers.c | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 42dcbc77..fa637c75 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -910,7 +910,7 @@ int dm_find_map_by_wwid(const char *wwid, char *name, struct dm_info *dmi)
 	if (safe_sprintf(tmp, UUID_PREFIX "%s", wwid))
 		return DMP_ERR;
 
-	return libmp_mapinfo(DM_MAP_BY_UUID,
+	return libmp_mapinfo(DM_MAP_BY_UUID | MAPINFO_MPATH_ONLY | MAPINFO_ID_IF_FOUND,
 			     (mapid_t) { .str = tmp },
 			     (mapinfo_t) { .name = name, .dmi = dmi });
 }
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 184c3f91..ec330d81 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -725,7 +725,13 @@ cli_add_map (void * v, struct strbuf *reply, void * data)
 		condlog(2, "%s: unknown map.", param);
 		return -ENODEV;
 	}
-	if (dm_find_map_by_wwid(refwwid, alias, &dmi) != DMP_OK) {
+	rc = dm_find_map_by_wwid(refwwid, alias, &dmi);
+	if (rc == DMP_NO_MATCH) {
+		condlog(2, "%s: wwid %s already in use by non-multipath device %s",
+			param, refwwid, alias);
+		return 1;
+	}
+	if (rc != DMP_OK) {
 		condlog(3, "%s: map not present. creating", param);
 		if (coalesce_paths(vecs, NULL, refwwid, FORCE_RELOAD_NONE,
 				   CMD_NONE) != CP_OK) {
-- 
2.46.2

[PATCH 6/6] libmultipath: handle a create corner case for empty devices

[Benjamin Marzinski]

Multipath already notices if there is any empty device with the same
alias and wwid as a device it is trying to create, and switches to a
reload. However, if the empty device has a different alias but the same
wwid, instead of reloading and renaming the device like it would for
a device with a table. multipath will attempt to create a new device
and fail.

Fix this by checking for these devices, and doing a reload and rename.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index a7257981..d0e9c958 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -835,7 +835,6 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 {
 	int r = DOMAP_FAIL;
 	struct config *conf;
-	char wwid[WWID_SIZE];
 
 	/*
 	 * last chance to quit before touching the devmaps
@@ -846,6 +845,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 	}
 
 	if (mpp->action == ACT_CREATE) {
+		char wwid[WWID_SIZE];
 		int rc = dm_get_wwid(mpp->alias, wwid, sizeof(wwid));
 
 		if (rc == DMP_OK && !strncmp(mpp->wwid, wwid, sizeof(wwid))) {
@@ -863,7 +863,26 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 			mpp->action = ACT_REJECT;
 		}
 	}
+	if (mpp->action == ACT_CREATE) {
+		char alias[WWID_SIZE];
+		int rc = dm_find_map_by_wwid(mpp->wwid, alias, NULL);
 
+		if (rc == DMP_NO_MATCH) {
+			condlog(1, "%s: wwid \"%s\" already in use by non-multipath map \"%s\"",
+				mpp->alias, mpp->wwid, alias);
+			mpp->action = ACT_REJECT;
+		} else if (rc == DMP_OK || rc == DMP_EMPTY) {
+			/*
+			 * we already handled the case were rc == DMO_OK and
+			 * the alias == mpp->alias above. So the alias must be
+			 * different here.
+			 */
+			condlog(3, "%s: map already present with a different name \"%s\". reloading",
+				mpp->alias, alias);
+			strlcpy(mpp->alias_old, alias, WWID_SIZE);
+			mpp->action = ACT_RELOAD_RENAME;
+		}
+	}
 	if (mpp->action == ACT_RENAME || mpp->action == ACT_SWITCHPG_RENAME ||
 	    mpp->action == ACT_RELOAD_RENAME ||
 	    mpp->action == ACT_RESIZE_RENAME) {
-- 
2.46.2

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

[Martin Wilck]

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

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

[Benjamin Marzinski]

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

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

[Martin Wilck]

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

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

[Martin Wilck]

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
> 

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

[Benjamin Marzinski]

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

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

[Martin Wilck]

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

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

[Benjamin Marzinski]

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