[PATCH 1/3] env: mmc: refactor mmc_offset_try_partition()

[Quentin Schulz]

Hi Rasmus,

On 9/19/24 8:53 AM, Rasmus Villemoes wrote:
> Quentin Schulz <quentin.schulz at cherry.de> writes:
> 
>>>    -	for (i = 1;;i++) {
>>> -		ret = part_get_info(desc, i, &info);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -
>>> -		if (str && !strncmp((const char *)info.name, str, sizeof(info.name)))
>>> -			break;
>>> -#ifdef CONFIG_PARTITION_TYPE_GUID
>>> -		if (!str) {
>>> -			const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
>>> -			efi_guid_t type_guid;
>>> -
>>> -			uuid_str_to_bin(info.type_guid, type_guid.b, UUID_STR_FORMAT_GUID);
>>> -			if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
>>> -				break;
>>> -		}
>>> -#endif
>>> +	if (str) {
>>> +		ret = mmc_env_partition_by_name(desc, str, &info);
>>> +	} else if (IS_ENABLED(CONFIG_PARTITION_TYPE_GUID) && !str) {
>>
>> nitpick: it's guaranteed that !str if reaching the else if based on
>> the condition of the above if condition.
> 
> Ah, yes of course. This was just because I tried to translate the
> existing
> 
>    #ifdef CONFIG_PARTITION_TYPE_GUID
>      if (!str)
> 
> logic as closely as possible. I can resend. However, I plan to send some
> followup cleanups and simplifications to env/mmc.c anyway later, but
> didn't want to entangle those with this bugfix, so perhaps it can be
> done as part of that.
> 

It was required with the old implementation because it is not an else if 
condition. It isn't in the new implementation as we'd be now using an 
else if condition there.

If we wanted to be really pedantic, I would suggest to do:

if (str)
    foo();
if (!str && IS_ENABLED(CONFIG_PARTITION_TYPE_GUID))
    bar();

to match exactly the same logic as what is being replaced.

But that would cost an extra check which really isn't necessary. I would 
recommend just ditching the !str check.

That's a nitpick, so up to you for a resend.

Cheers,
Quentin