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