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

["Kusiak, Mateusz" <mateusz.kusiak@linux.intel.com>]

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]