[PATCH 2/3] dtc: Add data_append_literal function

[Anton Staaf]

On Wed, Sep 21, 2011 at 7:33 PM, David Gibson
<david at gibson.dropbear.id.au> wrote:
> On Wed, Sep 21, 2011 at 01:42:10PM -0700, Anton Staaf wrote:
>> This function deals with appending literals of various sizes (8, 16
>> 32, and 64 bit currently).  It handles endianess conversions and
>> verifies that the given literal is not larger than the specified
>> size.
>>
>> Signed-off-by: Anton Staaf <robotboy at chromium.org>
>> Cc: David Gibson <david at gibson.dropbear.id.au>
>> Cc: Jon Loeliger <jdl at jdl.com>
>> Cc: Grant Likely <grant.likely at secretlab.ca>
>> ---
>>  data.c |   33 +++++++++++++++++++++++++++++++++
>>  dtc.h  |    1 +
>>  2 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/data.c b/data.c
>> index b5f3066..37acd6a 100644
>> --- a/data.c
>> +++ b/data.c
>> @@ -175,6 +175,39 @@ struct data data_append_cell(struct data d, cell_t word)
>>       return data_append_data(d, &beword, sizeof(beword));
>>  }
>>
>> +struct data data_append_literal(struct data d, uint64_t value, int len)
>
> I'd prefer to call this data_append_integer() since there are string
> and character literals too.  Plus by the time we get to the struct
> data level, it's not really relevant any more that this came from a
> literal in the parser.

Done

> And perhaps call it 'bits' or 'size' rather than 'len'.  'len' to me
> suggests a byte size rather than a bit size.

Done

>> +{
>> +     uint8_t value_8;
>> +     uint16_t be_value_16;
>> +     uint32_t be_value_32;
>> +     uint64_t be_value_64;
>
> I'd remove the 'be_', it doesn't really add anything of value.  Plus
> I've mostly avoided explicit references to BE (hence fdtXX_to_cpu() on
> the off chance that someone one day is stupid enough to use an LE
> variant of the fdt format.

Done

>> +
>> +     if ((len < 64) && (value >= (1ULL << len)))
>> +             die("Literal value 0x%x too large to fit in %d-bit cell\n",
>> +                 value, len);
>
> This really shouldn't be a die().  In general bad input should not
> directly trigger a die() during parse - it should give an error but
> continue parse as best it can and only drop out afterwards.

Hmm, so this check should never happen when called from the
parser because the parser uses eval_literal to read the cell values
and that function also checks this.

> In this case, I think the semantics of an overflow are clear enough
> that it shouldn't even be an error per se.  Just print a warning, and
> mask the number down to the relevant size.

To do this I would have to duplicate the functionality of eval_literal, or
add a flag that tells it to warn instead of error on overflow.  Do you have
a preference?

>
>> +
>> +     switch (len) {
>> +     case 8:
>> +             value_8 = value;
>> +             return data_append_data(d, &value_8, 1);
>> +
>> +     case 16:
>> +             be_value_16 = cpu_to_fdt16(value);
>> +             return data_append_data(d, &be_value_16, 2);
>> +
>> +     case 32:
>> +             be_value_32 = cpu_to_fdt32(value);
>> +             return data_append_data(d, &be_value_32, 4);
>> +
>> +     case 64:
>> +             be_value_64 = cpu_to_fdt64(value);
>> +             return data_append_data(d, &be_value_64, 8);
>> +
>> +     default:
>> +             die("Invalid literal size (%d)\n", len);
>
> This on the other hand is fine to be a die(), since it's essentially
> an assertion that should only be triggered by bad code elsewhere in
> dtc itself, not by bad input.

Yup

>> +     }
>> +}
>> +
>>  struct data data_append_re(struct data d, const struct fdt_reserve_entry *re)
>>  {
>>       struct fdt_reserve_entry bere;
>> diff --git a/dtc.h b/dtc.h
>> index f37c97e..50433f6 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -109,6 +109,7 @@ struct data data_insert_at_marker(struct data d, struct marker *m,
>>                                 const void *p, int len);
>>  struct data data_merge(struct data d1, struct data d2);
>>  struct data data_append_cell(struct data d, cell_t word);
>> +struct data data_append_literal(struct data d, uint64_t word, int len);
>>  struct data data_append_re(struct data d, const struct fdt_reserve_entry *re);
>>  struct data data_append_addr(struct data d, uint64_t addr);
>>  struct data data_append_byte(struct data d, uint8_t byte);
>
> --
> 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
>

Thanks,
    Anton