diff mbox series

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

Message ID 20240912134141.4143855-2-rasmus.villemoes@prevas.dk
State Accepted
Commit d7c59bfc3b20bc6d602b33ac24d7ae8698650b87
Delegated to: Tom Rini
Headers show
Series env: mmc: fix use of two separate partitions with proper type GUID | expand

Commit Message

Rasmus Villemoes Sept. 12, 2024, 1:41 p.m. UTC
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(-)

Comments

Quentin Schulz Sept. 18, 2024, 4:59 p.m. UTC | #1
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
Rasmus Villemoes Sept. 19, 2024, 6:53 a.m. UTC | #2
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.

Rasmus
Quentin Schulz Sept. 23, 2024, 11:04 a.m. UTC | #3
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
diff mbox series

Patch

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) {
+		ret = mmc_env_partition_by_guid(desc, &info);
 	}
+	if (ret < 0)
+		return ret;
 
 	/* round up to info.blksz */
 	len = DIV_ROUND_UP(CONFIG_ENV_SIZE, info.blksz);