On 11/25/2017 11:34 PM, Simon Glass wrote:
Hi Heinrich,
On 23 November 2017 at 14:29, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
On 11/21/2017 03:16 PM, Wolfgang Denk wrote:
Dear Heinrich Schuchardt,
In message <d643b22b-2511-9b40-772b-5f7f4486c...@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.
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.
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?
Best regards
Heinrich
So here we could only panic().
Is this really what you have in mind?
Well, which other options do you see when you run out of memory?
I can't see how you could recover from this, so if you don't abort
here with a clear error message you will either do the same
elsewhere (with a misleading error, as the actual problem happened
eventually long before), or you will just crash, or run into
undefined behaviour.
So I will resend the patch with panic().
Best regards
Heinrich
Ignoring error condistions is always a Bad thing (TM).
Best regards,
Wolfgang Denk
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
