Re: [PATCH 1/2] select_devices: fix scanning of container members with dev list

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3787 bytes --]

On Thu, 27 Jun 2013 21:38:15 +0200 Martin Wilck <mwilck@arcor.de> wrote:

> Hi Neil,
> 
> I wrote:
> > Well, the problem might be fixed differently by having "mdadm -Dbv" in
> > the container case print the container device.
> 
> I have come up with another patch, based on that idea, that I'll submit
> in a minute. Do you like it better?
> 

No I don't really.  The container name is already there is "container=" so
shouldn't be in "device=" as wel.  I did like the factored out "add_device",
so I kept that :-)

However you have helped me understand what the issue is and I know how I want
to fix it.

What I have ended up with is similar to your original patch, except that it
only ignores "device=" early if "container=" is set, as that is the only case
that is really a problem.

I've included my patch below and pushed it out into my git tree.

Thanks for your persistence.

NeilBrown


From c39b2e633fd6eb82a8a8e822ef01339806b05bfa Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Tue, 2 Jul 2013 11:07:38 +1000
Subject: [PATCH] Assemble: ignore devices= if container= is present.

If "container=" is present, then we are going to assemble from the
given container where that container is made of those devices or not.
So in this case the "devices=" is purely documentation and is best
ignored.

As part of this, move the test on the "container=" value when that
start with "/" up before the device is opened.  There sooner we test
things, the better.

Reported-by: Martin Wilck <mwilck@arcor.de>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Assemble.c b/Assemble.c
index a0041c6..afe5b05 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -171,8 +171,20 @@ static int select_devices(struct mddev_dev *devlist,
 		if (tmpdev->used > 1)
 			continue;
 
-		if (ident->devices &&
-		    !match_oneof(ident->devices, devname)) {
+		if (ident->container) {
+			if (ident->container[0] == '/' &&
+			    !same_dev(ident->container, devname)) {
+				if (report_mismatch)
+					pr_err("%s is not the container required (%s)\n",
+					       devname, ident->container);
+				continue;
+			}
+		} else if (ident->devices &&
+			   !match_oneof(ident->devices, devname)) {
+			/* Note that we ignore the "device=" identifier if a
+			 * "container=" is given.  Checking both is unnecessarily
+			 * complicated.
+			 */
 			if (report_mismatch)
 				pr_err("%s is not one of %s\n", devname, ident->devices);
 			continue;
@@ -289,29 +301,20 @@ static int select_devices(struct mddev_dev *devlist,
 			}
 			close(dfd);
 
-			if (ident->container) {
-				if (ident->container[0] == '/' &&
-				    !same_dev(ident->container, devname)) {
+			if (ident->container && ident->container[0] != '/') {
+				/* we have a uuid */
+				int uuid[4];
+
+				content = *contentp;
+				tst->ss->getinfo_super(tst, content, NULL);
+
+				if (!parse_uuid(ident->container, uuid) ||
+				    !same_uuid(content->uuid, uuid, tst->ss->swapuuid)) {
 					if (report_mismatch)
-						pr_err("%s is not the container required (%s)\n",
-						       devname, ident->container);
+						pr_err("%s has wrong UUID to be required container\n",
+						       devname);
 					goto loop;
 				}
-				if (ident->container[0] != '/') {
-					/* we have a uuid */
-					int uuid[4];
-
-					content = *contentp;
-					tst->ss->getinfo_super(tst, content, NULL);
-
-					if (!parse_uuid(ident->container, uuid) ||
-					    !same_uuid(content->uuid, uuid, tst->ss->swapuuid)) {
-						if (report_mismatch)
-							pr_err("%s has wrong UUID to be required container\n",
-							       devname);
-						goto loop;
-					}
-				}
 			}
 			/* It is worth looking inside this container.
 			 */

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]