On 13/09/2022 17:12, Coly Li wrote:
>
>
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@xxxxxxxxx> 写道：
>>
>> 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@xxxxxxxxx>
>
>
> 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]
