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

[Heinrich Schuchardt]

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()"

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

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
>>