Hi Rasmus,
On 9/19/24 8:53 AM, Rasmus Villemoes wrote:
Quentin Schulz quentin.schulz@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