Hi Heinrich,

Looking at this function can we clean it up since you are touching it already?

I think it would look nicer if you defined a local variable of struct efi_device_path * and always assign it in the if cases.

Then at the bottom of the function, we could store the ptr value if that exists. While at it the 'if (!*file)' seems to be misplaced.

Regards /Ilias

On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

According to our coding style guide #ifdef should be avoided. Use IS_ENABLED() instead.

Sort string comparisons alphabetically.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 20ad948498..a6a6ef0d6c 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1079,8 +1079,7 @@ struct efi_device_path *efi_dp_from_uart(void) return buf; }

-#ifdef CONFIG_NETDEVICES -struct efi_device_path *efi_dp_from_eth(void) +struct efi_device_path __maybe_unused *efi_dp_from_eth(void) { void *buf, *start; unsigned dpsize = 0; @@ -1099,7 +1098,6 @@ struct efi_device_path *efi_dp_from_eth(void)

    return start;

} -#endif

/* Construct a device-path for memory-mapped image */ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, @@ -1195,15 +1193,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (path && !file) return EFI_INVALID_PARAMETER;

•   if (!strcmp(dev, "Net")) {
  

-#ifdef CONFIG_NETDEVICES

•           if (device)
  

•                   *device = efi_dp_from_eth();
  

-#endif

•   } else if (!strcmp(dev, "Uart")) {
  

•           if (device)
  

•                   *device = efi_dp_from_uart();
  

•   } else if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
  

•   if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
            /* loadm command and semihosting */
            efi_get_image_parameters(&image_addr, &image_size);
  
  

@@ -1211,6 +1201,12 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, *device = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, (uintptr_t)image_addr, image_size);

•   } else if (IS_ENABLED(CONFIG_NETDEVICES) && !strcmp(dev, "Net")) {
  

•           if (device)
  

•                   *device = efi_dp_from_eth();
  

•   } else if (!strcmp(dev, "Uart")) {
  

•           if (device)
  

•                   *device = efi_dp_from_uart();
    } else {
            part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
                                           1);
  

-- 2.39.2

Hi Heinrich

On Mon, 15 May 2023 at 10:54, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

On 5/15/23 09:37, Ilias Apalodimas wrote:

...

Hi Heinrich,

Looking at this function can we clean it up since you are touching it already?

I think it would look nicer if you defined a local variable of struct efi_device_path * and always assign it in the if cases.

Please, have a look at another patch in the series: "efi_loader: simplify efi_dp_from_name()"

Ah haven't got that far. Yes, this does exactly what I asked, thanks!

...

Then at the bottom of the function, we could store the ptr value if that exists. While at it the 'if (!*file)' seems to be misplaced.

"if (!*file)" is not touched in this patch.

We cannot check the return value of efi_dp_from_file() before calling the function. We have checked that that parameter file is non-null above. Could you, please, describe what feels wrong for you.

Nothing, I misread that, if (!file) is already checked on the top of the function which is correct

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Best regards

Heinrich

...

Regards /Ilias

On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

According to our coding style guide #ifdef should be avoided. Use IS_ENABLED() instead.

Sort string comparisons alphabetically.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 20ad948498..a6a6ef0d6c 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1079,8 +1079,7 @@ struct efi_device_path *efi_dp_from_uart(void) return buf; }

-#ifdef CONFIG_NETDEVICES -struct efi_device_path *efi_dp_from_eth(void) +struct efi_device_path __maybe_unused *efi_dp_from_eth(void) { void *buf, *start; unsigned dpsize = 0; @@ -1099,7 +1098,6 @@ struct efi_device_path *efi_dp_from_eth(void)

     return start;

} -#endif

/* Construct a device-path for memory-mapped image */ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, @@ -1195,15 +1193,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (path && !file) return EFI_INVALID_PARAMETER;

•   if (!strcmp(dev, "Net")) {
  

-#ifdef CONFIG_NETDEVICES

•           if (device)
  

•                   *device = efi_dp_from_eth();
  

-#endif

•   } else if (!strcmp(dev, "Uart")) {
  

•           if (device)
  

•                   *device = efi_dp_from_uart();
  

•   } else if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
  

•   if (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))  {
             /* loadm command and semihosting */
             efi_get_image_parameters(&image_addr, &image_size);
  
  

@@ -1211,6 +1201,12 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, *device = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, (uintptr_t)image_addr, image_size);

•   } else if (IS_ENABLED(CONFIG_NETDEVICES) && !strcmp(dev, "Net")) {
  

•           if (device)
  

•                   *device = efi_dp_from_eth();
  

•   } else if (!strcmp(dev, "Uart")) {
  

•           if (device)
  

•                   *device = efi_dp_from_uart();
     } else {
             part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
                                            1);
  

-- 2.39.2

On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

efi_dp_from_name() has duplicate code to replace slash by backslash. path_to_uefi() called by efi_dp_from_file() already does this.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index a6a6ef0d6c..c4f0cc23a0 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1187,8 +1187,6 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, size_t image_size; void *image_addr; int part = 0;

•   char *filename;
  

•   char *s;
  
    if (path && !file)
            return EFI_INVALID_PARAMETER;
  

@@ -1220,17 +1218,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (!path) return EFI_SUCCESS;

•   filename = calloc(1, strlen(path) + 1);
  

•   if (!filename)
  

•           return EFI_OUT_OF_RESOURCES;
  

•   sprintf(filename, "%s", path);
  

•   /* DOS style file path: */
  

•   s = filename;
  

•   while ((s = strchr(s, '/')))
  

•           *s++ = '\\';
  

•   *file = efi_dp_from_file(desc, part, filename);
  

•   free(filename);
  

•   *file = efi_dp_from_file(desc, part, path);
  
    if (!*file)
            return EFI_INVALID_PARAMETER;
  

-- 2.39.2

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Hi Heinrich

On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

• Improve variable name usage: Use pos instead of buf to indicate the current position in a buffer.
• Avoid double assignment in a single code line.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index c4f0cc23a0..0f58082141 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1022,7 +1022,7 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part, const char *path) { struct efi_device_path_file_path *fp;

•   void *buf, *start;
  

•   void *buf, *pos;
    size_t dpsize = 0, fpsize;
  
    if (desc)
  

@@ -1035,26 +1035,28 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,

    dpsize += fpsize;

•   start = buf = efi_alloc(dpsize + sizeof(END));
  

•   buf = efi_alloc(dpsize + sizeof(END));
    if (!buf)
            return NULL;
  
    if (desc)
  

•           buf = dp_part_fill(buf, desc, part);
  

•           pos = dp_part_fill(buf, desc, part);
  

•   else
  

•           pos = buf;
  
    /* add file-path: */
    if (*path) {
  

•           fp = buf;
  

•           fp = pos;
            fp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
            fp->dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
            fp->dp.length = (u16)fpsize;
            path_to_uefi(fp->str, path);
  

•           buf += fpsize;
  

•           pos += fpsize;
    }
  
  

•   *((struct efi_device_path *)buf) = END;
  

•   memcpy(pos, &END, sizeof(END));
  
  

•   return start;
  

•   return buf;
  

}

struct efi_device_path *efi_dp_from_uart(void)

2.39.2

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

Use EFI_OUT_OF_RESOURCES if the device path cannot be constructed.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 0f58082141..1436244f99 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1223,7 +1223,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, *file = efi_dp_from_file(desc, part, path);

    if (!*file)

•           return EFI_INVALID_PARAMETER;
  

•           return EFI_OUT_OF_RESOURCES;
  
    return EFI_SUCCESS;
  

}

2.39.2

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

Don't do the same check and assignment in multiple places.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 1436244f99..a9b0ea4015 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1185,6 +1185,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, struct efi_device_path **file) { struct blk_desc *desc = NULL;

•   struct efi_device_path *dp;
    struct disk_partition fs_partition;
    size_t image_size;
    void *image_addr;
  

@@ -1197,25 +1198,22 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, /* loadm command and semihosting */ efi_get_image_parameters(&image_addr, &image_size);

•           if (device)
  

•                   *device = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
  

•                                             (uintptr_t)image_addr,
  

•                                             image_size);
  

•           dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
  

•                                (uintptr_t)image_addr, image_size);
    } else if (IS_ENABLED(CONFIG_NETDEVICES) && !strcmp(dev, "Net")) {
  

•           if (device)
  

•                   *device = efi_dp_from_eth();
  

•           dp = efi_dp_from_eth();
    } else if (!strcmp(dev, "Uart")) {
  

•           if (device)
  

•                   *device = efi_dp_from_uart();
  

•           dp = efi_dp_from_uart();
    } else {
            part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
                                           1);
            if (part < 0 || !desc)
                    return EFI_INVALID_PARAMETER;
  
  

•           if (device)
  

•                   *device = efi_dp_from_part(desc, part);
  

•           dp = efi_dp_from_part(desc, part);
    }
  

•   if (device)
  

•           *device = dp;
  
    if (!path)
            return EFI_SUCCESS;
  

-- 2.39.2

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org