[PATCH 00/10] Block update-subarray and refactor context update

[PATCH 00/10] Block update-subarray and refactor context update

[Mateusz Kusiak]

This patchset serves three main purposes.

Block updates on active volume with update-subarray and split subset of
options for --update and --update-subarray.

Remove dead code from super-ddf.

Change context->update from string to enum.

Mateusz Kusiak (10):
  mdadm: Add option validation for --update-subarray
  Fix --update-subarray on active volume
  Add code specific update options to enum.
  super-ddf: Remove update_super_ddf.
  super0: refactor the code for enum
  super1: refactor the code for enum
  super-intel: refactor the code for enum
  Change update to enum in update_super and update_subarray
  Manage&Incremental: code refactor, string to enum
  Change char* to enum in context->update & refactor code

 Assemble.c    |  46 ++++++++--------
 Examine.c     |   2 +-
 Grow.c        |  17 +++---
 Incremental.c |   8 +--
 Manage.c      |  42 ++++++++------
 ReadMe.c      |  31 +++++++++++
 maps.c        |  31 +++++++++++
 mdadm.c       | 124 ++++++++++++++---------------------------
 mdadm.h       |  65 +++++++++++++++++++---
 super-ddf.c   |  70 -----------------------
 super-intel.c |  49 ++++++++++-------
 super0.c      | 103 ++++++++++++++++++++--------------
 super1.c      | 150 +++++++++++++++++++++++++++++---------------------
 13 files changed, 399 insertions(+), 339 deletions(-)

-- 
2.26.2

[PATCH 01/10] mdadm: Add option validation for --update-subarray

[Mateusz Kusiak]

Subset of options available for "--update" is not same as for "--update-subarray".
Define maps and enum for update options and use them instead of direct comparisons.
Add proper error message.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 ReadMe.c | 31 ++++++++++++++++++
 maps.c   | 31 ++++++++++++++++++
 mdadm.c  | 99 ++++++++++++++++----------------------------------------
 mdadm.h  | 32 +++++++++++++++++-
 4 files changed, 121 insertions(+), 72 deletions(-)

diff --git a/ReadMe.c b/ReadMe.c
index 7518a32a..50e6f987 100644
--- a/ReadMe.c
+++ b/ReadMe.c
@@ -656,3 +656,34 @@ char *mode_help[mode_count] = {
 	[GROW]		= Help_grow,
 	[INCREMENTAL]	= Help_incr,
 };
+
+/**
+ * fprint_update_options() - Print valid update options depending on the mode.
+ * @outf: File (output stream)
+ * @update_mode: Used to distinguish update and update_subarray
+ */
+void fprint_update_options(FILE *outf, enum update_opt update_mode)
+{
+	int counter = UOPT_NAME, breakpoint = UOPT_HELP;
+	mapping_t *map = update_options;
+
+	if (!outf)
+		return;
+	if (update_mode == UOPT_SUBARRAY_ONLY) {
+		breakpoint = UOPT_SUBARRAY_ONLY;
+		fprintf(outf, "Valid --update options for update-subarray are:\n\t");
+	} else
+		fprintf(outf, "Valid --update options are:\n\t");
+	while (map->num) {
+		if (map->num >= breakpoint)
+			break;
+		fprintf(outf, "'%s', ", map->name);
+		if (counter % 5 == 0)
+			fprintf(outf, "\n\t");
+		counter++;
+		map++;
+	}
+	if ((counter - 1) % 5)
+		fprintf(outf, "\n");
+	fprintf(outf, "\r");
+}
diff --git a/maps.c b/maps.c
index 20fcf719..b586679a 100644
--- a/maps.c
+++ b/maps.c
@@ -165,6 +165,37 @@ mapping_t sysfs_array_states[] = {
 	{ "broken", ARRAY_BROKEN },
 	{ NULL, ARRAY_UNKNOWN_STATE }
 };
+/**
+ * mapping_t update_options - stores supported update options.
+ */
+mapping_t update_options[] = {
+	{ "name", UOPT_NAME },
+	{ "ppl", UOPT_PPL },
+	{ "no-ppl", UOPT_NO_PPL },
+	{ "bitmap", UOPT_BITMAP },
+	{ "no-bitmap", UOPT_NO_BITMAP },
+	{ "sparc2.2", UOPT_SPARC22 },
+	{ "super-minor", UOPT_SUPER_MINOR },
+	{ "summaries", UOPT_SUMMARIES },
+	{ "resync", UOPT_RESYNC },
+	{ "uuid", UOPT_UUID },
+	{ "homehost", UOPT_HOMEHOST },
+	{ "home-cluster", UOPT_HOME_CLUSTER },
+	{ "nodes", UOPT_NODES },
+	{ "devicesize", UOPT_DEVICESIZE },
+	{ "bbl", UOPT_BBL },
+	{ "no-bbl", UOPT_NO_BBL },
+	{ "force-no-bbl", UOPT_FORCE_NO_BBL },
+	{ "metadata", UOPT_METADATA },
+	{ "revert-reshape", UOPT_REVERT_RESHAPE },
+	{ "layout-original", UOPT_LAYOUT_ORIGINAL },
+	{ "layout-alternate", UOPT_LAYOUT_ALTERNATE },
+	{ "layout-unspecified", UOPT_LAYOUT_UNSPECIFIED },
+	{ "byteorder", UOPT_BYTEORDER },
+	{ "help", UOPT_HELP },
+	{ "?", UOPT_HELP },
+	{ NULL, UOPT_UNDEFINED}
+};
 
 /**
  * map_num_s() - Safer alternative of map_num() function.
diff --git a/mdadm.c b/mdadm.c
index 56722ed9..3705d114 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -101,7 +101,7 @@ int main(int argc, char *argv[])
 	char *dump_directory = NULL;
 
 	int print_help = 0;
-	FILE *outf;
+	FILE *outf = NULL;
 
 	int mdfd = -1;
 	int locked = 0;
@@ -753,82 +753,39 @@ int main(int argc, char *argv[])
 				pr_err("Only subarrays can be updated in misc mode\n");
 				exit(2);
 			}
+
 			c.update = optarg;
-			if (strcmp(c.update, "sparc2.2") == 0)
-				continue;
-			if (strcmp(c.update, "super-minor") == 0)
-				continue;
-			if (strcmp(c.update, "summaries") == 0)
-				continue;
-			if (strcmp(c.update, "resync") == 0)
-				continue;
-			if (strcmp(c.update, "uuid") == 0)
-				continue;
-			if (strcmp(c.update, "name") == 0)
-				continue;
-			if (strcmp(c.update, "homehost") == 0)
-				continue;
-			if (strcmp(c.update, "home-cluster") == 0)
-				continue;
-			if (strcmp(c.update, "nodes") == 0)
-				continue;
-			if (strcmp(c.update, "devicesize") == 0)
-				continue;
-			if (strcmp(c.update, "bitmap") == 0)
-				continue;
-			if (strcmp(c.update, "no-bitmap") == 0)
-				continue;
-			if (strcmp(c.update, "bbl") == 0)
-				continue;
-			if (strcmp(c.update, "no-bbl") == 0)
-				continue;
-			if (strcmp(c.update, "force-no-bbl") == 0)
-				continue;
-			if (strcmp(c.update, "ppl") == 0)
-				continue;
-			if (strcmp(c.update, "no-ppl") == 0)
-				continue;
-			if (strcmp(c.update, "metadata") == 0)
-				continue;
-			if (strcmp(c.update, "revert-reshape") == 0)
-				continue;
-			if (strcmp(c.update, "layout-original") == 0 ||
-			    strcmp(c.update, "layout-alternate") == 0 ||
-			    strcmp(c.update, "layout-unspecified") == 0)
-				continue;
-			if (strcmp(c.update, "byteorder") == 0) {
+			enum update_opt updateopt = map_name(update_options, c.update);
+			enum update_opt print_mode = UOPT_HELP;
+			const char *error_addon = "update option";
+
+			if (devmode == UpdateSubarray) {
+				print_mode = UOPT_SUBARRAY_ONLY;
+				error_addon = "update-subarray option";
+
+				if (updateopt > UOPT_SUBARRAY_ONLY && updateopt < UOPT_HELP)
+					updateopt = UOPT_UNDEFINED;
+			}
+
+			switch (updateopt) {
+			case UOPT_UNDEFINED:
+				pr_err("'--update=%s' is invalid %s. ",
+					c.update, error_addon);
+				outf = stderr;
+			case UOPT_HELP:
+				if (!outf)
+					outf = stdout;
+				fprint_update_options(outf, print_mode);
+				exit(outf == stdout ? 0 : 2);
+			case UOPT_BYTEORDER:
 				if (ss) {
 					pr_err("must not set metadata type with --update=byteorder.\n");
 					exit(2);
 				}
-				for(i = 0; !ss && superlist[i]; i++)
-					ss = superlist[i]->match_metadata_desc(
-						"0.swap");
-				if (!ss) {
-					pr_err("INTERNAL ERROR cannot find 0.swap\n");
-					exit(2);
-				}
-
-				continue;
+			default:
+				break;
 			}
-			if (strcmp(c.update,"?") == 0 ||
-			    strcmp(c.update, "help") == 0) {
-				outf = stdout;
-				fprintf(outf, "%s: ", Name);
-			} else {
-				outf = stderr;
-				fprintf(outf,
-					"%s: '--update=%s' is invalid.  ",
-					Name, c.update);
-			}
-			fprintf(outf, "Valid --update options are:\n"
-		"     'sparc2.2', 'super-minor', 'uuid', 'name', 'nodes', 'resync',\n"
-		"     'summaries', 'homehost', 'home-cluster', 'byteorder', 'devicesize',\n"
-		"     'bitmap', 'no-bitmap', 'metadata', 'revert-reshape'\n"
-		"     'bbl', 'no-bbl', 'force-no-bbl', 'ppl', 'no-ppl'\n"
-		"     'layout-original', 'layout-alternate', 'layout-unspecified'\n"
-				);
-			exit(outf == stdout ? 0 : 2);
+			continue;
 
 		case O(MANAGE,'U'):
 			/* update=devicesize is allowed with --re-add */
diff --git a/mdadm.h b/mdadm.h
index 163f4a49..43e6b544 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -497,6 +497,36 @@ enum special_options {
 	ConsistencyPolicy,
 };
 
+enum update_opt {
+	UOPT_NAME = 1,
+	UOPT_PPL,
+	UOPT_NO_PPL,
+	UOPT_BITMAP,
+	UOPT_NO_BITMAP,
+	UOPT_SUBARRAY_ONLY,
+	UOPT_SPARC22,
+	UOPT_SUPER_MINOR,
+	UOPT_SUMMARIES,
+	UOPT_RESYNC,
+	UOPT_UUID,
+	UOPT_HOMEHOST,
+	UOPT_HOME_CLUSTER,
+	UOPT_NODES,
+	UOPT_DEVICESIZE,
+	UOPT_BBL,
+	UOPT_NO_BBL,
+	UOPT_FORCE_NO_BBL,
+	UOPT_METADATA,
+	UOPT_REVERT_RESHAPE,
+	UOPT_LAYOUT_ORIGINAL,
+	UOPT_LAYOUT_ALTERNATE,
+	UOPT_LAYOUT_UNSPECIFIED,
+	UOPT_BYTEORDER,
+	UOPT_HELP,
+	UOPT_UNDEFINED
+};
+extern void fprint_update_options(FILE *outf, enum update_opt update_mode);
+
 enum prefix_standard {
 	JEDEC,
 	IEC
@@ -776,7 +806,7 @@ extern char *map_num(mapping_t *map, int num);
 extern int map_name(mapping_t *map, char *name);
 extern mapping_t r0layout[], r5layout[], r6layout[],
 	pers[], modes[], faultylayout[];
-extern mapping_t consistency_policies[], sysfs_array_states[];
+extern mapping_t consistency_policies[], sysfs_array_states[], update_options[];
 
 extern char *map_dev_preferred(int major, int minor, int create,
 			       char *prefer);
-- 
2.26.2

Re: [PATCH 01/10] mdadm: Add option validation for --update-subarray

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Subset of options available for "--update" is not same as for "--update-subarray".
> Define maps and enum for update options and use them instead of direct comparisons.
> Add proper error message.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>


Hi Mateusz,

I place my questions in line with code,


> ---
> ReadMe.c | 31 ++++++++++++++++++
> maps.c   | 31 ++++++++++++++++++
> mdadm.c  | 99 ++++++++++++++++----------------------------------------
> mdadm.h  | 32 +++++++++++++++++-
> 4 files changed, 121 insertions(+), 72 deletions(-)
> 
> diff --git a/ReadMe.c b/ReadMe.c
> index 7518a32a..50e6f987 100644
> --- a/ReadMe.c
> +++ b/ReadMe.c
> @@ -656,3 +656,34 @@ char *mode_help[mode_count] = {
> 	[GROW]		= Help_grow,
> 	[INCREMENTAL]	= Help_incr,
> };
> +
> +/**
> + * fprint_update_options() - Print valid update options depending on the mode.
> + * @outf: File (output stream)
> + * @update_mode: Used to distinguish update and update_subarray
> + */
> +void fprint_update_options(FILE *outf, enum update_opt update_mode)
> +{
> +	int counter = UOPT_NAME, breakpoint = UOPT_HELP;
> +	mapping_t *map = update_options;
> +
> +	if (!outf)
> +		return;
> +	if (update_mode == UOPT_SUBARRAY_ONLY) {
> +		breakpoint = UOPT_SUBARRAY_ONLY;
> +		fprintf(outf, "Valid --update options for update-subarray are:\n\t");
> +	} else
> +		fprintf(outf, "Valid --update options are:\n\t");
> +	while (map->num) {
> +		if (map->num >= breakpoint)
> +			break;
> +		fprintf(outf, "'%s', ", map->name);
> +		if (counter % 5 == 0)
> +			fprintf(outf, "\n\t");
> +		counter++;
> +		map++;
> +	}
> +	if ((counter - 1) % 5)
> +		fprintf(outf, "\n");
> +	fprintf(outf, "\r");


Why ‘\r’ is used here? I feel ‘\n’ should work fine as well.




> +}
> diff --git a/maps.c b/maps.c
> index 20fcf719..b586679a 100644
> --- a/maps.c
> +++ b/maps.c
> @@ -165,6 +165,37 @@ mapping_t sysfs_array_states[] = {
> 	{ "broken", ARRAY_BROKEN },
> 	{ NULL, ARRAY_UNKNOWN_STATE }
> };
> +/**
> + * mapping_t update_options - stores supported update options.
> + */
> +mapping_t update_options[] = {
> +	{ "name", UOPT_NAME },
> +	{ "ppl", UOPT_PPL },
> +	{ "no-ppl", UOPT_NO_PPL },
> +	{ "bitmap", UOPT_BITMAP },
> +	{ "no-bitmap", UOPT_NO_BITMAP },
> +	{ "sparc2.2", UOPT_SPARC22 },
> +	{ "super-minor", UOPT_SUPER_MINOR },
> +	{ "summaries", UOPT_SUMMARIES },
> +	{ "resync", UOPT_RESYNC },
> +	{ "uuid", UOPT_UUID },
> +	{ "homehost", UOPT_HOMEHOST },
> +	{ "home-cluster", UOPT_HOME_CLUSTER },
> +	{ "nodes", UOPT_NODES },
> +	{ "devicesize", UOPT_DEVICESIZE },
> +	{ "bbl", UOPT_BBL },
> +	{ "no-bbl", UOPT_NO_BBL },
> +	{ "force-no-bbl", UOPT_FORCE_NO_BBL },
> +	{ "metadata", UOPT_METADATA },
> +	{ "revert-reshape", UOPT_REVERT_RESHAPE },
> +	{ "layout-original", UOPT_LAYOUT_ORIGINAL },
> +	{ "layout-alternate", UOPT_LAYOUT_ALTERNATE },
> +	{ "layout-unspecified", UOPT_LAYOUT_UNSPECIFIED },
> +	{ "byteorder", UOPT_BYTEORDER },
> +	{ "help", UOPT_HELP },
> +	{ "?", UOPT_HELP },
> +	{ NULL, UOPT_UNDEFINED}
> +};
> 
> /**
>  * map_num_s() - Safer alternative of map_num() function.
> diff --git a/mdadm.c b/mdadm.c
> index 56722ed9..3705d114 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -101,7 +101,7 @@ int main(int argc, char *argv[])
> 	char *dump_directory = NULL;
> 
> 	int print_help = 0;
> -	FILE *outf;
> +	FILE *outf = NULL;
> 
> 	int mdfd = -1;
> 	int locked = 0;
> @@ -753,82 +753,39 @@ int main(int argc, char *argv[])
> 				pr_err("Only subarrays can be updated in misc mode\n");
> 				exit(2);
> 			}
> +
> 			c.update = optarg;
> -			if (strcmp(c.update, "sparc2.2") == 0)
> -				continue;
> -			if (strcmp(c.update, "super-minor") == 0)
> -				continue;
> -			if (strcmp(c.update, "summaries") == 0)
> -				continue;
> -			if (strcmp(c.update, "resync") == 0)
> -				continue;
> -			if (strcmp(c.update, "uuid") == 0)
> -				continue;
> -			if (strcmp(c.update, "name") == 0)
> -				continue;
> -			if (strcmp(c.update, "homehost") == 0)
> -				continue;
> -			if (strcmp(c.update, "home-cluster") == 0)
> -				continue;
> -			if (strcmp(c.update, "nodes") == 0)
> -				continue;
> -			if (strcmp(c.update, "devicesize") == 0)
> -				continue;
> -			if (strcmp(c.update, "bitmap") == 0)
> -				continue;
> -			if (strcmp(c.update, "no-bitmap") == 0)
> -				continue;
> -			if (strcmp(c.update, "bbl") == 0)
> -				continue;
> -			if (strcmp(c.update, "no-bbl") == 0)
> -				continue;
> -			if (strcmp(c.update, "force-no-bbl") == 0)
> -				continue;
> -			if (strcmp(c.update, "ppl") == 0)
> -				continue;
> -			if (strcmp(c.update, "no-ppl") == 0)
> -				continue;
> -			if (strcmp(c.update, "metadata") == 0)
> -				continue;
> -			if (strcmp(c.update, "revert-reshape") == 0)
> -				continue;
> -			if (strcmp(c.update, "layout-original") == 0 ||
> -			    strcmp(c.update, "layout-alternate") == 0 ||
> -			    strcmp(c.update, "layout-unspecified") == 0)
> -				continue;
> -			if (strcmp(c.update, "byteorder") == 0) {
> +			enum update_opt updateopt = map_name(update_options, c.update);
> +			enum update_opt print_mode = UOPT_HELP;
> +			const char *error_addon = "update option";
> +

Could you please move the local variables declaration to the beginning of the case O(MISC,'U’) code block?



> +			if (devmode == UpdateSubarray) {
> +				print_mode = UOPT_SUBARRAY_ONLY;
> +				error_addon = "update-subarray option";
> +
> +				if (updateopt > UOPT_SUBARRAY_ONLY && updateopt < UOPT_HELP)
> +					updateopt = UOPT_UNDEFINED;
> +			}
> +
> +			switch (updateopt) {
> +			case UOPT_UNDEFINED:
> +				pr_err("'--update=%s' is invalid %s. ",
> +					c.update, error_addon);
> +				outf = stderr;
> +			case UOPT_HELP:
> +				if (!outf)
> +					outf = stdout;
> +				fprint_update_options(outf, print_mode);
> +				exit(outf == stdout ? 0 : 2);


I tried to run update-subarray parameter but failed, obviously wrong command line format. Could you please give me a hint, on how to test the —update-subarray parameter? Then I can provide more feed back after experience the command.

The comments for rested patches will be posted after can I run and verify the change with my eyes.

Thanks.

Coly Li

> +			case UOPT_BYTEORDER:
> 				if (ss) {
> 					pr_err("must not set metadata type with --update=byteorder.\n");
> 					exit(2);
> 				}
> -				for(i = 0; !ss && superlist[i]; i++)
> -					ss = superlist[i]->match_metadata_desc(
> -						"0.swap");
> -				if (!ss) {
> -					pr_err("INTERNAL ERROR cannot find 0.swap\n");
> -					exit(2);
> -				}
> -
> -				continue;
> +			default:
> +				break;
> 			}
> -			if (strcmp(c.update,"?") == 0 ||
> -			    strcmp(c.update, "help") == 0) {
> -				outf = stdout;
> -				fprintf(outf, "%s: ", Name);
> -			} else {
> -				outf = stderr;
> -				fprintf(outf,
> -					"%s: '--update=%s' is invalid.  ",
> -					Name, c.update);
> -			}
> -			fprintf(outf, "Valid --update options are:\n"
> -		"     'sparc2.2', 'super-minor', 'uuid', 'name', 'nodes', 'resync',\n"
> -		"     'summaries', 'homehost', 'home-cluster', 'byteorder', 'devicesize',\n"
> -		"     'bitmap', 'no-bitmap', 'metadata', 'revert-reshape'\n"
> -		"     'bbl', 'no-bbl', 'force-no-bbl', 'ppl', 'no-ppl'\n"
> -		"     'layout-original', 'layout-alternate', 'layout-unspecified'\n"
> -				);
> -			exit(outf == stdout ? 0 : 2);
> +			continue;
> 
> 		case O(MANAGE,'U'):
> 			/* update=devicesize is allowed with --re-add */
> diff --git a/mdadm.h b/mdadm.h
> index 163f4a49..43e6b544 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -497,6 +497,36 @@ enum special_options {
> 	ConsistencyPolicy,
> };
> 
> +enum update_opt {
> +	UOPT_NAME = 1,
> +	UOPT_PPL,
> +	UOPT_NO_PPL,
> +	UOPT_BITMAP,
> +	UOPT_NO_BITMAP,
> +	UOPT_SUBARRAY_ONLY,
> +	UOPT_SPARC22,
> +	UOPT_SUPER_MINOR,
> +	UOPT_SUMMARIES,
> +	UOPT_RESYNC,
> +	UOPT_UUID,
> +	UOPT_HOMEHOST,
> +	UOPT_HOME_CLUSTER,
> +	UOPT_NODES,
> +	UOPT_DEVICESIZE,
> +	UOPT_BBL,
> +	UOPT_NO_BBL,
> +	UOPT_FORCE_NO_BBL,
> +	UOPT_METADATA,
> +	UOPT_REVERT_RESHAPE,
> +	UOPT_LAYOUT_ORIGINAL,
> +	UOPT_LAYOUT_ALTERNATE,
> +	UOPT_LAYOUT_UNSPECIFIED,
> +	UOPT_BYTEORDER,
> +	UOPT_HELP,
> +	UOPT_UNDEFINED
> +};
> +extern void fprint_update_options(FILE *outf, enum update_opt update_mode);
> +
> enum prefix_standard {
> 	JEDEC,
> 	IEC
> @@ -776,7 +806,7 @@ extern char *map_num(mapping_t *map, int num);
> extern int map_name(mapping_t *map, char *name);
> extern mapping_t r0layout[], r5layout[], r6layout[],
> 	pers[], modes[], faultylayout[];
> -extern mapping_t consistency_policies[], sysfs_array_states[];
> +extern mapping_t consistency_policies[], sysfs_array_states[], update_options[];
> 
> extern char *map_dev_preferred(int major, int minor, int create,
> 			       char *prefer);
> -- 
> 2.26.2
> 

Re: [PATCH 01/10] mdadm: Add option validation for --update-subarray

[Kusiak, Mateusz]

On 13/09/2022 17:12, Coly Li wrote:
> 
> 
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>
>> Subset of options available for "--update" is not same as for "--update-subarray".
>> Define maps and enum for update options and use them instead of direct comparisons.
>> Add proper error message.
>>
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> 
> 
> Hi Mateusz,
> 
> I place my questions in line with code,
> 
> 
>> ---
>> ReadMe.c | 31 ++++++++++++++++++
>> maps.c   | 31 ++++++++++++++++++
>> mdadm.c  | 99 ++++++++++++++++----------------------------------------
>> mdadm.h  | 32 +++++++++++++++++-
>> 4 files changed, 121 insertions(+), 72 deletions(-)
>>
>> diff --git a/ReadMe.c b/ReadMe.c
>> index 7518a32a..50e6f987 100644
>> --- a/ReadMe.c
>> +++ b/ReadMe.c
>> @@ -656,3 +656,34 @@ char *mode_help[mode_count] = {
>> 	[GROW]		= Help_grow,
>> 	[INCREMENTAL]	= Help_incr,
>> };
>> +
>> +/**
>> + * fprint_update_options() - Print valid update options depending on the mode.
>> + * @outf: File (output stream)
>> + * @update_mode: Used to distinguish update and update_subarray
>> + */
>> +void fprint_update_options(FILE *outf, enum update_opt update_mode)
>> +{
>> +	int counter = UOPT_NAME, breakpoint = UOPT_HELP;
>> +	mapping_t *map = update_options;
>> +
>> +	if (!outf)
>> +		return;
>> +	if (update_mode == UOPT_SUBARRAY_ONLY) {
>> +		breakpoint = UOPT_SUBARRAY_ONLY;
>> +		fprintf(outf, "Valid --update options for update-subarray are:\n\t");
>> +	} else
>> +		fprintf(outf, "Valid --update options are:\n\t");
>> +	while (map->num) {
>> +		if (map->num >= breakpoint)
>> +			break;
>> +		fprintf(outf, "'%s', ", map->name);
>> +		if (counter % 5 == 0)
>> +			fprintf(outf, "\n\t");
>> +		counter++;
>> +		map++;
>> +	}
>> +	if ((counter - 1) % 5)
>> +		fprintf(outf, "\n");
>> +	fprintf(outf, "\r");
> 
> 
> Why ‘\r’ is used here? I feel ‘\n’ should work fine as well.
> 
Hi Coly,
The reason is that '\n' leaves empty line after print.
> 
>> +}
>> diff --git a/maps.c b/maps.c
>> index 20fcf719..b586679a 100644
>> --- a/maps.c
>> +++ b/maps.c
>> @@ -165,6 +165,37 @@ mapping_t sysfs_array_states[] = {
>> 	{ "broken", ARRAY_BROKEN },
>> 	{ NULL, ARRAY_UNKNOWN_STATE }
>> };
>> +/**
>> + * mapping_t update_options - stores supported update options.
>> + */
>> +mapping_t update_options[] = {
>> +	{ "name", UOPT_NAME },
>> +	{ "ppl", UOPT_PPL },
>> +	{ "no-ppl", UOPT_NO_PPL },
>> +	{ "bitmap", UOPT_BITMAP },
>> +	{ "no-bitmap", UOPT_NO_BITMAP },
>> +	{ "sparc2.2", UOPT_SPARC22 },
>> +	{ "super-minor", UOPT_SUPER_MINOR },
>> +	{ "summaries", UOPT_SUMMARIES },
>> +	{ "resync", UOPT_RESYNC },
>> +	{ "uuid", UOPT_UUID },
>> +	{ "homehost", UOPT_HOMEHOST },
>> +	{ "home-cluster", UOPT_HOME_CLUSTER },
>> +	{ "nodes", UOPT_NODES },
>> +	{ "devicesize", UOPT_DEVICESIZE },
>> +	{ "bbl", UOPT_BBL },
>> +	{ "no-bbl", UOPT_NO_BBL },
>> +	{ "force-no-bbl", UOPT_FORCE_NO_BBL },
>> +	{ "metadata", UOPT_METADATA },
>> +	{ "revert-reshape", UOPT_REVERT_RESHAPE },
>> +	{ "layout-original", UOPT_LAYOUT_ORIGINAL },
>> +	{ "layout-alternate", UOPT_LAYOUT_ALTERNATE },
>> +	{ "layout-unspecified", UOPT_LAYOUT_UNSPECIFIED },
>> +	{ "byteorder", UOPT_BYTEORDER },
>> +	{ "help", UOPT_HELP },
>> +	{ "?", UOPT_HELP },
>> +	{ NULL, UOPT_UNDEFINED}
>> +};
>>
>> /**
>>  * map_num_s() - Safer alternative of map_num() function.
>> diff --git a/mdadm.c b/mdadm.c
>> index 56722ed9..3705d114 100644
>> --- a/mdadm.c
>> +++ b/mdadm.c
>> @@ -101,7 +101,7 @@ int main(int argc, char *argv[])
>> 	char *dump_directory = NULL;
>>
>> 	int print_help = 0;
>> -	FILE *outf;
>> +	FILE *outf = NULL;
>>
>> 	int mdfd = -1;
>> 	int locked = 0;
>> @@ -753,82 +753,39 @@ int main(int argc, char *argv[])
>> 				pr_err("Only subarrays can be updated in misc mode\n");
>> 				exit(2);
>> 			}
>> +
>> 			c.update = optarg;
>> -			if (strcmp(c.update, "sparc2.2") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "super-minor") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "summaries") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "resync") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "uuid") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "name") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "homehost") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "home-cluster") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "nodes") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "devicesize") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "bitmap") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "no-bitmap") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "bbl") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "no-bbl") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "force-no-bbl") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "ppl") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "no-ppl") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "metadata") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "revert-reshape") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "layout-original") == 0 ||
>> -			    strcmp(c.update, "layout-alternate") == 0 ||
>> -			    strcmp(c.update, "layout-unspecified") == 0)
>> -				continue;
>> -			if (strcmp(c.update, "byteorder") == 0) {
>> +			enum update_opt updateopt = map_name(update_options, c.update);
>> +			enum update_opt print_mode = UOPT_HELP;
>> +			const char *error_addon = "update option";
>> +
> 
> Could you please move the local variables declaration to the beginning of the case O(MISC,'U’) code block?
> 
Sure, I'll post it in v2.
> 
>> +			if (devmode == UpdateSubarray) {
>> +				print_mode = UOPT_SUBARRAY_ONLY;
>> +				error_addon = "update-subarray option";
>> +
>> +				if (updateopt > UOPT_SUBARRAY_ONLY && updateopt < UOPT_HELP)
>> +					updateopt = UOPT_UNDEFINED;
>> +			}
>> +
>> +			switch (updateopt) {
>> +			case UOPT_UNDEFINED:
>> +				pr_err("'--update=%s' is invalid %s. ",
>> +					c.update, error_addon);
>> +				outf = stderr;
>> +			case UOPT_HELP:
>> +				if (!outf)
>> +					outf = stdout;
>> +				fprint_update_options(outf, print_mode);
>> +				exit(outf == stdout ? 0 : 2);
> 
> 
> I tried to run update-subarray parameter but failed, obviously wrong command line format. Could you please give me a hint, on how to test the —update-subarray parameter? Then I can provide more feed back after experience the command.
>Sure, the exaple command is as follows:
# mdadm --update-subarray=0 --update=name --name=example
/dev/md/container

The command must be performed on a container, to succeed the volume must
be stopped.
All update options for update-subarray can be listed with:
# mdadm --update-subarray=0 --update=help
..and "global" update options with:
# mdadm -A --update=help

> The comments for rested patches will be posted after can I run and verify the change with my eyes.
> 
> Thanks.
> 
> Coly Li
> 

[Snipped]

[PATCH 02/10] Fix --update-subarray on active volume

[Mateusz Kusiak]

Options: bitmap, ppl and name should not be updated when array is active.
Those features are mutually exclusive and share the same data area in IMSM (danger of overwriting by kernel).
Remove check for active subarrays from super-intel.
Since ddf is not supported, apply it globally for all options.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 Manage.c      | 7 +++++++
 super-intel.c | 5 -----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Manage.c b/Manage.c
index e5e6abe4..b9f0b184 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1694,6 +1694,13 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
 		goto free_super;
 	}
 
+	if (is_subarray_active(subarray, st->devnm)) {
+		if (verbose >= 0)
+			pr_err("Subarray %s in %s is active, cannot update %s\n",
+			       subarray, dev, update);
+		goto free_super;
+	}
+
 	if (mdmon_running(st->devnm))
 		st->update_tail = &st->updates;
 
diff --git a/super-intel.c b/super-intel.c
index 4ddfcf94..672f946e 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -7914,11 +7914,6 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 		char *ep;
 		int vol;
 
-		if (is_subarray_active(subarray, st->devnm)) {
-			pr_err("Unable to update name of active subarray\n");
-			return 2;
-		}
-
 		if (!check_name(super, name, 0))
 			return 2;
 
-- 
2.26.2

Re: [PATCH 02/10] Fix --update-subarray on active volume

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Options: bitmap, ppl and name should not be updated when array is active.
> Those features are mutually exclusive and share the same data area in IMSM (danger of overwriting by kernel).
> Remove check for active subarrays from super-intel.
> Since ddf is not supported, apply it globally for all options.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>


Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
> Manage.c      | 7 +++++++
> super-intel.c | 5 -----
> 2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Manage.c b/Manage.c
> index e5e6abe4..b9f0b184 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -1694,6 +1694,13 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
> 		goto free_super;
> 	}
> 
> +	if (is_subarray_active(subarray, st->devnm)) {
> +		if (verbose >= 0)
> +			pr_err("Subarray %s in %s is active, cannot update %s\n",
> +			       subarray, dev, update);
> +		goto free_super;
> +	}
> +
> 	if (mdmon_running(st->devnm))
> 		st->update_tail = &st->updates;
> 
> diff --git a/super-intel.c b/super-intel.c
> index 4ddfcf94..672f946e 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -7914,11 +7914,6 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
> 		char *ep;
> 		int vol;
> 
> -		if (is_subarray_active(subarray, st->devnm)) {
> -			pr_err("Unable to update name of active subarray\n");
> -			return 2;
> -		}
> -
> 		if (!check_name(super, name, 0))
> 			return 2;
> 
> -- 
> 2.26.2
> 

[PATCH 03/10] Add code specific update options to enum.

[Mateusz Kusiak]

Some of update options aren't taken from user input, but are hard-coded
as strings.
Include those options in enum.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 maps.c  | 21 +++++++++++++++++++++
 mdadm.h | 15 +++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/maps.c b/maps.c
index b586679a..c59036f1 100644
--- a/maps.c
+++ b/maps.c
@@ -194,6 +194,27 @@ mapping_t update_options[] = {
 	{ "byteorder", UOPT_BYTEORDER },
 	{ "help", UOPT_HELP },
 	{ "?", UOPT_HELP },
+	/*
+	 * Those enries are temporary and will be removed in this patchset.
+	 *
+	 * Before update_super:update can be changed to enum,
+	 * all update_super sub-functions must be adapted first.
+	 * Update options will be passed as string (as it is for now),
+	 * and then mapped, so all options must be handled temporarily.
+	 *
+	 * Those options code specific and should not be accessible for user.
+	 */
+	{ "force-one", UOPT_SPEC_FORCE_ONE },
+	{ "force-array", UOPT_SPEC_FORCE_ARRAY },
+	{ "assemble", UOPT_SPEC_ASSEMBLE },
+	{ "linear-grow-new", UOPT_SPEC_LINEAR_GROW_NEW },
+	{ "linear-grow-update", UOPT_SPEC_LINEAR_GROW_UPDATE },
+	{ "_reshape_progress", UOPT_SPEC__RESHAPE_PROGRESS },
+	{ "writemostly", UOPT_SPEC_WRITEMOSTLY },
+	{ "readwrite", UOPT_SPEC_READWRITE },
+	{ "failfast", UOPT_SPEC_FAILFAST },
+	{ "nofailfast", UOPT_SPEC_NOFAILFAST },
+	{ "revert-reshape-nobackup", UOPT_SPEC_REVERT_RESHAPE_NOBACKUP },
 	{ NULL, UOPT_UNDEFINED}
 };
 
diff --git a/mdadm.h b/mdadm.h
index 43e6b544..7bc31b16 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -523,6 +523,21 @@ enum update_opt {
 	UOPT_LAYOUT_UNSPECIFIED,
 	UOPT_BYTEORDER,
 	UOPT_HELP,
+	UOPT_USER_ONLY,
+	/*
+	 * Code specific options, cannot be set by the user
+	 */
+	UOPT_SPEC_FORCE_ONE,
+	UOPT_SPEC_FORCE_ARRAY,
+	UOPT_SPEC_ASSEMBLE,
+	UOPT_SPEC_LINEAR_GROW_NEW,
+	UOPT_SPEC_LINEAR_GROW_UPDATE,
+	UOPT_SPEC__RESHAPE_PROGRESS,
+	UOPT_SPEC_WRITEMOSTLY,
+	UOPT_SPEC_READWRITE,
+	UOPT_SPEC_FAILFAST,
+	UOPT_SPEC_NOFAILFAST,
+	UOPT_SPEC_REVERT_RESHAPE_NOBACKUP,
 	UOPT_UNDEFINED
 };
 extern void fprint_update_options(FILE *outf, enum update_opt update_mode);
-- 
2.26.2

Re: [PATCH 03/10] Add code specific update options to enum.

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Some of update options aren't taken from user input, but are hard-coded
> as strings.
> Include those options in enum.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>

Acked-by: Coly Li <colyli@suse.de>


Thanks.

Coly Li


> ---
> maps.c  | 21 +++++++++++++++++++++
> mdadm.h | 15 +++++++++++++++
> 2 files changed, 36 insertions(+)
> 
> diff --git a/maps.c b/maps.c
> index b586679a..c59036f1 100644
> --- a/maps.c
> +++ b/maps.c
> @@ -194,6 +194,27 @@ mapping_t update_options[] = {
> 	{ "byteorder", UOPT_BYTEORDER },
> 	{ "help", UOPT_HELP },
> 	{ "?", UOPT_HELP },
> +	/*
> +	 * Those enries are temporary and will be removed in this patchset.
> +	 *
> +	 * Before update_super:update can be changed to enum,
> +	 * all update_super sub-functions must be adapted first.
> +	 * Update options will be passed as string (as it is for now),
> +	 * and then mapped, so all options must be handled temporarily.
> +	 *
> +	 * Those options code specific and should not be accessible for user.
> +	 */
> +	{ "force-one", UOPT_SPEC_FORCE_ONE },
> +	{ "force-array", UOPT_SPEC_FORCE_ARRAY },
> +	{ "assemble", UOPT_SPEC_ASSEMBLE },
> +	{ "linear-grow-new", UOPT_SPEC_LINEAR_GROW_NEW },
> +	{ "linear-grow-update", UOPT_SPEC_LINEAR_GROW_UPDATE },
> +	{ "_reshape_progress", UOPT_SPEC__RESHAPE_PROGRESS },
> +	{ "writemostly", UOPT_SPEC_WRITEMOSTLY },
> +	{ "readwrite", UOPT_SPEC_READWRITE },
> +	{ "failfast", UOPT_SPEC_FAILFAST },
> +	{ "nofailfast", UOPT_SPEC_NOFAILFAST },
> +	{ "revert-reshape-nobackup", UOPT_SPEC_REVERT_RESHAPE_NOBACKUP },
> 	{ NULL, UOPT_UNDEFINED}
> };
> 
> diff --git a/mdadm.h b/mdadm.h
> index 43e6b544..7bc31b16 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -523,6 +523,21 @@ enum update_opt {
> 	UOPT_LAYOUT_UNSPECIFIED,
> 	UOPT_BYTEORDER,
> 	UOPT_HELP,
> +	UOPT_USER_ONLY,
> +	/*
> +	 * Code specific options, cannot be set by the user
> +	 */
> +	UOPT_SPEC_FORCE_ONE,
> +	UOPT_SPEC_FORCE_ARRAY,
> +	UOPT_SPEC_ASSEMBLE,
> +	UOPT_SPEC_LINEAR_GROW_NEW,
> +	UOPT_SPEC_LINEAR_GROW_UPDATE,
> +	UOPT_SPEC__RESHAPE_PROGRESS,
> +	UOPT_SPEC_WRITEMOSTLY,
> +	UOPT_SPEC_READWRITE,
> +	UOPT_SPEC_FAILFAST,
> +	UOPT_SPEC_NOFAILFAST,
> +	UOPT_SPEC_REVERT_RESHAPE_NOBACKUP,
> 	UOPT_UNDEFINED
> };
> extern void fprint_update_options(FILE *outf, enum update_opt update_mode);
> -- 
> 2.26.2
> 

[PATCH 04/10] super-ddf: Remove update_super_ddf.

[Mateusz Kusiak]

This is not supported by ddf.
It hides errors by returning success status for some updates.
Remove update_super_dff().

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 super-ddf.c | 70 -----------------------------------------------------
 1 file changed, 70 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 949e7d15..ec59b8af 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -2139,75 +2139,6 @@ static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, cha
 		}
 }
 
-static int update_super_ddf(struct supertype *st, struct mdinfo *info,
-			    char *update,
-			    char *devname, int verbose,
-			    int uuid_set, char *homehost)
-{
-	/* For 'assemble' and 'force' we need to return non-zero if any
-	 * change was made.  For others, the return value is ignored.
-	 * Update options are:
-	 *  force-one : This device looks a bit old but needs to be included,
-	 *        update age info appropriately.
-	 *  assemble: clear any 'faulty' flag to allow this device to
-	 *		be assembled.
-	 *  force-array: Array is degraded but being forced, mark it clean
-	 *	   if that will be needed to assemble it.
-	 *
-	 *  newdev:  not used ????
-	 *  grow:  Array has gained a new device - this is currently for
-	 *		linear only
-	 *  resync: mark as dirty so a resync will happen.
-	 *  uuid:  Change the uuid of the array to match what is given
-	 *  homehost:  update the recorded homehost
-	 *  name:  update the name - preserving the homehost
-	 *  _reshape_progress: record new reshape_progress position.
-	 *
-	 * Following are not relevant for this version:
-	 *  sparc2.2 : update from old dodgey metadata
-	 *  super-minor: change the preferred_minor number
-	 *  summaries:  update redundant counters.
-	 */
-	int rv = 0;
-//	struct ddf_super *ddf = st->sb;
-//	struct vd_config *vd = find_vdcr(ddf, info->container_member);
-//	struct virtual_entry *ve = find_ve(ddf);
-
-	/* we don't need to handle "force-*" or "assemble" as
-	 * there is no need to 'trick' the kernel.  When the metadata is
-	 * first updated to activate the array, all the implied modifications
-	 * will just happen.
-	 */
-
-	if (strcmp(update, "grow") == 0) {
-		/* FIXME */
-	} else if (strcmp(update, "resync") == 0) {
-//		info->resync_checkpoint = 0;
-	} else if (strcmp(update, "homehost") == 0) {
-		/* homehost is stored in controller->vendor_data,
-		 * or it is when we are the vendor
-		 */
-//		if (info->vendor_is_local)
-//			strcpy(ddf->controller.vendor_data, homehost);
-		rv = -1;
-	} else if (strcmp(update, "name") == 0) {
-		/* name is stored in virtual_entry->name */
-//		memset(ve->name, ' ', 16);
-//		strncpy(ve->name, info->name, 16);
-		rv = -1;
-	} else if (strcmp(update, "_reshape_progress") == 0) {
-		/* We don't support reshape yet */
-	} else if (strcmp(update, "assemble") == 0 ) {
-		/* Do nothing, just succeed */
-		rv = 0;
-	} else
-		rv = -1;
-
-//	update_all_csum(ddf);
-
-	return rv;
-}
-
 static void make_header_guid(char *guid)
 {
 	be32 stamp;
@@ -5211,7 +5142,6 @@ struct superswitch super_ddf = {
 	.match_home	= match_home_ddf,
 	.uuid_from_super= uuid_from_super_ddf,
 	.getinfo_super  = getinfo_super_ddf,
-	.update_super	= update_super_ddf,
 
 	.avail_size	= avail_size_ddf,
 
-- 
2.26.2

Re: [PATCH 04/10] super-ddf: Remove update_super_ddf.

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> This is not supported by ddf.
> It hides errors by returning success status for some updates.
> Remove update_super_dff().
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>


Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
> super-ddf.c | 70 -----------------------------------------------------
> 1 file changed, 70 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index 949e7d15..ec59b8af 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -2139,75 +2139,6 @@ static void getinfo_super_ddf_bvd(struct supertype *st, struct mdinfo *info, cha
> 		}
> }
> 
> -static int update_super_ddf(struct supertype *st, struct mdinfo *info,
> -			    char *update,
> -			    char *devname, int verbose,
> -			    int uuid_set, char *homehost)
> -{
> -	/* For 'assemble' and 'force' we need to return non-zero if any
> -	 * change was made.  For others, the return value is ignored.
> -	 * Update options are:
> -	 *  force-one : This device looks a bit old but needs to be included,
> -	 *        update age info appropriately.
> -	 *  assemble: clear any 'faulty' flag to allow this device to
> -	 *		be assembled.
> -	 *  force-array: Array is degraded but being forced, mark it clean
> -	 *	   if that will be needed to assemble it.
> -	 *
> -	 *  newdev:  not used ????
> -	 *  grow:  Array has gained a new device - this is currently for
> -	 *		linear only
> -	 *  resync: mark as dirty so a resync will happen.
> -	 *  uuid:  Change the uuid of the array to match what is given
> -	 *  homehost:  update the recorded homehost
> -	 *  name:  update the name - preserving the homehost
> -	 *  _reshape_progress: record new reshape_progress position.
> -	 *
> -	 * Following are not relevant for this version:
> -	 *  sparc2.2 : update from old dodgey metadata
> -	 *  super-minor: change the preferred_minor number
> -	 *  summaries:  update redundant counters.
> -	 */
> -	int rv = 0;
> -//	struct ddf_super *ddf = st->sb;
> -//	struct vd_config *vd = find_vdcr(ddf, info->container_member);
> -//	struct virtual_entry *ve = find_ve(ddf);
> -
> -	/* we don't need to handle "force-*" or "assemble" as
> -	 * there is no need to 'trick' the kernel.  When the metadata is
> -	 * first updated to activate the array, all the implied modifications
> -	 * will just happen.
> -	 */
> -
> -	if (strcmp(update, "grow") == 0) {
> -		/* FIXME */
> -	} else if (strcmp(update, "resync") == 0) {
> -//		info->resync_checkpoint = 0;
> -	} else if (strcmp(update, "homehost") == 0) {
> -		/* homehost is stored in controller->vendor_data,
> -		 * or it is when we are the vendor
> -		 */
> -//		if (info->vendor_is_local)
> -//			strcpy(ddf->controller.vendor_data, homehost);
> -		rv = -1;
> -	} else if (strcmp(update, "name") == 0) {
> -		/* name is stored in virtual_entry->name */
> -//		memset(ve->name, ' ', 16);
> -//		strncpy(ve->name, info->name, 16);
> -		rv = -1;
> -	} else if (strcmp(update, "_reshape_progress") == 0) {
> -		/* We don't support reshape yet */
> -	} else if (strcmp(update, "assemble") == 0 ) {
> -		/* Do nothing, just succeed */
> -		rv = 0;
> -	} else
> -		rv = -1;
> -
> -//	update_all_csum(ddf);
> -
> -	return rv;
> -}
> -
> static void make_header_guid(char *guid)
> {
> 	be32 stamp;
> @@ -5211,7 +5142,6 @@ struct superswitch super_ddf = {
> 	.match_home	= match_home_ddf,
> 	.uuid_from_super= uuid_from_super_ddf,
> 	.getinfo_super  = getinfo_super_ddf,
> -	.update_super	= update_super_ddf,
> 
> 	.avail_size	= avail_size_ddf,
> 
> -- 
> 2.26.2
> 

[PATCH 05/10] super0: refactor the code for enum

[Mateusz Kusiak]

It prepares update_super0 for change context->update to enum.
Change if else statements to switch.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 super0.c | 102 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 63 insertions(+), 39 deletions(-)

diff --git a/super0.c b/super0.c
index 37f595ed..4e53f41e 100644
--- a/super0.c
+++ b/super0.c
@@ -502,19 +502,39 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 	int rv = 0;
 	int uuid[4];
 	mdp_super_t *sb = st->sb;
+	enum update_opt update_enum = map_name(update_options, update);
 
-	if (strcmp(update, "homehost") == 0 &&
-	    homehost) {
-		/* note that 'homehost' is special as it is really
+	if (update_enum == UOPT_HOMEHOST && homehost) {
+		/*
+		 * note that 'homehost' is special as it is really
 		 * a "uuid" update.
 		 */
 		uuid_set = 0;
-		update = "uuid";
+		update_enum = UOPT_UUID;
 		info->uuid[0] = sb->set_uuid0;
 		info->uuid[1] = sb->set_uuid1;
 	}
 
-	if (strcmp(update, "sparc2.2")==0 ) {
+	switch (update_enum) {
+	case UOPT_UUID:
+		if (!uuid_set && homehost) {
+			char buf[20];
+			memcpy(info->uuid+2,
+			       sha1_buffer(homehost, strlen(homehost), buf),
+			       8);
+		}
+		sb->set_uuid0 = info->uuid[0];
+		sb->set_uuid1 = info->uuid[1];
+		sb->set_uuid2 = info->uuid[2];
+		sb->set_uuid3 = info->uuid[3];
+		if (sb->state & (1<<MD_SB_BITMAP_PRESENT)) {
+			struct bitmap_super_s *bm;
+			bm = (struct bitmap_super_s *)(sb+1);
+			uuid_from_super0(st, uuid);
+			memcpy(bm->uuid, uuid, 16);
+		}
+		break;
+	case UOPT_SPARC22: {
 		/* 2.2 sparc put the events in the wrong place
 		 * So we copy the tail of the superblock
 		 * up 4 bytes before continuing
@@ -527,12 +547,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 		if (verbose >= 0)
 			pr_err("adjusting superblock of %s for 2.2/sparc compatibility.\n",
 			       devname);
-	} else if (strcmp(update, "super-minor") ==0) {
+		break;
+	}
+	case UOPT_SUPER_MINOR:
 		sb->md_minor = info->array.md_minor;
 		if (verbose > 0)
 			pr_err("updating superblock of %s with minor number %d\n",
 				devname, info->array.md_minor);
-	} else if (strcmp(update, "summaries") == 0) {
+		break;
+	case UOPT_SUMMARIES: {
 		unsigned int i;
 		/* set nr_disks, active_disks, working_disks,
 		 * failed_disks, spare_disks based on disks[]
@@ -559,7 +582,9 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 					sb->spare_disks++;
 			} else if (i >= sb->raid_disks && sb->disks[i].number == 0)
 				sb->disks[i].state = 0;
-	} else if (strcmp(update, "force-one")==0) {
+		break;
+	}
+	case UOPT_SPEC_FORCE_ONE: {
 		/* Not enough devices for a working array, so
 		 * bring this one up-to-date.
 		 */
@@ -569,7 +594,9 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 		if (sb->events_hi != ehi ||
 		    sb->events_lo != elo)
 			rv = 1;
-	} else if (strcmp(update, "force-array")==0) {
+		break;
+	}
+	case UOPT_SPEC_FORCE_ARRAY:
 		/* degraded array and 'force' requested, so
 		 * maybe need to mark it 'clean'
 		 */
@@ -579,7 +606,8 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 			sb->state |= (1 << MD_SB_CLEAN);
 			rv = 1;
 		}
-	} else if (strcmp(update, "assemble")==0) {
+		break;
+	case UOPT_SPEC_ASSEMBLE: {
 		int d = info->disk.number;
 		int wonly = sb->disks[d].state & (1<<MD_DISK_WRITEMOSTLY);
 		int failfast = sb->disks[d].state & (1<<MD_DISK_FAILFAST);
@@ -609,7 +637,9 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 			sb->reshape_position = info->reshape_progress;
 			rv = 1;
 		}
-	} else if (strcmp(update, "linear-grow-new") == 0) {
+		break;
+	}
+	case UOPT_SPEC_LINEAR_GROW_NEW:
 		memset(&sb->disks[info->disk.number], 0, sizeof(sb->disks[0]));
 		sb->disks[info->disk.number].number = info->disk.number;
 		sb->disks[info->disk.number].major = info->disk.major;
@@ -617,7 +647,8 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 		sb->disks[info->disk.number].raid_disk = info->disk.raid_disk;
 		sb->disks[info->disk.number].state = info->disk.state;
 		sb->this_disk = sb->disks[info->disk.number];
-	} else if (strcmp(update, "linear-grow-update") == 0) {
+		break;
+	case UOPT_SPEC_LINEAR_GROW_UPDATE:
 		sb->raid_disks = info->array.raid_disks;
 		sb->nr_disks = info->array.nr_disks;
 		sb->active_disks = info->array.active_disks;
@@ -628,29 +659,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 		sb->disks[info->disk.number].minor = info->disk.minor;
 		sb->disks[info->disk.number].raid_disk = info->disk.raid_disk;
 		sb->disks[info->disk.number].state = info->disk.state;
-	} else if (strcmp(update, "resync") == 0) {
-		/* make sure resync happens */
+		break;
+	case UOPT_RESYNC:
+		/**
+		 *make sure resync happens
+		 */
 		sb->state &= ~(1<<MD_SB_CLEAN);
 		sb->recovery_cp = 0;
-	} else if (strcmp(update, "uuid") == 0) {
-		if (!uuid_set && homehost) {
-			char buf[20];
-			char *hash = sha1_buffer(homehost,
-						 strlen(homehost),
-						 buf);
-			memcpy(info->uuid+2, hash, 8);
-		}
-		sb->set_uuid0 = info->uuid[0];
-		sb->set_uuid1 = info->uuid[1];
-		sb->set_uuid2 = info->uuid[2];
-		sb->set_uuid3 = info->uuid[3];
-		if (sb->state & (1<<MD_SB_BITMAP_PRESENT)) {
-			struct bitmap_super_s *bm;
-			bm = (struct bitmap_super_s*)(sb+1);
-			uuid_from_super0(st, uuid);
-			memcpy(bm->uuid, uuid, 16);
-		}
-	} else if (strcmp(update, "metadata") == 0) {
+		break;
+	case UOPT_METADATA:
 		/* Create some v1.0 metadata to match ours but make the
 		 * ctime bigger.  Also update info->array.*_version.
 		 * We need to arrange that store_super writes out
@@ -670,7 +687,8 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 			uuid_from_super0(st, info->uuid);
 			st->other = super1_make_v0(st, info, st->sb);
 		}
-	} else if (strcmp(update, "revert-reshape") == 0) {
+		break;
+	case UOPT_REVERT_RESHAPE:
 		rv = -2;
 		if (sb->minor_version <= 90)
 			pr_err("No active reshape to revert on %s\n",
@@ -702,16 +720,22 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 			sb->new_chunk = sb->chunk_size;
 			sb->chunk_size = tmp;
 		}
-	} else if (strcmp(update, "no-bitmap") == 0) {
+		break;
+	case UOPT_NO_BITMAP:
 		sb->state &= ~(1<<MD_SB_BITMAP_PRESENT);
-	} else if (strcmp(update, "_reshape_progress")==0)
+		break;
+	case UOPT_SPEC__RESHAPE_PROGRESS:
 		sb->reshape_position = info->reshape_progress;
-	else if (strcmp(update, "writemostly")==0)
+		break;
+	case UOPT_SPEC_WRITEMOSTLY:
 		sb->state |= (1<<MD_DISK_WRITEMOSTLY);
-	else if (strcmp(update, "readwrite")==0)
+		break;
+	case UOPT_SPEC_READWRITE:
 		sb->state &= ~(1<<MD_DISK_WRITEMOSTLY);
-	else
+		break;
+	default:
 		rv = -1;
+	}
 
 	sb->sb_csum = calc_sb0_csum(sb);
 	return rv;
-- 
2.26.2

Re: [PATCH 05/10] super0: refactor the code for enum

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> It prepares update_super0 for change context->update to enum.
> Change if else statements to switch.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>


This patch is fine to me almost, except for 2 questions I placed in line.



> ---
> super0.c | 102 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 63 insertions(+), 39 deletions(-)
> 
> diff --git a/super0.c b/super0.c
> index 37f595ed..4e53f41e 100644
> --- a/super0.c
> +++ b/super0.c
> @@ -502,19 +502,39 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
> 	int rv = 0;
> 	int uuid[4];
> 	mdp_super_t *sb = st->sb;
> +	enum update_opt update_enum = map_name(update_options, update);
> 
> -	if (strcmp(update, "homehost") == 0 &&
> -	    homehost) {
> -		/* note that 'homehost' is special as it is really
> +	if (update_enum == UOPT_HOMEHOST && homehost) {
> +		/*
> +		 * note that 'homehost' is special as it is really
> 		 * a "uuid" update.
> 		 */
> 		uuid_set = 0;
> -		update = "uuid";
> +		update_enum = UOPT_UUID;
> 		info->uuid[0] = sb->set_uuid0;
> 		info->uuid[1] = sb->set_uuid1;
> 	}
> 
> -	if (strcmp(update, "sparc2.2")==0 ) {
> +	switch (update_enum) {
> +	case UOPT_UUID:
> +		if (!uuid_set && homehost) {
> +			char buf[20];
> +			memcpy(info->uuid+2,
> +			       sha1_buffer(homehost, strlen(homehost), buf),
> +			       8);
> +		}
> +		sb->set_uuid0 = info->uuid[0];
> +		sb->set_uuid1 = info->uuid[1];
> +		sb->set_uuid2 = info->uuid[2];
> +		sb->set_uuid3 = info->uuid[3];
> +		if (sb->state & (1<<MD_SB_BITMAP_PRESENT)) {
> +			struct bitmap_super_s *bm;
> +			bm = (struct bitmap_super_s *)(sb+1);
> +			uuid_from_super0(st, uuid);
> +			memcpy(bm->uuid, uuid, 16);
> +		}
> +		break;
> +	case UOPT_SPARC22: {
> 		/* 2.2 sparc put the events in the wrong place
> 		 * So we copy the tail of the superblock
> 		 * up 4 bytes before continuing
> @@ -527,12 +547,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
> 		if (verbose >= 0)
> 			pr_err("adjusting superblock of %s for 2.2/sparc compatibility.\n",
> 			       devname);
> -	} else if (strcmp(update, "super-minor") ==0) {
> +		break;
> +	}


Wondering there isn't compiler warning for unmatched case/break pair, since this break is inside the {} code block.

Should the ‘break’ be placed after {} pair to match key word ‘case’?


> 
[snipped]
> @@ -628,29 +659,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
> 		sb->disks[info->disk.number].minor = info->disk.minor;
> 		sb->disks[info->disk.number].raid_disk = info->disk.raid_disk;
> 		sb->disks[info->disk.number].state = info->disk.state;
> -	} else if (strcmp(update, "resync") == 0) {
> -		/* make sure resync happens */
> +		break;
> +	case UOPT_RESYNC:
> +		/**
> +		 *make sure resync happens
> +		 */


The above change doesn’t follow existing code style for comments. How about using the previous one line version?

[snipped]

Thanks.

Coly Li

Re: [PATCH 05/10] super0: refactor the code for enum

[Kusiak, Mateusz]

On 14/09/2022 17:03, Coly Li wrote:
> 
> 
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>
>> It prepares update_super0 for change context->update to enum.
>> Change if else statements to switch.
>>
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> 
> 
> This patch is fine to me almost, except for 2 questions I placed in line.
> 
> 
> 
>> ---
>> super0.c | 102 ++++++++++++++++++++++++++++++++++---------------------
>> 1 file changed, 63 insertions(+), 39 deletions(-)
>>
>> diff --git a/super0.c b/super0.c
>> index 37f595ed..4e53f41e 100644
>> --- a/super0.c
>> +++ b/super0.c
>> @@ -502,19 +502,39 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
>> 	int rv = 0;
>> 	int uuid[4];
>> 	mdp_super_t *sb = st->sb;
>> +	enum update_opt update_enum = map_name(update_options, update);
>>
>> -	if (strcmp(update, "homehost") == 0 &&
>> -	    homehost) {
>> -		/* note that 'homehost' is special as it is really
>> +	if (update_enum == UOPT_HOMEHOST && homehost) {
>> +		/*
>> +		 * note that 'homehost' is special as it is really
>> 		 * a "uuid" update.
>> 		 */
>> 		uuid_set = 0;
>> -		update = "uuid";
>> +		update_enum = UOPT_UUID;
>> 		info->uuid[0] = sb->set_uuid0;
>> 		info->uuid[1] = sb->set_uuid1;
>> 	}
>>
>> -	if (strcmp(update, "sparc2.2")==0 ) {
>> +	switch (update_enum) {
>> +	case UOPT_UUID:
>> +		if (!uuid_set && homehost) {
>> +			char buf[20];
>> +			memcpy(info->uuid+2,
>> +			       sha1_buffer(homehost, strlen(homehost), buf),
>> +			       8);
>> +		}
>> +		sb->set_uuid0 = info->uuid[0];
>> +		sb->set_uuid1 = info->uuid[1];
>> +		sb->set_uuid2 = info->uuid[2];
>> +		sb->set_uuid3 = info->uuid[3];
>> +		if (sb->state & (1<<MD_SB_BITMAP_PRESENT)) {
>> +			struct bitmap_super_s *bm;
>> +			bm = (struct bitmap_super_s *)(sb+1);
>> +			uuid_from_super0(st, uuid);
>> +			memcpy(bm->uuid, uuid, 16);
>> +		}
>> +		break;
>> +	case UOPT_SPARC22: {
>> 		/* 2.2 sparc put the events in the wrong place
>> 		 * So we copy the tail of the superblock
>> 		 * up 4 bytes before continuing
>> @@ -527,12 +547,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
>> 		if (verbose >= 0)
>> 			pr_err("adjusting superblock of %s for 2.2/sparc compatibility.\n",
>> 			       devname);
>> -	} else if (strcmp(update, "super-minor") ==0) {
>> +		break;
>> +	}
> 
> 
> Wondering there isn't compiler warning for unmatched case/break pair, since this break is inside the {} code block.
> 
> Should the ‘break’ be placed after {} pair to match key word ‘case’?
> 
Hi Coly,
I do not get compiler warning, what's more, this approach is commonly
used across the code.
I can change it in v2 if you want me to.
> 
>>
> [snipped]
>> @@ -628,29 +659,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
>> 		sb->disks[info->disk.number].minor = info->disk.minor;
>> 		sb->disks[info->disk.number].raid_disk = info->disk.raid_disk;
>> 		sb->disks[info->disk.number].state = info->disk.state;
>> -	} else if (strcmp(update, "resync") == 0) {
>> -		/* make sure resync happens */
>> +		break;
>> +	case UOPT_RESYNC:
>> +		/**
>> +		 *make sure resync happens
>> +		 */
> 
> 
> The above change doesn’t follow existing code style for comments. How about using the previous one line version?
> 
Personaly, I'd rather change it from "/**" to "/*". I think we should
gradually adapt the code to kernel coding style.
Are you fine with that?

> [snipped]
> 
> Thanks.
> 
> Coly Li

Re: [PATCH 05/10] super0: refactor the code for enum

[Jes Sorensen]

On 9/22/22 07:21, Kusiak, Mateusz wrote:
> On 14/09/2022 17:03, Coly Li wrote:
>>
>>
>>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>>
>> [snipped]
>>> @@ -628,29 +659,15 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
>>> 		sb->disks[info->disk.number].minor = info->disk.minor;
>>> 		sb->disks[info->disk.number].raid_disk = info->disk.raid_disk;
>>> 		sb->disks[info->disk.number].state = info->disk.state;
>>> -	} else if (strcmp(update, "resync") == 0) {
>>> -		/* make sure resync happens */
>>> +		break;
>>> +	case UOPT_RESYNC:
>>> +		/**
>>> +		 *make sure resync happens
>>> +		 */
>>
>>
>> The above change doesn’t follow existing code style for comments. How about using the previous one line version?
>>
> Personaly, I'd rather change it from "/**" to "/*". I think we should
> gradually adapt the code to kernel coding style.
> Are you fine with that?

Kernel style is good, but that would be either

/* foo comment */

or

/*
 * bar comment
 */

Thanks,
Jes


>> [snipped]
>>
>> Thanks.
>>
>> Coly Li

[PATCH 06/10] super1: refactor the code for enum

[Mateusz Kusiak]

It prepares update_super1 for change context->update to enum.
Change if else statements into switch.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 super1.c | 149 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 87 insertions(+), 62 deletions(-)

diff --git a/super1.c b/super1.c
index 71af860c..6c81c1b9 100644
--- a/super1.c
+++ b/super1.c
@@ -1212,30 +1212,53 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 	int rv = 0;
 	struct mdp_superblock_1 *sb = st->sb;
 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
+	enum update_opt update_enum = map_name(update_options, update);
 
-	if (strcmp(update, "homehost") == 0 &&
-	    homehost) {
-		/* Note that 'homehost' is special as it is really
+	if (update_enum == UOPT_HOMEHOST && homehost) {
+		/*
+		 * Note that 'homehost' is special as it is really
 		 * a "name" update.
 		 */
 		char *c;
-		update = "name";
+		update_enum = UOPT_NAME;
 		c = strchr(sb->set_name, ':');
 		if (c)
-			strncpy(info->name, c+1, 31 - (c-sb->set_name));
+			snprintf(info->name, sizeof(info->name), "%s", c+1);
 		else
-			strncpy(info->name, sb->set_name, 32);
-		info->name[32] = 0;
+			snprintf(info->name, sizeof(info->name), "%s", sb->set_name);
 	}
 
-	if (strcmp(update, "force-one")==0) {
+	switch (update_enum) {
+	case UOPT_NAME:
+		if (!info->name[0])
+			snprintf(info->name, sizeof(info->name), "%d", info->array.md_minor);
+		memset(sb->set_name, 0, sizeof(sb->set_name));
+		int namelen;
+
+		namelen = strnlen(homehost, MD_NAME_MAX) + 1 + strnlen(info->name, MD_NAME_MAX);
+		if (homehost &&
+		    strchr(info->name, ':') == NULL &&
+		    namelen < MD_NAME_MAX) {
+			strcpy(sb->set_name, homehost);
+			strcat(sb->set_name, ":");
+			strcat(sb->set_name, info->name);
+		} else {
+			namelen = min((int)strnlen(info->name, MD_NAME_MAX),
+				      (int)sizeof(sb->set_name) - 1);
+			memcpy(sb->set_name, info->name, namelen);
+			memset(&sb->set_name[namelen], '\0',
+			       sizeof(sb->set_name) - namelen);
+		}
+		break;
+	case UOPT_SPEC_FORCE_ONE:
 		/* Not enough devices for a working array,
 		 * so bring this one up-to-date
 		 */
 		if (sb->events != __cpu_to_le64(info->events))
 			rv = 1;
 		sb->events = __cpu_to_le64(info->events);
-	} else if (strcmp(update, "force-array")==0) {
+		break;
+	case UOPT_SPEC_FORCE_ARRAY:
 		/* Degraded array and 'force' requests to
 		 * maybe need to mark it 'clean'.
 		 */
@@ -1248,7 +1271,8 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 				rv = 1;
 			sb->resync_offset = MaxSector;
 		}
-	} else if (strcmp(update, "assemble")==0) {
+		break;
+	case UOPT_SPEC_ASSEMBLE: {
 		int d = info->disk.number;
 		int want;
 		if (info->disk.state & (1<<MD_DISK_ACTIVE))
@@ -1281,7 +1305,9 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 				__cpu_to_le64(info->reshape_progress);
 			rv = 1;
 		}
-	} else if (strcmp(update, "linear-grow-new") == 0) {
+		break;
+	}
+	case UOPT_SPEC_LINEAR_GROW_NEW: {
 		int i;
 		int fd;
 		int max = __le32_to_cpu(sb->max_dev);
@@ -1324,7 +1350,9 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 					ds - __le64_to_cpu(sb->data_offset));
 			}
 		}
-	} else if (strcmp(update, "linear-grow-update") == 0) {
+		break;
+	}
+	case UOPT_SPEC_LINEAR_GROW_UPDATE: {
 		int max = __le32_to_cpu(sb->max_dev);
 		int i = info->disk.number;
 		if (max > MAX_DEVS || i > MAX_DEVS)
@@ -1336,19 +1364,20 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 		sb->raid_disks = __cpu_to_le32(info->array.raid_disks);
 		sb->dev_roles[info->disk.number] =
 			__cpu_to_le16(info->disk.raid_disk);
-	} else if (strcmp(update, "resync") == 0) {
-		/* make sure resync happens */
-		sb->resync_offset = 0ULL;
-	} else if (strcmp(update, "uuid") == 0) {
+		break;
+	}
+	case UOPT_UUID:
 		copy_uuid(sb->set_uuid, info->uuid, super1.swapuuid);
 
 		if (__le32_to_cpu(sb->feature_map) & MD_FEATURE_BITMAP_OFFSET)
 			memcpy(bms->uuid, sb->set_uuid, 16);
-	} else if (strcmp(update, "no-bitmap") == 0) {
+		break;
+	case UOPT_NO_BITMAP:
 		sb->feature_map &= ~__cpu_to_le32(MD_FEATURE_BITMAP_OFFSET);
 		if (bms->version == BITMAP_MAJOR_CLUSTERED && !IsBitmapDirty(devname))
 			sb->resync_offset = MaxSector;
-	} else if (strcmp(update, "bbl") == 0) {
+		break;
+	case UOPT_BBL: {
 		/* only possible if there is room after the bitmap, or if
 		 * there is no bitmap
 		 */
@@ -1377,14 +1406,12 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 				bb_offset = bitmap_offset + bm_sectors;
 			while (bb_offset < (long)sb_offset + 8 + 32*2 &&
 			       bb_offset + 8+8 <= (long)data_offset)
-				/* too close to bitmap, and room to grow */
 				bb_offset += 8;
 			if (bb_offset + 8 <= (long)data_offset) {
 				sb->bblog_size = __cpu_to_le16(8);
 				sb->bblog_offset = __cpu_to_le32(bb_offset);
 			}
 		} else {
-			/* 1.0 - Put bbl just before super block */
 			if (bm_sectors && bitmap_offset < 0)
 				space = -bitmap_offset - bm_sectors;
 			else
@@ -1395,7 +1422,9 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 				sb->bblog_offset = __cpu_to_le32((unsigned)-8);
 			}
 		}
-	} else if (strcmp(update, "no-bbl") == 0) {
+		break;
+	}
+	case UOPT_NO_BBL:
 		if (sb->feature_map & __cpu_to_le32(MD_FEATURE_BAD_BLOCKS))
 			pr_err("Cannot remove active bbl from %s\n",devname);
 		else {
@@ -1403,12 +1432,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 			sb->bblog_shift = 0;
 			sb->bblog_offset = 0;
 		}
-	} else if (strcmp(update, "force-no-bbl") == 0) {
+		break;
+	case UOPT_FORCE_NO_BBL:
 		sb->feature_map &= ~ __cpu_to_le32(MD_FEATURE_BAD_BLOCKS);
 		sb->bblog_size = 0;
 		sb->bblog_shift = 0;
 		sb->bblog_offset = 0;
-	} else if (strcmp(update, "ppl") == 0) {
+		break;
+	case UOPT_PPL: {
 		unsigned long long sb_offset = __le64_to_cpu(sb->super_offset);
 		unsigned long long data_offset = __le64_to_cpu(sb->data_offset);
 		unsigned long long data_size = __le64_to_cpu(sb->data_size);
@@ -1458,37 +1489,26 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 		sb->ppl.offset = __cpu_to_le16(offset);
 		sb->ppl.size = __cpu_to_le16(space);
 		sb->feature_map |= __cpu_to_le32(MD_FEATURE_PPL);
-	} else if (strcmp(update, "no-ppl") == 0) {
+		break;
+	}
+	case UOPT_NO_PPL:
 		sb->feature_map &= ~__cpu_to_le32(MD_FEATURE_PPL |
 						   MD_FEATURE_MUTLIPLE_PPLS);
-	} else if (strcmp(update, "name") == 0) {
-		if (info->name[0] == 0)
-			sprintf(info->name, "%d", info->array.md_minor);
-		memset(sb->set_name, 0, sizeof(sb->set_name));
-		if (homehost &&
-		    strchr(info->name, ':') == NULL &&
-		    strlen(homehost)+1+strlen(info->name) < 32) {
-			strcpy(sb->set_name, homehost);
-			strcat(sb->set_name, ":");
-			strcat(sb->set_name, info->name);
-		} else {
-			int namelen;
-
-			namelen = min((int)strlen(info->name),
-				      (int)sizeof(sb->set_name) - 1);
-			memcpy(sb->set_name, info->name, namelen);
-			memset(&sb->set_name[namelen], '\0',
-			       sizeof(sb->set_name) - namelen);
-		}
-	} else if (strcmp(update, "devicesize") == 0 &&
-		   __le64_to_cpu(sb->super_offset) <
-		   __le64_to_cpu(sb->data_offset)) {
-		/* set data_size to device size less data_offset */
+		break;
+	case UOPT_DEVICESIZE:
+		if (__le64_to_cpu(sb->super_offset) >=
+		    __le64_to_cpu(sb->data_offset))
+			break;
+		/*
+		 * set data_size to device size less data_offset
+		 */
 		struct misc_dev_info *misc = (struct misc_dev_info*)
 			(st->sb + MAX_SB_SIZE + BM_SUPER_SIZE);
 		sb->data_size = __cpu_to_le64(
 			misc->device_size - __le64_to_cpu(sb->data_offset));
-	} else if (strncmp(update, "revert-reshape", 14) == 0) {
+		break;
+	case UOPT_SPEC_REVERT_RESHAPE_NOBACKUP:
+	case UOPT_REVERT_RESHAPE:
 		rv = -2;
 		if (!(sb->feature_map &
 		      __cpu_to_le32(MD_FEATURE_RESHAPE_ACTIVE)))
@@ -1506,7 +1526,7 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 			 * If that couldn't happen, the "-nobackup" version
 			 * will be used.
 			 */
-			if (strcmp(update, "revert-reshape-nobackup") == 0 &&
+			if (update_enum == UOPT_SPEC_REVERT_RESHAPE_NOBACKUP &&
 			    sb->reshape_position == 0 &&
 			    (__le32_to_cpu(sb->delta_disks) > 0 ||
 			     (__le32_to_cpu(sb->delta_disks) == 0 &&
@@ -1569,32 +1589,37 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 			}
 		done:;
 		}
-	} else if (strcmp(update, "_reshape_progress") == 0)
+		break;
+	case UOPT_SPEC__RESHAPE_PROGRESS:
 		sb->reshape_position = __cpu_to_le64(info->reshape_progress);
-	else if (strcmp(update, "writemostly") == 0)
-		sb->devflags |= WriteMostly1;
-	else if (strcmp(update, "readwrite") == 0)
+		break;
+	case UOPT_SPEC_READWRITE:
 		sb->devflags &= ~WriteMostly1;
-	else if (strcmp(update, "failfast") == 0)
+		break;
+	case UOPT_SPEC_FAILFAST:
 		sb->devflags |= FailFast1;
-	else if (strcmp(update, "nofailfast") == 0)
+		break;
+	case UOPT_SPEC_NOFAILFAST:
 		sb->devflags &= ~FailFast1;
-	else if (strcmp(update, "layout-original") == 0 ||
-		 strcmp(update, "layout-alternate") == 0 ||
-		 strcmp(update, "layout-unspecified") == 0) {
+		break;
+	case UOPT_LAYOUT_ORIGINAL:
+	case UOPT_LAYOUT_ALTERNATE:
+	case UOPT_LAYOUT_UNSPECIFIED:
 		if (__le32_to_cpu(sb->level) != 0) {
 			pr_err("%s: %s only supported for RAID0\n",
-			       devname?:"", update);
+			       devname ?: "", map_num(update_options, update_enum));
 			rv = -1;
-		} else if (strcmp(update, "layout-unspecified") == 0) {
+		} else if (update_enum == UOPT_LAYOUT_UNSPECIFIED) {
 			sb->feature_map &= ~__cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
 			sb->layout = 0;
 		} else {
 			sb->feature_map |= __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
-			sb->layout = __cpu_to_le32(update[7] == 'o' ? 1 : 2);
+			sb->layout = __cpu_to_le32(update_enum == UOPT_LAYOUT_ORIGINAL ? 1 : 2);
 		}
-	} else
+		break;
+	default:
 		rv = -1;
+	}
 
 	sb->sb_csum = calc_sb_1_csum(sb);
 
-- 
2.26.2

Re: [PATCH 06/10] super1: refactor the code for enum

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> It prepares update_super1 for change context->update to enum.
> Change if else statements into switch.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> ---
> super1.c | 149 ++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 87 insertions(+), 62 deletions(-)
> 
> diff --git a/super1.c b/super1.c
> index 71af860c..6c81c1b9 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1212,30 +1212,53 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
> 	int rv = 0;
> 	struct mdp_superblock_1 *sb = st->sb;
> 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
> +	enum update_opt update_enum = map_name(update_options, update);
> 
> -	if (strcmp(update, "homehost") == 0 &&
> -	    homehost) {
> -		/* Note that 'homehost' is special as it is really
> +	if (update_enum == UOPT_HOMEHOST && homehost) {
> +		/*
> +		 * Note that 'homehost' is special as it is really
> 		 * a "name" update.
> 		 */
> 		char *c;
> -		update = "name";
> +		update_enum = UOPT_NAME;
> 		c = strchr(sb->set_name, ':');
> 		if (c)
> -			strncpy(info->name, c+1, 31 - (c-sb->set_name));
> +			snprintf(info->name, sizeof(info->name), "%s", c+1);
> 		else
> -			strncpy(info->name, sb->set_name, 32);
> -		info->name[32] = 0;
> +			snprintf(info->name, sizeof(info->name), "%s", sb->set_name);
> 	}
> 
> -	if (strcmp(update, "force-one")==0) {
> +	switch (update_enum) {
> +	case UOPT_NAME:
> +		if (!info->name[0])
> +			snprintf(info->name, sizeof(info->name), "%d", info->array.md_minor);
> +		memset(sb->set_name, 0, sizeof(sb->set_name));
> +		int namelen;
> +

The above variable ’namelen’ might be declared at beginning of this code block.


> +		namelen = strnlen(homehost, MD_NAME_MAX) + 1 + strnlen(info->name, MD_NAME_MAX);
> +		if (homehost &&
> +		    strchr(info->name, ':') == NULL &&
> +		    namelen < MD_NAME_MAX) {
> +			strcpy(sb->set_name, homehost);
> +			strcat(sb->set_name, ":");
> +			strcat(sb->set_name, info->name);
> +		} else {
> +			namelen = min((int)strnlen(info->name, MD_NAME_MAX),
> +				      (int)sizeof(sb->set_name) - 1);
> +			memcpy(sb->set_name, info->name, namelen);
> +			memset(&sb->set_name[namelen], '\0',
> +			       sizeof(sb->set_name) - namelen);
> +		}
> +		break;
> 
[snipped]
> @@ -1569,32 +1589,37 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
> 			}
> 		done:;
> 		}
> -	} else if (strcmp(update, "_reshape_progress") == 0)
> +		break;
> +	case UOPT_SPEC__RESHAPE_PROGRESS:
> 		sb->reshape_position = __cpu_to_le64(info->reshape_progress);
> -	else if (strcmp(update, "writemostly") == 0)
> -		sb->devflags |= WriteMostly1;
> -	else if (strcmp(update, "readwrite") == 0)
> +		break;
> +	case UOPT_SPEC_READWRITE:
> 		sb->devflags &= ~WriteMostly1;
> -	else if (strcmp(update, "failfast") == 0)

Writemostly-setting is removed here, is it on purpose ?

[snip]

Thanks.

Coly Li

Re: [PATCH 06/10] super1: refactor the code for enum

[Kusiak, Mateusz]

On 14/09/2022 17:03, Coly Li wrote:
> 
> 
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>
>> It prepares update_super1 for change context->update to enum.
>> Change if else statements into switch.
>>
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
>> ---
>> super1.c | 149 ++++++++++++++++++++++++++++++++-----------------------
>> 1 file changed, 87 insertions(+), 62 deletions(-)
>>
>> diff --git a/super1.c b/super1.c
>> index 71af860c..6c81c1b9 100644
>> --- a/super1.c
>> +++ b/super1.c
>> @@ -1212,30 +1212,53 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>> 	int rv = 0;
>> 	struct mdp_superblock_1 *sb = st->sb;
>> 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
>> +	enum update_opt update_enum = map_name(update_options, update);
>>
>> -	if (strcmp(update, "homehost") == 0 &&
>> -	    homehost) {
>> -		/* Note that 'homehost' is special as it is really
>> +	if (update_enum == UOPT_HOMEHOST && homehost) {
>> +		/*
>> +		 * Note that 'homehost' is special as it is really
>> 		 * a "name" update.
>> 		 */
>> 		char *c;
>> -		update = "name";
>> +		update_enum = UOPT_NAME;
>> 		c = strchr(sb->set_name, ':');
>> 		if (c)
>> -			strncpy(info->name, c+1, 31 - (c-sb->set_name));
>> +			snprintf(info->name, sizeof(info->name), "%s", c+1);
>> 		else
>> -			strncpy(info->name, sb->set_name, 32);
>> -		info->name[32] = 0;
>> +			snprintf(info->name, sizeof(info->name), "%s", sb->set_name);
>> 	}
>>
>> -	if (strcmp(update, "force-one")==0) {
>> +	switch (update_enum) {
>> +	case UOPT_NAME:
>> +		if (!info->name[0])
>> +			snprintf(info->name, sizeof(info->name), "%d", info->array.md_minor);
>> +		memset(sb->set_name, 0, sizeof(sb->set_name));
>> +		int namelen;
>> +
> 
> The above variable ’namelen’ might be declared at beginning of this code block.

I'll fix this in v2.
> 
>> +		namelen = strnlen(homehost, MD_NAME_MAX) + 1 + strnlen(info->name, MD_NAME_MAX);
>> +		if (homehost &&
>> +		    strchr(info->name, ':') == NULL &&
>> +		    namelen < MD_NAME_MAX) {
>> +			strcpy(sb->set_name, homehost);
>> +			strcat(sb->set_name, ":");
>> +			strcat(sb->set_name, info->name);
>> +		} else {
>> +			namelen = min((int)strnlen(info->name, MD_NAME_MAX),
>> +				      (int)sizeof(sb->set_name) - 1);
>> +			memcpy(sb->set_name, info->name, namelen);
>> +			memset(&sb->set_name[namelen], '\0',
>> +			       sizeof(sb->set_name) - namelen);
>> +		}
>> +		break;
>>
> [snipped]
>> @@ -1569,32 +1589,37 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>> 			}
>> 		done:;
>> 		}
>> -	} else if (strcmp(update, "_reshape_progress") == 0)
>> +		break;
>> +	case UOPT_SPEC__RESHAPE_PROGRESS:
>> 		sb->reshape_position = __cpu_to_le64(info->reshape_progress);
>> -	else if (strcmp(update, "writemostly") == 0)
>> -		sb->devflags |= WriteMostly1;
>> -	else if (strcmp(update, "readwrite") == 0)
>> +		break;
>> +	case UOPT_SPEC_READWRITE:
>> 		sb->devflags &= ~WriteMostly1;
>> -	else if (strcmp(update, "failfast") == 0)
> 
> Writemostly-setting is removed here, is it on purpose ?

No, thanks for noticing! I'll add this in v2.
> 
> [snip]
> 
> Thanks.
> 
> Coly Li

Re: [PATCH 06/10] super1: refactor the code for enum

[Jes Sorensen]

On 9/22/22 07:21, Kusiak, Mateusz wrote:
> On 14/09/2022 17:03, Coly Li wrote:
>>
>>
>>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>>
>>> It prepares update_super1 for change context->update to enum.
>>> Change if else statements into switch.
>>>
>>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
>>> ---
>>> super1.c | 149 ++++++++++++++++++++++++++++++++-----------------------
>>> 1 file changed, 87 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/super1.c b/super1.c
>>> index 71af860c..6c81c1b9 100644
>>> --- a/super1.c
>>> +++ b/super1.c
>>> @@ -1212,30 +1212,53 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>>> 	int rv = 0;
>>> 	struct mdp_superblock_1 *sb = st->sb;
>>> 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
>>> +	enum update_opt update_enum = map_name(update_options, update);
>>>
>>> -	if (strcmp(update, "homehost") == 0 &&
>>> -	    homehost) {
>>> -		/* Note that 'homehost' is special as it is really
>>> +	if (update_enum == UOPT_HOMEHOST && homehost) {
>>> +		/*
>>> +		 * Note that 'homehost' is special as it is really
>>> 		 * a "name" update.
>>> 		 */
>>> 		char *c;
>>> -		update = "name";
>>> +		update_enum = UOPT_NAME;
>>> 		c = strchr(sb->set_name, ':');
>>> 		if (c)
>>> -			strncpy(info->name, c+1, 31 - (c-sb->set_name));
>>> +			snprintf(info->name, sizeof(info->name), "%s", c+1);
>>> 		else
>>> -			strncpy(info->name, sb->set_name, 32);
>>> -		info->name[32] = 0;
>>> +			snprintf(info->name, sizeof(info->name), "%s", sb->set_name);
>>> 	}
>>>
>>> -	if (strcmp(update, "force-one")==0) {
>>> +	switch (update_enum) {
>>> +	case UOPT_NAME:
>>> +		if (!info->name[0])
>>> +			snprintf(info->name, sizeof(info->name), "%d", info->array.md_minor);
>>> +		memset(sb->set_name, 0, sizeof(sb->set_name));
>>> +		int namelen;
>>> +
>>
>> The above variable ’namelen’ might be declared at beginning of this code block.
> 
> I'll fix this in v2.

Hi Mateusz,

Did you ever post a v2 of this?

Thanks,
Jes


>>> +		namelen = strnlen(homehost, MD_NAME_MAX) + 1 + strnlen(info->name, MD_NAME_MAX);
>>> +		if (homehost &&
>>> +		    strchr(info->name, ':') == NULL &&
>>> +		    namelen < MD_NAME_MAX) {
>>> +			strcpy(sb->set_name, homehost);
>>> +			strcat(sb->set_name, ":");
>>> +			strcat(sb->set_name, info->name);
>>> +		} else {
>>> +			namelen = min((int)strnlen(info->name, MD_NAME_MAX),
>>> +				      (int)sizeof(sb->set_name) - 1);
>>> +			memcpy(sb->set_name, info->name, namelen);
>>> +			memset(&sb->set_name[namelen], '\0',
>>> +			       sizeof(sb->set_name) - namelen);
>>> +		}
>>> +		break;
>>>
>> [snipped]
>>> @@ -1569,32 +1589,37 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
>>> 			}
>>> 		done:;
>>> 		}
>>> -	} else if (strcmp(update, "_reshape_progress") == 0)
>>> +		break;
>>> +	case UOPT_SPEC__RESHAPE_PROGRESS:
>>> 		sb->reshape_position = __cpu_to_le64(info->reshape_progress);
>>> -	else if (strcmp(update, "writemostly") == 0)
>>> -		sb->devflags |= WriteMostly1;
>>> -	else if (strcmp(update, "readwrite") == 0)
>>> +		break;
>>> +	case UOPT_SPEC_READWRITE:
>>> 		sb->devflags &= ~WriteMostly1;
>>> -	else if (strcmp(update, "failfast") == 0)
>>
>> Writemostly-setting is removed here, is it on purpose ?
> 
> No, thanks for noticing! I'll add this in v2.
>>
>> [snip]
>>
>> Thanks.
>>
>> Coly Li

[PATCH 07/10] super-intel: refactor the code for enum

[Mateusz Kusiak]

It prepares super-intel for change context->update to enum.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 super-intel.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 672f946e..3de3873e 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -3930,7 +3930,8 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
 
 	mpb = super->anchor;
 
-	if (strcmp(update, "uuid") == 0) {
+	switch (map_name(update_options, update)) {
+	case UOPT_UUID:
 		/* We take this to mean that the family_num should be updated.
 		 * However that is much smaller than the uuid so we cannot really
 		 * allow an explicit uuid to be given.  And it is hard to reliably
@@ -3954,10 +3955,14 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
 		}
 		if (rv == 0)
 			mpb->orig_family_num = info->uuid[0];
-	} else if (strcmp(update, "assemble") == 0)
+		break;
+	case UOPT_SPEC_ASSEMBLE:
 		rv = 0;
-	else
+		break;
+	default:
 		rv = -1;
+		break;
+	}
 
 	/* successful update? recompute checksum */
 	if (rv == 0)
@@ -7888,18 +7893,25 @@ static int kill_subarray_imsm(struct supertype *st, char *subarray_id)
 
 	return 0;
 }
-
-static int get_rwh_policy_from_update(char *update)
+/**
+ * get_rwh_policy_from_update() - Get the rwh policy for update option.
+ * @update: Update option.
+ */
+static int get_rwh_policy_from_update(enum update_opt update)
 {
-	if (strcmp(update, "ppl") == 0)
+	switch (update) {
+	case UOPT_PPL:
 		return RWH_MULTIPLE_DISTRIBUTED;
-	else if (strcmp(update, "no-ppl") == 0)
+	case UOPT_NO_PPL:
 		return RWH_MULTIPLE_OFF;
-	else if (strcmp(update, "bitmap") == 0)
+	case UOPT_BITMAP:
 		return RWH_BITMAP;
-	else if (strcmp(update, "no-bitmap") == 0)
+	case UOPT_NO_BITMAP:
 		return RWH_OFF;
-	return -1;
+	default:
+		break;
+	}
+	return UOPT_UNDEFINED;
 }
 
 static int update_subarray_imsm(struct supertype *st, char *subarray,
@@ -7909,7 +7921,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 	struct intel_super *super = st->sb;
 	struct imsm_super *mpb = super->anchor;
 
-	if (strcmp(update, "name") == 0) {
+	if (map_name(update_options, update) == UOPT_NAME) {
 		char *name = ident->name;
 		char *ep;
 		int vol;
@@ -7943,7 +7955,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 			}
 			super->updates_pending++;
 		}
-	} else if (get_rwh_policy_from_update(update) != -1) {
+	} else if (get_rwh_policy_from_update(map_name(update_options, update)) != UOPT_UNDEFINED) {
 		int new_policy;
 		char *ep;
 		int vol = strtoul(subarray, &ep, 10);
@@ -7951,7 +7963,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 		if (*ep != '\0' || vol >= super->anchor->num_raid_devs)
 			return 2;
 
-		new_policy = get_rwh_policy_from_update(update);
+		new_policy = get_rwh_policy_from_update(map_name(update_options, update));
 
 		if (st->update_tail) {
 			struct imsm_update_rwh_policy *u = xmalloc(sizeof(*u));
-- 
2.26.2

Re: [PATCH 07/10] super-intel: refactor the code for enum

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> It prepares super-intel for change context->update to enum.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> ---
> super-intel.c | 38 +++++++++++++++++++++++++-------------
> 1 file changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index 672f946e..3de3873e 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -3930,7 +3930,8 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
> 
> 	mpb = super->anchor;
> 
> -	if (strcmp(update, "uuid") == 0) {
> +	switch (map_name(update_options, update)) {
> +	case UOPT_UUID:
> 		/* We take this to mean that the family_num should be updated.
> 		 * However that is much smaller than the uuid so we cannot really
> 		 * allow an explicit uuid to be given.  And it is hard to reliably
> @@ -3954,10 +3955,14 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
> 		}
> 		if (rv == 0)
> 			mpb->orig_family_num = info->uuid[0];
> -	} else if (strcmp(update, "assemble") == 0)
> +		break;
> +	case UOPT_SPEC_ASSEMBLE:
> 		rv = 0;
> -	else
> +		break;
> +	default:
> 		rv = -1;
> +		break;
> +	}
> 
> 	/* successful update? recompute checksum */
> 	if (rv == 0)
> @@ -7888,18 +7893,25 @@ static int kill_subarray_imsm(struct supertype *st, char *subarray_id)
> 
> 	return 0;
> }
> -
> -static int get_rwh_policy_from_update(char *update)
> +/**
> + * get_rwh_policy_from_update() - Get the rwh policy for update option.
> + * @update: Update option.
> + */


The above comment format is not the existed code comments style.

For example for getinfo_super_disks_imsm() in same file,

 3862 /* allocates memory and fills disk in mdinfo structure
 3863  * for each disk in array */
 3864 struct mdinfo *getinfo_super_disks_imsm(struct supertype *st)


[snipped]

The rested part is fine to me.

Thanks.

Coly Li

Re: [PATCH 07/10] super-intel: refactor the code for enum

[Kusiak, Mateusz]

On 14/09/2022 17:03, Coly Li wrote:
> 
> 
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>
>> It prepares super-intel for change context->update to enum.
>>
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
>> ---
>> super-intel.c | 38 +++++++++++++++++++++++++-------------
>> 1 file changed, 25 insertions(+), 13 deletions(-)
>>
>> diff --git a/super-intel.c b/super-intel.c
>> index 672f946e..3de3873e 100644
>> --- a/super-intel.c
>> +++ b/super-intel.c
>> @@ -3930,7 +3930,8 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
>>
>> 	mpb = super->anchor;
>>
>> -	if (strcmp(update, "uuid") == 0) {
>> +	switch (map_name(update_options, update)) {
>> +	case UOPT_UUID:
>> 		/* We take this to mean that the family_num should be updated.
>> 		 * However that is much smaller than the uuid so we cannot really
>> 		 * allow an explicit uuid to be given.  And it is hard to reliably
>> @@ -3954,10 +3955,14 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
>> 		}
>> 		if (rv == 0)
>> 			mpb->orig_family_num = info->uuid[0];
>> -	} else if (strcmp(update, "assemble") == 0)
>> +		break;
>> +	case UOPT_SPEC_ASSEMBLE:
>> 		rv = 0;
>> -	else
>> +		break;
>> +	default:
>> 		rv = -1;
>> +		break;
>> +	}
>>
>> 	/* successful update? recompute checksum */
>> 	if (rv == 0)
>> @@ -7888,18 +7893,25 @@ static int kill_subarray_imsm(struct supertype *st, char *subarray_id)
>>
>> 	return 0;
>> }
>> -
>> -static int get_rwh_policy_from_update(char *update)
>> +/**
>> + * get_rwh_policy_from_update() - Get the rwh policy for update option.
>> + * @update: Update option.
>> + */
> 
> 
> The above comment format is not the existed code comments style.
> 
> For example for getinfo_super_disks_imsm() in same file,
> 
>  3862 /* allocates memory and fills disk in mdinfo structure
>  3863  * for each disk in array */
>  3864 struct mdinfo *getinfo_super_disks_imsm(struct supertype *st)
> 
I believe it matches kernel style descriptions, like in
imsm_get_free_size(). I can add "Return" part if you want me to.

I just noticed that empty line is missing before the function
description, I'll fix this in v2.
> 
> [snipped]
> 
> The rested part is fine to me.
> 
> Thanks.
> 
> Coly Li

[PATCH 08/10] Change update to enum in update_super and update_subarray

[Mateusz Kusiak]

Use already existing enum, change update_super and update_subarray
update to enum globally.
Refactor function references also.
Remove code specific options from update_options.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 Assemble.c    | 14 +++++++++-----
 Examine.c     |  2 +-
 Grow.c        |  9 +++++----
 Manage.c      | 14 ++++++++------
 maps.c        | 21 ---------------------
 mdadm.h       | 12 +++++++++---
 super-intel.c | 16 ++++++++--------
 super0.c      |  9 ++++-----
 super1.c      | 17 ++++++++---------
 9 files changed, 52 insertions(+), 62 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 6df6bfbc..8cd3d533 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -695,12 +695,16 @@ static int load_devices(struct devs *devices, char *devmap,
 			} else if (strcmp(c->update, "revert-reshape") == 0 &&
 				   c->invalid_backup)
 				err = tst->ss->update_super(tst, content,
-							    "revert-reshape-nobackup",
+							    UOPT_SPEC_REVERT_RESHAPE_NOBACKUP,
 							    devname, c->verbose,
 							    ident->uuid_set,
 							    c->homehost);
 			else
-				err = tst->ss->update_super(tst, content, c->update,
+				/*
+				 * Mapping is temporary, will be removed in this patchset
+				 */
+				err = tst->ss->update_super(tst, content,
+							    map_name(update_options, c->update),
 							    devname, c->verbose,
 							    ident->uuid_set,
 							    c->homehost);
@@ -960,7 +964,7 @@ static int force_array(struct mdinfo *content,
 			continue;
 		}
 		content->events = devices[most_recent].i.events;
-		tst->ss->update_super(tst, content, "force-one",
+		tst->ss->update_super(tst, content, UOPT_SPEC_FORCE_ONE,
 				      devices[chosen_drive].devname, c->verbose,
 				      0, NULL);
 
@@ -1789,7 +1793,7 @@ try_again:
 		if (!(devices[j].i.array.state & 1))
 			clean = 0;
 
-		if (st->ss->update_super(st, &devices[j].i, "assemble", NULL,
+		if (st->ss->update_super(st, &devices[j].i, UOPT_SPEC_ASSEMBLE, NULL,
 					 c->verbose, 0, NULL)) {
 			if (c->force) {
 				if (c->verbose >= 0)
@@ -1813,7 +1817,7 @@ try_again:
 	    !enough(content->array.level, content->array.raid_disks,
 		    content->array.layout, clean,
 		    avail)) {
-		change += st->ss->update_super(st, content, "force-array",
+		change += st->ss->update_super(st, content, UOPT_SPEC_FORCE_ARRAY,
 					       devices[chosen_drive].devname, c->verbose,
 					       0, NULL);
 		was_forced = 1;
diff --git a/Examine.c b/Examine.c
index 9574a3cc..c9605a60 100644
--- a/Examine.c
+++ b/Examine.c
@@ -117,7 +117,7 @@ int Examine(struct mddev_dev *devlist,
 		}
 
 		if (c->SparcAdjust)
-			st->ss->update_super(st, NULL, "sparc2.2",
+			st->ss->update_super(st, NULL, UOPT_SPARC22,
 					     devlist->devname, 0, 0, NULL);
 		/* Ok, its good enough to try, though the checksum could be wrong */
 
diff --git a/Grow.c b/Grow.c
index 97f22c75..83b38a71 100644
--- a/Grow.c
+++ b/Grow.c
@@ -196,7 +196,7 @@ int Grow_Add_device(char *devname, int fd, char *newdev)
 	info.disk.minor = minor(rdev);
 	info.disk.raid_disk = d;
 	info.disk.state = (1 << MD_DISK_SYNC) | (1 << MD_DISK_ACTIVE);
-	if (st->ss->update_super(st, &info, "linear-grow-new", newdev,
+	if (st->ss->update_super(st, &info, UOPT_SPEC_LINEAR_GROW_NEW, newdev,
 				 0, 0, NULL) != 0) {
 		pr_err("Preparing new metadata failed on %s\n", newdev);
 		close(nfd);
@@ -254,7 +254,7 @@ int Grow_Add_device(char *devname, int fd, char *newdev)
 		info.array.active_disks = nd+1;
 		info.array.working_disks = nd+1;
 
-		if (st->ss->update_super(st, &info, "linear-grow-update", dv,
+		if (st->ss->update_super(st, &info, UOPT_SPEC_LINEAR_GROW_UPDATE, dv,
 				     0, 0, NULL) != 0) {
 			pr_err("Updating metadata failed on %s\n", dv);
 			close(fd2);
@@ -665,7 +665,7 @@ int Grow_consistency_policy(char *devname, int fd, struct context *c, struct sha
 					goto free_info;
 				}
 
-				ret = st->ss->update_super(st, sra, "ppl",
+				ret = st->ss->update_super(st, sra, UOPT_PPL,
 							   devname,
 							   c->verbose, 0, NULL);
 				if (ret) {
@@ -4941,7 +4941,8 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist,
 				continue;
 			st->ss->getinfo_super(st, &dinfo, NULL);
 			dinfo.reshape_progress = info->reshape_progress;
-			st->ss->update_super(st, &dinfo, "_reshape_progress",
+			st->ss->update_super(st, &dinfo,
+					     UOPT_SPEC__RESHAPE_PROGRESS,
 					     NULL,0, 0, NULL);
 			st->ss->store_super(st, fdlist[j]);
 			st->ss->free_super(st);
diff --git a/Manage.c b/Manage.c
index b9f0b184..c47b6262 100644
--- a/Manage.c
+++ b/Manage.c
@@ -605,6 +605,7 @@ int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
 	struct mdinfo mdi;
 	int duuid[4];
 	int ouuid[4];
+	enum update_opt update_enum = map_name(update_options, update);
 
 	dev_st->ss->getinfo_super(dev_st, &mdi, NULL);
 	dev_st->ss->uuid_from_super(dev_st, ouuid);
@@ -666,23 +667,23 @@ int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
 
 			if (dv->writemostly == FlagSet)
 				rv = dev_st->ss->update_super(
-					dev_st, NULL, "writemostly",
+					dev_st, NULL, UOPT_SPEC_WRITEMOSTLY,
 					devname, verbose, 0, NULL);
 			if (dv->writemostly == FlagClear)
 				rv = dev_st->ss->update_super(
-					dev_st, NULL, "readwrite",
+					dev_st, NULL, UOPT_SPEC_READWRITE,
 					devname, verbose, 0, NULL);
 			if (dv->failfast == FlagSet)
 				rv = dev_st->ss->update_super(
-					dev_st, NULL, "failfast",
+					dev_st, NULL, UOPT_SPEC_FAILFAST,
 					devname, verbose, 0, NULL);
 			if (dv->failfast == FlagClear)
 				rv = dev_st->ss->update_super(
-					dev_st, NULL, "nofailfast",
+					dev_st, NULL, UOPT_SPEC_NOFAILFAST,
 					devname, verbose, 0, NULL);
 			if (update)
 				rv = dev_st->ss->update_super(
-					dev_st, NULL, update,
+					dev_st, NULL, update_enum,
 					devname, verbose, 0, NULL);
 			if (rv == 0)
 				rv = dev_st->ss->store_super(dev_st, tfd);
@@ -1680,6 +1681,7 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
 	struct supertype supertype, *st = &supertype;
 	int fd, rv = 2;
 	struct mdinfo *info = NULL;
+	enum update_opt update_enum = map_name(update_options, update);
 
 	memset(st, 0, sizeof(*st));
 
@@ -1711,7 +1713,7 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
 		goto free_super;
 	}
 
-	rv = st->ss->update_subarray(st, subarray, update, ident);
+	rv = st->ss->update_subarray(st, subarray, update_enum, ident);
 
 	if (rv) {
 		if (verbose >= 0)
diff --git a/maps.c b/maps.c
index c59036f1..b586679a 100644
--- a/maps.c
+++ b/maps.c
@@ -194,27 +194,6 @@ mapping_t update_options[] = {
 	{ "byteorder", UOPT_BYTEORDER },
 	{ "help", UOPT_HELP },
 	{ "?", UOPT_HELP },
-	/*
-	 * Those enries are temporary and will be removed in this patchset.
-	 *
-	 * Before update_super:update can be changed to enum,
-	 * all update_super sub-functions must be adapted first.
-	 * Update options will be passed as string (as it is for now),
-	 * and then mapped, so all options must be handled temporarily.
-	 *
-	 * Those options code specific and should not be accessible for user.
-	 */
-	{ "force-one", UOPT_SPEC_FORCE_ONE },
-	{ "force-array", UOPT_SPEC_FORCE_ARRAY },
-	{ "assemble", UOPT_SPEC_ASSEMBLE },
-	{ "linear-grow-new", UOPT_SPEC_LINEAR_GROW_NEW },
-	{ "linear-grow-update", UOPT_SPEC_LINEAR_GROW_UPDATE },
-	{ "_reshape_progress", UOPT_SPEC__RESHAPE_PROGRESS },
-	{ "writemostly", UOPT_SPEC_WRITEMOSTLY },
-	{ "readwrite", UOPT_SPEC_READWRITE },
-	{ "failfast", UOPT_SPEC_FAILFAST },
-	{ "nofailfast", UOPT_SPEC_NOFAILFAST },
-	{ "revert-reshape-nobackup", UOPT_SPEC_REVERT_RESHAPE_NOBACKUP },
 	{ NULL, UOPT_UNDEFINED}
 };
 
diff --git a/mdadm.h b/mdadm.h
index 7bc31b16..afc2e2a8 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1010,7 +1010,7 @@ extern struct superswitch {
 	 *                    it will resume going in the opposite direction.
 	 */
 	int (*update_super)(struct supertype *st, struct mdinfo *info,
-			    char *update,
+			    enum update_opt update,
 			    char *devname, int verbose,
 			    int uuid_set, char *homehost);
 
@@ -1136,9 +1136,15 @@ extern struct superswitch {
 	/* Permit subarray's to be deleted from inactive containers */
 	int (*kill_subarray)(struct supertype *st,
 			     char *subarray_id); /* optional */
-	/* Permit subarray's to be modified */
+	/**
+	 * update_subarray() - Permit subarray to be modified.
+	 * @st: Supertype.
+	 * @subarray: Subarray name.
+	 * @update: Update option.
+	 * @ident: Optional identifiers.
+	 */
 	int (*update_subarray)(struct supertype *st, char *subarray,
-			       char *update, struct mddev_ident *ident); /* optional */
+			       enum update_opt update, struct mddev_ident *ident);
 	/* Check if reshape is supported for this external format.
 	 * st is obtained from super_by_fd() where st->subarray[0] is
 	 * initialized to indicate if reshape is being performed at the
diff --git a/super-intel.c b/super-intel.c
index 3de3873e..ce1db58b 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -3893,8 +3893,8 @@ struct mdinfo *getinfo_super_disks_imsm(struct supertype *st)
 }
 
 static int update_super_imsm(struct supertype *st, struct mdinfo *info,
-			     char *update, char *devname, int verbose,
-			     int uuid_set, char *homehost)
+			     enum update_opt update, char *devname,
+			     int verbose, int uuid_set, char *homehost)
 {
 	/* For 'assemble' and 'force' we need to return non-zero if any
 	 * change was made.  For others, the return value is ignored.
@@ -3930,7 +3930,7 @@ static int update_super_imsm(struct supertype *st, struct mdinfo *info,
 
 	mpb = super->anchor;
 
-	switch (map_name(update_options, update)) {
+	switch (update) {
 	case UOPT_UUID:
 		/* We take this to mean that the family_num should be updated.
 		 * However that is much smaller than the uuid so we cannot really
@@ -6538,7 +6538,7 @@ static int validate_ppl_imsm(struct supertype *st, struct mdinfo *info,
 		if (mdmon_running(st->container_devnm))
 			st->update_tail = &st->updates;
 
-		if (st->ss->update_subarray(st, subarray, "ppl", NULL)) {
+		if (st->ss->update_subarray(st, subarray, UOPT_PPL, NULL)) {
 			pr_err("Failed to update subarray %s\n",
 			      subarray);
 		} else {
@@ -7915,13 +7915,13 @@ static int get_rwh_policy_from_update(enum update_opt update)
 }
 
 static int update_subarray_imsm(struct supertype *st, char *subarray,
-				char *update, struct mddev_ident *ident)
+				enum update_opt update, struct mddev_ident *ident)
 {
 	/* update the subarray currently referenced by ->current_vol */
 	struct intel_super *super = st->sb;
 	struct imsm_super *mpb = super->anchor;
 
-	if (map_name(update_options, update) == UOPT_NAME) {
+	if (update == UOPT_NAME) {
 		char *name = ident->name;
 		char *ep;
 		int vol;
@@ -7955,7 +7955,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 			}
 			super->updates_pending++;
 		}
-	} else if (get_rwh_policy_from_update(map_name(update_options, update)) != UOPT_UNDEFINED) {
+	} else if (get_rwh_policy_from_update(update) != UOPT_UNDEFINED) {
 		int new_policy;
 		char *ep;
 		int vol = strtoul(subarray, &ep, 10);
@@ -7963,7 +7963,7 @@ static int update_subarray_imsm(struct supertype *st, char *subarray,
 		if (*ep != '\0' || vol >= super->anchor->num_raid_devs)
 			return 2;
 
-		new_policy = get_rwh_policy_from_update(map_name(update_options, update));
+		new_policy = get_rwh_policy_from_update(update);
 
 		if (st->update_tail) {
 			struct imsm_update_rwh_policy *u = xmalloc(sizeof(*u));
diff --git a/super0.c b/super0.c
index 4e53f41e..0804993e 100644
--- a/super0.c
+++ b/super0.c
@@ -491,7 +491,7 @@ static struct mdinfo *container_content0(struct supertype *st, char *subarray)
 }
 
 static int update_super0(struct supertype *st, struct mdinfo *info,
-			 char *update,
+			 enum update_opt update,
 			 char *devname, int verbose,
 			 int uuid_set, char *homehost)
 {
@@ -502,20 +502,19 @@ static int update_super0(struct supertype *st, struct mdinfo *info,
 	int rv = 0;
 	int uuid[4];
 	mdp_super_t *sb = st->sb;
-	enum update_opt update_enum = map_name(update_options, update);
 
-	if (update_enum == UOPT_HOMEHOST && homehost) {
+	if (update == UOPT_HOMEHOST && homehost) {
 		/*
 		 * note that 'homehost' is special as it is really
 		 * a "uuid" update.
 		 */
 		uuid_set = 0;
-		update_enum = UOPT_UUID;
+		update = UOPT_UUID;
 		info->uuid[0] = sb->set_uuid0;
 		info->uuid[1] = sb->set_uuid1;
 	}
 
-	switch (update_enum) {
+	switch (update) {
 	case UOPT_UUID:
 		if (!uuid_set && homehost) {
 			char buf[20];
diff --git a/super1.c b/super1.c
index 6c81c1b9..e6cc9b00 100644
--- a/super1.c
+++ b/super1.c
@@ -1202,7 +1202,7 @@ static struct mdinfo *container_content1(struct supertype *st, char *subarray)
 }
 
 static int update_super1(struct supertype *st, struct mdinfo *info,
-			 char *update, char *devname, int verbose,
+			 enum update_opt update, char *devname, int verbose,
 			 int uuid_set, char *homehost)
 {
 	/* NOTE: for 'assemble' and 'force' we need to return non-zero
@@ -1212,15 +1212,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 	int rv = 0;
 	struct mdp_superblock_1 *sb = st->sb;
 	bitmap_super_t *bms = (bitmap_super_t*)(((char*)sb) + MAX_SB_SIZE);
-	enum update_opt update_enum = map_name(update_options, update);
 
-	if (update_enum == UOPT_HOMEHOST && homehost) {
+	if (update == UOPT_HOMEHOST && homehost) {
 		/*
 		 * Note that 'homehost' is special as it is really
 		 * a "name" update.
 		 */
 		char *c;
-		update_enum = UOPT_NAME;
+		update = UOPT_NAME;
 		c = strchr(sb->set_name, ':');
 		if (c)
 			snprintf(info->name, sizeof(info->name), "%s", c+1);
@@ -1228,7 +1227,7 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 			snprintf(info->name, sizeof(info->name), "%s", sb->set_name);
 	}
 
-	switch (update_enum) {
+	switch (update) {
 	case UOPT_NAME:
 		if (!info->name[0])
 			snprintf(info->name, sizeof(info->name), "%d", info->array.md_minor);
@@ -1526,7 +1525,7 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 			 * If that couldn't happen, the "-nobackup" version
 			 * will be used.
 			 */
-			if (update_enum == UOPT_SPEC_REVERT_RESHAPE_NOBACKUP &&
+			if (update == UOPT_SPEC_REVERT_RESHAPE_NOBACKUP &&
 			    sb->reshape_position == 0 &&
 			    (__le32_to_cpu(sb->delta_disks) > 0 ||
 			     (__le32_to_cpu(sb->delta_disks) == 0 &&
@@ -1607,14 +1606,14 @@ static int update_super1(struct supertype *st, struct mdinfo *info,
 	case UOPT_LAYOUT_UNSPECIFIED:
 		if (__le32_to_cpu(sb->level) != 0) {
 			pr_err("%s: %s only supported for RAID0\n",
-			       devname ?: "", map_num(update_options, update_enum));
+			       devname ?: "", map_num(update_options, update));
 			rv = -1;
-		} else if (update_enum == UOPT_LAYOUT_UNSPECIFIED) {
+		} else if (update == UOPT_LAYOUT_UNSPECIFIED) {
 			sb->feature_map &= ~__cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
 			sb->layout = 0;
 		} else {
 			sb->feature_map |= __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
-			sb->layout = __cpu_to_le32(update_enum == UOPT_LAYOUT_ORIGINAL ? 1 : 2);
+			sb->layout = __cpu_to_le32(update == UOPT_LAYOUT_ORIGINAL ? 1 : 2);
 		}
 		break;
 	default:
-- 
2.26.2

Re: [PATCH 08/10] Change update to enum in update_super and update_subarray

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Use already existing enum, change update_super and update_subarray
> update to enum globally.
> Refactor function references also.
> Remove code specific options from update_options.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> ---
> Assemble.c    | 14 +++++++++-----
> Examine.c     |  2 +-
> Grow.c        |  9 +++++----
> Manage.c      | 14 ++++++++------
> maps.c        | 21 ---------------------
> mdadm.h       | 12 +++++++++---
> super-intel.c | 16 ++++++++--------
> super0.c      |  9 ++++-----
> super1.c      | 17 ++++++++---------
> 9 files changed, 52 insertions(+), 62 deletions(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index 6df6bfbc..8cd3d533 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> 

> @@ -1813,7 +1817,7 @@ try_again:
> 	    !enough(content->array.level, content->array.raid_disks,
> 		    content->array.layout, clean,
> 		    avail)) {
> -		change += st->ss->update_super(st, content, "force-array",
> +		change += st->ss->update_super(st, content, UOPT_SPEC_FORCE_ARRAY,
> 					       devices[chosen_drive].devname, c->verbose,
> 					       0, NULL);
> 		was_forced = 1;

Hi Mateusz,

The above part doesn’t apply on my current mdadm-CI queue. Just FYI, after I finish to review all this series, I will ask you to confirm the rebased patch or repost another version.

Thanks.

Coly Li

Re: [PATCH 08/10] Change update to enum in update_super and update_subarray

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Use already existing enum, change update_super and update_subarray
> update to enum globally.
> Refactor function references also.
> Remove code specific options from update_options.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> ---
> Assemble.c    | 14 +++++++++-----
> Examine.c     |  2 +-
> Grow.c        |  9 +++++----
> Manage.c      | 14 ++++++++------
> maps.c        | 21 ---------------------
> mdadm.h       | 12 +++++++++---
> super-intel.c | 16 ++++++++--------
> super0.c      |  9 ++++-----
> super1.c      | 17 ++++++++---------
> 9 files changed, 52 insertions(+), 62 deletions(-)
> 
> 

[snipped]

> diff --git a/mdadm.h b/mdadm.h
> index 7bc31b16..afc2e2a8 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1010,7 +1010,7 @@ extern struct superswitch {
> 	 *                    it will resume going in the opposite direction.
> 	 */
> 	int (*update_super)(struct supertype *st, struct mdinfo *info,
> -			    char *update,
> +			    enum update_opt update,
> 			    char *devname, int verbose,
> 			    int uuid_set, char *homehost);
> 
> @@ -1136,9 +1136,15 @@ extern struct superswitch {
> 	/* Permit subarray's to be deleted from inactive containers */
> 	int (*kill_subarray)(struct supertype *st,
> 			     char *subarray_id); /* optional */
> -	/* Permit subarray's to be modified */
> +	/**
> +	 * update_subarray() - Permit subarray to be modified.
> +	 * @st: Supertype.
> +	 * @subarray: Subarray name.
> +	 * @update: Update option.
> +	 * @ident: Optional identifiers.
> +	 */

Maybe we should follow existing comment code style like,

/* Commet start here,
 * and second line.
 */

This patch doesn’t apply on latest mdadm upstream, in the mdadm-CI tree, I rebased the patch and push it into remotes/origin/20220903-testing.
Could you please to check the rebased patch?

Thanks.

Coly Li

Re: [PATCH 08/10] Change update to enum in update_super and update_subarray

[Kusiak, Mateusz]

On 14/09/2022 17:03, Coly Li wrote:
> 
> 
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
>>
>> Use already existing enum, change update_super and update_subarray
>> update to enum globally.
>> Refactor function references also.
>> Remove code specific options from update_options.
>>
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
>> ---
>> Assemble.c    | 14 +++++++++-----
>> Examine.c     |  2 +-
>> Grow.c        |  9 +++++----
>> Manage.c      | 14 ++++++++------
>> maps.c        | 21 ---------------------
>> mdadm.h       | 12 +++++++++---
>> super-intel.c | 16 ++++++++--------
>> super0.c      |  9 ++++-----
>> super1.c      | 17 ++++++++---------
>> 9 files changed, 52 insertions(+), 62 deletions(-)
>>
>>
> 
> [snipped]
> 
>> diff --git a/mdadm.h b/mdadm.h
>> index 7bc31b16..afc2e2a8 100644
>> --- a/mdadm.h
>> +++ b/mdadm.h
>> @@ -1010,7 +1010,7 @@ extern struct superswitch {
>> 	 *                    it will resume going in the opposite direction.
>> 	 */
>> 	int (*update_super)(struct supertype *st, struct mdinfo *info,
>> -			    char *update,
>> +			    enum update_opt update,
>> 			    char *devname, int verbose,
>> 			    int uuid_set, char *homehost);
>>
>> @@ -1136,9 +1136,15 @@ extern struct superswitch {
>> 	/* Permit subarray's to be deleted from inactive containers */
>> 	int (*kill_subarray)(struct supertype *st,
>> 			     char *subarray_id); /* optional */
>> -	/* Permit subarray's to be modified */
>> +	/**
>> +	 * update_subarray() - Permit subarray to be modified.
>> +	 * @st: Supertype.
>> +	 * @subarray: Subarray name.
>> +	 * @update: Update option.
>> +	 * @ident: Optional identifiers.
>> +	 */
> 
> Maybe we should follow existing comment code style like,
> 
> /* Commet start here,
>  * and second line.
>  */
> 
I am not concerned, IMO the comment (function description) follows kerel
standards.
There already are functions with this type of description inside the
file like signal_s() or close_fd().

> This patch doesn’t apply on latest mdadm upstream, in the mdadm-CI tree, I rebased the patch and push it into remotes/origin/20220903-testing.
> Could you please to check the rebased patch?
> 
I checked the branch and it looks good to me.
However, since there are allready fixes to be made, I'll rebase the
whole patchset anyway, on top of the master, when sending v2.
I'm planing to resend whole patchset to avoid complications.
I'm looking forward your response regarding the rest of the questions,
and I will resend patchset when everything is clear.

Thanks,
Mateusz

> Thanks.
> 
> Coly Li

[PATCH 09/10] Manage&Incremental: code refactor, string to enum

[Mateusz Kusiak]

Prepare Manage and Incremental for later changing context->update to enum.
Change update from string to enum in multiple functions and pass enum
where already possible.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 Grow.c        |  8 ++++----
 Incremental.c |  8 ++++----
 Manage.c      | 35 +++++++++++++++++------------------
 mdadm.c       | 23 ++++++++++++++++++-----
 mdadm.h       |  4 ++--
 5 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/Grow.c b/Grow.c
index 83b38a71..3cd4db48 100644
--- a/Grow.c
+++ b/Grow.c
@@ -602,12 +602,12 @@ int Grow_consistency_policy(char *devname, int fd, struct context *c, struct sha
 	}
 
 	if (subarray) {
-		char *update;
+		enum update_opt update;
 
 		if (s->consistency_policy == CONSISTENCY_POLICY_PPL)
-			update = "ppl";
+			update = UOPT_PPL;
 		else
-			update = "no-ppl";
+			update = UOPT_NO_PPL;
 
 		sprintf(container_dev, "/dev/%s", st->container_devnm);
 
@@ -3234,7 +3234,7 @@ static int reshape_array(char *container, int fd, char *devname,
 	 * level and frozen, we can safely add them.
 	 */
 	if (devlist) {
-		if (Manage_subdevs(devname, fd, devlist, verbose, 0, NULL, 0))
+		if (Manage_subdevs(devname, fd, devlist, verbose, 0, UOPT_UNDEFINED, 0))
 			goto release;
 	}
 
diff --git a/Incremental.c b/Incremental.c
index 4d0cd9d6..cfee582f 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1025,7 +1025,7 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
 			close(dfd);
 			*dfdp = -1;
 			rv =  Manage_subdevs(chosen->sys_name, mdfd, &devlist,
-					     -1, 0, NULL, 0);
+					     -1, 0, UOPT_UNDEFINED, 0);
 			close(mdfd);
 		}
 		if (verbose > 0) {
@@ -1666,7 +1666,7 @@ static void remove_from_member_array(struct mdstat_ent *memb,
 
 	if (subfd >= 0) {
 		rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
-				    0, NULL, 0);
+				    0, UOPT_UNDEFINED, 0);
 		if (rv & 2) {
 			if (sysfs_init(&mmdi, -1, memb->devnm))
 				pr_err("unable to initialize sysfs for: %s\n",
@@ -1758,7 +1758,7 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
 		free_mdstat(mdstat);
 	} else {
 		rv |= Manage_subdevs(ent->devnm, mdfd, &devlist,
-				    verbose, 0, NULL, 0);
+				    verbose, 0, UOPT_UNDEFINED, 0);
 		if (rv & 2) {
 		/* Failed due to EBUSY, try to stop the array.
 		 * Give udisks a chance to unmount it first.
@@ -1770,7 +1770,7 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
 
 	devlist.disposition = 'r';
 	rv = Manage_subdevs(ent->devnm, mdfd, &devlist,
-			    verbose, 0, NULL, 0);
+			    verbose, 0, UOPT_UNDEFINED, 0);
 end:
 	close(mdfd);
 	free_mdstat(ent);
diff --git a/Manage.c b/Manage.c
index c47b6262..e560bdb9 100644
--- a/Manage.c
+++ b/Manage.c
@@ -598,14 +598,12 @@ static void add_set(struct mddev_dev *dv, int fd, char set_char)
 
 int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
 		   struct supertype *dev_st, struct supertype *tst,
-		   unsigned long rdev,
-		   char *update, char *devname, int verbose,
-		   mdu_array_info_t *array)
+		   unsigned long rdev, enum update_opt update,
+		   char *devname, int verbose, mdu_array_info_t *array)
 {
 	struct mdinfo mdi;
 	int duuid[4];
 	int ouuid[4];
-	enum update_opt update_enum = map_name(update_options, update);
 
 	dev_st->ss->getinfo_super(dev_st, &mdi, NULL);
 	dev_st->ss->uuid_from_super(dev_st, ouuid);
@@ -683,7 +681,7 @@ int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
 					devname, verbose, 0, NULL);
 			if (update)
 				rv = dev_st->ss->update_super(
-					dev_st, NULL, update_enum,
+					dev_st, NULL, update,
 					devname, verbose, 0, NULL);
 			if (rv == 0)
 				rv = dev_st->ss->store_super(dev_st, tfd);
@@ -715,8 +713,8 @@ skip_re_add:
 int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 	       struct supertype *tst, mdu_array_info_t *array,
 	       int force, int verbose, char *devname,
-	       char *update, unsigned long rdev, unsigned long long array_size,
-	       int raid_slot)
+	       enum update_opt update, unsigned long rdev,
+	       unsigned long long array_size, int raid_slot)
 {
 	unsigned long long ldsize;
 	struct supertype *dev_st;
@@ -1288,7 +1286,7 @@ int Manage_with(struct supertype *tst, int fd, struct mddev_dev *dv,
 
 int Manage_subdevs(char *devname, int fd,
 		   struct mddev_dev *devlist, int verbose, int test,
-		   char *update, int force)
+		   enum update_opt update, int force)
 {
 	/* Do something to each dev.
 	 * devmode can be
@@ -1676,12 +1674,13 @@ int autodetect(void)
 	return rv;
 }
 
-int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident *ident, int verbose)
+int Update_subarray(char *dev, char *subarray, enum update_opt update,
+		    struct mddev_ident *ident, int verbose)
 {
 	struct supertype supertype, *st = &supertype;
 	int fd, rv = 2;
 	struct mdinfo *info = NULL;
-	enum update_opt update_enum = map_name(update_options, update);
+	char *update_verb = map_num(update_options, update);
 
 	memset(st, 0, sizeof(*st));
 
@@ -1699,7 +1698,7 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
 	if (is_subarray_active(subarray, st->devnm)) {
 		if (verbose >= 0)
 			pr_err("Subarray %s in %s is active, cannot update %s\n",
-			       subarray, dev, update);
+				subarray, dev, update_verb);
 		goto free_super;
 	}
 
@@ -1708,23 +1707,23 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
 
 	info = st->ss->container_content(st, subarray);
 
-	if (strncmp(update, "ppl", 3) == 0 && !is_level456(info->array.level)) {
+	if (update == UOPT_PPL && !is_level456(info->array.level)) {
 		pr_err("RWH policy ppl is supported only for raid4, raid5 and raid6.\n");
 		goto free_super;
 	}
 
-	rv = st->ss->update_subarray(st, subarray, update_enum, ident);
+	rv = st->ss->update_subarray(st, subarray, update, ident);
 
 	if (rv) {
 		if (verbose >= 0)
 			pr_err("Failed to update %s of subarray-%s in %s\n",
-				update, subarray, dev);
+				update_verb, subarray, dev);
 	} else if (st->update_tail)
 		flush_metadata_updates(st);
 	else
 		st->ss->sync_metadata(st);
 
-	if (rv == 0 && strcmp(update, "name") == 0 && verbose >= 0)
+	if (rv == 0 && update == UOPT_NAME && verbose >= 0)
 		pr_err("Updated subarray-%s name from %s, UUIDs may have changed\n",
 		       subarray, dev);
 
@@ -1765,10 +1764,10 @@ int move_spare(char *from_devname, char *to_devname, dev_t devid)
 	sprintf(devname, "%d:%d", major(devid), minor(devid));
 
 	devlist.disposition = 'r';
-	if (Manage_subdevs(from_devname, fd2, &devlist, -1, 0, NULL, 0) == 0) {
+	if (Manage_subdevs(from_devname, fd2, &devlist, -1, 0, UOPT_UNDEFINED, 0) == 0) {
 		devlist.disposition = 'a';
 		if (Manage_subdevs(to_devname, fd1, &devlist, -1, 0,
-				   NULL, 0) == 0) {
+				   UOPT_UNDEFINED, 0) == 0) {
 			/* make sure manager is aware of changes */
 			ping_manager(to_devname);
 			ping_manager(from_devname);
@@ -1778,7 +1777,7 @@ int move_spare(char *from_devname, char *to_devname, dev_t devid)
 		}
 		else
 			Manage_subdevs(from_devname, fd2, &devlist,
-				       -1, 0, NULL, 0);
+				       -1, 0, UOPT_UNDEFINED, 0);
 	}
 	close(fd1);
 	close(fd2);
diff --git a/mdadm.c b/mdadm.c
index 3705d114..b55e0d9a 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1433,10 +1433,22 @@ int main(int argc, char *argv[])
 		/* readonly, add/remove, readwrite, runstop */
 		if (c.readonly > 0)
 			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
-		if (!rv && devs_found>1)
-			rv = Manage_subdevs(devlist->devname, mdfd,
-					    devlist->next, c.verbose, c.test,
-					    c.update, c.force);
+		if (!rv && devs_found > 1) {
+			/*
+			 * This is temporary and will be removed in next patches
+			 * Null c.update will cause segfault
+			 */
+			if (c.update)
+				rv = Manage_subdevs(devlist->devname, mdfd,
+						devlist->next, c.verbose, c.test,
+						map_name(update_options, c.update),
+						c.force);
+			else
+				rv = Manage_subdevs(devlist->devname, mdfd,
+						devlist->next, c.verbose, c.test,
+						UOPT_UNDEFINED,
+						c.force);
+		}
 		if (!rv && c.readonly < 0)
 			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
 		if (!rv && c.runstop > 0)
@@ -1964,7 +1976,8 @@ static int misc_list(struct mddev_dev *devlist,
 				continue;
 			}
 			rv |= Update_subarray(dv->devname, c->subarray,
-					      c->update, ident, c->verbose);
+					      map_name(update_options, c->update),
+					      ident, c->verbose);
 			continue;
 		case Dump:
 			rv |= Dump_metadata(dv->devname, dump_directory, c, ss);
diff --git a/mdadm.h b/mdadm.h
index afc2e2a8..fe09fd46 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1477,7 +1477,7 @@ extern int Manage_stop(char *devname, int fd, int quiet,
 		       int will_retry);
 extern int Manage_subdevs(char *devname, int fd,
 			  struct mddev_dev *devlist, int verbose, int test,
-			  char *update, int force);
+			  enum update_opt update, int force);
 extern int autodetect(void);
 extern int Grow_Add_device(char *devname, int fd, char *newdev);
 extern int Grow_addbitmap(char *devname, int fd,
@@ -1533,7 +1533,7 @@ extern int Monitor(struct mddev_dev *devlist,
 
 extern int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl);
 extern int Kill_subarray(char *dev, char *subarray, int verbose);
-extern int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident *ident, int quiet);
+extern int Update_subarray(char *dev, char *subarray, enum update_opt update, struct mddev_ident *ident, int quiet);
 extern int Wait(char *dev);
 extern int WaitClean(char *dev, int verbose);
 extern int SetAction(char *dev, char *action);
-- 
2.26.2

Re: [PATCH 09/10] Manage&Incremental: code refactor, string to enum

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Prepare Manage and Incremental for later changing context->update to enum.
> Change update from string to enum in multiple functions and pass enum
> where already possible.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>


Although I don’t test this change yet, it looks fine to me.

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
> Grow.c        |  8 ++++----
> Incremental.c |  8 ++++----
> Manage.c      | 35 +++++++++++++++++------------------
> mdadm.c       | 23 ++++++++++++++++++-----
> mdadm.h       |  4 ++--
> 5 files changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/Grow.c b/Grow.c
> index 83b38a71..3cd4db48 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -602,12 +602,12 @@ int Grow_consistency_policy(char *devname, int fd, struct context *c, struct sha
> 	}
> 
> 	if (subarray) {
> -		char *update;
> +		enum update_opt update;
> 
> 		if (s->consistency_policy == CONSISTENCY_POLICY_PPL)
> -			update = "ppl";
> +			update = UOPT_PPL;
> 		else
> -			update = "no-ppl";
> +			update = UOPT_NO_PPL;
> 
> 		sprintf(container_dev, "/dev/%s", st->container_devnm);
> 
> @@ -3234,7 +3234,7 @@ static int reshape_array(char *container, int fd, char *devname,
> 	 * level and frozen, we can safely add them.
> 	 */
> 	if (devlist) {
> -		if (Manage_subdevs(devname, fd, devlist, verbose, 0, NULL, 0))
> +		if (Manage_subdevs(devname, fd, devlist, verbose, 0, UOPT_UNDEFINED, 0))
> 			goto release;
> 	}
> 
> diff --git a/Incremental.c b/Incremental.c
> index 4d0cd9d6..cfee582f 100644
> --- a/Incremental.c
> +++ b/Incremental.c
> @@ -1025,7 +1025,7 @@ static int array_try_spare(char *devname, int *dfdp, struct dev_policy *pol,
> 			close(dfd);
> 			*dfdp = -1;
> 			rv =  Manage_subdevs(chosen->sys_name, mdfd, &devlist,
> -					     -1, 0, NULL, 0);
> +					     -1, 0, UOPT_UNDEFINED, 0);
> 			close(mdfd);
> 		}
> 		if (verbose > 0) {
> @@ -1666,7 +1666,7 @@ static void remove_from_member_array(struct mdstat_ent *memb,
> 
> 	if (subfd >= 0) {
> 		rv = Manage_subdevs(memb->devnm, subfd, devlist, verbose,
> -				    0, NULL, 0);
> +				    0, UOPT_UNDEFINED, 0);
> 		if (rv & 2) {
> 			if (sysfs_init(&mmdi, -1, memb->devnm))
> 				pr_err("unable to initialize sysfs for: %s\n",
> @@ -1758,7 +1758,7 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
> 		free_mdstat(mdstat);
> 	} else {
> 		rv |= Manage_subdevs(ent->devnm, mdfd, &devlist,
> -				    verbose, 0, NULL, 0);
> +				    verbose, 0, UOPT_UNDEFINED, 0);
> 		if (rv & 2) {
> 		/* Failed due to EBUSY, try to stop the array.
> 		 * Give udisks a chance to unmount it first.
> @@ -1770,7 +1770,7 @@ int IncrementalRemove(char *devname, char *id_path, int verbose)
> 
> 	devlist.disposition = 'r';
> 	rv = Manage_subdevs(ent->devnm, mdfd, &devlist,
> -			    verbose, 0, NULL, 0);
> +			    verbose, 0, UOPT_UNDEFINED, 0);
> end:
> 	close(mdfd);
> 	free_mdstat(ent);
> diff --git a/Manage.c b/Manage.c
> index c47b6262..e560bdb9 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -598,14 +598,12 @@ static void add_set(struct mddev_dev *dv, int fd, char set_char)
> 
> int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
> 		   struct supertype *dev_st, struct supertype *tst,
> -		   unsigned long rdev,
> -		   char *update, char *devname, int verbose,
> -		   mdu_array_info_t *array)
> +		   unsigned long rdev, enum update_opt update,
> +		   char *devname, int verbose, mdu_array_info_t *array)
> {
> 	struct mdinfo mdi;
> 	int duuid[4];
> 	int ouuid[4];
> -	enum update_opt update_enum = map_name(update_options, update);
> 
> 	dev_st->ss->getinfo_super(dev_st, &mdi, NULL);
> 	dev_st->ss->uuid_from_super(dev_st, ouuid);
> @@ -683,7 +681,7 @@ int attempt_re_add(int fd, int tfd, struct mddev_dev *dv,
> 					devname, verbose, 0, NULL);
> 			if (update)
> 				rv = dev_st->ss->update_super(
> -					dev_st, NULL, update_enum,
> +					dev_st, NULL, update,
> 					devname, verbose, 0, NULL);
> 			if (rv == 0)
> 				rv = dev_st->ss->store_super(dev_st, tfd);
> @@ -715,8 +713,8 @@ skip_re_add:
> int Manage_add(int fd, int tfd, struct mddev_dev *dv,
> 	       struct supertype *tst, mdu_array_info_t *array,
> 	       int force, int verbose, char *devname,
> -	       char *update, unsigned long rdev, unsigned long long array_size,
> -	       int raid_slot)
> +	       enum update_opt update, unsigned long rdev,
> +	       unsigned long long array_size, int raid_slot)
> {
> 	unsigned long long ldsize;
> 	struct supertype *dev_st;
> @@ -1288,7 +1286,7 @@ int Manage_with(struct supertype *tst, int fd, struct mddev_dev *dv,
> 
> int Manage_subdevs(char *devname, int fd,
> 		   struct mddev_dev *devlist, int verbose, int test,
> -		   char *update, int force)
> +		   enum update_opt update, int force)
> {
> 	/* Do something to each dev.
> 	 * devmode can be
> @@ -1676,12 +1674,13 @@ int autodetect(void)
> 	return rv;
> }
> 
> -int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident *ident, int verbose)
> +int Update_subarray(char *dev, char *subarray, enum update_opt update,
> +		    struct mddev_ident *ident, int verbose)
> {
> 	struct supertype supertype, *st = &supertype;
> 	int fd, rv = 2;
> 	struct mdinfo *info = NULL;
> -	enum update_opt update_enum = map_name(update_options, update);
> +	char *update_verb = map_num(update_options, update);
> 
> 	memset(st, 0, sizeof(*st));
> 
> @@ -1699,7 +1698,7 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
> 	if (is_subarray_active(subarray, st->devnm)) {
> 		if (verbose >= 0)
> 			pr_err("Subarray %s in %s is active, cannot update %s\n",
> -			       subarray, dev, update);
> +				subarray, dev, update_verb);
> 		goto free_super;
> 	}
> 
> @@ -1708,23 +1707,23 @@ int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident
> 
> 	info = st->ss->container_content(st, subarray);
> 
> -	if (strncmp(update, "ppl", 3) == 0 && !is_level456(info->array.level)) {
> +	if (update == UOPT_PPL && !is_level456(info->array.level)) {
> 		pr_err("RWH policy ppl is supported only for raid4, raid5 and raid6.\n");
> 		goto free_super;
> 	}
> 
> -	rv = st->ss->update_subarray(st, subarray, update_enum, ident);
> +	rv = st->ss->update_subarray(st, subarray, update, ident);
> 
> 	if (rv) {
> 		if (verbose >= 0)
> 			pr_err("Failed to update %s of subarray-%s in %s\n",
> -				update, subarray, dev);
> +				update_verb, subarray, dev);
> 	} else if (st->update_tail)
> 		flush_metadata_updates(st);
> 	else
> 		st->ss->sync_metadata(st);
> 
> -	if (rv == 0 && strcmp(update, "name") == 0 && verbose >= 0)
> +	if (rv == 0 && update == UOPT_NAME && verbose >= 0)
> 		pr_err("Updated subarray-%s name from %s, UUIDs may have changed\n",
> 		       subarray, dev);
> 
> @@ -1765,10 +1764,10 @@ int move_spare(char *from_devname, char *to_devname, dev_t devid)
> 	sprintf(devname, "%d:%d", major(devid), minor(devid));
> 
> 	devlist.disposition = 'r';
> -	if (Manage_subdevs(from_devname, fd2, &devlist, -1, 0, NULL, 0) == 0) {
> +	if (Manage_subdevs(from_devname, fd2, &devlist, -1, 0, UOPT_UNDEFINED, 0) == 0) {
> 		devlist.disposition = 'a';
> 		if (Manage_subdevs(to_devname, fd1, &devlist, -1, 0,
> -				   NULL, 0) == 0) {
> +				   UOPT_UNDEFINED, 0) == 0) {
> 			/* make sure manager is aware of changes */
> 			ping_manager(to_devname);
> 			ping_manager(from_devname);
> @@ -1778,7 +1777,7 @@ int move_spare(char *from_devname, char *to_devname, dev_t devid)
> 		}
> 		else
> 			Manage_subdevs(from_devname, fd2, &devlist,
> -				       -1, 0, NULL, 0);
> +				       -1, 0, UOPT_UNDEFINED, 0);
> 	}
> 	close(fd1);
> 	close(fd2);
> diff --git a/mdadm.c b/mdadm.c
> index 3705d114..b55e0d9a 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1433,10 +1433,22 @@ int main(int argc, char *argv[])
> 		/* readonly, add/remove, readwrite, runstop */
> 		if (c.readonly > 0)
> 			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
> -		if (!rv && devs_found>1)
> -			rv = Manage_subdevs(devlist->devname, mdfd,
> -					    devlist->next, c.verbose, c.test,
> -					    c.update, c.force);
> +		if (!rv && devs_found > 1) {
> +			/*
> +			 * This is temporary and will be removed in next patches
> +			 * Null c.update will cause segfault
> +			 */
> +			if (c.update)
> +				rv = Manage_subdevs(devlist->devname, mdfd,
> +						devlist->next, c.verbose, c.test,
> +						map_name(update_options, c.update),
> +						c.force);
> +			else
> +				rv = Manage_subdevs(devlist->devname, mdfd,
> +						devlist->next, c.verbose, c.test,
> +						UOPT_UNDEFINED,
> +						c.force);
> +		}
> 		if (!rv && c.readonly < 0)
> 			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
> 		if (!rv && c.runstop > 0)
> @@ -1964,7 +1976,8 @@ static int misc_list(struct mddev_dev *devlist,
> 				continue;
> 			}
> 			rv |= Update_subarray(dv->devname, c->subarray,
> -					      c->update, ident, c->verbose);
> +					      map_name(update_options, c->update),
> +					      ident, c->verbose);
> 			continue;
> 		case Dump:
> 			rv |= Dump_metadata(dv->devname, dump_directory, c, ss);
> diff --git a/mdadm.h b/mdadm.h
> index afc2e2a8..fe09fd46 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1477,7 +1477,7 @@ extern int Manage_stop(char *devname, int fd, int quiet,
> 		       int will_retry);
> extern int Manage_subdevs(char *devname, int fd,
> 			  struct mddev_dev *devlist, int verbose, int test,
> -			  char *update, int force);
> +			  enum update_opt update, int force);
> extern int autodetect(void);
> extern int Grow_Add_device(char *devname, int fd, char *newdev);
> extern int Grow_addbitmap(char *devname, int fd,
> @@ -1533,7 +1533,7 @@ extern int Monitor(struct mddev_dev *devlist,
> 
> extern int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl);
> extern int Kill_subarray(char *dev, char *subarray, int verbose);
> -extern int Update_subarray(char *dev, char *subarray, char *update, struct mddev_ident *ident, int quiet);
> +extern int Update_subarray(char *dev, char *subarray, enum update_opt update, struct mddev_ident *ident, int quiet);
> extern int Wait(char *dev);
> extern int WaitClean(char *dev, int verbose);
> extern int SetAction(char *dev, char *action);
> -- 
> 2.26.2
> 

[PATCH 10/10] Change char* to enum in context->update & refactor code

[Mateusz Kusiak]

Storing update option in string is bad for frequent comparisons and
error prone.
Replace char array with enum so already existing enum is passed around
instead of string.
Adapt code to changes.

Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
---
 Assemble.c | 40 +++++++++++++++++-----------------------
 mdadm.c    | 52 +++++++++++++++++++---------------------------------
 mdadm.h    |  2 +-
 3 files changed, 37 insertions(+), 57 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 8cd3d533..942e352e 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -135,17 +135,17 @@ static int ident_matches(struct mddev_ident *ident,
 			 struct mdinfo *content,
 			 struct supertype *tst,
 			 char *homehost, int require_homehost,
-			 char *update, char *devname)
+			 enum update_opt update, char *devname)
 {
 
-	if (ident->uuid_set && (!update || strcmp(update, "uuid")!= 0) &&
+	if (ident->uuid_set && update != UOPT_UUID &&
 	    same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0 &&
 	    memcmp(content->uuid, uuid_zero, sizeof(int[4])) != 0) {
 		if (devname)
 			pr_err("%s has wrong uuid.\n", devname);
 		return 0;
 	}
-	if (ident->name[0] && (!update || strcmp(update, "name")!= 0) &&
+	if (ident->name[0] && update != UOPT_NAME &&
 	    name_matches(content->name, ident->name, homehost, require_homehost)==0) {
 		if (devname)
 			pr_err("%s has wrong name.\n", devname);
@@ -648,11 +648,10 @@ static int load_devices(struct devs *devices, char *devmap,
 			int err;
 			fstat(mdfd, &stb2);
 
-			if (strcmp(c->update, "uuid") == 0 && !ident->uuid_set)
+			if (c->update == UOPT_UUID && !ident->uuid_set)
 				random_uuid((__u8 *)ident->uuid);
 
-			if (strcmp(c->update, "ppl") == 0 &&
-			    ident->bitmap_fd >= 0) {
+			if (c->update == UOPT_PPL && ident->bitmap_fd >= 0) {
 				pr_err("PPL is not compatible with bitmap\n");
 				close(mdfd);
 				free(devices);
@@ -684,34 +683,30 @@ static int load_devices(struct devs *devices, char *devmap,
 			strcpy(content->name, ident->name);
 			content->array.md_minor = minor(stb2.st_rdev);
 
-			if (strcmp(c->update, "byteorder") == 0)
+			if (c->update == UOPT_BYTEORDER)
 				err = 0;
-			else if (strcmp(c->update, "home-cluster") == 0) {
+			else if (c->update == UOPT_HOME_CLUSTER) {
 				tst->cluster_name = c->homecluster;
 				err = tst->ss->write_bitmap(tst, dfd, NameUpdate);
-			} else if (strcmp(c->update, "nodes") == 0) {
+			} else if (c->update == UOPT_NODES) {
 				tst->nodes = c->nodes;
 				err = tst->ss->write_bitmap(tst, dfd, NodeNumUpdate);
-			} else if (strcmp(c->update, "revert-reshape") == 0 &&
-				   c->invalid_backup)
+			} else if (c->update == UOPT_REVERT_RESHAPE && c->invalid_backup)
 				err = tst->ss->update_super(tst, content,
 							    UOPT_SPEC_REVERT_RESHAPE_NOBACKUP,
 							    devname, c->verbose,
 							    ident->uuid_set,
 							    c->homehost);
 			else
-				/*
-				 * Mapping is temporary, will be removed in this patchset
-				 */
 				err = tst->ss->update_super(tst, content,
-							    map_name(update_options, c->update),
+							    c->update,
 							    devname, c->verbose,
 							    ident->uuid_set,
 							    c->homehost);
 			if (err < 0) {
 				if (err == -1)
 					pr_err("--update=%s not understood for %s metadata\n",
-					       c->update, tst->ss->name);
+					       map_num(update_options, c->update), tst->ss->name);
 				tst->ss->free_super(tst);
 				free(tst);
 				close(mdfd);
@@ -721,7 +716,7 @@ static int load_devices(struct devs *devices, char *devmap,
 				*stp = st;
 				return -1;
 			}
-			if (strcmp(c->update, "uuid")==0 &&
+			if (c->update == UOPT_UUID &&
 			    !ident->uuid_set) {
 				ident->uuid_set = 1;
 				memcpy(ident->uuid, content->uuid, 16);
@@ -730,7 +725,7 @@ static int load_devices(struct devs *devices, char *devmap,
 				pr_err("Could not re-write superblock on %s.\n",
 				       devname);
 
-			if (strcmp(c->update, "uuid")==0 &&
+			if (c->update == UOPT_UUID &&
 			    ident->bitmap_fd >= 0 && !bitmap_done) {
 				if (bitmap_update_uuid(ident->bitmap_fd,
 						       content->uuid,
@@ -1188,8 +1183,7 @@ static int start_array(int mdfd,
 				pr_err("%s: Need a backup file to complete reshape of this array.\n",
 				       mddev);
 				pr_err("Please provided one with \"--backup-file=...\"\n");
-				if (c->update &&
-				    strcmp(c->update, "revert-reshape") == 0)
+				if (c->update == UOPT_REVERT_RESHAPE)
 					pr_err("(Don't specify --update=revert-reshape again, that part succeeded.)\n");
 				return 1;
 			}
@@ -1487,7 +1481,7 @@ try_again:
 	 */
 	if (map_lock(&map))
 		pr_err("failed to get exclusive lock on mapfile - continue anyway...\n");
-	if (c->update && strcmp(c->update,"uuid") == 0)
+	if (c->update == UOPT_UUID)
 		mp = NULL;
 	else
 		mp = map_by_uuid(&map, content->uuid);
@@ -1635,7 +1629,7 @@ try_again:
 		goto out;
 	}
 
-	if (c->update && strcmp(c->update, "byteorder")==0)
+	if (c->update == UOPT_BYTEORDER)
 		st->minor_version = 90;
 
 	st->ss->getinfo_super(st, content, NULL);
@@ -1904,7 +1898,7 @@ try_again:
 	/* First, fill in the map, so that udev can find our name
 	 * as soon as we become active.
 	 */
-	if (c->update && strcmp(c->update, "metadata")==0) {
+	if (c->update == UOPT_METADATA) {
 		content->array.major_version = 1;
 		content->array.minor_version = 0;
 		strcpy(content->text_version, "1.0");
diff --git a/mdadm.c b/mdadm.c
index b55e0d9a..dc6d6a95 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -746,7 +746,7 @@ int main(int argc, char *argv[])
 		case O(MISC,'U'):
 			if (c.update) {
 				pr_err("Can only update one aspect of superblock, both %s and %s given.\n",
-					c.update, optarg);
+					map_num(update_options, c.update), optarg);
 				exit(2);
 			}
 			if (mode == MISC && !c.subarray) {
@@ -754,8 +754,7 @@ int main(int argc, char *argv[])
 				exit(2);
 			}
 
-			c.update = optarg;
-			enum update_opt updateopt = map_name(update_options, c.update);
+			c.update = map_name(update_options, optarg);
 			enum update_opt print_mode = UOPT_HELP;
 			const char *error_addon = "update option";
 
@@ -763,14 +762,14 @@ int main(int argc, char *argv[])
 				print_mode = UOPT_SUBARRAY_ONLY;
 				error_addon = "update-subarray option";
 
-				if (updateopt > UOPT_SUBARRAY_ONLY && updateopt < UOPT_HELP)
-					updateopt = UOPT_UNDEFINED;
+				if (c.update > UOPT_SUBARRAY_ONLY && c.update < UOPT_HELP)
+					c.update = UOPT_UNDEFINED;
 			}
 
-			switch (updateopt) {
+			switch (c.update) {
 			case UOPT_UNDEFINED:
 				pr_err("'--update=%s' is invalid %s. ",
-					c.update, error_addon);
+					optarg, error_addon);
 				outf = stderr;
 			case UOPT_HELP:
 				if (!outf)
@@ -795,14 +794,14 @@ int main(int argc, char *argv[])
 			}
 			if (c.update) {
 				pr_err("Can only update one aspect of superblock, both %s and %s given.\n",
-					c.update, optarg);
+					map_num(update_options, c.update), optarg);
 				exit(2);
 			}
-			c.update = optarg;
-			if (strcmp(c.update, "devicesize") != 0 &&
-			    strcmp(c.update, "bbl") != 0 &&
-			    strcmp(c.update, "force-no-bbl") != 0 &&
-			    strcmp(c.update, "no-bbl") != 0) {
+			c.update = map_name(update_options, optarg);
+			if (c.update != UOPT_DEVICESIZE &&
+			    c.update != UOPT_BBL &&
+			    c.update != UOPT_NO_BBL &&
+			    c.update != UOPT_FORCE_NO_BBL) {
 				pr_err("only 'devicesize', 'bbl', 'no-bbl', and 'force-no-bbl' can be updated with --re-add\n");
 				exit(2);
 			}
@@ -1388,7 +1387,7 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if (c.update && strcmp(c.update, "nodes") == 0 && c.nodes == 0) {
+	if (c.update && c.update == UOPT_NODES && c.nodes == 0) {
 		pr_err("Please specify nodes number with --nodes\n");
 		exit(1);
 	}
@@ -1433,22 +1432,10 @@ int main(int argc, char *argv[])
 		/* readonly, add/remove, readwrite, runstop */
 		if (c.readonly > 0)
 			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
-		if (!rv && devs_found > 1) {
-			/*
-			 * This is temporary and will be removed in next patches
-			 * Null c.update will cause segfault
-			 */
-			if (c.update)
-				rv = Manage_subdevs(devlist->devname, mdfd,
-						devlist->next, c.verbose, c.test,
-						map_name(update_options, c.update),
-						c.force);
-			else
-				rv = Manage_subdevs(devlist->devname, mdfd,
-						devlist->next, c.verbose, c.test,
-						UOPT_UNDEFINED,
-						c.force);
-		}
+		if (!rv && devs_found > 1)
+			rv = Manage_subdevs(devlist->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);
 		if (!rv && c.runstop > 0)
@@ -1970,14 +1957,13 @@ static int misc_list(struct mddev_dev *devlist,
 			rv |= Kill_subarray(dv->devname, c->subarray, c->verbose);
 			continue;
 		case UpdateSubarray:
-			if (c->update == NULL) {
+			if (!c->update) {
 				pr_err("-U/--update must be specified with --update-subarray\n");
 				rv |= 1;
 				continue;
 			}
 			rv |= Update_subarray(dv->devname, c->subarray,
-					      map_name(update_options, c->update),
-					      ident, c->verbose);
+					      c->update, ident, c->verbose);
 			continue;
 		case Dump:
 			rv |= Dump_metadata(dv->devname, dump_directory, c, ss);
diff --git a/mdadm.h b/mdadm.h
index fe09fd46..c732a936 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -616,7 +616,7 @@ struct context {
 	int	export;
 	int	test;
 	char	*subarray;
-	char	*update;
+	enum	update_opt update;
 	int	scan;
 	int	SparcAdjust;
 	int	autof;
-- 
2.26.2

Re: [PATCH 10/10] Change char* to enum in context->update & refactor code

[Coly Li]

> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@intel.com> 写道：
> 
> Storing update option in string is bad for frequent comparisons and
> error prone.
> Replace char array with enum so already existing enum is passed around
> instead of string.
> Adapt code to changes.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>

Although I don’t test this patch yet, it looks good to me.

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
> Assemble.c | 40 +++++++++++++++++-----------------------
> mdadm.c    | 52 +++++++++++++++++++---------------------------------
> mdadm.h    |  2 +-
> 3 files changed, 37 insertions(+), 57 deletions(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index 8cd3d533..942e352e 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -135,17 +135,17 @@ static int ident_matches(struct mddev_ident *ident,
> 			 struct mdinfo *content,
> 			 struct supertype *tst,
> 			 char *homehost, int require_homehost,
> -			 char *update, char *devname)
> +			 enum update_opt update, char *devname)
> {
> 
> -	if (ident->uuid_set && (!update || strcmp(update, "uuid")!= 0) &&
> +	if (ident->uuid_set && update != UOPT_UUID &&
> 	    same_uuid(content->uuid, ident->uuid, tst->ss->swapuuid)==0 &&
> 	    memcmp(content->uuid, uuid_zero, sizeof(int[4])) != 0) {
> 		if (devname)
> 			pr_err("%s has wrong uuid.\n", devname);
> 		return 0;
> 	}
> -	if (ident->name[0] && (!update || strcmp(update, "name")!= 0) &&
> +	if (ident->name[0] && update != UOPT_NAME &&
> 	    name_matches(content->name, ident->name, homehost, require_homehost)==0) {
> 		if (devname)
> 			pr_err("%s has wrong name.\n", devname);
> @@ -648,11 +648,10 @@ static int load_devices(struct devs *devices, char *devmap,
> 			int err;
> 			fstat(mdfd, &stb2);
> 
> -			if (strcmp(c->update, "uuid") == 0 && !ident->uuid_set)
> +			if (c->update == UOPT_UUID && !ident->uuid_set)
> 				random_uuid((__u8 *)ident->uuid);
> 
> -			if (strcmp(c->update, "ppl") == 0 &&
> -			    ident->bitmap_fd >= 0) {
> +			if (c->update == UOPT_PPL && ident->bitmap_fd >= 0) {
> 				pr_err("PPL is not compatible with bitmap\n");
> 				close(mdfd);
> 				free(devices);
> @@ -684,34 +683,30 @@ static int load_devices(struct devs *devices, char *devmap,
> 			strcpy(content->name, ident->name);
> 			content->array.md_minor = minor(stb2.st_rdev);
> 
> -			if (strcmp(c->update, "byteorder") == 0)
> +			if (c->update == UOPT_BYTEORDER)
> 				err = 0;
> -			else if (strcmp(c->update, "home-cluster") == 0) {
> +			else if (c->update == UOPT_HOME_CLUSTER) {
> 				tst->cluster_name = c->homecluster;
> 				err = tst->ss->write_bitmap(tst, dfd, NameUpdate);
> -			} else if (strcmp(c->update, "nodes") == 0) {
> +			} else if (c->update == UOPT_NODES) {
> 				tst->nodes = c->nodes;
> 				err = tst->ss->write_bitmap(tst, dfd, NodeNumUpdate);
> -			} else if (strcmp(c->update, "revert-reshape") == 0 &&
> -				   c->invalid_backup)
> +			} else if (c->update == UOPT_REVERT_RESHAPE && c->invalid_backup)
> 				err = tst->ss->update_super(tst, content,
> 							    UOPT_SPEC_REVERT_RESHAPE_NOBACKUP,
> 							    devname, c->verbose,
> 							    ident->uuid_set,
> 							    c->homehost);
> 			else
> -				/*
> -				 * Mapping is temporary, will be removed in this patchset
> -				 */
> 				err = tst->ss->update_super(tst, content,
> -							    map_name(update_options, c->update),
> +							    c->update,
> 							    devname, c->verbose,
> 							    ident->uuid_set,
> 							    c->homehost);
> 			if (err < 0) {
> 				if (err == -1)
> 					pr_err("--update=%s not understood for %s metadata\n",
> -					       c->update, tst->ss->name);
> +					       map_num(update_options, c->update), tst->ss->name);
> 				tst->ss->free_super(tst);
> 				free(tst);
> 				close(mdfd);
> @@ -721,7 +716,7 @@ static int load_devices(struct devs *devices, char *devmap,
> 				*stp = st;
> 				return -1;
> 			}
> -			if (strcmp(c->update, "uuid")==0 &&
> +			if (c->update == UOPT_UUID &&
> 			    !ident->uuid_set) {
> 				ident->uuid_set = 1;
> 				memcpy(ident->uuid, content->uuid, 16);
> @@ -730,7 +725,7 @@ static int load_devices(struct devs *devices, char *devmap,
> 				pr_err("Could not re-write superblock on %s.\n",
> 				       devname);
> 
> -			if (strcmp(c->update, "uuid")==0 &&
> +			if (c->update == UOPT_UUID &&
> 			    ident->bitmap_fd >= 0 && !bitmap_done) {
> 				if (bitmap_update_uuid(ident->bitmap_fd,
> 						       content->uuid,
> @@ -1188,8 +1183,7 @@ static int start_array(int mdfd,
> 				pr_err("%s: Need a backup file to complete reshape of this array.\n",
> 				       mddev);
> 				pr_err("Please provided one with \"--backup-file=...\"\n");
> -				if (c->update &&
> -				    strcmp(c->update, "revert-reshape") == 0)
> +				if (c->update == UOPT_REVERT_RESHAPE)
> 					pr_err("(Don't specify --update=revert-reshape again, that part succeeded.)\n");
> 				return 1;
> 			}
> @@ -1487,7 +1481,7 @@ try_again:
> 	 */
> 	if (map_lock(&map))
> 		pr_err("failed to get exclusive lock on mapfile - continue anyway...\n");
> -	if (c->update && strcmp(c->update,"uuid") == 0)
> +	if (c->update == UOPT_UUID)
> 		mp = NULL;
> 	else
> 		mp = map_by_uuid(&map, content->uuid);
> @@ -1635,7 +1629,7 @@ try_again:
> 		goto out;
> 	}
> 
> -	if (c->update && strcmp(c->update, "byteorder")==0)
> +	if (c->update == UOPT_BYTEORDER)
> 		st->minor_version = 90;
> 
> 	st->ss->getinfo_super(st, content, NULL);
> @@ -1904,7 +1898,7 @@ try_again:
> 	/* First, fill in the map, so that udev can find our name
> 	 * as soon as we become active.
> 	 */
> -	if (c->update && strcmp(c->update, "metadata")==0) {
> +	if (c->update == UOPT_METADATA) {
> 		content->array.major_version = 1;
> 		content->array.minor_version = 0;
> 		strcpy(content->text_version, "1.0");
> diff --git a/mdadm.c b/mdadm.c
> index b55e0d9a..dc6d6a95 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -746,7 +746,7 @@ int main(int argc, char *argv[])
> 		case O(MISC,'U'):
> 			if (c.update) {
> 				pr_err("Can only update one aspect of superblock, both %s and %s given.\n",
> -					c.update, optarg);
> +					map_num(update_options, c.update), optarg);
> 				exit(2);
> 			}
> 			if (mode == MISC && !c.subarray) {
> @@ -754,8 +754,7 @@ int main(int argc, char *argv[])
> 				exit(2);
> 			}
> 
> -			c.update = optarg;
> -			enum update_opt updateopt = map_name(update_options, c.update);
> +			c.update = map_name(update_options, optarg);
> 			enum update_opt print_mode = UOPT_HELP;
> 			const char *error_addon = "update option";
> 
> @@ -763,14 +762,14 @@ int main(int argc, char *argv[])
> 				print_mode = UOPT_SUBARRAY_ONLY;
> 				error_addon = "update-subarray option";
> 
> -				if (updateopt > UOPT_SUBARRAY_ONLY && updateopt < UOPT_HELP)
> -					updateopt = UOPT_UNDEFINED;
> +				if (c.update > UOPT_SUBARRAY_ONLY && c.update < UOPT_HELP)
> +					c.update = UOPT_UNDEFINED;
> 			}
> 
> -			switch (updateopt) {
> +			switch (c.update) {
> 			case UOPT_UNDEFINED:
> 				pr_err("'--update=%s' is invalid %s. ",
> -					c.update, error_addon);
> +					optarg, error_addon);
> 				outf = stderr;
> 			case UOPT_HELP:
> 				if (!outf)
> @@ -795,14 +794,14 @@ int main(int argc, char *argv[])
> 			}
> 			if (c.update) {
> 				pr_err("Can only update one aspect of superblock, both %s and %s given.\n",
> -					c.update, optarg);
> +					map_num(update_options, c.update), optarg);
> 				exit(2);
> 			}
> -			c.update = optarg;
> -			if (strcmp(c.update, "devicesize") != 0 &&
> -			    strcmp(c.update, "bbl") != 0 &&
> -			    strcmp(c.update, "force-no-bbl") != 0 &&
> -			    strcmp(c.update, "no-bbl") != 0) {
> +			c.update = map_name(update_options, optarg);
> +			if (c.update != UOPT_DEVICESIZE &&
> +			    c.update != UOPT_BBL &&
> +			    c.update != UOPT_NO_BBL &&
> +			    c.update != UOPT_FORCE_NO_BBL) {
> 				pr_err("only 'devicesize', 'bbl', 'no-bbl', and 'force-no-bbl' can be updated with --re-add\n");
> 				exit(2);
> 			}
> @@ -1388,7 +1387,7 @@ int main(int argc, char *argv[])
> 		}
> 	}
> 
> -	if (c.update && strcmp(c.update, "nodes") == 0 && c.nodes == 0) {
> +	if (c.update && c.update == UOPT_NODES && c.nodes == 0) {
> 		pr_err("Please specify nodes number with --nodes\n");
> 		exit(1);
> 	}
> @@ -1433,22 +1432,10 @@ int main(int argc, char *argv[])
> 		/* readonly, add/remove, readwrite, runstop */
> 		if (c.readonly > 0)
> 			rv = Manage_ro(devlist->devname, mdfd, c.readonly);
> -		if (!rv && devs_found > 1) {
> -			/*
> -			 * This is temporary and will be removed in next patches
> -			 * Null c.update will cause segfault
> -			 */
> -			if (c.update)
> -				rv = Manage_subdevs(devlist->devname, mdfd,
> -						devlist->next, c.verbose, c.test,
> -						map_name(update_options, c.update),
> -						c.force);
> -			else
> -				rv = Manage_subdevs(devlist->devname, mdfd,
> -						devlist->next, c.verbose, c.test,
> -						UOPT_UNDEFINED,
> -						c.force);
> -		}
> +		if (!rv && devs_found > 1)
> +			rv = Manage_subdevs(devlist->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);
> 		if (!rv && c.runstop > 0)
> @@ -1970,14 +1957,13 @@ static int misc_list(struct mddev_dev *devlist,
> 			rv |= Kill_subarray(dv->devname, c->subarray, c->verbose);
> 			continue;
> 		case UpdateSubarray:
> -			if (c->update == NULL) {
> +			if (!c->update) {
> 				pr_err("-U/--update must be specified with --update-subarray\n");
> 				rv |= 1;
> 				continue;
> 			}
> 			rv |= Update_subarray(dv->devname, c->subarray,
> -					      map_name(update_options, c->update),
> -					      ident, c->verbose);
> +					      c->update, ident, c->verbose);
> 			continue;
> 		case Dump:
> 			rv |= Dump_metadata(dv->devname, dump_directory, c, ss);
> diff --git a/mdadm.h b/mdadm.h
> index fe09fd46..c732a936 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -616,7 +616,7 @@ struct context {
> 	int	export;
> 	int	test;
> 	char	*subarray;
> -	char	*update;
> +	enum	update_opt update;
> 	int	scan;
> 	int	SparcAdjust;
> 	int	autof;
> -- 
> 2.26.2
>