On Wed, 2018-10-03 at 14:44 -0500, Rob Herring wrote:
> On Wed, Oct 3, 2018 at 1:25 PM Lubomir Rintel <lkundrak@xxxxx> wrote:
> > On Wed, 2018-10-03 at 11:08 -0500, Rob Herring wrote:
> > > On Wed, Oct 3, 2018 at 8:22 AM Lubomir Rintel <lkundrak@xxxxx>
> > > wrote:
> > > > If none of the markers bears type information we can't just
> > > > prepend a new
> > > > one. Otherwise two subsequent markers will begin at offset 0,
> > > > blowing up
> > > > the chunk_len calculation a few lines after.
> > > >
> > > > A particular case where this causes havoc is a string property
> > > > in a tree
> > > > generated from "fs" input. A string property ends up with a
> > > > TYPE_NULL
> > >
> > > You mean TYPE_NONE?
> >
> > Yes.
> >
> > > > marker. Upon output, in write_propval(), we correctly guess it
> > > > looks
> > > > like a TYPE_STRING. With the dummy marker prepended, We'd and
> > > > up with two
> > > > markers:
> > > >
> > > > { type = TYPE_STRING, offset = 0 }
> > > > { type = TYPE_NONE, offset = 0 }
> > > >
> > > > The chunk_len for the fist marker will end up being calculated
> > > > as 0.
> > > > This blows up later on because a zero-length string should
> > > > never happen
> > > > because it lacks the terminating \0 (valgrind catches this).
> > >
> > > I'm slightly confused as to when this is broken. This sounds like
> > > the
> > > output from fs input is completely broken for all strings, but I
> > > don't
> > > think that is true given we've had reports of only minor issues.
> >
> > Yes, it's indeed broken for all strings. On my system:
> >
> > $ ./dtc -I fs -O dts tests/fs/test_tree1 |grep '"'
> > compatible = """test_tree1";
> > prop-str = """hello world";
> > compatible = """subsubnode2\0subsubnode";
> > compatible = """subnode1";
> > compatible = """subsubnode1\0subsubnode";
> > placeholder = """this is a placeholder string\0string2";
> >
> > Note the extra "". But also:
>
> Ah, the extra "" should be fixed with the patch I mentioned. I guess
> TYPE_NONE is the same as REF_PATH combined with TYPE_STRING.
>
> > $ valgrind ./dtc -I fs -O dts tests/fs/test_tree1 |grep '"'
>
> And valgrind passes for me with the patch.
Yes, thank you. It indeed now works beautifully for me.
I'll follow up by sending out the test part of the patch, because I
still think it would be useful to test that the output DTS is exactly
what we expect it to be. It would have caught this sort of issue.
Lubo
>
> Rob
