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@xxxxxxx> > > > > 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:
signature.asc
Description: PGP signature
