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

[Simon Glass]

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.

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

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.

[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"?

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
>