[PATCH v2 0/6] mdadm: POSIX portable naming rules

[PATCH v2 0/6] mdadm: POSIX portable naming rules

[Mariusz Tkaczyk]

Hi Jes,
To avoid problem with udev and VROC UEFI driver I developed stronger
naming policy, basing on POSIX portable names standard. Verification is
added for names and config entries. In case of an issue, user can update
name to make it compatible (for IMSM and native).

The changes here may cause /dev/md/ link will be different than before
mdadm update. To make any of that happen user need to use unusual naming
convention, like:
- special, non standard signs like, $,%,&,* or space.
- '-' used as first sign.
- locals.

Note: I didn't analyze configurations with "names=1". If name cannot be
determined mdadm should fallback to default numbered dev creation.

If you are planning release soon then feel free to merge it after the
release. It is a potential regression point.

It is a new version of [1] but it is strongly rebuild. Here is a list
of changes:
1. negative and positive test scenarios for both create and config
   entries are added.
2. Save parsed parameters in dedicated structs. It is a way to control
   what is parsed, assuming that we should use dedicated set_* function.
3. Verification for config entries is added.
5. Improved error logging for names:
   - during creation, these messages are errors, printed to stderr.
   - for config entries, messages are just a warnings printed to stdout.
6. Error messages reworked.
7. Updates in manual.

[1] https://lore.kernel.org/linux-raid/20221221115019.26276-1-mariusz.tkaczyk@linux.intel.com/

Mariusz Tkaczyk (6):
  tests: create names_template
  tests: create 00confnames
  mdadm: set ident.devname if applicable
  mdadm: refactor ident->name handling
  mdadm: define ident_set_devname()
  mdadm: Follow POSIX Portable Character Set

 Build.c                        |  21 ++--
 Create.c                       |  35 +++----
 Detail.c                       |  17 ++-
 config.c                       | 184 +++++++++++++++++++++++++++------
 lib.c                          |  76 +++++++++++---
 mdadm.8.in                     |  70 ++++++-------
 mdadm.c                        |  73 +++++--------
 mdadm.conf.5.in                |   4 -
 mdadm.h                        |  36 ++++---
 super-intel.c                  |  47 +++++----
 tests/00confnames              | 107 +++++++++++++++++++
 tests/00createnames            |  89 ++++------------
 tests/templates/names_template |  80 ++++++++++++++
 13 files changed, 551 insertions(+), 288 deletions(-)
 create mode 100644 tests/00confnames
 create mode 100644 tests/templates/names_template

-- 
2.26.2

[PATCH 1/6] tests: create names_template

[Mariusz Tkaczyk]

Create templates directory and names_template. Move code from
00createnames. This code will be reused for 00confnames in next patch.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 tests/00createnames            | 86 +++++++---------------------------
 tests/templates/names_template | 53 +++++++++++++++++++++
 2 files changed, 70 insertions(+), 69 deletions(-)
 create mode 100644 tests/templates/names_template

diff --git a/tests/00createnames b/tests/00createnames
index 64b81b92..064eeef2 100644
--- a/tests/00createnames
+++ b/tests/00createnames
@@ -1,93 +1,41 @@
 set -x -e
+. tests/templates/names_template
 
 # Test how <devname> and --name= are handled for create mode.
-# We need to check three properties, generated from those parameters:
-# - devnode name
-# - link in /dev/md/ (MD_DEVNAME property from --detail --export)
-# - name in metadata (MD_NAME property from --examine --export)
-
-function _verify() {
-  local DEVNODE_NAME="$1"
-  local WANTED_LINK="$2"
-  local WANTED_NAME="$3"
-
-  local RES="$(mdadm -D --export $DEVNODE_NAME | grep MD_DEVNAME)"
-  if [[ "$?" != "0" ]]; then
-    echo "Cannot get details for $DEVNODE_NAME - unexpected devnode."
-    exit 1
-  fi
-
-  if [[ "$WANTED_LINK" != "empty" ]]; then
-    local EXPECTED="MD_DEVNAME=$WANTED_LINK"
-      if [[ "$RES" != "$EXPECTED" ]]; then
-        echo "$RES doesn't match $EXPECTED."
-        exit 1
-      fi
-  fi
-
-
-  local RES="$(mdadm -E --export $dev0 | grep MD_NAME)"
-  if [[ "$?" != "0" ]]; then
-    echo "Cannot get metadata from $dev0."
-    exit 1
-  fi
-
-  local EXPECTED="MD_NAME=$(hostname):$WANTED_NAME"
-  if [[ "$RES" != "$EXPECTED" ]]; then
-    echo "$RES doesn't match $EXPECTED."
-    exit 1
-  fi
-}
-
-function _create() {
-  local DEVNAME=$1
-  local NAME=$2
-
-  if [[ -z "$NAME" ]]; then
-    mdadm -CR "$DEVNAME" -l0 -n 1 $dev0 --force
-  else
-    mdadm -CR "$DEVNAME" --name="$NAME" -l0 -n 1 $dev0 --force
-  fi
-
-  if [[ "$?" != "0" ]]; then
-    echo "Cannot create device."
-    exit 1
-  fi
-}
 
 # The most trivial case.
-_create "/dev/md/name"
-_verify "/dev/md127" "name" "name"
+names_create "/dev/md/name"
+names_verify "/dev/md127" "name" "name"
 mdadm -S "/dev/md127"
 
-_create "name"
-_verify "/dev/md127" "name" "name"
+names_create "name"
+names_verify "/dev/md127" "name" "name"
 mdadm -S "/dev/md127"
 
 # Use 'mdX' as name.
-_create "/dev/md/md0"
-_verify "/dev/md127" "md0" "md0"
+names_create "/dev/md/md0"
+names_verify "/dev/md127" "md0" "md0"
 mdadm -S "/dev/md127"
 
-_create "md0"
-_verify "/dev/md127" "md0" "md0"
+names_create "md0"
+names_verify "/dev/md127" "md0" "md0"
 mdadm -S "/dev/md127"
 
 # <devnode> is used to create MD_DEVNAME but, name is used to create MD_NAME.
-_create "/dev/md/devnode" "name"
-_verify "/dev/md127" "devnode" "name"
+names_create "/dev/md/devnode" "name"
+names_verify "/dev/md127" "devnode" "name"
 mdadm -S "/dev/md127"
 
-_create "devnode" "name"
-_verify "/dev/md127" "devnode" "name"
+names_create "devnode" "name"
+names_verify "/dev/md127" "devnode" "name"
 mdadm -S "/dev/md127"
 
 # Devnode points to /dev/ directory. MD_DEVNAME doesn't exist.
-_create "/dev/md0"
-_verify "/dev/md0" "empty" "0"
+names_create "/dev/md0"
+names_verify "/dev/md0" "empty" "0"
 mdadm -S "/dev/md0"
 
 # Devnode points to /dev/ directory and name is set.
-_create "/dev/md0" "name"
-_verify "/dev/md0" "empty" "name"
+names_create "/dev/md0" "name"
+names_verify "/dev/md0" "empty" "name"
 mdadm -S "/dev/md0"
diff --git a/tests/templates/names_template b/tests/templates/names_template
new file mode 100644
index 00000000..9f09be9e
--- /dev/null
+++ b/tests/templates/names_template
@@ -0,0 +1,53 @@
+# NAME is optional. Testing with native 1.2 superblock.
+function names_create() {
+	local DEVNAME=$1
+	local NAME=$2
+
+	if [[ -z "$NAME" ]]; then
+		mdadm -CR "$DEVNAME" -l0 -n 1 $dev0 --force
+	else
+		mdadm -CR "$DEVNAME" --name="$NAME" --metadata=1.2 -l0 -n 1 $dev0 --force
+	fi
+
+	if [[ "$?" != "0" ]]; then
+		echo "Cannot create device."
+		exit 1
+	fi
+}
+
+# Three properties to check:
+# - devnode name
+# - link in /dev/md/ (MD_DEVNAME property from --detail --export)
+# - name in metadata (MD_NAME property from --detail --export)- that works only with 1.2 sb.
+function names_verify() {
+	local DEVNODE_NAME="$1"
+	local WANTED_LINK="$2"
+	local WANTED_NAME="$3"
+
+	local RES="$(mdadm -D --export $DEVNODE_NAME | grep MD_DEVNAME)"
+	if [[ "$?" != "0" ]]; then
+		echo "Cannot get details for $DEVNODE_NAME - unexpected devnode."
+		exit 1
+	fi
+
+	if [[ "$WANTED_LINK" != "empty" ]]; then
+		local EXPECTED="MD_DEVNAME=$WANTED_LINK"
+	fi
+
+	if [[ "$RES" != "$EXPECTED" ]]; then
+		echo "$RES doesn't match $EXPECTED."
+		exit 1
+	fi
+
+	local RES="$(mdadm -D --export $DEVNODE_NAME | grep MD_NAME)"
+	if [[ "$?" != "0" ]]; then
+		echo "Cannot get metadata from $dev0."
+		exit 1
+	fi
+
+	local EXPECTED="MD_NAME=$(hostname):$WANTED_NAME"
+	if [[ "$RES" != "$EXPECTED" ]]; then
+		echo "$RES doesn't match $EXPECTED."
+		exit 1
+	fi
+}
-- 
2.26.2

[PATCH 2/6] tests: create 00confnames

[Mariusz Tkaczyk]

The test is an attempt to document current implementation of devnode
and name handling for config entries. It is focused on incremental-
default way of array assembling on boot.
The expectations are aligned to current implementation for native
metadata because it is the most complicated scenario- both variables
can be set.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 tests/00confnames              | 107 +++++++++++++++++++++++++++++++++
 tests/templates/names_template |  20 ++++++
 2 files changed, 127 insertions(+)
 create mode 100644 tests/00confnames

diff --git a/tests/00confnames b/tests/00confnames
new file mode 100644
index 00000000..4990cb5e
--- /dev/null
+++ b/tests/00confnames
@@ -0,0 +1,107 @@
+set -x -e
+. tests/templates/names_template
+
+# Test how <devname> and <name> from config are handled during Incremental assemblation.
+# 1-6 <devnode> only tests (no <name> in config).
+# 6-10 <devname> and <name> combinations are tested.
+# 11-13 corner cases.
+
+names_create "/dev/md/name"
+local _UUID="$(mdadm -D --export /dev/md127 | grep MD_UUID | cut -d'=' -f2)"
+[[ "$_UUID" == "" ]] && echo "Cannot obtain UUID for $DEVNODE_NAME" && exit 1
+
+
+# 1. <devname> definition consistent with metadata name.
+names_make_conf $_UUID "/dev/md/name" "empty" $config
+mdadm -S "/dev/md127"
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 2. Same as 1, but use short name form of <devname>.
+names_make_conf $_UUID "name" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 3. Same as 1, but use different <devname> than metadata provides.
+names_make_conf $_UUID "/dev/md/other" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "other" "name"
+mdadm -S "/dev/md127"
+
+# 4. Same as 3, but use short name form of <devname>.
+names_make_conf $_UUID "other" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "other" "name"
+mdadm -S "/dev/md127"
+
+# 5. Force particular node creation by setting <devname> to /dev/mdX. Link is not created in this
+# case.
+names_make_conf $_UUID "/dev/md4" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md4" "empty" "name"
+mdadm -S "/dev/md4"
+
+# 6. <devname> set to /dev/mdX, <name> same as in metadata.
+# Metadata name and default node used - controversial. Current behavior documented.
+names_make_conf $_UUID "/dev/md22" "name" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 7. <devname> set to /dev/mdX, <name> different than in metadata.
+# Metadata name and default node used - controversial. Current behavior documented.
+names_make_conf $_UUID "/dev/md8" "other" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 8. Both <devname> and <name> different than in metadata.
+# Metadata name and default node used - controversial. Current behavior documented.
+names_make_conf $_UUID "devnode" "other_name" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 9. <devname> set to metadata name, <name> different than in metadata.
+# Metadata name and default node used - controversial. Current behavior documented.
+names_make_conf $_UUID "name" "other_name" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 10. Bad <devname> set, no <name>.
+# Metadata name and default node used - expected.
+names_make_conf $_UUID "/im/bad/devname" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 11. <devname> with some special symbols and locales, no <name>.
+# It needs to wait a while for timeout because udev cannot create a link - known issue.
+names_make_conf $_UUID "tźż-\.,<>st+-" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "tźż-\.,<>st+-" "name"
+mdadm -S "/dev/md127"
+
+# 12. No <devname> and <name> set.
+# Metadata name and default node used - expected.
+names_make_conf $_UUID "empty" "empty" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 13. No <devname>, <name> set to /dev/mdX.
+# Entry should be ignored, it is not ignored but result is good anyway.
+names_make_conf $_UUID "empty" "/dev/md12" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# 13. No <devname>, <name> with special symbols and locales.
+# Entry should be ignored, it is not ignored but result is good anyway.
+names_make_conf $_UUID "empty" "./\śćń#&" $config
+mdadm -I $dev0 --config=$config
+names_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
diff --git a/tests/templates/names_template b/tests/templates/names_template
index 9f09be9e..8d2b5c81 100644
--- a/tests/templates/names_template
+++ b/tests/templates/names_template
@@ -51,3 +51,23 @@ function names_verify() {
 		exit 1
 	fi
 }
+
+# Generate ARRAYLINE for tested array.
+names_make_conf() {
+	local UUID="$1"
+	local WANTED_DEVNAME="$2"
+	local WANTED_NAME="$3"
+	local CONF="$4"
+
+	local LINE="ARRAY metadata=1.2 UUID=$UUID"
+
+	if [[ "$WANTED_DEVNAME" != "empty" ]]; then
+		LINE="$LINE $WANTED_DEVNAME"
+	fi
+
+	if [[ "$WANTED_NAME" != "empty" ]]; then
+		LINE="$LINE name=$WANTED_NAME"
+	fi
+
+	echo $LINE > $CONF
+}
-- 
2.26.2

[PATCH 3/6] mdadm: set ident.devname if applicable

[Mariusz Tkaczyk]

This patch tries to propagate the usage of struct mddev_ident for cmdline
where it is applicable. To avoid regression, this value is derived
from devlist->devname for applicable modes only.
As a result, the whole structure is passed to some functions. It produces
some changes for Build, Create and Assemble.
No functional changes intended.

The goal of the change is to unify devname validation which is done in
next patches.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Build.c  | 21 +++++++++------------
 Create.c | 35 ++++++++++++++++------------------
 mdadm.c  | 57 +++++++++++++++++++++++++-------------------------------
 mdadm.h  | 13 +++++--------
 4 files changed, 55 insertions(+), 71 deletions(-)

diff --git a/Build.c b/Build.c
index 8d6f6f58..657ab315 100644
--- a/Build.c
+++ b/Build.c
@@ -24,8 +24,8 @@
 
 #include "mdadm.h"
 
-int Build(char *mddev, struct mddev_dev *devlist,
-	  struct shape *s, struct context *c)
+int Build(struct mddev_ident *ident, struct mddev_dev *devlist, struct shape *s,
+	  struct context *c)
 {
 	/* Build a linear or raid0 arrays without superblocks
 	 * We cannot really do any checks, we just do it.
@@ -75,13 +75,12 @@ int Build(char *mddev, struct mddev_dev *devlist,
 
 	/* We need to create the device.  It can have no name. */
 	map_lock(&map);
-	mdfd = create_mddev(mddev, NULL, c->autof, LOCAL,
+	mdfd = create_mddev(ident->devname, NULL, c->autof, LOCAL,
 			    chosen_name, 0);
 	if (mdfd < 0) {
 		map_unlock(&map);
 		return 1;
 	}
-	mddev = chosen_name;
 
 	map_update(&map, fd2devnm(mdfd), "none", uuid, chosen_name);
 	map_unlock(&map);
@@ -93,7 +92,7 @@ int Build(char *mddev, struct mddev_dev *devlist,
 	array.nr_disks = s->raiddisks;
 	array.raid_disks = s->raiddisks;
 	array.md_minor = 0;
-	if (fstat_is_blkdev(mdfd, mddev, &rdev))
+	if (fstat_is_blkdev(mdfd, chosen_name, &rdev))
 		array.md_minor = minor(rdev);
 	array.not_persistent = 1;
 	array.state = 0; /* not clean, but no errors */
@@ -108,8 +107,7 @@ int Build(char *mddev, struct mddev_dev *devlist,
 	array.chunk_size = s->chunk*1024;
 	array.layout = s->layout;
 	if (md_set_array_info(mdfd, &array)) {
-		pr_err("md_set_array_info() failed for %s: %s\n",
-		       mddev, strerror(errno));
+		pr_err("md_set_array_info() failed for %s: %s\n", chosen_name, strerror(errno));
 		goto abort;
 	}
 
@@ -178,8 +176,8 @@ int Build(char *mddev, struct mddev_dev *devlist,
 		}
 		if (bitmap_fd >= 0) {
 			if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) {
-				pr_err("Cannot set bitmap file for %s: %s\n",
-				       mddev, strerror(errno));
+				pr_err("Cannot set bitmap file for %s: %s\n", chosen_name,
+				       strerror(errno));
 				goto abort;
 			}
 		}
@@ -193,9 +191,8 @@ int Build(char *mddev, struct mddev_dev *devlist,
 	}
 
 	if (c->verbose >= 0)
-		pr_err("array %s built and started.\n",
-			mddev);
-	wait_for(mddev, mdfd);
+		pr_err("array %s built and started.\n", chosen_name);
+	wait_for(chosen_name, mdfd);
 	close(mdfd);
 	return 0;
 
diff --git a/Create.c b/Create.c
index ea6a4745..a280c7bc 100644
--- a/Create.c
+++ b/Create.c
@@ -471,11 +471,8 @@ out:
 	return ret;
 }
 
-int Create(struct supertype *st, char *mddev,
-	   char *name, int *uuid,
-	   int subdevs, struct mddev_dev *devlist,
-	   struct shape *s,
-	   struct context *c)
+int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
+	   struct mddev_dev *devlist, struct shape *s, struct context *c)
 {
 	/*
 	 * Create a new raid array.
@@ -497,6 +494,8 @@ int Create(struct supertype *st, char *mddev,
 	unsigned long long minsize = 0, maxsize = 0;
 	char *mindisc = NULL;
 	char *maxdisc = NULL;
+	char *name = ident->name;
+	int *uuid = ident->uuid_set == 1 ? ident->uuid : NULL;
 	int dnum;
 	struct mddev_dev *dv;
 	dev_t rdev;
@@ -1015,7 +1014,7 @@ int Create(struct supertype *st, char *mddev,
 
 	/* We need to create the device */
 	map_lock(&map);
-	mdfd = create_mddev(mddev, name, c->autof, LOCAL, chosen_name, 1);
+	mdfd = create_mddev(ident->devname, ident->name, c->autof, LOCAL, chosen_name, 1);
 	if (mdfd < 0) {
 		map_unlock(&map);
 		return 1;
@@ -1032,7 +1031,6 @@ int Create(struct supertype *st, char *mddev,
 		udev_unblock();
 		return 1;
 	}
-	mddev = chosen_name;
 
 	memset(&inf, 0, sizeof(inf));
 	md_get_array_info(mdfd, &inf);
@@ -1050,7 +1048,7 @@ int Create(struct supertype *st, char *mddev,
 	 * with, but it chooses to trust me instead. Sigh
 	 */
 	info.array.md_minor = 0;
-	if (fstat_is_blkdev(mdfd, mddev, &rdev))
+	if (fstat_is_blkdev(mdfd, chosen_name, &rdev))
 		info.array.md_minor = minor(rdev);
 	info.array.not_persistent = 0;
 
@@ -1102,8 +1100,8 @@ int Create(struct supertype *st, char *mddev,
 	info.array.layout = s->layout;
 	info.array.chunk_size = s->chunk*1024;
 
-	if (name == NULL || *name == 0) {
-		/* base name on mddev */
+	if (*name == 0) {
+		/* base name on devname */
 		/*  /dev/md0 -> 0
 		 *  /dev/md_d0 -> d0
 		 *  /dev/md_foo -> foo
@@ -1113,15 +1111,16 @@ int Create(struct supertype *st, char *mddev,
 		 *  /dev/mdhome -> home
 		 */
 		/* FIXME compare this with rules in create_mddev */
-		name = strrchr(mddev, '/');
+		name = strrchr(chosen_name, '/');
+
 		if (name) {
 			name++;
 			if (strncmp(name, "md_", 3) == 0 &&
-			    strlen(name) > 3 && (name-mddev) == 5 /* /dev/ */)
+			    strlen(name) > 3 && (name - chosen_name) == 5 /* /dev/ */)
 				name += 3;
 			else if (strncmp(name, "md", 2) == 0 &&
 				 strlen(name) > 2 && isdigit(name[2]) &&
-				 (name-mddev) == 5 /* /dev/ */)
+				 (name - chosen_name) == 5 /* /dev/ */)
 				name += 2;
 		}
 	}
@@ -1215,8 +1214,7 @@ int Create(struct supertype *st, char *mddev,
 	}
 	rv = set_array_info(mdfd, st, &info);
 	if (rv) {
-		pr_err("failed to set array info for %s: %s\n",
-			mddev, strerror(errno));
+		pr_err("failed to set array info for %s: %s\n", chosen_name, strerror(errno));
 		goto abort_locked;
 	}
 
@@ -1237,8 +1235,7 @@ int Create(struct supertype *st, char *mddev,
 			goto abort_locked;
 		}
 		if (ioctl(mdfd, SET_BITMAP_FILE, bitmap_fd) < 0) {
-			pr_err("Cannot set bitmap file for %s: %s\n",
-				mddev, strerror(errno));
+			pr_err("Cannot set bitmap file for %s: %s\n", chosen_name, strerror(errno));
 			goto abort_locked;
 		}
 	}
@@ -1254,7 +1251,7 @@ int Create(struct supertype *st, char *mddev,
 		 * create links */
 		sysfs_uevent(&info, "change");
 		if (c->verbose >= 0)
-			pr_err("container %s prepared.\n", mddev);
+			pr_err("container %s prepared.\n", chosen_name);
 		wait_for(chosen_name, mdfd);
 	} else if (c->runstop == 1 || subdevs >= s->raiddisks) {
 		if (st->ss->external) {
@@ -1312,7 +1309,7 @@ int Create(struct supertype *st, char *mddev,
 			ioctl(mdfd, RESTART_ARRAY_RW, NULL);
 		}
 		if (c->verbose >= 0)
-			pr_info("array %s started.\n", mddev);
+			pr_info("array %s started.\n", chosen_name);
 		if (st->ss->external && st->container_devnm[0]) {
 			if (need_mdmon)
 				start_mdmon(st->container_devnm);
diff --git a/mdadm.c b/mdadm.c
index 076b45e0..14072ec1 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1290,37 +1290,39 @@ int main(int argc, char *argv[])
 			pr_err("an md device must be given in this mode\n");
 			exit(2);
 		}
+		ident.devname = devlist->devname;
+
 		if ((int)ident.super_minor == -2 && c.autof) {
 			pr_err("--super-minor=dev is incompatible with --auto\n");
 			exit(2);
 		}
 		if (mode == MANAGE || mode == GROW) {
-			mdfd = open_mddev(devlist->devname, 1);
+			mdfd = open_mddev(ident.devname, 1);
 			if (mdfd < 0)
 				exit(1);
 
 			ret = fstat(mdfd, &stb);
 			if (ret) {
-				pr_err("fstat failed on %s.\n", devlist->devname);
+				pr_err("fstat failed on %s.\n", ident.devname);
 				exit(1);
 			}
 		} else {
-			char *bname = basename(devlist->devname);
+			char *bname = basename(ident.devname);
 
 			if (strlen(bname) > MD_NAME_MAX) {
-				pr_err("Name %s is too long.\n", devlist->devname);
+				pr_err("Name %s is too long.\n", ident.devname);
 				exit(1);
 			}
 
-			ret = stat(devlist->devname, &stb);
+			ret = stat(ident.devname, &stb);
 			if (ident.super_minor == -2 && ret != 0) {
 				pr_err("--super-minor=dev given, and listed device %s doesn't exist.\n",
-				       devlist->devname);
+				       ident.devname);
 				exit(1);
 			}
 
 			if (!ret && !stat_is_md_dev(&stb)) {
-				pr_err("device %s exists but is not an md array.\n", devlist->devname);
+				pr_err("device %s exists but is not an md array.\n", ident.devname);
 				exit(1);
 			}
 		}
@@ -1409,17 +1411,17 @@ int main(int argc, char *argv[])
 	case MANAGE:
 		/* readonly, add/remove, readwrite, runstop */
 		if (c.readonly > 0)
-			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
+			rv = Manage_ro(ident.devname, mdfd, c.readonly);
 		if (!rv && devs_found > 1)
-			rv = Manage_subdevs(devlist->devname, mdfd,
+			rv = Manage_subdevs(ident.devname, mdfd,
 					    devlist->next, c.verbose,
 					    c.test, c.update, c.force);
 		if (!rv && c.readonly < 0)
-			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
+			rv = Manage_ro(ident.devname, mdfd, c.readonly);
 		if (!rv && c.runstop > 0)
-			rv = Manage_run(devlist->devname, mdfd, &c);
+			rv = Manage_run(ident.devname, mdfd, &c);
 		if (!rv && c.runstop < 0)
-			rv = Manage_stop(devlist->devname, mdfd, c.verbose, 0);
+			rv = Manage_stop(ident.devname, mdfd, c.verbose, 0);
 		break;
 	case ASSEMBLE:
 		if (!c.scan && c.runstop == -1) {
@@ -1429,22 +1431,19 @@ int main(int argc, char *argv[])
 			   ident.super_minor == UnSet && ident.name[0] == 0 &&
 			   !c.scan) {
 			/* Only a device has been given, so get details from config file */
-			struct mddev_ident *array_ident = conf_get_ident(devlist->devname);
+			struct mddev_ident *array_ident = conf_get_ident(ident.devname);
 			if (array_ident == NULL) {
-				pr_err("%s not identified in config file.\n",
-					devlist->devname);
+				pr_err("%s not identified in config file.\n", ident.devname);
 				rv |= 1;
 				if (mdfd >= 0)
 					close(mdfd);
 			} else {
 				if (array_ident->autof == 0)
 					array_ident->autof = c.autof;
-				rv |= Assemble(ss, devlist->devname, array_ident,
-					       NULL, &c);
+				rv |= Assemble(ss, ident.devname, array_ident, NULL, &c);
 			}
 		} else if (!c.scan)
-			rv = Assemble(ss, devlist->devname, &ident,
-				      devlist->next, &c);
+			rv = Assemble(ss, ident.devname, &ident, devlist->next, &c);
 		else if (devs_found > 0) {
 			if (c.update && devs_found > 1) {
 				pr_err("can only update a single array at a time\n");
@@ -1502,7 +1501,7 @@ int main(int argc, char *argv[])
 				break;
 			}
 		}
-		rv = Build(devlist->devname, devlist->next, &s, &c);
+		rv = Build(&ident, devlist->next, &s, &c);
 		break;
 	case CREATE:
 		if (c.delay == 0)
@@ -1539,9 +1538,7 @@ int main(int argc, char *argv[])
 			break;
 		}
 
-		rv = Create(ss, devlist->devname,
-			    ident.name, ident.uuid_set ? ident.uuid : NULL,
-			    devs_found - 1, devlist->next, &s, &c);
+		rv = Create(ss, &ident, devs_found - 1, devlist->next, &s, &c);
 		break;
 	case MISC:
 		if (devmode == 'E') {
@@ -1638,8 +1635,7 @@ int main(int argc, char *argv[])
 				break;
 			}
 			for (dv = devlist->next; dv; dv = dv->next) {
-				rv = Grow_Add_device(devlist->devname, mdfd,
-						     dv->devname);
+				rv = Grow_Add_device(ident.devname, mdfd, dv->devname);
 				if (rv)
 					break;
 			}
@@ -1652,18 +1648,15 @@ int main(int argc, char *argv[])
 			}
 			if (c.delay == 0)
 				c.delay = DEFAULT_BITMAP_DELAY;
-			rv = Grow_addbitmap(devlist->devname, mdfd, &c, &s);
+			rv = Grow_addbitmap(ident.devname, mdfd, &c, &s);
 		} else if (grow_continue)
-			rv = Grow_continue_command(devlist->devname,
-						   mdfd, c.backup_file,
-						   c.verbose);
+			rv = Grow_continue_command(ident.devname, mdfd, c.backup_file, c.verbose);
 		else if (s.size > 0 || s.raiddisks || s.layout_str ||
 			 s.chunk != 0 || s.level != UnSet ||
 			 s.data_offset != INVALID_SECTORS) {
-			rv = Grow_reshape(devlist->devname, mdfd,
-					  devlist->next, &c, &s);
+			rv = Grow_reshape(ident.devname, mdfd, devlist->next, &c, &s);
 		} else if (s.consistency_policy != CONSISTENCY_POLICY_UNKNOWN) {
-			rv = Grow_consistency_policy(devlist->devname, mdfd, &c, &s);
+			rv = Grow_consistency_policy(ident.devname, mdfd, &c, &s);
 		} else if (array_size == 0)
 			pr_err("no changes to --grow\n");
 		break;
diff --git a/mdadm.h b/mdadm.h
index 83f2cf7f..782d7996 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1531,14 +1531,11 @@ extern int Assemble(struct supertype *st, char *mddev,
 		    struct mddev_dev *devlist,
 		    struct context *c);
 
-extern int Build(char *mddev, struct mddev_dev *devlist,
-		 struct shape *s, struct context *c);
-
-extern int Create(struct supertype *st, char *mddev,
-		  char *name, int *uuid,
-		  int subdevs, struct mddev_dev *devlist,
-		  struct shape *s,
-		  struct context *c);
+extern int Build(struct mddev_ident *ident, struct mddev_dev *devlist, struct shape *s,
+		 struct context *c);
+
+extern int Create(struct supertype *st, struct mddev_ident *ident, int subdevs,
+		  struct mddev_dev *devlist, struct shape *s, struct context *c);
 
 extern int Detail(char *dev, struct context *c);
 extern int Detail_Platform(struct superswitch *ss, int scan, int verbose, int export, char *controller_path);
-- 
2.26.2

[PATCH 4/6] mdadm: refactor ident->name handling

[Mariusz Tkaczyk]

Create dedicated setter for name in mddev_ident and propagate it.
Following changes are made:
- move duplicated code from  config.c and mdadm.c into new function.
- Add error enum in mdadm.h.
- Use MD_NAME_MAX instead of hardcoded value in mddev_ident.
- Use secure functions.
- Add more detailed verification of the name.
- make error messages reusable for cmdline and config:
    - for cmdline, these are errors so use pr_err().
    - for config, these are just warnings, so use pr_info().

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 config.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 lib.c    | 18 +++++++++++++
 mdadm.c  | 12 +++------
 mdadm.h  | 20 ++++++++++-----
 4 files changed, 104 insertions(+), 23 deletions(-)

diff --git a/config.c b/config.c
index 450880e3..76a8384a 100644
--- a/config.c
+++ b/config.c
@@ -131,6 +131,34 @@ bool is_devname_ignore(char *devname)
 	return false;
 }
 
+/**
+ * ident_log() - generate and write message to the user.
+ * @param_name: name of the property.
+ * @value: value of the property.
+ * @reason: meaningful description.
+ * @cmdline: context dependent actions, see below.
+ *
+ * The function is made to provide similar error handling for both config and cmdline. The behavior
+ * is configurable via @cmdline. Message has following format:
+ * "Value "@value" cannot be set for @param_name. Reason: @reason."
+ *
+ * If cmdline is on:
+ * - message is written to stderr.
+ * otherwise:
+ * - message is written to stdout.
+ * - "Value ignored" is added at the end of the message.
+ */
+static void ident_log(const char *param_name, const char *value, const char *reason,
+		      const bool cmdline)
+{
+	if (cmdline == true)
+		pr_err("Value \"%s\" cannot be set as %s. Reason: %s.\n", value, param_name,
+		       reason);
+	else
+		pr_info("Value \"%s\" cannot be set as %s. Reason: %s. Value ignored.\n", value,
+			param_name, reason);
+}
+
 /**
  * ident_init() - Set defaults.
  * @ident: ident pointer, not NULL.
@@ -159,6 +187,46 @@ inline void ident_init(struct mddev_ident *ident)
 	ident->uuid_set = 0;
 }
 
+/**
+ * _ident_set_name()- set name in &mddev_ident.
+ * @ident: pointer to &mddev_ident.
+ * @name: name to be set.
+ * @cmdline: context dependent actions.
+ *
+ * If criteria passed, set name in @ident.
+ *
+ * Return: %MDADM_STATUS_SUCCESS or %MDADM_STATUS_ERROR.
+ */
+static mdadm_status_t _ident_set_name(struct mddev_ident *ident, const char *name,
+				      const bool cmdline)
+{
+	assert(name);
+	assert(ident);
+
+	const char *prop_name = "name";
+
+	if (ident->name[0]) {
+		ident_log(prop_name, name, "Already defined", cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+
+	if (is_string_lq(name, MD_NAME_MAX + 1) == false) {
+		ident_log(prop_name, name, "Too long or empty", cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+
+	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
+	return MDADM_STATUS_SUCCESS;
+}
+
+/**
+ * ident_set_name()- exported, for cmdline.
+ */
+mdadm_status_t ident_set_name(struct mddev_ident *ident, const char *name)
+{
+	return _ident_set_name(ident, name, true);
+}
+
 struct conf_dev {
 	struct conf_dev *next;
 	char *name;
@@ -444,14 +512,7 @@ void arrayline(char *line)
 					mis.super_minor = minor;
 			}
 		} else if (strncasecmp(w, "name=", 5) == 0) {
-			if (mis.name[0])
-				pr_err("only specify name once, %s ignored.\n",
-					w);
-			else if (strlen(w + 5) > 32)
-				pr_err("name too long, ignoring %s\n", w);
-			else
-				strcpy(mis.name, w + 5);
-
+			_ident_set_name(&mis, w + 5, false);
 		} else if (strncasecmp(w, "bitmap=", 7) == 0) {
 			if (mis.bitmap_file)
 				pr_err("only specify bitmap file once. %s ignored\n",
diff --git a/lib.c b/lib.c
index fe5c8d2c..999cd520 100644
--- a/lib.c
+++ b/lib.c
@@ -27,6 +27,24 @@
 #include	<ctype.h>
 #include	<limits.h>
 
+/**
+ * is_string_lq() - Check if string length with NULL byte is lower or equal to requested.
+ * @str: string to check.
+ * @max_len: max length.
+ *
+ * @str length must be bigger than 0 and be lower or equal @max_len, including termination byte.
+ */
+bool is_string_lq(const char * const str, size_t max_len)
+{
+	assert(str);
+
+	size_t _len = strnlen(str, max_len);
+
+	if (_len > 0 && _len < max_len)
+		return true;
+	return false;
+}
+
 bool is_dev_alive(char *path)
 {
 	if (!path)
diff --git a/mdadm.c b/mdadm.c
index 14072ec1..5084c348 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -690,20 +690,14 @@ int main(int argc, char *argv[])
 		case O(CREATE,'N'):
 		case O(ASSEMBLE,'N'):
 		case O(MISC,'N'):
-			if (ident.name[0]) {
-				pr_err("name cannot be set twice.   Second value %s.\n", optarg);
-				exit(2);
-			}
 			if (mode == MISC && !c.subarray) {
 				pr_err("-N/--name only valid with --update-subarray in misc mode\n");
 				exit(2);
 			}
-			if (strlen(optarg) > 32) {
-				pr_err("name '%s' is too long, 32 chars max.\n",
-					optarg);
+
+			if (ident_set_name(&ident, optarg) != MDADM_STATUS_SUCCESS)
 				exit(2);
-			}
-			strcpy(ident.name, optarg);
+
 			continue;
 
 		case O(ASSEMBLE,'m'): /* super-minor for array */
diff --git a/mdadm.h b/mdadm.h
index 782d7996..62e15dfa 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -294,6 +294,11 @@ static inline void __put_unaligned32(__u32 val, void *p)
 #define KIB_TO_BYTES(x)	((x) << 10)
 #define SEC_TO_BYTES(x)	((x) << 9)
 
+/**
+ * This is true for native and DDF, IMSM allows 16.
+ */
+#define MD_NAME_MAX 32
+
 extern const char Name[];
 
 struct md_bb_entry {
@@ -425,6 +430,12 @@ struct spare_criteria {
 	unsigned int sector_size;
 };
 
+typedef enum mdadm_status {
+	MDADM_STATUS_SUCCESS = 0,
+	MDADM_STATUS_ERROR,
+	MDADM_STATUS_UNDEF,
+} mdadm_status_t;
+
 enum mode {
 	ASSEMBLE=1,
 	BUILD,
@@ -593,7 +604,7 @@ struct mddev_ident {
 
 	int	uuid_set;
 	int	uuid[4];
-	char	name[33];
+	char	name[MD_NAME_MAX + 1];
 
 	int super_minor;
 
@@ -1609,6 +1620,7 @@ extern int check_partitions(int fd, char *dname,
 extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
 extern int stat_is_blkdev(char *devname, dev_t *rdev);
 
+extern bool is_string_lq(const char * const str, size_t max_len);
 extern bool is_dev_alive(char *path);
 extern int get_mdp_major(void);
 extern int get_maj_min(char *dev, int *major, int *minor);
@@ -1626,6 +1638,7 @@ extern void manage_fork_fds(int close_all);
 extern int continue_via_systemd(char *devnm, char *service_name, char *prefix);
 
 extern void ident_init(struct mddev_ident *ident);
+extern mdadm_status_t ident_set_name(struct mddev_ident *ident, const char *name);
 
 extern int parse_auto(char *str, char *msg, int config);
 extern struct mddev_ident *conf_get_ident(char *dev);
@@ -2001,11 +2014,6 @@ enum r0layout {
 /* And another special number needed for --data_offset=variable */
 #define VARIABLE_OFFSET 3
 
-/**
- * This is true for native and DDF, IMSM allows 16.
- */
-#define MD_NAME_MAX 32
-
 /**
  * is_container() - check if @level is &LEVEL_CONTAINER
  * @level: level value
-- 
2.26.2

[PATCH 5/6] mdadm: define ident_set_devname()

[Mariusz Tkaczyk]

Use dedicated set method for ident->devname. Now, devname validation
is done early for modes where device is created (Build, Create and
Assemble). The rules, used for devname validation are derived from
config file.

It could cause regression with execeptional cases where existing device
has name which doesn't match criteria for Manage and Grow modes. It is
low risk and those modes are not omitted from early devname validation.
Use can used main numbered devnode to avoid this problem.
Messages exposed to user are changed so it might cause a regression
in negative scenarios. Error codes are not changed.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 config.c                       | 102 +++++++++++++++++++++++++--------
 mdadm.c                        |  10 +---
 mdadm.h                        |   3 +-
 tests/00createnames            |   3 +
 tests/templates/names_template |   7 +++
 5 files changed, 92 insertions(+), 33 deletions(-)

diff --git a/config.c b/config.c
index 76a8384a..d80747a2 100644
--- a/config.c
+++ b/config.c
@@ -122,7 +122,7 @@ int match_keyword(char *word)
 /**
  * is_devname_ignore() - check if &devname is a special "<ignore>" keyword.
  */
-bool is_devname_ignore(char *devname)
+bool is_devname_ignore(const char *devname)
 {
 	static const char keyword[] = "<ignore>";
 
@@ -187,6 +187,74 @@ inline void ident_init(struct mddev_ident *ident)
 	ident->uuid_set = 0;
 }
 
+/**
+ * _ident_set_devname()- verify devname and set it in &mddev_ident.
+ * @ident: pointer to &mddev_ident.
+ * @devname: devname to be set.
+ * @cmdline: context dependent actions. If set, ignore keyword is not allowed.
+ *
+ * @devname can have following forms:
+ *	'<ignore>' keyword (if allowed)
+ *	/dev/md{number}
+ *	/dev/md_d{number} (legacy)
+ *	/dev/md_{name}
+ *	/dev/md/{name}
+ *	{name} - anything that doesn't start from '/' or '<'.
+ *
+ * {name} must follow name's criteria.
+ * If criteria passed, duplicate memory and set devname in @ident.
+ *
+ * Return: %MDADM_STATUS_SUCCESS or %MDADM_STATUS_ERROR.
+ */
+mdadm_status_t _ident_set_devname(struct mddev_ident *ident, const char *devname,
+				  const bool cmdline)
+{
+	assert(ident);
+	assert(devname);
+
+	static const char named_dev_pref[] = DEV_NUM_PREF "_";
+	static const int named_dev_pref_size = sizeof(named_dev_pref) - 1;
+	const char *prop_name = "devname";
+	const char *name;
+
+	if (ident->devname) {
+		ident_log(prop_name, devname, "Already defined", cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+
+	if (is_devname_ignore(devname) == true) {
+		if (!cmdline)
+			goto pass;
+
+		ident_log(prop_name, devname, "Special keyword is invalid in this context",
+			  cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+
+	if (is_devname_md_numbered(devname) == true || is_devname_md_d_numbered(devname) == true)
+		goto pass;
+
+	if (strncmp(devname, DEV_MD_DIR, DEV_MD_DIR_LEN) == 0)
+		name = devname + DEV_MD_DIR_LEN;
+	else if (strncmp(devname, named_dev_pref, named_dev_pref_size) == 0)
+		name = devname + named_dev_pref_size;
+	else
+		name = devname;
+
+	if (*name == '/' || *name == '<') {
+		ident_log(prop_name, devname, "Cannot be started from \'/\' or \'<\'", cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+
+	if (is_string_lq(name, MD_NAME_MAX + 1) == false) {
+		ident_log(prop_name, devname, "Invalid length", cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+pass:
+	ident->devname = xstrdup(devname);
+	return MDADM_STATUS_SUCCESS;
+}
+
 /**
  * _ident_set_name()- set name in &mddev_ident.
  * @ident: pointer to &mddev_ident.
@@ -219,6 +287,14 @@ static mdadm_status_t _ident_set_name(struct mddev_ident *ident, const char *nam
 	return MDADM_STATUS_SUCCESS;
 }
 
+/**
+ * ident_set_devname()- exported, for cmdline.
+ */
+mdadm_status_t ident_set_devname(struct mddev_ident *ident, const char *name)
+{
+	return _ident_set_devname(ident, name, true);
+}
+
 /**
  * ident_set_name()- exported, for cmdline.
  */
@@ -464,29 +540,7 @@ void arrayline(char *line)
 
 	for (w = dl_next(line); w != line; w = dl_next(w)) {
 		if (w[0] == '/' || strchr(w, '=') == NULL) {
-			/* This names the device, or is '<ignore>'.
-			 * The rules match those in create_mddev.
-			 * 'w' must be:
-			 *  /dev/md/{anything}
-			 *  /dev/mdNN
-			 *  /dev/md_dNN
-			 *  <ignore>
-			 *  or anything that doesn't start '/' or '<'
-			 */
-			if (is_devname_ignore(w) == true ||
-			    strncmp(w, DEV_MD_DIR, DEV_MD_DIR_LEN) == 0 ||
-			    (w[0] != '/' && w[0] != '<') ||
-			    is_devname_md_numbered(w) == true ||
-			    is_devname_md_d_numbered(w) == true) {
-				/* This is acceptable */;
-				if (mis.devname)
-					pr_err("only give one device per ARRAY line: %s and %s\n",
-						mis.devname, w);
-				else
-					mis.devname = w;
-			}else {
-				pr_err("%s is an invalid name for an md device - ignored.\n", w);
-			}
+			_ident_set_devname(&mis, w, false);
 		} else if (strncasecmp(w, "uuid=", 5) == 0) {
 			if (mis.uuid_set)
 				pr_err("only specify uuid once, %s ignored.\n",
diff --git a/mdadm.c b/mdadm.c
index 5084c348..5f8dc1a6 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1284,7 +1284,8 @@ int main(int argc, char *argv[])
 			pr_err("an md device must be given in this mode\n");
 			exit(2);
 		}
-		ident.devname = devlist->devname;
+		if (ident_set_devname(&ident, devlist->devname) != MDADM_STATUS_SUCCESS)
+			exit(1);
 
 		if ((int)ident.super_minor == -2 && c.autof) {
 			pr_err("--super-minor=dev is incompatible with --auto\n");
@@ -1301,13 +1302,6 @@ int main(int argc, char *argv[])
 				exit(1);
 			}
 		} else {
-			char *bname = basename(ident.devname);
-
-			if (strlen(bname) > MD_NAME_MAX) {
-				pr_err("Name %s is too long.\n", ident.devname);
-				exit(1);
-			}
-
 			ret = stat(ident.devname, &stb);
 			if (ident.super_minor == -2 && ret != 0) {
 				pr_err("--super-minor=dev given, and listed device %s doesn't exist.\n",
diff --git a/mdadm.h b/mdadm.h
index 62e15dfa..92a0beb5 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1638,6 +1638,7 @@ extern void manage_fork_fds(int close_all);
 extern int continue_via_systemd(char *devnm, char *service_name, char *prefix);
 
 extern void ident_init(struct mddev_ident *ident);
+extern mdadm_status_t ident_set_devname(struct mddev_ident *ident, const char *devname);
 extern mdadm_status_t ident_set_name(struct mddev_ident *ident, const char *name);
 
 extern int parse_auto(char *str, char *msg, int config);
@@ -1660,7 +1661,7 @@ extern void print_escape(char *str);
 extern int use_udev(void);
 extern unsigned long GCD(unsigned long a, unsigned long b);
 extern int conf_name_is_free(char *name);
-extern bool is_devname_ignore(char *devname);
+extern bool is_devname_ignore(const char *devname);
 extern bool is_devname_md_numbered(const char *devname);
 extern bool is_devname_md_d_numbered(const char *devname);
 extern int conf_verify_devnames(struct mddev_ident *array_list);
diff --git a/tests/00createnames b/tests/00createnames
index 064eeef2..a95e7d2b 100644
--- a/tests/00createnames
+++ b/tests/00createnames
@@ -39,3 +39,6 @@ mdadm -S "/dev/md0"
 names_create "/dev/md0" "name"
 names_verify "/dev/md0" "empty" "name"
 mdadm -S "/dev/md0"
+
+# Devnode is a special ignore keyword. Should be rejected.
+names_create "<ignore>" "name", "true"
diff --git a/tests/templates/names_template b/tests/templates/names_template
index 8d2b5c81..6181bfaa 100644
--- a/tests/templates/names_template
+++ b/tests/templates/names_template
@@ -2,6 +2,7 @@
 function names_create() {
 	local DEVNAME=$1
 	local NAME=$2
+	local NEG_TEST=$3
 
 	if [[ -z "$NAME" ]]; then
 		mdadm -CR "$DEVNAME" -l0 -n 1 $dev0 --force
@@ -9,6 +10,12 @@ function names_create() {
 		mdadm -CR "$DEVNAME" --name="$NAME" --metadata=1.2 -l0 -n 1 $dev0 --force
 	fi
 
+	if [[ "$NEG_TEST" == "true" ]]; then
+		[[ "$?" == "0" ]] && return 0
+		echo "Negative verification failed"
+		exit 1
+	fi
+
 	if [[ "$?" != "0" ]]; then
 		echo "Cannot create device."
 		exit 1
-- 
2.26.2

[PATCH 6/6] mdadm: Follow POSIX Portable Character Set

[Mariusz Tkaczyk]

When the user creates a device with a name that contains whitespace,
mdadm timeouts and throws an error. This issue is caused by udev, which
truncates /dev/md link until the first whitespace.

This patch introduces prohibition of characters other than A-Za-z0-9.-_
in the device name. Also, it prohibits using leading "-" in device name,
so name won't be confused with cli parameter.
Set of allowed characters is taken from POSIX 3.280 Portable Character
Set. Also, device name length now is limited to NAME_MAX.

In some places, there are other requirements for string length (e.g. size
up to MD_NAME_MAX for device name). This routine is made to follow POSIX
and other, more strict limitations should be checked separately.
We are aware of the risk of regression in exceptional cases (as
escape_devname function is removed) that should be fixed by updating
the array name.

The POSIX validation is added for:
- 'name' parameter in every mode.
- first devlist entry, for Build, Create, Assemble, Manage, Grow.
- config entries, both devname and "name=".

Additionally, some manual cleanups are made.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Detail.c          | 17 ++++--------
 config.c          | 13 ++++++---
 lib.c             | 58 ++++++++++++++++++++++++++++-----------
 mdadm.8.in        | 70 ++++++++++++++++++++---------------------------
 mdadm.conf.5.in   |  4 ---
 mdadm.h           |  2 +-
 super-intel.c     | 47 ++++++++++++++++---------------
 tests/00confnames |  4 +--
 8 files changed, 113 insertions(+), 102 deletions(-)

diff --git a/Detail.c b/Detail.c
index 206d88e3..57ac336f 100644
--- a/Detail.c
+++ b/Detail.c
@@ -254,11 +254,9 @@ int Detail(char *dev, struct context *c)
 			fname_from_uuid(st, info, nbuf, ':');
 			printf("MD_UUID=%s\n", nbuf + 5);
 			mp = map_by_uuid(&map, info->uuid);
-			if (mp && mp->path && strncmp(mp->path, DEV_MD_DIR, DEV_MD_DIR_LEN) == 0) {
-				printf("MD_DEVNAME=");
-				print_escape(mp->path + DEV_MD_DIR_LEN);
-				putchar('\n');
-			}
+
+			if (mp && mp->path && strncmp(mp->path, DEV_MD_DIR, DEV_MD_DIR_LEN) == 0)
+				printf("MD_DEVNAME=%s\n", mp->path + DEV_MD_DIR_LEN);
 
 			if (st->ss->export_detail_super)
 				st->ss->export_detail_super(st);
@@ -271,12 +269,9 @@ int Detail(char *dev, struct context *c)
 				__fname_from_uuid(mp->uuid, 0, nbuf, ':');
 				printf("MD_UUID=%s\n", nbuf+5);
 			}
-			if (mp && mp->path &&
-			    strncmp(mp->path, DEV_MD_DIR, DEV_MD_DIR_LEN) == 0) {
-				printf("MD_DEVNAME=");
-				print_escape(mp->path + DEV_MD_DIR_LEN);
-				putchar('\n');
-			}
+			if (mp && mp->path && strncmp(mp->path, DEV_MD_DIR, DEV_MD_DIR_LEN) == 0)
+				printf("MD_DEVNAME=%s\n", mp->path + DEV_MD_DIR_LEN);
+
 			map_free(map);
 		}
 		if (!c->no_devices && sra) {
diff --git a/config.c b/config.c
index d80747a2..38a903c5 100644
--- a/config.c
+++ b/config.c
@@ -199,9 +199,9 @@ inline void ident_init(struct mddev_ident *ident)
  *	/dev/md_d{number} (legacy)
  *	/dev/md_{name}
  *	/dev/md/{name}
- *	{name} - anything that doesn't start from '/' or '<'.
+ *	{name}
  *
- * {name} must follow name's criteria.
+ * {name} must follow name's criteria and be POSIX compatible.
  * If criteria passed, duplicate memory and set devname in @ident.
  *
  * Return: %MDADM_STATUS_SUCCESS or %MDADM_STATUS_ERROR.
@@ -241,8 +241,8 @@ mdadm_status_t _ident_set_devname(struct mddev_ident *ident, const char *devname
 	else
 		name = devname;
 
-	if (*name == '/' || *name == '<') {
-		ident_log(prop_name, devname, "Cannot be started from \'/\' or \'<\'", cmdline);
+	if (is_name_posix_compatible(name) == false) {
+		ident_log(prop_name, name, "Not POSIX compatible", cmdline);
 		return MDADM_STATUS_ERROR;
 	}
 
@@ -283,6 +283,11 @@ static mdadm_status_t _ident_set_name(struct mddev_ident *ident, const char *nam
 		return MDADM_STATUS_ERROR;
 	}
 
+	if (is_name_posix_compatible(name) == false) {
+		ident_log(prop_name, name, "Not POSIX compatible", cmdline);
+		return MDADM_STATUS_ERROR;
+	}
+
 	snprintf(ident->name, MD_NAME_MAX + 1, "%s", name);
 	return MDADM_STATUS_SUCCESS;
 }
diff --git a/lib.c b/lib.c
index 999cd520..d40922d9 100644
--- a/lib.c
+++ b/lib.c
@@ -483,24 +483,50 @@ void print_quoted(char *str)
 	putchar(q);
 }
 
-void print_escape(char *str)
+/**
+ * is_alphanum() - Check if sign is letter or digit.
+ * @c: char to analyze.
+ *
+ * Similar to isalnum() but additional locales are excluded.
+ *
+ * Return: %true on success, %false otherwise.
+ */
+bool is_alphanum(const char c)
 {
-	/* print str, but change space and tab to '_'
-	 * as is suitable for device names
-	 */
-	for (; *str; str++) {
-		switch (*str) {
-		case ' ':
-		case '\t':
-			putchar('_');
-			break;
-		case '/':
-			putchar('-');
-			break;
-		default:
-			putchar(*str);
-		}
+	if (isupper(c) || islower(c) || isdigit(c) != 0)
+		return true;
+	return false;
+}
+
+/**
+ * is_name_posix_compatible() - Check if name is POSIX compatible.
+ * @name: name to check.
+ *
+ *  POSIX portable file name character set contains ASCII letters,
+ *  digits, '_', '.', and '-'. Also forbid leading '-'.
+ *  The length of the name cannot exceed NAME_MAX - 1 (ensure NULL ending).
+ *
+ * Return: %true on success, %false otherwise.
+ */
+bool is_name_posix_compatible(const char * const name)
+{
+	assert(name);
+
+	char allowed_symbols[] = "-_.";
+	const char *n = name;
+
+	if (!is_string_lq(name, NAME_MAX))
+		return false;
+
+	if (*n == '-')
+		return false;
+
+	while (*n != '\0') {
+		if (!is_alphanum(*n) && !strchr(allowed_symbols, *n))
+			return false;
+		n++;
 	}
+	return true;
 }
 
 int check_env(char *name)
diff --git a/mdadm.8.in b/mdadm.8.in
index b7159509..3142436f 100644
--- a/mdadm.8.in
+++ b/mdadm.8.in
@@ -364,7 +364,7 @@ Use the Intel(R) Matrix Storage Manager metadata format.  This creates a
 which is managed in a similar manner to DDF, and is supported by an
 option-rom on some platforms:
 .IP
-.B https://www.intel.com/content/www/us/en/support/products/122484/memory-and-storage/ssd-software/intel-virtual-raid-on-cpu-intel-vroc.html
+.B https://www.intel.com/content/www/us/en/support/products/122484
 .PP
 .RE
 
@@ -932,17 +932,14 @@ option will be ignored.
 .BR \-N ", " \-\-name=
 Set a
 .B name
-for the array.  This is currently only effective when creating an
-array with a version-1 superblock, or an array in a DDF container.
-The name is a simple textual string that can be used to identify array
-components when assembling.  If name is needed but not specified, it
-is taken from the basename of the device that is being created.
-e.g. when creating
-.I /dev/md/home
-the
-.B name
-will default to
-.IR home .
+for the array. It must be
+.BR "POSIX PORTABLE NAME"
+compatible and cannot be longer than 32 chars. This is effective when creating an array
+with a v1 metadata, or an external array.
+
+If name is needed but not specified, it is taken from the basename of the device
+that is being created. See
+.BR "DEVICE NAMES"
 
 .TP
 .BR \-R ", " \-\-run
@@ -1132,8 +1129,10 @@ is much safer.
 
 .TP
 .BR \-N ", " \-\-name=
-Specify the name of the array to assemble.  This must be the name
-that was specified when creating the array.  It must either match
+Specify the name of the array to assemble. It must be
+.BR "POSIX PORTABLE NAME"
+compatible and cannot be longer than 32 chars. This must be the name
+that was specified when creating the array. It must either match
 the name stored in the superblock exactly, or it must match
 with the current
 .I homehost
@@ -2179,14 +2178,17 @@ Usage:
 .I md-device
 .BI \-\-chunk= X
 .BI \-\-level= Y
-.br
 .BI \-\-raid\-devices= Z
 .I devices
 
 .PP
-This usage will initialise a new md array, associate some devices with
+This usage will initialize a new md array, associate some devices with
 it, and activate the array.
 
+.I md-device
+is a new device. This could be standard name or chosen name. For details see:
+.BR "DEVICE NAMES"
+
 The named device will normally not exist when
 .I "mdadm \-\-create"
 is run, but will be created by
@@ -2227,24 +2229,6 @@ array.  This feature can be overridden with the
 .B \-\-force
 option.
 
-When creating an array with version-1 metadata a name for the array is
-required.
-If this is not given with the
-.B \-\-name
-option,
-.I mdadm
-will choose a name based on the last component of the name of the
-device being created.  So if
-.B /dev/md3
-is being created, then the name
-.B 3
-will be chosen.
-If
-.B /dev/md/home
-is being created, then the name
-.B home
-will be used.
-
 When creating a partition based array, using
 .I mdadm
 with version-1.x metadata, the partition type should be set to
@@ -2429,12 +2413,10 @@ and
 
 The
 .B name
-option updates the subarray name in the metadata, it may not affect the
-device node name or the device node symlink until the subarray is
-re\-assembled.  If updating
-.B name
-would change the UUID of an active subarray this operation is blocked,
-and the command will end in an error.
+option updates the subarray name in the metadata. It must be
+.BR "POSIX PORTABLE NAME"
+compatible and cannot be longer than 32 chars. If successes, new value will be respected after
+next assembly.
 
 The
 .B ppl
@@ -3395,6 +3377,10 @@ When
 .B \-\-incremental
 mode is used, this file gets a list of arrays currently being created.
 
+.SH POSIX PORTABLE NAME
+A valid name can only consist of characters "A-Za-z0-9.-_".
+The name cannot start with a leading "-" and cannot exceed 255 chars.
+
 .SH DEVICE NAMES
 
 .I mdadm
@@ -3416,6 +3402,10 @@ can be given, or just the suffix of the second sort of name, such as
 .I home
 can be given.
 
+In every style, raw name must be compatible with
+.BR "POSIX PORTABLE NAME"
+and has to be no longer than 32 chars.
+
 When
 .I mdadm
 chooses device names during auto-assembly or incremental assembly, it
diff --git a/mdadm.conf.5.in b/mdadm.conf.5.in
index bc2295c2..94e23dd0 100644
--- a/mdadm.conf.5.in
+++ b/mdadm.conf.5.in
@@ -717,10 +717,6 @@ ARRAY /dev/md/home UUID=9187a482:5dde19d9:eea3cc4a:d646ab8b
 .br
            auto=part
 .br
-# The name of this array contains a space.
-.br
-ARRAY /dev/md9 name='Data Storage'
-.sp
 POLICY domain=domain1 metadata=imsm path=pci-0000:00:1f.2-scsi-*
 .br
            action=spare
diff --git a/mdadm.h b/mdadm.h
index 92a0beb5..db05d810 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1617,6 +1617,7 @@ extern int check_raid(int fd, char *name);
 extern int check_partitions(int fd, char *dname,
 			    unsigned long long freesize,
 			    unsigned long long size);
+extern bool is_name_posix_compatible(const char *path);
 extern int fstat_is_blkdev(int fd, char *devname, dev_t *rdev);
 extern int stat_is_blkdev(char *devname, dev_t *rdev);
 
@@ -1657,7 +1658,6 @@ extern int conf_get_monitor_delay(void);
 extern char *conf_line(FILE *file);
 extern char *conf_word(FILE *file, int allow_key);
 extern void print_quoted(char *str);
-extern void print_escape(char *str);
 extern int use_udev(void);
 extern unsigned long GCD(unsigned long a, unsigned long b);
 extern int conf_name_is_free(char *name);
diff --git a/super-intel.c b/super-intel.c
index ae0f4a8c..1aa5fec9 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5533,40 +5533,37 @@ static void imsm_update_version_info(struct intel_super *super)
 	}
 }
 
-static int check_name(struct intel_super *super, char *name, int quiet)
+/**
+ * imsm_check_name() - check imsm naming criteria.
+ * @super: &intel_super pointer, not NULL.
+ * @name: name to check.
+ * @verbose: verbose level.
+ *
+ * Name must be no longer than &MAX_RAID_SERIAL_LEN and must be unique across volumes.
+ *
+ * Returns: &true if @name matches, &false otherwise.
+ */
+static bool imsm_is_name_allowed(struct intel_super *super, const char * const name,
+				 const int verbose)
 {
 	struct imsm_super *mpb = super->anchor;
-	char *reason = NULL;
-	char *start = name;
-	size_t len = strlen(name);
 	int i;
 
-	if (len > 0) {
-		while (isspace(start[len - 1]))
-			start[--len] = 0;
-		while (*start && isspace(*start))
-			++start, --len;
-		memmove(name, start, len + 1);
+	if (is_string_lq(name, MAX_RAID_SERIAL_LEN + 1) == false) {
+		pr_vrb("imsm: Name \"%s\" is too long\n", name);
+		return false;
 	}
 
-	if (len > MAX_RAID_SERIAL_LEN)
-		reason = "must be 16 characters or less";
-	else if (len == 0)
-		reason = "must be a non-empty string";
-
 	for (i = 0; i < mpb->num_raid_devs; i++) {
 		struct imsm_dev *dev = get_imsm_dev(super, i);
 
 		if (strncmp((char *) dev->volume, name, MAX_RAID_SERIAL_LEN) == 0) {
-			reason = "already exists";
-			break;
+			pr_vrb("imsm: Name \"%s\" already exists\n", name);
+			return false;
 		}
 	}
 
-	if (reason && !quiet)
-		pr_err("imsm volume name %s\n", reason);
-
-	return !reason;
+	return true;
 }
 
 static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
@@ -5661,8 +5658,9 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
 		}
 	}
 
-	if (!check_name(super, name, 0))
+	if (imsm_is_name_allowed(super, name, 1) == false)
 		return 0;
+
 	dv = xmalloc(sizeof(*dv));
 	dev = xcalloc(1, sizeof(*dev) + sizeof(__u32) * (info->raid_disks - 1));
 	/*
@@ -7975,7 +7973,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 		char *ep;
 		int vol;
 
-		if (!check_name(super, name, 0))
+		if (imsm_is_name_allowed(super, name, 1) == false)
 			return 2;
 
 		vol = strtoul(subarray, &ep, 10);
@@ -10302,7 +10300,8 @@ static void imsm_process_update(struct supertype *st,
 			if (a->info.container_member == target)
 				break;
 		dev = get_imsm_dev(super, u->dev_idx);
-		if (a || !check_name(super, name, 1)) {
+
+		if (a || !dev || imsm_is_name_allowed(super, name, 0) == false) {
 			dprintf("failed to rename subarray-%d\n", target);
 			break;
 		}
diff --git a/tests/00confnames b/tests/00confnames
index 4990cb5e..9961772c 100644
--- a/tests/00confnames
+++ b/tests/00confnames
@@ -79,10 +79,10 @@ names_verify "/dev/md127" "name" "name"
 mdadm -S "/dev/md127"
 
 # 11. <devname> with some special symbols and locales, no <name>.
-# It needs to wait a while for timeout because udev cannot create a link - known issue.
+# <devname> should be ignored.
 names_make_conf $_UUID "tźż-\.,<>st+-" "empty" $config
 mdadm -I $dev0 --config=$config
-names_verify "/dev/md127" "tźż-\.,<>st+-" "name"
+names_verify "/dev/md127" "name" "name"
 mdadm -S "/dev/md127"
 
 # 12. No <devname> and <name> set.
-- 
2.26.2

Re: [PATCH v2 0/6] mdadm: POSIX portable naming rules

[Paul Menzel]

Dear Mariusz,


Am 01.06.23 um 09:27 schrieb Mariusz Tkaczyk:

> To avoid problem with udev and VROC UEFI driver I developed stronger
> naming policy, basing on POSIX portable names standard. Verification is

s/basing/based/

> added for names and config entries. In case of an issue, user can update
> name to make it compatible (for IMSM and native).

What is the VROC UEFI driver, and what is the problem exactly to risk 
regressions on the user side? Why can’t the UEFI driver be fixed?


Kind regards,

Paul


> The changes here may cause /dev/md/ link will be different than before
> mdadm update. To make any of that happen user need to use unusual naming
> convention, like:
> - special, non standard signs like, $,%,&,* or space.
> - '-' used as first sign.
> - locals.
> 
> Note: I didn't analyze configurations with "names=1". If name cannot be
> determined mdadm should fallback to default numbered dev creation.
> 
> If you are planning release soon then feel free to merge it after the
> release. It is a potential regression point.
> 
> It is a new version of [1] but it is strongly rebuild. Here is a list
> of changes:
> 1. negative and positive test scenarios for both create and config
>     entries are added.
> 2. Save parsed parameters in dedicated structs. It is a way to control
>     what is parsed, assuming that we should use dedicated set_* function.
> 3. Verification for config entries is added.
> 5. Improved error logging for names:
>     - during creation, these messages are errors, printed to stderr.
>     - for config entries, messages are just a warnings printed to stdout.
> 6. Error messages reworked.
> 7. Updates in manual.
> 
> [1] https://lore.kernel.org/linux-raid/20221221115019.26276-1-mariusz.tkaczyk@linux.intel.com/
> 
> Mariusz Tkaczyk (6):
>    tests: create names_template
>    tests: create 00confnames
>    mdadm: set ident.devname if applicable
>    mdadm: refactor ident->name handling
>    mdadm: define ident_set_devname()
>    mdadm: Follow POSIX Portable Character Set
> 
>   Build.c                        |  21 ++--
>   Create.c                       |  35 +++----
>   Detail.c                       |  17 ++-
>   config.c                       | 184 +++++++++++++++++++++++++++------
>   lib.c                          |  76 +++++++++++---
>   mdadm.8.in                     |  70 ++++++-------
>   mdadm.c                        |  73 +++++--------
>   mdadm.conf.5.in                |   4 -
>   mdadm.h                        |  36 ++++---
>   super-intel.c                  |  47 +++++----
>   tests/00confnames              | 107 +++++++++++++++++++
>   tests/00createnames            |  89 ++++------------
>   tests/templates/names_template |  80 ++++++++++++++
>   13 files changed, 551 insertions(+), 288 deletions(-)
>   create mode 100644 tests/00confnames
>   create mode 100644 tests/templates/names_template
> 

Re: [PATCH v2 0/6] mdadm: POSIX portable naming rules

[Mariusz Tkaczyk]

On Thu, 1 Jun 2023 10:57:47 +0200
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Dear Mariusz,
> 
> 
> Am 01.06.23 um 09:27 schrieb Mariusz Tkaczyk:
> 
> > To avoid problem with udev and VROC UEFI driver I developed stronger
> > naming policy, basing on POSIX portable names standard. Verification is  
> 
> s/basing/based/

Noted, thanks.

> 
> > added for names and config entries. In case of an issue, user can update
> > name to make it compatible (for IMSM and native).  
> 
> What is the VROC UEFI driver, and what is the problem exactly to risk 
> regressions on the user side? Why can’t the UEFI driver be fixed?

Thanks for your question. I should mark this as [RFC].

VROC solution (called IMSM here, or super-intel) comes with UEFI support. You
can manipulate arrays in UEFI, for example you can create an array to have it
available as installation destination for new OS.

I decided that it is worth to mention that we have UEFI driver and there are
some differences in allowed names. Now I think, that it is irrelevant in this
context because my main concern is udevd. The "POSIX portable names" are strict
and that should prevent any differences in names.

Current form is aggressive (do not allow at all) and if you think that I should
lower the requirements or bring the option to pass without verification to
ensure backward compatibility- will do, just let me know.

I would like to add exceptions based on community input because it affect
multiple metadata formats.
I think that we should require to follow those rules for new arrays.

Thanks,
Mariusz
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> > The changes here may cause /dev/md/ link will be different than before
> > mdadm update. To make any of that happen user need to use unusual naming
> > convention, like:
> > - special, non standard signs like, $,%,&,* or space.
> > - '-' used as first sign.
> > - locals.
> > 
> > Note: I didn't analyze configurations with "names=1". If name cannot be
> > determined mdadm should fallback to default numbered dev creation.
> > 
> > If you are planning release soon then feel free to merge it after the
> > release. It is a potential regression point.
> > 
> > It is a new version of [1] but it is strongly rebuild. Here is a list
> > of changes:
> > 1. negative and positive test scenarios for both create and config
> >     entries are added.
> > 2. Save parsed parameters in dedicated structs. It is a way to control
> >     what is parsed, assuming that we should use dedicated set_* function.
> > 3. Verification for config entries is added.
> > 5. Improved error logging for names:
> >     - during creation, these messages are errors, printed to stderr.
> >     - for config entries, messages are just a warnings printed to stdout.
> > 6. Error messages reworked.
> > 7. Updates in manual.
> > 
> > [1]
> > https://lore.kernel.org/linux-raid/20221221115019.26276-1-mariusz.tkaczyk@linux.intel.com/
> > 
> > Mariusz Tkaczyk (6):
> >    tests: create names_template
> >    tests: create 00confnames
> >    mdadm: set ident.devname if applicable
> >    mdadm: refactor ident->name handling
> >    mdadm: define ident_set_devname()
> >    mdadm: Follow POSIX Portable Character Set
> > 
> >   Build.c                        |  21 ++--
> >   Create.c                       |  35 +++----
> >   Detail.c                       |  17 ++-
> >   config.c                       | 184 +++++++++++++++++++++++++++------
> >   lib.c                          |  76 +++++++++++---
> >   mdadm.8.in                     |  70 ++++++-------
> >   mdadm.c                        |  73 +++++--------
> >   mdadm.conf.5.in                |   4 -
> >   mdadm.h                        |  36 ++++---
> >   super-intel.c                  |  47 +++++----
> >   tests/00confnames              | 107 +++++++++++++++++++
> >   tests/00createnames            |  89 ++++------------
> >   tests/templates/names_template |  80 ++++++++++++++
> >   13 files changed, 551 insertions(+), 288 deletions(-)
> >   create mode 100644 tests/00confnames
> >   create mode 100644 tests/templates/names_template
> >   

Re: [PATCH v2 0/6] mdadm: POSIX portable naming rules

[Jes Sorensen]

On 6/1/23 03:27, Mariusz Tkaczyk wrote:
> Hi Jes,
> To avoid problem with udev and VROC UEFI driver I developed stronger
> naming policy, basing on POSIX portable names standard. Verification is
> added for names and config entries. In case of an issue, user can update
> name to make it compatible (for IMSM and native).
> 
> The changes here may cause /dev/md/ link will be different than before
> mdadm update. To make any of that happen user need to use unusual naming
> convention, like:
> - special, non standard signs like, $,%,&,* or space.
> - '-' used as first sign.
> - locals.
> 
> Note: I didn't analyze configurations with "names=1". If name cannot be
> determined mdadm should fallback to default numbered dev creation.
> 
> If you are planning release soon then feel free to merge it after the
> release. It is a potential regression point.
> 
> It is a new version of [1] but it is strongly rebuild. Here is a list
> of changes:
> 1. negative and positive test scenarios for both create and config
>    entries are added.
> 2. Save parsed parameters in dedicated structs. It is a way to control
>    what is parsed, assuming that we should use dedicated set_* function.
> 3. Verification for config entries is added.
> 5. Improved error logging for names:
>    - during creation, these messages are errors, printed to stderr.
>    - for config entries, messages are just a warnings printed to stdout.
> 6. Error messages reworked.
> 7. Updates in manual.
> 
> [1] https://lore.kernel.org/linux-raid/20221221115019.26276-1-mariusz.tkaczyk@linux.intel.com/
> 
> Mariusz Tkaczyk (6):
>   tests: create names_template
>   tests: create 00confnames
>   mdadm: set ident.devname if applicable
>   mdadm: refactor ident->name handling
>   mdadm: define ident_set_devname()
>   mdadm: Follow POSIX Portable Character Set

Applied!

Thanks,
Jes

Re: [PATCH v2 0/6] mdadm: POSIX portable naming rules

[Stefan Fleischmann]

On 6/1/23 03:27, Mariusz Tkaczyk wrote:
> Hi Jes,
> To avoid problem with udev and VROC UEFI driver I developed stronger
> naming policy, basing on POSIX portable names standard. Verification is
> added for names and config entries. In case of an issue, user can update
> name to make it compatible (for IMSM and native).
> 
> The changes here may cause /dev/md/ link will be different than before
> mdadm update. To make any of that happen user need to use unusual naming
> convention, like:
> - special, non standard signs like, $,%,&,* or space.
> - '-' used as first sign.
> - locals.
> 
> Note: I didn't analyze configurations with "names=1". If name cannot be
> determined mdadm should fallback to default numbered dev creation.
> 
> If you are planning release soon then feel free to merge it after the
> release. It is a potential regression point.
> 
> It is a new version of [1] but it is strongly rebuild. Here is a list
> of changes:
> 1. negative and positive test scenarios for both create and config
>    entries are added.
> 2. Save parsed parameters in dedicated structs. It is a way to control
>    what is parsed, assuming that we should use dedicated set_* function.
> 3. Verification for config entries is added.
> 5. Improved error logging for names:
>    - during creation, these messages are errors, printed to stderr.
>    - for config entries, messages are just a warnings printed to stdout.
> 6. Error messages reworked.
> 7. Updates in manual.
> 
> [1] https://lore.kernel.org/linux-raid/20221221115019.26276-1-mariusz.tkaczyk@linux.intel.com/
> 
> Mariusz Tkaczyk (6):
>   tests: create names_template
>   tests: create 00confnames
>   mdadm: set ident.devname if applicable
>   mdadm: refactor ident->name handling
>   mdadm: define ident_set_devname()
>   mdadm: Follow POSIX Portable Character Set


Hi,

it seems to me that the behavior after this change is inconsistent. The
default naming scheme for a created device is still <HOSTNAME>:<ID>. When
I create a new MD device with:

 # mdadm --version
 mdadm - v4.2 - 2021-12-30 - Debian 4.2+20231121-1
 # mdadm --create /dev/md32 -l 1 -n 2 /dev/loop2 /dev/loop3
 [...]
 # mdadm --detail /dev/md32
  /dev/md32:
           Version : 1.2
     Creation Time : Mon Jan  1 12:33:05 2024
  [...]
              Name : gondolin:32  (local to host gondolin)
              UUID : d84b8e26:3675a722:61869b83:670699da

The name set automatically by mdadm is not allowed by the new naming
rules, and mdadm complains about that later on. So for consistency one
would either have to include : in the allowed character list or change
the default naming scheme. Or am I missing something here?

Happy new year!
Stefan

Re: [PATCH v2 0/6] mdadm: POSIX portable naming rules

[Mariusz Tkaczyk]

On Mon, 1 Jan 2024 13:32:55 +0100
Stefan Fleischmann <sfle@kth.se> wrote:

> On 6/1/23 03:27, Mariusz Tkaczyk wrote:
> > Hi Jes,
> > To avoid problem with udev and VROC UEFI driver I developed stronger
> > naming policy, basing on POSIX portable names standard. Verification is
> > added for names and config entries. In case of an issue, user can update
> > name to make it compatible (for IMSM and native).
> > 
> > The changes here may cause /dev/md/ link will be different than before
> > mdadm update. To make any of that happen user need to use unusual naming
> > convention, like:
> > - special, non standard signs like, $,%,&,* or space.
> > - '-' used as first sign.
> > - locals.
> > 
> > Note: I didn't analyze configurations with "names=1". If name cannot be
> > determined mdadm should fallback to default numbered dev creation.
> > 
> > If you are planning release soon then feel free to merge it after the
> > release. It is a potential regression point.
> > 
> > It is a new version of [1] but it is strongly rebuild. Here is a list
> > of changes:
> > 1. negative and positive test scenarios for both create and config
> >    entries are added.
> > 2. Save parsed parameters in dedicated structs. It is a way to control
> >    what is parsed, assuming that we should use dedicated set_* function.
> > 3. Verification for config entries is added.
> > 5. Improved error logging for names:
> >    - during creation, these messages are errors, printed to stderr.
> >    - for config entries, messages are just a warnings printed to stdout.
> > 6. Error messages reworked.
> > 7. Updates in manual.
> > 
> > [1]
> > https://lore.kernel.org/linux-raid/20221221115019.26276-1-mariusz.tkaczyk@linux.intel.com/
> > 
> > Mariusz Tkaczyk (6):
> >   tests: create names_template
> >   tests: create 00confnames
> >   mdadm: set ident.devname if applicable
> >   mdadm: refactor ident->name handling
> >   mdadm: define ident_set_devname()
> >   mdadm: Follow POSIX Portable Character Set  
> 
> 
> Hi,
> 
> it seems to me that the behavior after this change is inconsistent. The
> default naming scheme for a created device is still <HOSTNAME>:<ID>. When
> I create a new MD device with:
> 
>  # mdadm --version
>  mdadm - v4.2 - 2021-12-30 - Debian 4.2+20231121-1
>  # mdadm --create /dev/md32 -l 1 -n 2 /dev/loop2 /dev/loop3
>  [...]
>  # mdadm --detail /dev/md32
>   /dev/md32:
>            Version : 1.2
>      Creation Time : Mon Jan  1 12:33:05 2024
>   [...]
>               Name : gondolin:32  (local to host gondolin)
>               UUID : d84b8e26:3675a722:61869b83:670699da
> 
> The name set automatically by mdadm is not allowed by the new naming
> rules, and mdadm complains about that later on. So for consistency one
> would either have to include : in the allowed character list or change
> the default naming scheme. Or am I missing something here?
> 
> Happy new year!
> Stefan

Hi Stefan,
Yeah, I noticed this problem recently but I do not fixed it yet. For sure it
will be fixed before release.
It does not prompt about the name in metadata but about name entry in config
file. I analyzed the usage on name from config and it seems to be something
which can be dropped but I will reconsider it deeper later.

Could you please test if removing "name=hostanme:id" from ARRAYLINE in
mdadm.conf fixes issue for you?

Thanks,
Mariusz