Broken build on OpenBSD

[Alex G.]

On 2/23/21 3:18 PM, Simon Glass wrote:
> Hi Alex,
> 
> On Tue, 23 Feb 2021 at 14:48, Alex G. <mr.nuke.me at gmail.com> wrote:
>>
>> On 2/23/21 1:07 PM, Mark Kettenis wrote:
>>> Hi Simon,
>>>
>>> Commit c5819701a3de61e2ba2ef7ad0b616565b32305e5 broke the build on
>>> OpenBSD and probably other non-Linux systems.  ENODATA, which is now
>>> used in fit_check_format(), isn't defined.  It isn't part of POSIX[1]
>>> and generally not available on BSD-derived systems.  Could you pick
>>> another error code for this case?
>>
>> Hi Mark,
>>
>> I looked at the commit you mentioned, and I think it's fundamentally
>> broken. The errors represent -EINVAL, and trying to assign different
>> error codes doesn't make sense.
>>
>> "Wrong FIT format: no images parent node":
>> -ENOENT "No such file or directory".
>> This just doesn't make sense. We obviously have the file data at this
>> point, and we know the data is wrong. This should be -EINVAL.
>>
>> "Wrong FIT format: no description":
>> -ENOMSG "No message of desired type".
>> Again, this doesn't make sense. We're not dealing with messaging APIs or
>> send()/recv(). I think this should be -EINVAL.
>>
>> "Wrong FIT format: not a flattened device tree":
>> -ENOEXEC "Exec format error"
>> This one is amusing, as it's comparing a flattened devicetree to an
>> executable. An FDT might have executable code, which is in the wrong
>> format, but this is not why we're failing here.
>>
>> Simon,
>> I'd suggest using the correct error code, which, for each case is
>> -EINVAL, as the log messages also confirm: "Wrong [input value] format".
>> We might have issues with the "configurations", an "@" in a signature
>> name, and so forth. There just aren't enough error codes to cover the
>> set of possible failures. And in any case, there likely can't be a
>> reasonable 1:1 mapping to _distinct_ errno codes.
>>
>> Does any user even check the error code beyond "less than zero"? Take
>> different decisions based on what the negative code indicates? If
>> information as to what is wrong with the input value (FIT) is needed,
>> then I'd suggest using a separate enum, and stick to -EINVAL.
> 
> Actually I make an effort to use different codes where possible, so
> there is some indication what went wrong. Of course devs can whip out
> the JTAG debugger or start filling the code with printf()s but normal
> users cannot, so having an idea what is wrong is helpful.
> 
> We don't have to cover every case, but years ago U-Boot used to return
> -1 for lots of failures and it was certainly frustrating to debug
> things.

I agree with most of these arguments. And I agree with using errno codes 
to represent errno codes. However, when we deviate from the agreed upon 
convention, can we still apply the said convention? Each function 
acquires its own set of rules. And when each function has its own set of 
rules, the source code is needed to derive the meaning.

You make the argument that these codes give normal users an idea of what 
is wrong. I assume that normal users respond better to human-readable 
strings than to negative integers -- for which they would have to go to 
he source code anyway to decipher the meaning. Because, in order to be 
useful, error codes require the, they cannot be useful for normal users.

I believe this rebukes your central point around the unconventional use 
of errno codes.

So then the question is how to cover error cases without returning '-1', 
and without making things a nightmare to debug.

If you need to tell the user that there are "no images parent node", 
then tell the user -ENOFDTIMAGESNODE, or FIT_ERROR_NO_IMAGES_NODE. How 
can someone know that -ENOENT really comes from fit_check_format() 
instead of the FAT code, and really means "FIT has no images node" 
instead of "there is no FIT file"? I guess we could  bust out the old 
JTAG to check.


> BTW -EINVAL is mostly reserved for of_to_plat() failure in U-Boot. It
> indicates something is wrong with your devicetree data for a device.

Reserving -EINVAL for a special class of input value errors, but not 
others is breaking convention, so all my arguments above apply.

Alex