On Sat, Jul 07, 2018 at 01:11:07PM -0700, Simon Glass wrote:
> Hi David,
>
> On 1 July 2018 at 17:34, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, Jun 25, 2018 at 09:43:08PM -0600, Simon Glass wrote:
> > > It is useful to be able to create a device tree from scratch using
> > > software. This is supported in libfdt but not currently available in the
> > > Python bindings.
> > >
> > > Add a new FdtSw class to handle this, with various methods corresponding
> > > to the libfdt functions. When the tree is complete, calling AsFdt() will
> > > return the completed device-tree object.
> > >
> > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> >
> > Looking pretty good, but I still have some comments.
> >
> > > ---
> > >
> > > Changes in v4:
> > > - Drop patches previously indicated as applied (but not yet present
> > > in master)
> >
> > Ah, sorry, I keep forgetting to push to the kernel.org master tree.
> > Note that you can often get my "tentative" master from github:
> > git://github.com/dgibson/dtc
> >
> > [Background: The reason this happens is that I usually push first to
> > github to run the Travis CI build then, if that passes, push to
> > kernel.org. However, if the Travis build is delayed - which often
> > happens when I have a bunch of much slower qemu builds in my Travis
> > queue - then I sometimes forgot to look back and push to kernel.org]
>
> It's no problem, yes you pointed out that master and I checked that
> before sending this patch on top of it.
Great.
>
> >
> > > - Make resizing reactive (to -FDT_ERR_NOTFOUND) instead of proactive
> > >
> > > Changes in v3:
> > > - Add a separate comment explaining the purpose of the FdtSw context manager
> > > - Adjust operation of as_fdt() to include calling fdt_finish()
> > > - Drop unnecessary data copy in resize() an rely on fdt_resize() instead
> > > - Rename AddNode() to add_node()
> > > - Rename AsFdt() to as_fdt()
> > > - Rename _fdt_property() to fdt_property_stub()
> > > - Rename the size passed to the FdtSw() to a hint only
> > > - Use fdtsw instead of fdtrw as internal variable in FdtSw
> > >
> > > pylibfdt/libfdt.i | 296 ++++++++++++++++++++++++++++++++++++++++
> > > tests/pylibfdt_tests.py | 89 +++++++++++-
> > > 2 files changed, 379 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > > index aed5390..abd8e61 100644
> > > --- a/pylibfdt/libfdt.i
> > > +++ b/pylibfdt/libfdt.i
> > > @@ -57,6 +57,18 @@
> > > %{
> > > #define SWIG_FILE_WITH_INIT
> > > #include "libfdt.h"
> > > +
> > > +/*
> > > + * We rename this function here to avoid problems with swig, since we also have
> > > + * a struct called fdt_property. That struct causes swig to create a class in
> > > + * libfdt.py called fdt_property(), which confuses things.
> > > + */
> > > +static int fdt_property_stub(void *fdt, const char *name, const char *val,
> > > + int len)
> > > +{
> > > + return fdt_property(fdt, name, val, len);
> > > +}
> > > +
> > > %}
> > >
> > > %pythoncode %{
> > > @@ -88,6 +100,7 @@ import struct
> > > # Pass this as the 'quiet' parameter to return -ENOTFOUND on NOTFOUND errors,
> > > # instead of raising an exception.
> > > QUIET_NOTFOUND = (NOTFOUND,)
> > > +QUIET_NOSPACE = (NOSPACE,)
> > >
> > >
> > > class FdtException(Exception):
> > > @@ -693,6 +706,275 @@ class Property(bytearray):
> > > if 0 in self[:-1]:
> > > raise ValueError('Property contains embedded nul characters')
> > > return self[:-1].decode('utf-8')
> > > +
> > > +
> > > +class FdtSw(object):
> >
> > So I realized one drawback of having a completely separate FdtSw
> > object is that the read-only methods of class Fdt won't be usable on
> > it, whereas the read only functions in libfdt do (by design) work on
> > sw mode trees.
>
> Yes - it never occurred to be to use those functions in that way.
>
> What do you suggest here? People can use the function to convert it to
> one. I don't think it's a great idea to make FdtSw a subclass, since
> Fdt has read-write functions which will not work on the sw tree.
>
> I could just combine the two classes and add comments about the two
> ways to use them.
That's one option. Another would be to make both FdtRw and FdtSw
subclasses of an FdtRo class which has only the read-only functions.
> [..]
>
> > > + def as_fdt(self):
> > > + """Convert a FdtSw into an Fdt so it can be accessed as normal
> > > +
> > > + Creates a new Fdt object from the work-in-progress device tree. This
> > > + does not call fdt_finish() on the current object, so it is possible to
> > > + add more nodes/properties and call as_fdt() again to get an updated
> > > + tree.
> > > +
> > > + Returns:
> > > + Fdt object allowing access to the newly created device tree
> > > + """
> > > + fdtsw = bytearray(self._fdtsw)
> > > + fdtsw = self._fdtsw
> >
> > I'm pretty sure this isn't what you meant. You initialize fdtsw to a
> > new bytearray, then throw that away, replacing it with a reference to
> > the existing one inside this object.
>
> Thanks for that - I did hit a strange bug related to that but it went
> away. I added a test.
Ok.
> > > + check_err(fdt_finish(fdtsw))
> > > + return Fdt(fdtsw)
> > > +
> > > + def check_space(self, val):
> > > + """Check if we need to add more space to the FDT
> > > +
> > > + This should be called with the error code from an operation. If this is
> > > + -NOSPACE then the FDT will be expanded to have more space, and True will
> > > + be returned, indicating that the operation needs to be tried again.
> > > +
> > > + Args:
> > > + val: Return value from the operation that was attempted
> > > +
> > > + Returns:
> > > + True if the operation must be retried, else False
> > > + """
> > > + if check_err(val, QUIET_NOSPACE) < 0:
> > > + self.resize(len(self._fdtsw) + self.INC_SIZE)
> > > + return True
> > > + return False
> > > +
> > > + def resize(self, size):
> > > + """Resize the buffer to accommodate a larger tree
> > > +
> > > + Args:
> > > + size: New size of tree
> > > +
> > > + Raises:
> > > + FdtException on any error
> > > + """
> > > + fdt = bytearray(size)
> > > + # This line should not be needed. Without it, we get BADSTRUCTURE
> > > + fdt[:len(self._fdtsw)] = self._fdtsw
> >
> > This is weird, we really need to track down why it's not working with
> > that line. It definitely shouldn't be necessary.
>
> I don't see any comments on fdt_resize() so I'm actually not sure what
> it is supposed to do. I inferred the purpose of arguments from the
> name and looking at the memcpy()s in the code.
>
> But I don't see where it copies the data in the struct region itself.
> It seems to copy up to that point, and from the start of the string
> region. But I suspect I'm confused because I don't understand the
> structure of the sw tree, which is a bit different.
As you obviously deduced from your next version, fdt_resize() is
indeed buggy. I realized I didn't actually have a test for that way
of using it. I've added one, and found some more subtle bugs on top
of the one you fixed, so I'll probably incoporate your patch with some
more of my own shortly.
--
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
