On Tue, Feb 28, 2017 at 10:40:16PM -0700, Simon Glass wrote:
> Hi David,
>
> On 23 February 2017 at 19:43, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Tue, Feb 21, 2017 at 09:33:36PM -0700, Simon Glass wrote:
> >> Add Python bindings for a bare-bones set of libfdt functions. These allow
> >> navigating the tree and reading node names and properties.
> >>
> >> Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> >> ---
> >>
> >> Changes in v7:
> >> - Add QUIET_ALL to silence all exceptions
> >>
> >> Changes in v6:
> >> - Use a tuple instead of list for the default quiert parameter
> >> - Use a tuple instead of list for QUIET_NOTFOUND
> >> - Use 'list' instead of 'tuple' for the comment in check_err_null()
> >> - Return a bytearray from getprop()
> >> - Adjust the Property constructor to accept the name and value
> >> - Use uint8_t for pylibfdt_copy_value
> >>
> >> Changes in v5:
> >> - Use a 'quiet' parameter instead of quiet versions of functions
> >> - Add a Property object to hold a property's name and value
> >> - Drop the data() and string() functions which are not needed now
> >> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
> >> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
> >> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
> >> - Use $(SWIG) to call swig from the Makefile
> >> - Review function comments
> >>
> >> Changes in v4:
> >> - Make the library less pythonic to avoid a shaky illusion
> >> - Drop classes for Node and Prop, along with associated methods
> >> - Include libfdt.h instead of repeating it
> >> - Add support for fdt_getprop()
> >> - Bring in all libfdt functions (but Python support is missing for many)
> >> - Add full comments for Python methods
> >>
> >> Changes in v3:
> >> - Make the library more pythonic
> >> - Add classes for Node and Prop along with methods
> >> - Add an exception class
> >> - Use Python to generate exeptions instead of SWIG
> >>
> >> Changes in v2:
> >> - Add exceptions when functions return an error
> >> - Correct Python naming to following PEP8
> >> - Use a class to encapsulate the various methods
> >> - Include fdt.h instead of redefining struct fdt_property
> >> - Use bytearray to avoid the SWIG warning 454
> >> - Add comments
> >>
> >> Makefile | 1 +
> >> pylibfdt/.gitignore | 3 +
> >> pylibfdt/Makefile.pylibfdt | 18 ++
> >> pylibfdt/libfdt.swig | 477 +++++++++++++++++++++++++++++++++++++++++++++
> >> pylibfdt/setup.py | 34 ++++
> >> 5 files changed, 533 insertions(+)
> >> create mode 100644 pylibfdt/.gitignore
> >> create mode 100644 pylibfdt/Makefile.pylibfdt
> >> create mode 100644 pylibfdt/libfdt.swig
> >> create mode 100644 pylibfdt/setup.py
> >>
> [...]
>
> >> +
> >> +_NUM_ERRORS = 18
> >> +
> >> +# Error codes, corresponding to FDT_ERR_... in libfdt.h
> >> +(NOTFOUND,
> >> + EXISTS,
> >> + NOSPACE,
> >> + BADOFFSET,
> >> + BADPATH,
> >> + BADPHANDLE,
> >> + BADSTATE,
> >> + TRUNCATED,
> >> + BADMAGIC,
> >> + BADVERSION,
> >> + BADSTRUCTURE,
> >> + BADLAYOUT,
> >> + INTERNAL,
> >> + BADNCELLS,
> >> + BADVALUE,
> >> + BADOVERLAY,
> >> + NOPHANDLES) = range(1, _NUM_ERRORS)
> >
> > Nit: you could actually assign QUIET_ALL right here.
>
> it means the comment stands alone, but OK.
>
> >
> >> +# Pass this as the 'quiet' parameter to return -ENOTFOUND on NOTFOUND errors,
> >> +# instead of raising an exception.
> >> +QUIET_NOTFOUND = (NOTFOUND,)
> >> +
> >> +# Pass this as the 'quiet' parameter to avoid exceptions altogether. All
> >> +# functions passed this value will return an error instead of raising an
> >> +# exception.
> >> +QUIET_ALL = range(1, _NUM_ERRORS)
> >> +
> >> +
> >> +class FdtException(Exception):
> >> + """An exception caused by an error such as one of the codes above"""
> >> + def __init__(self, err):
> >> + self.err = err
> >> +
> >> + def __str__(self):
> >> + return 'pylibfdt error %d: %s' % (self.err, fdt_strerror(self.err))
> >> +
> >> +def fdt32_to_cpu(val):
> >> + """Convert a device-tree cell value into a native integer"""
> >> + return struct.unpack("=I", struct.pack(">I", val))[0]
> >
> > I think this is conceptually wrong, although I think it will give the
> > right results. AIUI this is used when swig's built in type conversion
> > has incorrectly treated a structure with an integer member as being in
> > native format when it's actually in BE format.
> >
> > So rather than storing as BE, then loading as native, you should store
> > as native - "undoing" the incorrect load within swig - then correctly
> > loading as BE.
> >
> > Even better would be configuring swig not to make the mistake in the
> > first place, but I can accept that that's more trouble than it's
> > worth.
>
> Well I can drop this function and incorporate this code into the typemap.
Ok, that sounds good.
>
> [...]
>
> >> +
> >> + def get_property_by_offset(self, prop_offset, quiet=()):
> >> + """Obtains a property that can be examined
> >> +
> >> + Args:
> >> + prop_offset: Offset of property (e.g. from first_property_offset())
> >> + quiet: Errors to ignore (empty to raise on all errors)
> >> +
> >> + Returns:
> >> + Property object, or None if not found
> >> +
> >> + Raises:
> >> + FdtException on error (e.g. invalid prop_offset or device
> >> + tree format)
> >> + """
> >> + pdata = check_err_null(
> >> + fdt_get_property_by_offset(self._fdt, prop_offset), quiet)
> >> + if isinstance(pdata, (int)):
> >> + return pdata
> >> + name = fdt_string(self._fdt, fdt32_to_cpu(pdata[0].nameoff))
> >> + value = bytearray(fdt32_to_cpu(pdata[0].len))
> >> + pylibfdt_copy_value(value, pdata[0])
> >> + return Property(name, value)
> >
> > So.. just to check I understand. swig is translating the the return
> > value of fdt_get_property_by_offset() into:
> > If it returns NULL, an integer from the error code in *lenp
> > Otherwise, a tuple, where the second element is the length
> > from *lenp, the first element is a Python object with nameoff
> > and len attributes with those fields from the C structure?
> >
> > I still find the copy_value() thing ugly, but I think I've finally
> > understood why it's the easiest way to accomplish what you need. I
> > take it swig guarantees that if you pass that structure/object back
> > into a C function it will pass an identical pointer, not a copy of the
> > C structure (which might no longer have the data in the variable
> > length field).
>
> Yes, but I can do it with a typemap so I'll go with that for the next version.
Right, the version in the latest posting looks much better, thanks.
> [...]
>
> >> +%inline %{
> >> +
> >> +/**
> >> + * pylibfdt_copy_value() - Copy value from a property to the given buffer
> >> + *
> >> + * This is used by the Property class to place the contents of a property
> >> + * into a bytearray.
> >> + *
> >> + * @buf: Destination pointer (typically the start of the bytearray)
> >> + * @size: Number of bytes to copy (size of bytearray)
> >> + * @prop: Property to copy
> >> + */
> >> +void pylibfdt_copy_value(uint8_t *buf, size_t size, const struct fdt_property *prop)
> >> +{
> >> + memcpy(buf, prop + 1, size);
> >
> > Oh.. one more nit. I was recently reminded that memcpy(dst, src, 0)
> > isn't guaranteed to be a no-op so may invoke undefined behaviour (some
> > checkers like Coverity complain about it). So it's probably worth
> > having a check for zero length either here or in the Python caller.
>
> I think I can drop this.
>
> >
> >> +}
> >> +
> >> +%}
> >> +
> >> +%apply int *OUTPUT { int *lenp };
> >> +
> >> +/* typemap used for fdt_getprop() */
> >> +%typemap(out) (const void *) {
> >> + if (!$1)
> >> + $result = Py_None;
> >> + else
> >> + /* TODO(sjg@xxxxxxxxxxxx): Can we avoid the 'arg4'? */
> >> + $result = Py_BuildValue("s#", $1, *arg4);
> >
> > What's the arg4 about? This isn't relying on swig internals which are
> > subject to change is it?
>
> Possibly, as it is not documented. I'm not sure if there is a good
> way share input parameters with an output typemap. The typemap.i file
> is quite mystical to me, but it does seem to be a rule that parameters
> are called 'arg' and numbered. If you have any better solutions I am
> all ears.
>
> I will ask on the swig-users mailing list two as I cannot seem to find
> a solution already mentioned.
>
> [...]
>
> Regards,
> Simon
--
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
