[U-Boot] [PATCH v2 01/18] efi_loader: efi_bootmgr: do not make hidden assignments

[Rob Clark]

On Fri, Nov 17, 2017 at 12:46 PM, Heinrich Schuchardt
<xypron.glpk at gmx.de> wrote:
> On 11/17/2017 03:04 PM, Rob Clark wrote:
>>
>> On Sun, Nov 12, 2017 at 9:02 AM, Heinrich Schuchardt <xypron.glpk at gmx.de>
>> wrote:
>>>
>>> Assignments should not be made in the middle of nowhere.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>> v2
>>>          Call efi_dp_str in debug mode only.
>>> ---
>>>   lib/efi_loader/efi_bootmgr.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 857d88a879..a55f210f0b 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -120,11 +120,13 @@ static void *try_load_entry(uint16_t n, struct
>>> efi_device_path **device_path,
>>>
>>>          if (lo.attributes & LOAD_OPTION_ACTIVE) {
>>>                  efi_status_t ret;
>>> -               u16 *str = NULL;
>>> +#ifdef _DEBUG
>>> +               u16 *str = efi_dp_str(lo.file_path);
>>>
>>>                  debug("%s: trying to load \"%ls\" from: %ls\n",
>>> __func__,
>>> -                     lo.label, (str = efi_dp_str(lo.file_path)));
>>> +                     lo.label, str);
>>>                  efi_free_pool(str);
>>> +#endif /* _DEBUG */
>>
>>
>> I was trying to avoid the #ifdef DEBUG in the first place.. ;-)
>>
>> btw, don't have the code in front of me atm, but I thought _DEBUG was
>> unconditionally defined to either 0 or 1.. so maybe you mean '#if
>> _DEBUG' or '#ifdef DEBUG' (no underscore)?
>
>
> Thanks for catching this
>
> I will change this to
> #if _DEBUG
>
> I often found the necessity to change the value of _DEBUG multiple times
> within a single C file to get only the relevant debug messages. This is why
> I prefer checking _DEBUG.
>
>>
>> btw2, possibly a better/cleaner option is add printf support for
>> format modifier to print device-paths?
>
>
> This is the only place where efi_dp_str is called.
>

I have had locally other callers while debugging things.. maybe I
should have left some of those in the code (and maybe I would have if
printf() supported it :-P)

Device-paths are fairly heavily used in UEFI.. so it tends to be
something you end up wanting to print in various places when
debugging.

BR,
-R

> Regards
>
> Heinrich
>
>
>>
>> BR,
>> -R
>>
>>>
>>>                  ret = efi_load_image_from_path(lo.file_path, &image);
>>>
>>> --
>>> 2.15.0
>>>
>>
>