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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Heinrich Schuchardt
For debugging efi_loader we need the capability to print EFI
device paths. With this patch we can write:

    debug("device path: %pD", dp);

A possible output would be

    device path: /MemoryMapped(0x0,0x3ff93a82,0x3ff93a82)

Suggested-by: Rob Clark <[hidden email]>
Signed-off-by: Heinrich Schuchardt <[hidden email]>
---
I suggest Alex picks up this patch for the EFI tree.
---
 lib/vsprintf.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd572d2868..68797c672c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -18,6 +18,7 @@
 
 #include <common.h>
 #include <charset.h>
+#include <efi_loader.h>
 #include <uuid.h>
 
 #include <div64.h>
@@ -292,6 +293,18 @@ static char *string16(char *buf, char *end, u16 *s, int field_width,
  return buf;
 }
 
+#ifdef CONFIG_EFI_LOADER
+static char *device_path_string(char *buf, char *end, void *dp, int field_width,
+ int precision, int flags)
+{
+ u16 *str = efi_dp_str((struct efi_device_path *)dp);
+
+ buf = string16(buf, end, str, field_width, precision, flags);
+ efi_free_pool(str);
+ return buf;
+}
+#endif
+
 #ifdef CONFIG_CMD_NET
 static const char hex_asc[] = "0123456789abcdef";
 #define hex_asc_lo(x) hex_asc[((x) & 0x0f)]
@@ -435,6 +448,11 @@ static char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 #endif
 
  switch (*fmt) {
+#ifdef CONFIG_EFI_LOADER
+ case 'D':
+ return device_path_string(buf, end, ptr, field_width,
+  precision, flags);
+#endif
 #ifdef CONFIG_CMD_NET
  case 'a':
  flags |= SPECIAL | ZEROPAD;
--
2.15.0

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Wolfgang Denk
Dear Heinrich,

In message <[hidden email]> you wrote:
> For debugging efi_loader we need the capability to print EFI
> device paths. With this patch we can write:
>
>     debug("device path: %pD", dp);
...

> +#ifdef CONFIG_EFI_LOADER
> +static char *device_path_string(char *buf, char *end, void *dp, int field_width,
> + int precision, int flags)
> +{
> + 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 thhis not be handled?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
Old programmers never die, they just become managers.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Heinrich Schuchardt-2
On 11/19/2017 12:41 PM, Wolfgang Denk wrote:

> Dear Heinrich,
>
> In message <[hidden email]> you wrote:
>> For debugging efi_loader we need the capability to print EFI
>> device paths. With this patch we can write:
>>
>>     debug("device path: %pD", dp);
> ...
>
>> +#ifdef CONFIG_EFI_LOADER
>> +static char *device_path_string(char *buf, char *end, void *dp, int field_width,
>> + int precision, int flags)
>> +{
>> + 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>";

It can only occur if we are out of memory. All other error handling
should be added to efi_convert_device_path_to_text().

Best regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Wolfgang Denk
Dear Heinrich,

In message <[hidden email]> you wrote:

>
> >> +#ifdef CONFIG_EFI_LOADER
> >> +static char *device_path_string(char *buf, char *end, void *dp, int field_width,
> >> + int precision, int flags)
> >> +{
> >> + 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!" ?

> It can only occur if we are out of memory. All other error handling
> should be added to efi_convert_device_path_to_text().

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.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
All men should freely use those seven words which have the  power  to
make any marriage run smoothly: You know dear, you may be right.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Heinrich Schuchardt
On 11/20/2017 04:44 PM, Wolfgang Denk wrote:

> Dear Heinrich,
>
> In message <[hidden email]> you wrote:
>>
>>>> +#ifdef CONFIG_EFI_LOADER
>>>> +static char *device_path_string(char *buf, char *end, void *dp, int field_width,
>>>> + int precision, int flags)
>>>> +{
>>>> + 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?

I can understand that we might improve error handling in the EFI coding
but I do not believe we should do it here.

>
>> It can only occur if we are out of memory. All other error handling
>> should be added to efi_convert_device_path_to_text().
>
> 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. So here we could only panic().

Is this really what you have in mind?

Best regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Wolfgang Denk
Dear Heinrich Schuchardt,

In message <[hidden email]> 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.

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

Ignoring error condistions is always a Bad thing (TM).

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
How long does it take a  DEC  field  service  engineer  to  change  a
lightbulb?       It depends on how many bad ones he brought with him.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Heinrich Schuchardt
On 11/21/2017 03:16 PM, Wolfgang Denk wrote:

> Dear Heinrich Schuchardt,
>
> In message <[hidden email]> 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.

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

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

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Simon Glass-3
Hi Heinrich,

On 23 November 2017 at 14:29, Heinrich Schuchardt <[hidden email]> wrote:

> On 11/21/2017 03:16 PM, Wolfgang Denk wrote:
>> Dear Heinrich Schuchardt,
>>
>> In message <[hidden email]> 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?

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

>
>>
>>> 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
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Heinrich Schuchardt


On 11/25/2017 11:34 PM, Simon Glass wrote:

> Hi Heinrich,
>
> On 23 November 2017 at 14:29, Heinrich Schuchardt <[hidden email]> wrote:
>> On 11/21/2017 03:16 PM, Wolfgang Denk wrote:
>>> Dear Heinrich Schuchardt,
>>>
>>> In message <[hidden email]> 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
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Simon Glass-3
Hi Heinrich,

On 25 November 2017 at 22:00, Heinrich Schuchardt <[hidden email]> wrote:

>
>
> On 11/25/2017 11:34 PM, Simon Glass wrote:
>>
>> Hi Heinrich,
>>
>> On 23 November 2017 at 14:29, Heinrich Schuchardt <[hidden email]>
>> wrote:
>>>
>>> On 11/21/2017 03:16 PM, Wolfgang Denk wrote:
>>>>
>>>> Dear Heinrich Schuchardt,
>>>>
>>>> In message <[hidden email]> 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
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Wolfgang Denk
In reply to this post by Heinrich Schuchardt
Dear Heinrich,

In message <[hidden email]> you wrote:
>
> You are absolutely right. The C standard defines printf as returning a
> negative number if an error arises.

This is what we (what to) have in U-Boot, too.

> set_config_filename, dbg_snprintf_key(), bootstage_mark_code() - to name
> a few - will access illegal memory addresses.

In this case you have identified a number of bugs, that need fixing.

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

Come on, be serious.  Of course we MUST assume that all callaers of
a function behave according o the specs.  If they don;t then need
fixing.  But you must NEVEr change behaviour of any code to deviate
from the spec and documention just because you fear thay you lose
bug compatibility.  That would be crazy.


Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
The IQ of the group is the lowest IQ of a member of the group divided
by the number of people in the group.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Heinrich Schuchardt
On 11/26/2017 02:11 PM, Wolfgang Denk wrote:

> Dear Heinrich,
>
> In message <[hidden email]> you wrote:
>>
>> You are absolutely right. The C standard defines printf as returning a
>> negative number if an error arises.
>
> This is what we (what to) have in U-Boot, too.
>
>> set_config_filename, dbg_snprintf_key(), bootstage_mark_code() - to name
>> a few - will access illegal memory addresses.
>
> In this case you have identified a number of bugs, that need fixing.
>
>> 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().
>
> Come on, be serious.  Of course we MUST assume that all callaers of
> a function behave according o the specs.  If they don;t then need
> fixing.  But you must NEVEr change behaviour of any code to deviate
> from the spec and documention just because you fear thay you lose
> bug compatibility.  That would be crazy.
>
>
> Best regards,
>
> Wolfgang Denk
>
Hello Wolfgang,

to cut it short, would you prefer

https://patchwork.ozlabs.org/patch/840912/

or

https://github.com/xypron/u-boot-odroid-c2/blob/qemu-x86_64/patch/0001-vsprintf.c-add-EFI-device-path-printing.patch

Best regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Wolfgang Denk
In reply to this post by Heinrich Schuchardt
Dear Heinrich,

In message <[hidden email]> you wrote:
>
> %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.

You misinterpret me.  the behaviour / output of *printf() for
incorrect arguments etc. depends on the specification of the API.
It may result in special output (like "<NULL>" for a null pointer
reference).

But we should always return an error for unrecoverable error
situations that prevented proper operation accourding to the spec,
like out-of-memory situations.

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

No.  Whener we run into bugs or bad code, we should fix it.  Ot at
least flag it, so it can be fixed later.

We shall never, never ever paper over known issues.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
Madness takes its toll. Please have exact change.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Wolfgang Denk
In reply to this post by Heinrich Schuchardt
Dear Heinrich,

In message <[hidden email]> you wrote:
>
> to cut it short, would you prefer
>
> https://patchwork.ozlabs.org/patch/840912/
>
> or
>
> https://github.com/xypron/u-boot-odroid-c2/blob/qemu-x86_64/patch/0001-vsprintf.c-add-EFI-device-path-printing.patch

Urgh.  I really hate such external references.

IIUC, then [2] has as only difference an additional early return in
printf().  The only effect of this is to skip the
"puts(printbuffer);".

I can't say if this is worth the effort.  In any case, you introduce
inconsistent behaviour here.  If you do it in printf(), it should
(at least) also be done in vprintf() [and probably one should check
the *snprintf() functions, too].

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
Drawing on my fine command of language, I said nothing.
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] vsprintf.c: add EFI device path printing

Heinrich Schuchardt
On 11/26/2017 02:39 PM, Wolfgang Denk wrote:

> Dear Heinrich,
>
> In message <[hidden email]> you wrote:
>>
>> to cut it short, would you prefer
>>
>> https://patchwork.ozlabs.org/patch/840912/
>>
>> or
>>
>> https://github.com/xypron/u-boot-odroid-c2/blob/qemu-x86_64/patch/0001-vsprintf.c-add-EFI-device-path-printing.patch
>
> Urgh.  I really hate such external references.
>
> IIUC, then [2] has as only difference an additional early return in
> printf().  The only effect of this is to skip the
> "puts(printbuffer);".

1 panics, 2 returns an error code and does not panic

>
> I can't say if this is worth the effort.  In any case, you introduce
> inconsistent behaviour here.  If you do it in printf(), it should
> (at least) also be done in vprintf() [and probably one should check
> the *snprintf() functions, too].

Yes you are right concerning vsprintf.

puts would output the incorrect data from the internal buffer.
The s*printf functions work on external buffer. So the caller can react
on the return code.

Regards

Heinrich
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot