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

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3878 bytes --]

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?

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]