[U-Boot] [PATCH 1/1] vsprintf.c: add EFI device path printing

[Simon Glass]

Hi Heinrich,

On 25 November 2017 at 22:00, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
> On 11/25/2017 11:34 PM, Simon Glass wrote:
>>
>> Hi Heinrich,
>>
>> On 23 November 2017 at 14:29, Heinrich Schuchardt <xypron.glpk at gmx.de>
>> wrote:
>>>
>>> On 11/21/2017 03:16 PM, Wolfgang Denk wrote:
>>>>
>>>> Dear Heinrich Schuchardt,
>>>>
>>>> In message <d643b22b-2511-9b40-772b-5f7f4486c289 at gmx.de> you wrote:
>>>>>
>>>>>
>>>>>>>>> +        u16 *str = efi_dp_str((struct efi_device_path *)dp);
>>>>>>>>> +
>>>>>>>>> +        buf = string16(buf, end, str, field_width, precision,
>>>>>>>>> flags);
>>>>>>>>> +        efi_free_pool(str);
>>>>>>>>
>>>>>>>>
>>>>>>>> efi_dp_str() can return NULL. Should this not be handled?
>>>>>>>
>>>>>>>
>>>>>>> Thanks for reviewing.
>>>>>>>
>>>>>>> This situation is caught in string16:
>>>>>>> u16 *str = s ? s : L"<NULL>";
>>>>>>
>>>>>>
>>>>>> This is crash-preventing cosmetics, but no real error handling.  I
>>>>>> mean, should we not wave a big red flag and shout at the user: "Hey,
>>>>>> your system is going berserk here!" ?
>>>>>
>>>>>
>>>>> Why do you think that the error handling should be in THIS function?
>>>>
>>>>
>>>> It should be somewhere - but you cannot handle an error condition
>>>> that you don't report to the caller.
>>>>
>>>> Instead of returning from the function with a clear error message
>>>> and an error return code, you ignore it.
>>>>
>>>> The minimum action to take here would be that device_path_string()
>>>> returns NULL when the error occurs, if there was a chance for the
>>>> caller to handle that.
>>>>
>>>>> I can understand that we might improve error handling in the EFI coding
>>>>> but I do not believe we should do it here.
>>>>
>>>>
>>>> But if you don't even return an error code you have no chance to
>>>> handle it elsewhere.
>>>>
>>>>>> Yes, and running out of memory at such a place is likely to be
>>>>>> unrecoverable. So we should terminate all further processing baed on
>>>>>> this result, and not continue with bogus data.
>>>>>
>>>>>
>>>>> printf() does not have any return value.
>>>>
>>>>
>>>> This is incorrect. printf() is declared as
>>>>
>>>>        int    printf(const char *fmt, ...);
>>>>
>>>> as usual.
>>>
>>>
>>> You are absolutely right. The C standard defines printf as returning a
>>> negative number if an error arises.
>>>
>>> Unfortunately the consumers of printf behave quite differently:
>>>
>>> xasprintf(), PyString_FromFormat() correctly identify negative values as
>>> an error.
>>>
>>> set_config_filename, dbg_snprintf_key(), bootstage_mark_code() - to name
>>> a few - will access illegal memory addresses.
>>
>>
>> I think you are tarring with too broad a brush. I don't see how
>> sprintf() can return an error. What would an 'output error' be in that
>> case
>
> There are two sides of you question:
>
> 1) Does the printing function have a return type that can be used to signal
> an error?
> 2) Can the printing function called with an argument that can result in an
> error?
>
> 1) In lib/vsprintf all printing functions return int. So we could return a
> negative number if an error situation arises.

Yes.

>
> 2) Currently we ignore error situations:
> %ps replaces illegal code points by question marks.
> If we would follow Wolfgang's reasoning we should create an error for
> sprintf("%ps\n", ptr) if ptr contains an unconvertible code.

I don't think you need to take that view. The function is free to
define what it does in this case. The C library man page refers to
'output error' which I take to mean that sprintf() would never
generate an error. While in this case it is possible, in general,
printf() cannot check its args at run-time.

>
>>
>>>
>>> As long as we cannot assure that each and every caller of a printf
>>> function handles negative return values correctly the only safe handling
>>> of errors is to return 0 or panic().
>>
>>
>> I suppose that is OK, but we should try to return the expected error
>> from standard functions. If that causes other code to fail, then we
>> need to fix that other code.
>
>
> Does this imply:
>
> as none of the current conversion functions in lib/vsprintf.c is creating an
> error value we only have to care about future callers printing device paths?

You could add this to the docs for sprintf(). I think it is
unfortunate that we have to alloc memory in a printf call. Is it
possible to use the supplied buffer? Perhaps the EFI function should
provide this option.

In general, EFI code does way too much memory allocation in U-Boot. I
recall seeing hundreds of memory allocations just while it was

[..[

Regards,Simon