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.
[...]
>> +
>> + 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.
[...]
>> +%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
--
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
