Re: [PATCH 04/14] libfdt: fdt_add_string_(): Fix comparison warning

["André Przywara" <andre.przywara-5wv7dgnIgG8@public.gmane.org>]

On 02/10/2020 01:01, David Gibson wrote:

Hi David,

> On Thu, Oct 01, 2020 at 04:33:50PM +0100, André Przywara wrote:
>> On 24/09/2020 02:01, David Gibson wrote:
>>> On Mon, Sep 21, 2020 at 05:52:53PM +0100, Andre Przywara wrote:
>>>> With -Wsign-compare, compilers warn about a mismatching signedness
>>>> in a comparison in fdt_add_string_().
>>>>
>>>> As struct_top can only be positive, just use an unsigned type for it,
>>>> and avoid the signedness difference.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
>>>
>>> I'm not sure this is right.  Well.. I'm also not sure it was right
>>> before.  Adding some more context to explain why..
>>>
>>>> ---
>>>>  libfdt/fdt_sw.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
>>>> index d10a720..d65e9c8 100644
>>>> --- a/libfdt/fdt_sw.c
>>>> +++ b/libfdt/fdt_sw.c
>>>> @@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
>>>>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
>>>>  	int strtabsize = fdt_size_dt_strings(fdt);
>>>>  	int len = strlen(s) + 1;
>>>> -	int struct_top, offset;
>>>> +	unsigned int struct_top;
>>>> +	int offset;
>>>>  
>>>>  	offset = -strtabsize - len;
>>>>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>>>> 	if (fdt_totalsize(fdt) + offset < struct_top)
>>>> 		return 0; /* no more room :( */
>>>
>>> So strtabsize and len will always be positive (or, if they're not,
>>> that's another problem), so offset is always negative.  Which means we
>>> need the signed addition between totalsize and offset for this to be
>>> correct.
>>>
>>> So I suspect we want to make 'len' and 'offset' unsigned as well,
>>> reverse the sign on offset and make it a subtraction in the if instead
>>> of an addition-of-negative.
>>
>> Ah, yes, much better! To be honest I just did my due diligence on *this*
>> function before, because all those minus signs confused me quite a lot.
>> But your approach makes it much clearer what is going on here.
>>
>> So if I get this correctly, this function really returns the *negative*
>> offset of the new string, which will end up in the property, for now?
>> And at the end we translate all negative values into the actual offsets?
>> And there is initially a gap between the dt_struct part and the string
>> table, to allow both growing towards each other, without needing to
>> rewrite everything, every time?
> 
> Yes, that's correct.  During sequential write mode, to avoid excessive
> memmove()s, the structure block grows up from the bottom of the
> buffer, while the strings block grows down from the top.  Because the
> "start" (lowest offset) of the strings block is moving, we can't use
> offsets from that (without having to adjust all the offsets in the
> structure block all the time).  So, we use negative offsets from the
> "end" (highest offset).  I'm scare quoting "start" and "end" because
> the header records the highest offset of the string block as its
> "start", which is why the offsets are negative - that makes some of
> the code paths more common with the normal dtb case.
> 
>> Is this documented somewhere? Shall this approach be described in a
>> comment at the top of this file?
> 
> No, I don't think it is.  So, yes there probably should be a big
> explanatory comment.  Patches welcome?
Yes, that was the idea, just want to double check that I got this right.

> Note that this isn't supposed to be an externally visible format,
> you're always supposed to use fdt_finish() to convert it into a
> regular dtb before handing it off to any other piece of code.  We use
> a different magic number in the header while we're in this state to
> avoid anything mistaking this for a regular dtb.

Yeah, I understand, but the different magics and the negative offsets
were quite confusing at first, until I discovered the whole story.
Since the library is typically embedded into other projects, I think
it's a good idea to document this, in case people follow some rabbit
with ctags ...

Cheers,
Andre