On Fri, Oct 02, 2020 at 10:31:06AM +0100, André Przywara wrote: > 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 ... Yes, that makes sense. -- 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
