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.
>
> > - 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.
[..]
> > + 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.
>
> > + 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.
Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
