Hi Heinrich On Mon, 15 May 2023 at 10:54, Heinrich Schuchardt <heinrich.schucha...@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.apalodi...@linaro.org> > > Best regards > > Heinrich > > > > > Regards > > /Ilias > > > > On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt > > <heinrich.schucha...@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.schucha...@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 > >> >