[PATCH v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

Thanks for your comments.

On Sun, Sep 11, 2011 at 5:53 PM, David Gibson
<david at gibson.dropbear.id.au> wrote:
> On Thu, Sep 08, 2011 at 10:44:40PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> A bit rushed as late here but I wanted to respond before your weekend.
>>
>> On Thu, Sep 8, 2011 at 9:49 PM, David Gibson
>> <david at gibson.dropbear.id.au> wrote:
>> > On Thu, Sep 08, 2011 at 05:47:34AM -0700, Simon Glass wrote:
> [snip]
>> > 1) Use a subset of printf() specifiers.  Despite your balking at that,
>> > I don't really think it would be any harder than what you have now.
>> > Last character is always the non-optional "format" specifier, anything
>> > preceding it is modifiers.  So for the integer formats "d", "x" and
>> > "o", valid modifiers are size, either "hh" (1 byte), "h" (2 byte), "l"
>> > (4 byte, default) or "ll" (8 byte).  For the string format "s", no
>> > modifiers are valid.
>>
>> OK, I like this option more. Particularly if you think I might get
>> away with also supporting 'b' as a shortcut for 'hh'.
>
> Hrm, I have mixed feelings about that. "hh" is kind of obscure.  And
> as far as I can tell "b" is not used for anything else at present (at
> least in glibc printf()).  But there are so many letters with existing
> meanings for printf() that redefining any makes me a little nervous.

Well yes - I think I will support both hh and b and see how it looks.

>
>> > 2) Treat the size as a different parameter.  So the -f option gives a
>> > single format char (or instead use -s, -x, -d, .. options).  Then have
>> > a -s option for size (-s1 ... -s8) which is valid only with the
>> > integer format options.
>>
>> More like what I had - have moved away from that and feel more
>> comfortable in my new space.
>
> Ok.
>
>> > Either way does leave a couple of unanswered questions:
>> >
>> > A) What to do if the format doesn't consume the whole parameter.  I
>> > see two options:
>> >        A1) Just print as much as the format says, then stop
>> >        A2) Keep repeating the same format until the property is
>> > consumed (note that this makes sense for "s" as well, and would be an
>> > obvious way of handling the pretty common list-of-strings properties).
>>
>> Well I think you have to keep printing. This is not a true format
>> string, but just a data type for each element. Again we have moved
>> away from being able to say -f "The %d item is %d and has a flag word
>> of %d".
>>
>> So no need to use -tddd if there are three cells - just -td is enough.
>
> Yes, I tend to agree.
>
>> > B) What to do when the property just doesn't fit into that format -
>> > either it's length is not a multiple of the fixed integer size, or
>> > it's not NUL-terminated for "s".  Obviously some kind of error is in
>> > order, but in case A2 above, do we *just* error, or print what we can
>> > before giving up.
>>
>> Prefer to error since any output might be confusing. But I will take a
>> look to make sure that is easy to do.
>
> It is trivial to check in advance for the existing types - for
> integers just check if the property has length a multiple of the
> integer size, for strings check if the last byte is '\0'.  Well.. then
> it depends a bit on how safe you want "s" to be - do you check for
> reasonably printable characters first or not.  How easy it is to
> pre-check for possible future types is not neccessarily obvious of
> course.

Yes I have added a check for (length % size) == 0. For strings I don't
think we should be picky so long as there is a NUL terminator. If you
don't specify a type, the utility already tries to guess, and will
fall back to integer or even byte if it finds the string has ctrl
characters. When the user says -ts I think we need to try hard to
obey. I can't think of any reason to store strange characters in a
string property, but others might.

I will send the V3 patch set in the next few days and we'll see how it looks.

Regards,
Simon

>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>