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

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

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