[PATCH 1/6] efi_loader: avoid #ifdef in efi_dp_from_name()

[Ilias Apalodimas]

Hi Heinrich

On Mon, 15 May 2023 at 10:54, Heinrich Schuchardt
<heinrich.schuchardt at 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 at linaro.org>

>
> Best regards
>
> Heinrich
>
> >
> > Regards
> > /Ilias
> >
> > On Sat, 13 May 2023 at 11:48, Heinrich Schuchardt
> > <heinrich.schuchardt at 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 at 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
> >>
>