[PATCH v2 1/6] Add utilfdt for common functions

[David Gibson]

On Thu, Sep 08, 2011 at 05:37:08AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Wed, Sep 7, 2011 at 10:20 PM, David Gibson
> <david at gibson.dropbear.id.au> wrote:
> > On Wed, Sep 07, 2011 at 12:54:15PM -0700, Simon Glass wrote:
> >> This adds a new utility library for performing libfdt operations.
> >>
> >> Signed-off-by: Simon Glass <sjg at chromium.org>
> >> ---
> >> Changes in v2:
> >> - Remove util_decode_key
> >> - Add utilfdt_decode_type to be used by fdtget/put
> >> - Remove limits on device tree binary size
> 
> [snip]
> 
> >> +     if (strcmp(filename, "-") == 0) {
> >> +             fp = stdin;
> >> +             toread = 64 * 1024;     /* Suitable likely maximum */
> >
> > Arbitrary limits, yuck.
> 
> There is no arbitrary limit here - it is just the number of bytes read
> initially.

Hrm, ok.

> >> +     } else {
> >> +             fp = fopen(filename, "rb");
> >> +             if (fp == NULL) {
> >> +                     fprintf(stderr, "unable to open '%s'\n", filename);
> >> +                     return NULL;
> >> +             }
> >> +             if (fstat(fileno(fp), &stat_buf)) {
> >> +                     fprintf(stderr, "couldn't get size of file\n");
> >> +                     return NULL;
> >
> > stdin is not the only possibility for something that's not stat()able.
> >
> > You shouldn't use stat() at all to get the size, but either just
> > read() until EOF, or read the header first and use the size in there.
> >
> > Wait.. I already wrote this function.. look at load_blob() in
> > tests/testutils.c.  Why don't you give it a cleaner name, rather than
> > rewriting it badly.
> 
> Thanks for the pointer to the existing code. It looks very similar
> except for my use of stat and its use of xmalloc() and CONFIG. Also I
> note that it uses open() rather than fopen(). I will need to move it
> to the library and change it a bit.
> 
> It uses this macro to display errors:
> 
> #define CONFIG(fmt, ...)				\
> 	do {						\
> 		cleanup();				\
> 		printf("Bad configuration: " fmt "\n", ##__VA_ARGS__);	\
> 		exit(RC_CONFIG);			\
> 	} while (0)
> 
> Given that I am trying to write a general utility function I would
> prefer not to bomb out in a low-level function, but to return an error
> result.

Yes, you'll need to fix that for a general library version.  It
should indeed return an error rather than dying.

> So are you ok with the error message bring different in this case (not
> printing 'Bad configuration' before the error, and going out on
> stderr)? Then I can make all the test code call this function in
> libfdt.

I don't think you'll want to direct replace the tests' load_blob()
with your util.  Instead make load_blob() a little wrapper that uses
your load function and bombs out with CONFIG() if it fails.  Use of
CONFIG() in the testcases is important for correctly collating the
test results (specifically it means "we couldn't complete this test
because the environment hasn't been set up correctly" rather than a
true pass or fail).

[snip]
> [snip]
> > [snip]
> >> +/**
> >> + * Decode a data type string.
> >> + *
> >> + * The string consists of:
> >> + *   One optional character (s=string, i=int, u=unsigned b=byte)
> >> + *   Optional format character (x=hex)
> >> + *
> >> + * If either of type or format is not found, then that variable is not
> >> + * updated, and the caller's default will be used.
> >
> > Yes, but what does the function actually *do*?
> 
> I don't follow - are you asking that I make it more specific like
> "Decode a data type string as used by fdtget and fdtput to specify the
> data type format to use for property values"?

Well, I've found almost every description of your format strings to be
confusing.  But in this case I'm asking what does "decode" mean - in
particular what are the outputs.  From further reading, it seems it
means splitting the "type" part from the "format" part, but that's far
from obvious at first glance.

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