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

[Simon Glass]

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:
>> Hi David,
>
> [snip]
>> >> Usage:
>> >>       fdtget <options> <dt file> [<node> <property>]...
>> >> Options:
>> >>       -t [<type>][<format>]   Type of data
>> >>       -h              Print this help
>> >>
>> >>       <type>          One character (s=string, i=int, u=unsigned, b=byte)
>> >>       <format>        Optional format character (x=hex)
>> >
>> > It is not terribly clear how these are combined.
>>
>> A type character, then an optional format character. In fact the type
>> character is also optional at present since integer is assumed.
>>
>> Please can you suggest a suitable help message to convey this?
>
> Hrm, ok, so.  I think part of the problem is that I'm finding what is
> covered by "type" and what is covered by "format" is only obvious once
> you've already understood what they mean.  They're not fully
> independent either, since "format" makes no sense with type "s".

Yes I'm really just trying to explain a format similar to printf.

>
>> > Is there any way to make these more closely resemble printf() style
>> > format specifiers?
>>
>> The first patch series allowed printf specifiers but could segfault if
>> the user specified %s for something that was in fact not a string. I
>> have moved away from that as you suggested at the time.
>
> Yeah, I better understand your motivation for doing so now.  But it's
> just not safe to do it that way.
>
>> I am not keen
>> to re-implement printf(), or manually decode a partial subset of
>> printf strings, to avoid that problem. So something simple like this
>> is what I have come up with.
>
> Hrm.  The problem I have is that this sort-of-like printf() but not
> really like printf() may violate least surprise.

Well I mean that I don't want to reimplement it, but just using the
same syntax for a tiny subset would be fine.

>
>> We need to encode the data size in there also - byte or integer - so
>> it is not quite the same as what printf is trying to do. While
>> printf() has %c I don't think it is a good idea to use that, since we
>> are printing a byte as a number, not an ASCII character.
>
> Printing a byte value as a number would be %hhx or %hhd in printf.

Ick.

>
>> But it is fairly close to printf, in that you can use:
>>
>> -ts
>> -tx
>> -ti
>> -tu
>> -tix
>> -tbx
>>
>> We could combine -tbx into a single letter somehow but I'm not sure I
>> like that idea.
>>
>> I don't see any point in requiring a % since then users would expect
>> to be able to provide an arbitrary printf() string (see first patch
>> set).
>>
>> So, suggestions please.
>
> So, I think the first thing is I think you need to treat the "format"
> as the primary parameter, with the size (roughly what you're calling
> "type" at the moment) as a subordinate modifier to that.  The trouble
> with making format subordinate to type, or supposedly independent
> parameters is that what sizes or "types" are acceptable depends on the
> format.  A string format always accepts variable length input, and the
> integer formats always require a specified fixed length.  Future
> extensions could conceivably have other constraints.

OK sounds good.

>
> With that basic principle, I basically see two reasonable ways to go.
>
> 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'.

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

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

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

Thanks very much for your helpful comments. I will work on a new patch set.

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
>