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

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
[off-list ref] wrote:

On Thu, Sep 08, 2011 at 05:47:34AM -0700, Simon Glass wrote:

quoted

Hi David,

[snip]

quoted

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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