On 2/23/21 3:18 PM, Simon Glass wrote:
Hi Alex,
On Tue, 23 Feb 2021 at 14:48, Alex G. <mr.nuke...@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
