[PATCH 0/2] Add map_num_s

[PATCH 0/2] Add map_num_s

[Mariusz Tkaczyk]

Hi Jes,
In this patchset not null version of map_num() was added.
Additionally, I propagaded default_layout() for Build mode.

I tested changes and I didn't find any regression.

Mariusz Tkaczyk (2):
  Create, Build: use default_layout()
  mdadm: add map_num_s()

 Assemble.c    |  6 ++---
 Build.c       | 23 +-----------------
 Create.c      | 67 +++++++++++++++++++++++++++++++--------------------
 Detail.c      |  4 +--
 Grow.c        | 16 ++++++------
 Query.c       |  4 +--
 maps.c        | 24 ++++++++++++++++++
 mdadm.c       | 20 +++++++--------
 mdadm.h       |  3 ++-
 super-ddf.c   |  6 ++---
 super-intel.c |  2 +-
 super0.c      |  2 +-
 super1.c      |  2 +-
 sysfs.c       |  9 ++++---
 14 files changed, 103 insertions(+), 85 deletions(-)

-- 
2.26.2

[PATCH 1/2] Create, Build: use default_layout()

[Mariusz Tkaczyk]

This code is duplicated for Build mode so make default_layout() extern
and use it. Simplify the function structure.

It introduced change for Build mode, now for raid0 RAID0_ORIG_LAYOUT
will be returned same as for Create.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Build.c  | 23 +------------------
 Create.c | 67 ++++++++++++++++++++++++++++++++++----------------------
 mdadm.h  |  1 +
 3 files changed, 43 insertions(+), 48 deletions(-)

diff --git a/Build.c b/Build.c
index 962c2e3..8d6f6f5 100644
--- a/Build.c
+++ b/Build.c
@@ -71,28 +71,7 @@ int Build(char *mddev, struct mddev_dev *devlist,
 	}
 
 	if (s->layout == UnSet)
-		switch(s->level) {
-		default: /* no layout */
-			s->layout = 0;
-			break;
-		case 10:
-			s->layout = 0x102; /* near=2, far=1 */
-			if (c->verbose > 0)
-				pr_err("layout defaults to n1\n");
-			break;
-		case 5:
-		case 6:
-			s->layout = map_name(r5layout, "default");
-			if (c->verbose > 0)
-				pr_err("layout defaults to %s\n", map_num(r5layout, s->layout));
-			break;
-		case LEVEL_FAULTY:
-			s->layout = map_name(faultylayout, "default");
-
-			if (c->verbose > 0)
-				pr_err("layout defaults to %s\n", map_num(faultylayout, s->layout));
-			break;
-		}
+		s->layout = default_layout(NULL, s->level, c->verbose);
 
 	/* We need to create the device.  It can have no name. */
 	map_lock(&map);
diff --git a/Create.c b/Create.c
index 0ff1922..9ea19de 100644
--- a/Create.c
+++ b/Create.c
@@ -39,39 +39,54 @@ static int round_size_and_verify(unsigned long long *size, int chunk)
 	return 0;
 }
 
-static int default_layout(struct supertype *st, int level, int verbose)
+/**
+ * default_layout() - Get default layout for level.
+ * @st: metadata requested, could be NULL.
+ * @level: raid level requested.
+ * @verbose: verbose level.
+ *
+ * Try to ask metadata handler first, otherwise use global defaults.
+ *
+ * Return: Layout or &UnSet, return value meaning depends of level used.
+ */
+int default_layout(struct supertype *st, int level, int verbose)
 {
 	int layout = UnSet;
+	mapping_t *layout_map = NULL;
+	char *layout_name = NULL;
 
 	if (st && st->ss->default_geometry)
 		st->ss->default_geometry(st, &level, &layout, NULL);
 
-	if (layout == UnSet)
-		switch(level) {
-		default: /* no layout */
-			layout = 0;
-			break;
-		case 0:
-			layout = RAID0_ORIG_LAYOUT;
-			break;
-		case 10:
-			layout = 0x102; /* near=2, far=1 */
-			if (verbose > 0)
-				pr_err("layout defaults to n2\n");
-			break;
-		case 5:
-		case 6:
-			layout = map_name(r5layout, "default");
-			if (verbose > 0)
-				pr_err("layout defaults to %s\n", map_num(r5layout, layout));
-			break;
-		case LEVEL_FAULTY:
-			layout = map_name(faultylayout, "default");
+	if (layout != UnSet)
+		return layout;
 
-			if (verbose > 0)
-				pr_err("layout defaults to %s\n", map_num(faultylayout, layout));
-			break;
-		}
+	switch (level) {
+	default: /* no layout */
+		layout = 0;
+		break;
+	case 0:
+		layout = RAID0_ORIG_LAYOUT;
+		break;
+	case 10:
+		layout = 0x102; /* near=2, far=1 */
+		layout_name = "n2";
+		break;
+	case 5:
+	case 6:
+		layout_map = r5layout;
+		break;
+	case LEVEL_FAULTY:
+		layout_map = faultylayout;
+		break;
+	}
+
+	if (layout_map) {
+		layout = map_name(layout_map, "default");
+		layout_name = map_num(layout_map, layout);
+	}
+	if (layout_name && verbose > 0)
+		pr_err("layout defaults to %s\n", layout_name);
 
 	return layout;
 }
diff --git a/mdadm.h b/mdadm.h
index c7268a7..6aff034 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1511,6 +1511,7 @@ extern int get_linux_version(void);
 extern int mdadm_version(char *version);
 extern unsigned long long parse_size(char *size);
 extern int parse_uuid(char *str, int uuid[4]);
+int default_layout(struct supertype *st, int level, int verbose);
 extern int is_near_layout_10(int layout);
 extern int parse_layout_10(char *layout);
 extern int parse_layout_faulty(char *layout);
-- 
2.26.2

[PATCH 2/2] mdadm: add map_num_s()

[Mariusz Tkaczyk]

map_num() returns NULL if key is not defined. This patch adds
alternative, non NULL version for cases where NULL is not expected.

There are many printf() calls where map_num() is called on variable
without NULL verification. It works, even if NULL is passed because
gcc is able to ignore NULL argument quietly but the behavior is
undefined. For safety reasons such usages will use map_num_s() now.
It is a potential point of regression.

Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 Assemble.c    |  6 ++----
 Create.c      |  2 +-
 Detail.c      |  4 ++--
 Grow.c        | 16 ++++++++--------
 Query.c       |  4 ++--
 maps.c        | 24 ++++++++++++++++++++++++
 mdadm.c       | 20 ++++++++++----------
 mdadm.h       |  2 +-
 super-ddf.c   |  6 +++---
 super-intel.c |  2 +-
 super0.c      |  2 +-
 super1.c      |  2 +-
 sysfs.c       |  9 +++++----
 13 files changed, 61 insertions(+), 38 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 704b829..9eac9ce 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -63,7 +63,7 @@ static void set_array_assembly_status(struct context *c,
 				   struct assembly_array_info *arr)
 {
 	int raid_disks = arr->preexist_cnt + arr->new_cnt;
-	char *status_msg = map_num(assemble_statuses, status);
+	char *status_msg = map_num_s(assemble_statuses, status);
 
 	if (c->export && result)
 		*result |= status;
@@ -77,9 +77,7 @@ static void set_array_assembly_status(struct context *c,
 		fprintf(stderr, " (%d new)", arr->new_cnt);
 	if (arr->exp_cnt)
 		fprintf(stderr, " ( + %d for expansion)", arr->exp_cnt);
-	if (status_msg)
-		fprintf(stderr, " %s", status_msg);
-	fprintf(stderr, ".\n");
+	fprintf(stderr, " %s.\n", status_msg);
 }
 
 static int name_matches(char *found, char *required, char *homehost, int require_homehost)
diff --git a/Create.c b/Create.c
index 9ea19de..c84c1ac 100644
--- a/Create.c
+++ b/Create.c
@@ -83,7 +83,7 @@ int default_layout(struct supertype *st, int level, int verbose)
 
 	if (layout_map) {
 		layout = map_name(layout_map, "default");
-		layout_name = map_num(layout_map, layout);
+		layout_name = map_num_s(layout_map, layout);
 	}
 	if (layout_name && verbose > 0)
 		pr_err("layout defaults to %s\n", layout_name);
diff --git a/Detail.c b/Detail.c
index 95d4cc7..ce7a844 100644
--- a/Detail.c
+++ b/Detail.c
@@ -495,8 +495,8 @@ int Detail(char *dev, struct context *c)
 			if (array.state & (1 << MD_SB_CLEAN)) {
 				if ((array.level == 0) ||
 				    (array.level == LEVEL_LINEAR))
-					arrayst = map_num(sysfs_array_states,
-							  sra->array_state);
+					arrayst = map_num_s(sysfs_array_states,
+							       sra->array_state);
 				else
 					arrayst = "clean";
 			} else {
diff --git a/Grow.c b/Grow.c
index 9c6fc95..bb505c3 100644
--- a/Grow.c
+++ b/Grow.c
@@ -548,7 +548,7 @@ int Grow_consistency_policy(char *devname, int fd, struct context *c, struct sha
 	if (s->consistency_policy != CONSISTENCY_POLICY_RESYNC &&
 	    s->consistency_policy != CONSISTENCY_POLICY_PPL) {
 		pr_err("Operation not supported for consistency policy %s\n",
-		       map_num(consistency_policies, s->consistency_policy));
+		       map_num_s(consistency_policies, s->consistency_policy));
 		return 1;
 	}
 
@@ -579,14 +579,14 @@ int Grow_consistency_policy(char *devname, int fd, struct context *c, struct sha
 
 	if (sra->consistency_policy == (unsigned)s->consistency_policy) {
 		pr_err("Consistency policy is already %s\n",
-		       map_num(consistency_policies, s->consistency_policy));
+		       map_num_s(consistency_policies, s->consistency_policy));
 		ret = 1;
 		goto free_info;
 	} else if (sra->consistency_policy != CONSISTENCY_POLICY_RESYNC &&
 		   sra->consistency_policy != CONSISTENCY_POLICY_PPL) {
 		pr_err("Current consistency policy is %s, cannot change to %s\n",
-		       map_num(consistency_policies, sra->consistency_policy),
-		       map_num(consistency_policies, s->consistency_policy));
+		       map_num_s(consistency_policies, sra->consistency_policy),
+		       map_num_s(consistency_policies, s->consistency_policy));
 		ret = 1;
 		goto free_info;
 	}
@@ -705,8 +705,8 @@ int Grow_consistency_policy(char *devname, int fd, struct context *c, struct sha
 	}
 
 	ret = sysfs_set_str(sra, NULL, "consistency_policy",
-			    map_num(consistency_policies,
-				    s->consistency_policy));
+			    map_num_s(consistency_policies,
+					 s->consistency_policy));
 	if (ret)
 		pr_err("Failed to change array consistency policy\n");
 
@@ -2236,7 +2236,7 @@ size_change_error:
 		info.new_layout = UnSet;
 		if (info.array.level == 6 && info.new_level == UnSet) {
 			char l[40], *h;
-			strcpy(l, map_num(r6layout, info.array.layout));
+			strcpy(l, map_num_s(r6layout, info.array.layout));
 			h = strrchr(l, '-');
 			if (h && strcmp(h, "-6") == 0) {
 				*h = 0;
@@ -2261,7 +2261,7 @@ size_change_error:
 			info.new_layout = info.array.layout;
 		else if (info.array.level == 5 && info.new_level == 6) {
 			char l[40];
-			strcpy(l, map_num(r5layout, info.array.layout));
+			strcpy(l, map_num_s(r5layout, info.array.layout));
 			strcat(l, "-6");
 			info.new_layout = map_name(r6layout, l);
 		} else {
diff --git a/Query.c b/Query.c
index 23fbf8a..adcd231 100644
--- a/Query.c
+++ b/Query.c
@@ -93,7 +93,7 @@ int Query(char *dev)
 	else {
 		printf("%s: %s %s %d devices, %d spare%s. Use mdadm --detail for more detail.\n",
 		       dev, human_size_brief(larray_size,IEC),
-		       map_num(pers, level), raid_disks,
+		       map_num_s(pers, level), raid_disks,
 		       spare_disks, spare_disks == 1 ? "" : "s");
 	}
 	st = guess_super(fd);
@@ -131,7 +131,7 @@ int Query(char *dev)
 		       dev,
 		       info.disk.number, info.array.raid_disks,
 		       activity,
-		       map_num(pers, info.array.level),
+		       map_num_s(pers, info.array.level),
 		       mddev);
 		if (st->ss == &super0)
 			put_md_name(mddev);
diff --git a/maps.c b/maps.c
index a4fd279..20fcf71 100644
--- a/maps.c
+++ b/maps.c
@@ -166,6 +166,30 @@ mapping_t sysfs_array_states[] = {
 	{ NULL, ARRAY_UNKNOWN_STATE }
 };
 
+/**
+ * map_num_s() - Safer alternative of map_num() function.
+ * @map: map to search.
+ * @num: key to match.
+ *
+ * Shall be used only if key existence is quaranted.
+ *
+ * Return: Pointer to name of the element.
+ */
+char *map_num_s(mapping_t *map, int num)
+{
+	char *ret = map_num(map, num);
+
+	assert(ret);
+	return ret;
+}
+
+/**
+ * map_num() - get element name by key.
+ * @map: map to search.
+ * @num: key to match.
+ *
+ * Return: Pointer to name of the element or NULL.
+ */
 char *map_num(mapping_t *map, int num)
 {
 	while (map->name) {
diff --git a/mdadm.c b/mdadm.c
index 26299b2..be40686 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -280,8 +280,8 @@ int main(int argc, char *argv[])
 			else
 				fprintf(stderr, "-%c", opt);
 			fprintf(stderr, " would set mdadm mode to \"%s\", but it is already set to \"%s\".\n",
-				map_num(modes, newmode),
-				map_num(modes, mode));
+				map_num_s(modes, newmode),
+				map_num_s(modes, mode));
 			exit(2);
 		} else if (!mode && newmode) {
 			mode = newmode;
@@ -544,7 +544,7 @@ int main(int argc, char *argv[])
 			switch(s.level) {
 			default:
 				pr_err("layout not meaningful for %s arrays.\n",
-					map_num(pers, s.level));
+					map_num_s(pers, s.level));
 				exit(2);
 			case UnSet:
 				pr_err("raid level must be given before layout.\n");
@@ -1248,10 +1248,10 @@ int main(int argc, char *argv[])
 		if (option_index > 0)
 			pr_err(":option --%s not valid in %s mode\n",
 				long_options[option_index].name,
-				map_num(modes, mode));
+				map_num_s(modes, mode));
 		else
 			pr_err("option -%c not valid in %s mode\n",
-				opt, map_num(modes, mode));
+				opt, map_num_s(modes, mode));
 		exit(2);
 
 	}
@@ -1276,7 +1276,7 @@ int main(int argc, char *argv[])
 		if (s.consistency_policy != CONSISTENCY_POLICY_UNKNOWN &&
 		    s.consistency_policy != CONSISTENCY_POLICY_JOURNAL) {
 			pr_err("--write-journal is not supported with consistency policy: %s\n",
-			       map_num(consistency_policies, s.consistency_policy));
+			       map_num_s(consistency_policies, s.consistency_policy));
 			exit(2);
 		}
 	}
@@ -1285,12 +1285,12 @@ int main(int argc, char *argv[])
 	    s.consistency_policy != CONSISTENCY_POLICY_UNKNOWN) {
 		if (s.level <= 0) {
 			pr_err("--consistency-policy not meaningful with level %s.\n",
-			       map_num(pers, s.level));
+			       map_num_s(pers, s.level));
 			exit(2);
 		} else if (s.consistency_policy == CONSISTENCY_POLICY_JOURNAL &&
 			   !s.journaldisks) {
 			pr_err("--write-journal is required for consistency policy: %s\n",
-			       map_num(consistency_policies, s.consistency_policy));
+			       map_num_s(consistency_policies, s.consistency_policy));
 			exit(2);
 		} else if (s.consistency_policy == CONSISTENCY_POLICY_PPL &&
 			   s.level != 5) {
@@ -1300,14 +1300,14 @@ int main(int argc, char *argv[])
 			   (!s.bitmap_file ||
 			    strcmp(s.bitmap_file, "none") == 0)) {
 			pr_err("--bitmap is required for consistency policy: %s\n",
-			       map_num(consistency_policies, s.consistency_policy));
+			       map_num_s(consistency_policies, s.consistency_policy));
 			exit(2);
 		} else if (s.bitmap_file &&
 			   strcmp(s.bitmap_file, "none") != 0 &&
 			   s.consistency_policy != CONSISTENCY_POLICY_BITMAP &&
 			   s.consistency_policy != CONSISTENCY_POLICY_JOURNAL) {
 			pr_err("--bitmap is not compatible with consistency policy: %s\n",
-			       map_num(consistency_policies, s.consistency_policy));
+			       map_num_s(consistency_policies, s.consistency_policy));
 			exit(2);
 		}
 	}
diff --git a/mdadm.h b/mdadm.h
index 6aff034..9e9c4d8 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -769,7 +769,7 @@ extern int restore_stripes(int *dest, unsigned long long *offsets,
 #endif
 
 #define SYSLOG_FACILITY LOG_DAEMON
-
+char *map_num_s(mapping_t *map, int num);
 extern char *map_num(mapping_t *map, int num);
 extern int map_name(mapping_t *map, char *name);
 extern mapping_t r0layout[], r5layout[], r6layout[],
diff --git a/super-ddf.c b/super-ddf.c
index 3f304cd..8cda23a 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1477,13 +1477,13 @@ static void examine_vds(struct ddf_super *sb)
 		printf("\n");
 		printf("         unit[%d] : %d\n", i, be16_to_cpu(ve->unit));
 		printf("        state[%d] : %s, %s%s\n", i,
-		       map_num(ddf_state, ve->state & 7),
+		       map_num_s(ddf_state, ve->state & 7),
 		       (ve->state & DDF_state_morphing) ? "Morphing, ": "",
 		       (ve->state & DDF_state_inconsistent)? "Not Consistent" : "Consistent");
 		printf("   init state[%d] : %s\n", i,
-		       map_num(ddf_init_state, ve->init_state&DDF_initstate_mask));
+		       map_num_s(ddf_init_state, ve->init_state & DDF_initstate_mask));
 		printf("       access[%d] : %s\n", i,
-		       map_num(ddf_access, (ve->init_state & DDF_access_mask) >> 6));
+		       map_num_s(ddf_access, (ve->init_state & DDF_access_mask) >> 6));
 		printf("         Name[%d] : %.16s\n", i, ve->name);
 		examine_vd(i, sb, ve->guid);
 	}
diff --git a/super-intel.c b/super-intel.c
index d5fad10..f72d485 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5625,7 +5625,7 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
 		free(dev);
 		free(dv);
 		pr_err("imsm does not support consistency policy %s\n",
-		       map_num(consistency_policies, s->consistency_policy));
+		       map_num_s(consistency_policies, s->consistency_policy));
 		return 0;
 	}
 
diff --git a/super0.c b/super0.c
index b79b97a..61c9ec1 100644
--- a/super0.c
+++ b/super0.c
@@ -288,7 +288,7 @@ static void export_examine_super0(struct supertype *st)
 {
 	mdp_super_t *sb = st->sb;
 
-	printf("MD_LEVEL=%s\n", map_num(pers, sb->level));
+	printf("MD_LEVEL=%s\n", map_num_s(pers, sb->level));
 	printf("MD_DEVICES=%d\n", sb->raid_disks);
 	if (sb->minor_version >= 90)
 		printf("MD_UUID=%08x:%08x:%08x:%08x\n",
diff --git a/super1.c b/super1.c
index a12a5bc..e3e2f95 100644
--- a/super1.c
+++ b/super1.c
@@ -671,7 +671,7 @@ static void export_examine_super1(struct supertype *st)
 	int len = 32;
 	int layout;
 
-	printf("MD_LEVEL=%s\n", map_num(pers, __le32_to_cpu(sb->level)));
+	printf("MD_LEVEL=%s\n", map_num_s(pers, __le32_to_cpu(sb->level)));
 	printf("MD_DEVICES=%d\n", __le32_to_cpu(sb->raid_disks));
 	for (i = 0; i < 32; i++)
 		if (sb->set_name[i] == '\n' || sb->set_name[i] == '\0') {
diff --git a/sysfs.c b/sysfs.c
index 2995713..0d98a65 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -689,7 +689,7 @@ int sysfs_set_array(struct mdinfo *info, int vers)
 	if (info->array.level < 0)
 		return 0; /* FIXME */
 	rv |= sysfs_set_str(info, NULL, "level",
-			    map_num(pers, info->array.level));
+			    map_num_s(pers, info->array.level));
 	if (info->reshape_active && info->delta_disks != UnSet)
 		raid_disks -= info->delta_disks;
 	rv |= sysfs_set_num(info, NULL, "raid_disks", raid_disks);
@@ -724,9 +724,10 @@ int sysfs_set_array(struct mdinfo *info, int vers)
 	}
 
 	if (info->consistency_policy == CONSISTENCY_POLICY_PPL) {
-		if (sysfs_set_str(info, NULL, "consistency_policy",
-				  map_num(consistency_policies,
-					  info->consistency_policy))) {
+		char *policy = map_num_s(consistency_policies,
+					    info->consistency_policy);
+
+		if (sysfs_set_str(info, NULL, "consistency_policy", policy)) {
 			pr_err("This kernel does not support PPL. Falling back to consistency-policy=resync.\n");
 			info->consistency_policy = CONSISTENCY_POLICY_RESYNC;
 		}
-- 
2.26.2

Re: [PATCH 0/2] Add map_num_s

[Mariusz Tkaczyk]

Hi Coly,
Could you review?

Thanks,
Mariusz

On Thu, 20 Jan 2022 13:18:31 +0100
Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:
> Hi Jes,
> In this patchset not null version of map_num() was added.
> Additionally, I propagaded default_layout() for Build mode.
> 
> I tested changes and I didn't find any regression.
> 
> Mariusz Tkaczyk (2):
>   Create, Build: use default_layout()
>   mdadm: add map_num_s()
> 
>  Assemble.c    |  6 ++---
>  Build.c       | 23 +-----------------
>  Create.c      | 67
> +++++++++++++++++++++++++++++++-------------------- Detail.c      |
> 4 +-- Grow.c        | 16 ++++++------
>  Query.c       |  4 +--
>  maps.c        | 24 ++++++++++++++++++
>  mdadm.c       | 20 +++++++--------
>  mdadm.h       |  3 ++-
>  super-ddf.c   |  6 ++---
>  super-intel.c |  2 +-
>  super0.c      |  2 +-
>  super1.c      |  2 +-
>  sysfs.c       |  9 ++++---
>  14 files changed, 103 insertions(+), 85 deletions(-)
> 

Re: [PATCH 1/2] Create, Build: use default_layout()

[Jes Sorensen]

On 1/20/22 07:18, Mariusz Tkaczyk wrote:
> This code is duplicated for Build mode so make default_layout() extern
> and use it. Simplify the function structure.
> 
> It introduced change for Build mode, now for raid0 RAID0_ORIG_LAYOUT
> will be returned same as for Create.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
>  Build.c  | 23 +------------------
>  Create.c | 67 ++++++++++++++++++++++++++++++++++----------------------
>  mdadm.h  |  1 +
>  3 files changed, 43 insertions(+), 48 deletions(-)

Looks good, applied!

Thanks,
Jes

Re: [PATCH 2/2] mdadm: add map_num_s()

[Jes Sorensen]

On 1/20/22 07:18, Mariusz Tkaczyk wrote:
> map_num() returns NULL if key is not defined. This patch adds
> alternative, non NULL version for cases where NULL is not expected.
> 
> There are many printf() calls where map_num() is called on variable
> without NULL verification. It works, even if NULL is passed because
> gcc is able to ignore NULL argument quietly but the behavior is
> undefined. For safety reasons such usages will use map_num_s() now.
> It is a potential point of regression.

Hi Mariusz,

I'll be honest with you, I don't like assert(), I consider it a lame
excuse for proper error handling. That said, not blaming you as this is
old code and it would take a lot of cleaning up, so this is better than
nothing.

I have applied it with one minor change:

> diff --git a/mdadm.h b/mdadm.h
> index 6aff034..9e9c4d8 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -769,7 +769,7 @@ extern int restore_stripes(int *dest, unsigned long long *offsets,
>  #endif
>  
>  #define SYSLOG_FACILITY LOG_DAEMON
> -
> +char *map_num_s(mapping_t *map, int num);
>  extern char *map_num(mapping_t *map, int num);
>  extern int map_name(mapping_t *map, char *name);
>  extern mapping_t r0layout[], r5layout[], r6layout[],

I changed this to be extern to be consistent with the other declarations.

Thanks,
Jes

Re: [PATCH 2/2] mdadm: add map_num_s()

[Mariusz Tkaczyk]

On Mon, 4 Apr 2022 21:32:09 -0400
Jes Sorensen <jes@trained-monkey.org> wrote:

> On 1/20/22 07:18, Mariusz Tkaczyk wrote:
> > map_num() returns NULL if key is not defined. This patch adds
> > alternative, non NULL version for cases where NULL is not expected.
> > 
> > There are many printf() calls where map_num() is called on variable
> > without NULL verification. It works, even if NULL is passed because
> > gcc is able to ignore NULL argument quietly but the behavior is
> > undefined. For safety reasons such usages will use map_num_s() now.
> > It is a potential point of regression.  
> 
> Hi Mariusz,
> 
> I'll be honest with you, I don't like assert(), I consider it a lame
> excuse for proper error handling. That said, not blaming you as this
> is old code and it would take a lot of cleaning up, so this is better
> than nothing.

And that is true, assert() is not for errors handling. Is was made for
verifying function/application flows. Like here, I made not null
function and I guarantee that won't be returned. If it comes, then it
is a developer mistake and assert() should discover that during
testing.

Please see man:
https://man7.org/linux/man-pages/man3/assert.3.html

       If the macro NDEBUG is defined at the moment <assert.h> was last
       included, the macro assert() generates no code, and hence does
       nothing at all.  It is not recommended to define NDEBUG if using
       assert() to detect error conditions since the software may behave
       non-deterministically.

For production, the macro should be set and it generates no code. So
the behavior is configurable and OSV should add this flag to their
mdadm builds. In this case, we will end with previous implementation but
no additional error handling is required to satisfy our and external
requirements (like static code analysis).

It is also useful to validate function parameters which must be set, to
not insert dead conditions. Ideally, we should execute test with
asserts enabled to verify use cases.

Thanks,
Mariusz