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

[Quentin Schulz]

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.villem...@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