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