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

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Rasmus,

On 9/12/24 3:41 PM, Rasmus Villemoes wrote:
> In preparation for fixing the handling of a the case of redundant
> environment defined in two separate partitions with the U-Boot env
> GUID, refactor the
> 
>    for ()
>      if (str)
>        ...
>    #ifdef CONFIG_FOO
>      if (!str)
>        ..
>    #endif
> 
> to
> 
>    if (str)
>      for ()
>    else if (CONFIG_FOO && !str)
>      for ()
> 
> and put those for loops in separate functions.
> 
> No functional change intended, but I did change the direct access of
> info.type_guid into using the disk_partition_type_guid() helper, so
> that I could avoid the #ifdef and use IS_ENABLED() in the if() statement.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   env/mmc.c | 59 ++++++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index 0338aa6c56a..db2d35e9bd4 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -53,11 +53,45 @@ DECLARE_GLOBAL_DATA_PTR;
>   #endif
>   
>   #if CONFIG_IS_ENABLED(OF_CONTROL)
> +
> +static int mmc_env_partition_by_name(struct blk_desc *desc, const char *str,
> +				     struct disk_partition *info)
> +{
> +	int i, ret;
> +
> +	for (i = 1;; i++) {
> +		ret = part_get_info(desc, i, info);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!strncmp((const char *)info->name, str, sizeof(info->name)))
> +			return 0;
> +	}
> +}
> +
> +static int mmc_env_partition_by_guid(struct blk_desc *desc, struct disk_partition *info)
> +{
> +	const efi_guid_t env_guid = PARTITION_U_BOOT_ENVIRONMENT;
> +	efi_guid_t type_guid;
> +	int i, ret;
> +
> +	for (i = 1;; i++) {
> +		ret = part_get_info(desc, i, info);
> +		if (ret < 0)
> +			return ret;
> +
> +		uuid_str_to_bin(disk_partition_type_guid(info), type_guid.b, UUID_STR_FORMAT_GUID);
> +		if (!memcmp(&env_guid, &type_guid, sizeof(efi_guid_t)))
> +			return 0;
> +	}
> +}
> +
> +
>   static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
>   {
>   	struct disk_partition info;
>   	struct blk_desc *desc;
> -	int len, i, ret;
> +	int len, ret;
>   	char dev_str[4];
>   
>   	snprintf(dev_str, sizeof(dev_str), "%d", mmc_get_env_dev());
> @@ -65,24 +99,13 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val)
>   	if (ret < 0)
>   		return (ret);
>   
> -	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.

Cheers,
Quentin