[PATCH 0/3] Add test for names.

[PATCH 0/3] Add test for names.

[Mariusz Tkaczyk]

Hi Jes,
In first patch test for MD device names is added. This is entry for
future create_mddev() updates - we need test to avoid regression.
Beside that, small code improvements are done.

Thanks,
Mariusz

Mariusz Tkaczyk (3):
  tests: add test for names
  mdadm: remove symlink option
  mdadm: move data_offset to struct shape

 Create.c            | 16 ++++----
 Grow.c              |  7 ++--
 ReadMe.c            |  1 -
 config.c            |  7 +---
 mdadm.8.in          |  9 -----
 mdadm.c             | 40 +++++--------------
 mdadm.conf.5.in     | 15 --------
 mdadm.h             |  7 +---
 tests/00createnames | 93 +++++++++++++++++++++++++++++++++++++++++++++
 9 files changed, 116 insertions(+), 79 deletions(-)
 create mode 100644 tests/00createnames

-- 
2.26.2

[PATCH 1/3] tests: add test for names

[Mariusz Tkaczyk]

Current behavior is not documented and tested. This test is a base for
future improvements. It is enough to test it only with native metadata,
because it is generic code. Generated properties are passed to metadata
handler.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 tests/00createnames | 93 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 tests/00createnames

diff --git a/tests/00createnames b/tests/00createnames
new file mode 100644
index 0000000..64b81b9
--- /dev/null
+++ b/tests/00createnames
@@ -0,0 +1,93 @@
+set -x -e
+
+# 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"
+mdadm -S "/dev/md127"
+
+_create "name"
+_verify "/dev/md127" "name" "name"
+mdadm -S "/dev/md127"
+
+# Use 'mdX' as name.
+_create "/dev/md/md0"
+_verify "/dev/md127" "md0" "md0"
+mdadm -S "/dev/md127"
+
+_create "md0"
+_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"
+mdadm -S "/dev/md127"
+
+_create "devnode" "name"
+_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"
+mdadm -S "/dev/md0"
+
+# Devnode points to /dev/ directory and name is set.
+_create "/dev/md0" "name"
+_verify "/dev/md0" "empty" "name"
+mdadm -S "/dev/md0"
-- 
2.26.2

[PATCH 2/3] mdadm: remove symlink option

[Mariusz Tkaczyk]

The option is not used. Remove it from code.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 ReadMe.c        |  1 -
 config.c        |  7 +------
 mdadm.8.in      |  9 ---------
 mdadm.c         | 20 --------------------
 mdadm.conf.5.in | 15 ---------------
 mdadm.h         |  2 --
 6 files changed, 1 insertion(+), 53 deletions(-)

diff --git a/ReadMe.c b/ReadMe.c
index bec1be9..14a5214 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -146,7 +146,6 @@ struct option long_options[] = {
     {"nofailfast",0, 0,  NoFailFast},
     {"re-add",    0, 0,  ReAdd},
     {"homehost",  1, 0,  HomeHost},
-    {"symlinks",  1, 0,  Symlinks},
     {"data-offset",1, 0, DataOffset},
     {"nodes",1, 0, Nodes}, /* also for --assemble */
     {"home-cluster",1, 0, ClusterName},
diff --git a/config.c b/config.c
index 9c72545..dc1620c 100644
--- a/config.c
+++ b/config.c
@@ -194,7 +194,6 @@ struct mddev_dev *load_containers(void)
 
 struct createinfo createinfo = {
 	.autof = 2, /* by default, create devices with standard names */
-	.symlinks = 1,
 	.names = 0, /* By default, stick with numbered md devices. */
 	.bblist = 1, /* Use a bad block list by default */
 #ifdef DEBIAN
@@ -310,11 +309,7 @@ static void createline(char *line)
 			if (!createinfo.supertype)
 				pr_err("metadata format %s unknown, ignoring\n",
 					w+9);
-		} else if (strncasecmp(w, "symlinks=yes", 12) == 0)
-			createinfo.symlinks = 1;
-		else if  (strncasecmp(w, "symlinks=no", 11) == 0)
-			createinfo.symlinks = 0;
-		else if (strncasecmp(w, "names=yes", 12) == 0)
+		} else if (strncasecmp(w, "names=yes", 12) == 0)
 			createinfo.names = 1;
 		else if  (strncasecmp(w, "names=no", 11) == 0)
 			createinfo.names = 0;
diff --git a/mdadm.8.in b/mdadm.8.in
index 0be02e4..f273622 100644
--- a/mdadm.8.in
+++ b/mdadm.8.in
@@ -1048,11 +1048,6 @@ simultaneously. If not specified, this defaults to 4.
 Specify journal device for the RAID-4/5/6 array. The journal device
 should be a SSD with reasonable lifetime.
 
-.TP
-.BR \-\-symlinks
-Auto creation of symlinks in /dev to /dev/md, option --symlinks must
-be 'no' or 'yes' and work with --create and --build.
-
 .TP
 .BR \-k ", " \-\-consistency\-policy=
 Specify how the array maintains consistency in case of unexpected shutdown.
@@ -1405,10 +1400,6 @@ Reshape can be continued later using the
 .B \-\-continue
 option for the grow command.
 
-.TP
-.BR \-\-symlinks
-See this option under Create and Build options.
-
 .SH For Manage mode:
 
 .TP
diff --git a/mdadm.c b/mdadm.c
index be40686..3e8bfef 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -59,7 +59,6 @@ int main(int argc, char *argv[])
 	struct mddev_dev *dv;
 	mdu_array_info_t array;
 	int devs_found = 0;
-	char *symlinks = NULL;
 	int grow_continue = 0;
 	/* autof indicates whether and how to create device node.
 	 * bottom 3 bits are style.  Rest (when shifted) are number of parts
@@ -662,13 +661,6 @@ int main(int argc, char *argv[])
 		case O(ASSEMBLE,Auto): /* auto-creation of device node */
 			c.autof = parse_auto(optarg, "--auto flag", 0);
 			continue;
-
-		case O(CREATE,Symlinks):
-		case O(BUILD,Symlinks):
-		case O(ASSEMBLE,Symlinks): /* auto creation of symlinks in /dev to /dev/md */
-			symlinks = optarg;
-			continue;
-
 		case O(BUILD,'f'): /* force honouring '-n 1' */
 		case O(BUILD,Force): /* force honouring '-n 1' */
 		case O(GROW,'f'): /* ditto */
@@ -1323,18 +1315,6 @@ int main(int argc, char *argv[])
 		exit(2);
 	}
 
-	if (symlinks) {
-		struct createinfo *ci = conf_get_create_info();
-
-		if (strcasecmp(symlinks, "yes") == 0)
-			ci->symlinks = 1;
-		else if (strcasecmp(symlinks, "no") == 0)
-			ci->symlinks = 0;
-		else {
-			pr_err("option --symlinks must be 'no' or 'yes'\n");
-			exit(2);
-		}
-	}
 	/* Ok, got the option parsing out of the way
 	 * hopefully it's mostly right but there might be some stuff
 	 * missing
diff --git a/mdadm.conf.5.in b/mdadm.conf.5.in
index cd4e6a9..bc2295c 100644
--- a/mdadm.conf.5.in
+++ b/mdadm.conf.5.in
@@ -338,21 +338,6 @@ missing device entries should be created.
 The name of the metadata format to use if none is explicitly given.
 This can be useful to impose a system-wide default of version-1 superblocks.
 
-.TP
-.B symlinks=no
-Normally when creating devices in
-.B /dev/md/
-.I mdadm
-will create a matching symlink from
-.B /dev/
-with a name starting
-.B md
-or
-.BR md_ .
-Give
-.B symlinks=no
-to suppress this symlink creation.
-
 .TP
 .B names=yes
 Since Linux 2.6.29 it has been possible to create
diff --git a/mdadm.h b/mdadm.h
index 974415b..8d1af86 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -394,7 +394,6 @@ struct createinfo {
 	int	gid;
 	int	autof;
 	int	mode;
-	int	symlinks;
 	int	names;
 	int	bblist;
 	struct supertype *supertype;
@@ -441,7 +440,6 @@ enum special_options {
 	BackupFile,
 	HomeHost,
 	AutoHomeHost,
-	Symlinks,
 	AutoDetect,
 	Waitclean,
 	DetailPlatform,
-- 
2.26.2

[PATCH 3/3] mdadm: move data_offset to struct shape

[Mariusz Tkaczyk]

Data offset is a shape property so move it there to remove additional
parameter from some functions.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Create.c | 16 ++++++++--------
 Grow.c   |  7 +++----
 mdadm.c  | 20 +++++++++-----------
 mdadm.h  |  5 ++---
 4 files changed, 22 insertions(+), 26 deletions(-)

diff --git a/Create.c b/Create.c
index c84c1ac..e06ec2a 100644
--- a/Create.c
+++ b/Create.c
@@ -95,7 +95,7 @@ int Create(struct supertype *st, char *mddev,
 	   char *name, int *uuid,
 	   int subdevs, struct mddev_dev *devlist,
 	   struct shape *s,
-	   struct context *c, unsigned long long data_offset)
+	   struct context *c)
 {
 	/*
 	 * Create a new raid array.
@@ -288,7 +288,7 @@ int Create(struct supertype *st, char *mddev,
 	newsize = s->size * 2;
 	if (st && ! st->ss->validate_geometry(st, s->level, s->layout, s->raiddisks,
 					      &s->chunk, s->size*2,
-					      data_offset, NULL,
+					      s->data_offset, NULL,
 					      &newsize, s->consistency_policy,
 					      c->verbose >= 0))
 		return 1;
@@ -323,10 +323,10 @@ int Create(struct supertype *st, char *mddev,
 	info.array.working_disks = 0;
 	dnum = 0;
 	for (dv = devlist; dv; dv = dv->next)
-		if (data_offset == VARIABLE_OFFSET)
+		if (s->data_offset == VARIABLE_OFFSET)
 			dv->data_offset = INVALID_SECTORS;
 		else
-			dv->data_offset = data_offset;
+			dv->data_offset = s->data_offset;
 
 	for (dv=devlist; dv && !have_container; dv=dv->next, dnum++) {
 		char *dname = dv->devname;
@@ -342,7 +342,7 @@ int Create(struct supertype *st, char *mddev,
 			missing_disks ++;
 			continue;
 		}
-		if (data_offset == VARIABLE_OFFSET) {
+		if (s->data_offset == VARIABLE_OFFSET) {
 			doff = strchr(dname, ':');
 			if (doff) {
 				*doff++ = 0;
@@ -350,7 +350,7 @@ int Create(struct supertype *st, char *mddev,
 			} else
 				dv->data_offset = INVALID_SECTORS;
 		} else
-			dv->data_offset = data_offset;
+			dv->data_offset = s->data_offset;
 
 		dfd = open(dname, O_RDONLY);
 		if (dfd < 0) {
@@ -535,7 +535,7 @@ int Create(struct supertype *st, char *mddev,
 			if (!st->ss->validate_geometry(st, s->level, s->layout,
 						       s->raiddisks,
 						       &s->chunk, minsize*2,
-						       data_offset,
+						       s->data_offset,
 						       NULL, NULL,
 						       s->consistency_policy, 0)) {
 				pr_err("devices too large for RAID level %d\n", s->level);
@@ -754,7 +754,7 @@ int Create(struct supertype *st, char *mddev,
 		}
 	}
 	if (!st->ss->init_super(st, &info.array, s, name, c->homehost, uuid,
-				data_offset))
+				s->data_offset))
 		goto abort_locked;
 
 	total_slots = info.array.nr_disks;
diff --git a/Grow.c b/Grow.c
index 8c520d4..e2ee91b 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1775,7 +1775,6 @@ static int reshape_container(char *container, char *devname,
 
 int Grow_reshape(char *devname, int fd,
 		 struct mddev_dev *devlist,
-		 unsigned long long data_offset,
 		 struct context *c, struct shape *s)
 {
 	/* Make some changes in the shape of an array.
@@ -1821,7 +1820,7 @@ int Grow_reshape(char *devname, int fd,
 		return 1;
 	}
 
-	if (data_offset != INVALID_SECTORS && array.level != 10 &&
+	if (s->data_offset != INVALID_SECTORS && array.level != 10 &&
 	    (array.level < 4 || array.level > 6)) {
 		pr_err("--grow --data-offset not yet supported\n");
 		return 1;
@@ -2179,7 +2178,7 @@ size_change_error:
 	if ((s->level == UnSet || s->level == array.level) &&
 	    (s->layout_str == NULL) &&
 	    (s->chunk == 0 || s->chunk == array.chunk_size) &&
-	    data_offset == INVALID_SECTORS &&
+	    s->data_offset == INVALID_SECTORS &&
 	    (s->raiddisks == 0 || s->raiddisks == array.raid_disks)) {
 		/* Nothing more to do */
 		if (!changed && c->verbose >= 0)
@@ -2379,7 +2378,7 @@ size_change_error:
 		}
 		sync_metadata(st);
 		rv = reshape_array(container, fd, devname, st, &info, c->force,
-				   devlist, data_offset, c->backup_file,
+				   devlist, s->data_offset, c->backup_file,
 				   c->verbose, 0, 0, 0);
 		frozen = 0;
 	}
diff --git a/mdadm.c b/mdadm.c
index 3e8bfef..cf7175c 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -49,7 +49,6 @@ int main(int argc, char *argv[])
 	int i;
 
 	unsigned long long array_size = 0;
-	unsigned long long data_offset = INVALID_SECTORS;
 	struct mddev_ident ident;
 	char *configfile = NULL;
 	int devmode = 0;
@@ -79,6 +78,7 @@ int main(int argc, char *argv[])
 		.layout		= UnSet,
 		.bitmap_chunk	= UnSet,
 		.consistency_policy	= CONSISTENCY_POLICY_UNKNOWN,
+		.data_offset = INVALID_SECTORS,
 	};
 
 	char sys_hostname[256];
@@ -478,15 +478,15 @@ int main(int argc, char *argv[])
 
 		case O(CREATE,DataOffset):
 		case O(GROW,DataOffset):
-			if (data_offset != INVALID_SECTORS) {
+			if (s.data_offset != INVALID_SECTORS) {
 				pr_err("data-offset may only be specified one. Second value is %s.\n", optarg);
 				exit(2);
 			}
 			if (mode == CREATE && strcmp(optarg, "variable") == 0)
-				data_offset = VARIABLE_OFFSET;
+				s.data_offset = VARIABLE_OFFSET;
 			else
-				data_offset = parse_size(optarg);
-			if (data_offset == INVALID_SECTORS) {
+				s.data_offset = parse_size(optarg);
+			if (s.data_offset == INVALID_SECTORS) {
 				pr_err("invalid data-offset: %s\n",
 					optarg);
 				exit(2);
@@ -1414,7 +1414,7 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
-	if (c.backup_file && data_offset != INVALID_SECTORS) {
+	if (c.backup_file && s.data_offset != INVALID_SECTORS) {
 		pr_err("--backup-file and --data-offset are incompatible\n");
 		exit(2);
 	}
@@ -1585,8 +1585,7 @@ int main(int argc, char *argv[])
 
 		rv = Create(ss, devlist->devname,
 			    ident.name, ident.uuid_set ? ident.uuid : NULL,
-			    devs_found-1, devlist->next,
-			    &s, &c, data_offset);
+			    devs_found - 1, devlist->next, &s, &c);
 		break;
 	case MISC:
 		if (devmode == 'E') {
@@ -1704,10 +1703,9 @@ int main(int argc, char *argv[])
 						   c.verbose);
 		else if (s.size > 0 || s.raiddisks || s.layout_str ||
 			 s.chunk != 0 || s.level != UnSet ||
-			 data_offset != INVALID_SECTORS) {
+			 s.data_offset != INVALID_SECTORS) {
 			rv = Grow_reshape(devlist->devname, mdfd,
-					  devlist->next,
-					  data_offset, &c, &s);
+					  devlist->next, &c, &s);
 		} else if (s.consistency_policy != CONSISTENCY_POLICY_UNKNOWN) {
 			rv = Grow_consistency_policy(devlist->devname, mdfd, &c, &s);
 		} else if (array_size == 0)
diff --git a/mdadm.h b/mdadm.h
index 8d1af86..74ec05b 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -594,6 +594,7 @@ struct shape {
 	int	assume_clean;
 	int	write_behind;
 	unsigned long long size;
+	unsigned long long data_offset;
 	int	consistency_policy;
 };
 
@@ -1430,7 +1431,6 @@ extern int Grow_addbitmap(char *devname, int fd,
 			  struct context *c, struct shape *s);
 extern int Grow_reshape(char *devname, int fd,
 			struct mddev_dev *devlist,
-			unsigned long long data_offset,
 			struct context *c, struct shape *s);
 extern int Grow_restart(struct supertype *st, struct mdinfo *info,
 			int *fdlist, int cnt, char *backup_file, int verbose);
@@ -1461,8 +1461,7 @@ extern int Create(struct supertype *st, char *mddev,
 		  char *name, int *uuid,
 		  int subdevs, struct mddev_dev *devlist,
 		  struct shape *s,
-		  struct context *c,
-		  unsigned long long data_offset);
+		  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

Re: [PATCH 0/3] Add test for names.

[Jes Sorensen]

On 7/19/22 08:48, Mariusz Tkaczyk wrote:
> Hi Jes,
> In first patch test for MD device names is added. This is entry for
> future create_mddev() updates - we need test to avoid regression.
> Beside that, small code improvements are done.

Hi Mariusz,

All applied!

Thanks,
Jes