On 14/09/2022 17:03, Coly Li wrote:
>
>
>> 2022年8月18日 22:56，Mateusz Kusiak <mateusz.kusiak@xxxxxxxxx> 写道：
>>
>> It prepares update_super0 for change context->update to enum.
>> Change if else statements to switch.
>>
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@xxxxxxxxx>
>
>
> 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
