Hi David,
On 4 December 2016 at 23:23, Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> Hi David,
>
> On 4 December 2016 at 21:35, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>> On Sat, Dec 03, 2016 at 05:48:07PM -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 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
>>>
>>> pylibfdt/.gitignore | 3 +
>>> pylibfdt/Makefile.pylibfdt | 18 ++
>>> pylibfdt/libfdt.swig | 602 +++++++++++++++++++++++++++++++++++++++++++++
>>> pylibfdt/setup.py | 34 +++
>>> 4 files changed, 657 insertions(+)
>>> create mode 100644 pylibfdt/.gitignore
>>> create mode 100644 pylibfdt/Makefile.pylibfdt
>>> create mode 100644 pylibfdt/libfdt.swig
>>> create mode 100644 pylibfdt/setup.py
Could you please take a look at this email? I'd like to get your
further feedback before going any further.
Thanks,
Simon
>>>
> [..]
>
>>> +# 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) = range(1, 17)
>>
>> Pity we can't get swig to autogenerate those.
>
> Maybe we can if you want it to change to an enum?
>
>>
>>> +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]
>>> +
>>> +def data(prop):
>>> + """Extract the data from a property
>>> +
>>> + Args:
>>> + prop: Property structure, as returned by get_property_by_offset()
>>> +
>>> + Returns:
>>> + The property data as a bytearray
>>> + """
>>> + buf = bytearray(fdt32_to_cpu(prop.len))
>>
>> prop.len should be a native value, not requiring byteswap.
>>
>>> + pylibfdt_copy_data(buf, prop)
>>> + return buf
>>
>> Urgh.. all this fdt_property() wrangling stuff is really ugly. Any
>> chance we could just exclude it from the wrapper, providing only
>> fdt_getprop() interfaces which simply return a Python bytestring
>> directly?
>
> I effectively have, by hiding it in here. Client code need never use this.
>
>>
>>> +def strerror(fdt_err):
>>> + """Get the string for an error number
>>> +
>>> + Args:
>>> + fdt_err: Error number (-ve)
>>> +
>>> + Returns:
>>> + String containing the associated error
>>> + """
>>> + return fdt_strerror(fdt_err)
>>> +
>>> +def check_err(val, quiet=False):
>>> + """Raise an error if the return value is -ve
>>> +
>>> + This is used to check for errors returned by libfdt C functions.
>>> +
>>> + Args:
>>> + val: Return value from a libfdt function
>>> + quiet: True to ignore the NOTFOUND error, False to raise on all errors
>>> +
>>> + Returns:
>>> + val if val >= 0
>>> +
>>> + Raises
>>> + FdtException if val is < 0
>>> + """
>>> + if val < 0:
>>> + if not quiet or val != -NOTFOUND:
>>> + raise FdtException(val)
>>> + return val
>>> +
>>> +def check_err_null(val):
>>> + """Raise an error if the return value is NULL
>>> +
>>> + This is used to check for a NULL return value from certain libfdt C
>>> + functions
>>> +
>>> + Args:
>>> + val: Return value from a libfdt function
>>> + quiet: True to ignore the NOTFOUND error, False to raise on all errors
>>> +
>>> + Returns:
>>> + val if val is not NULL
>>> +
>>> + Raises
>>> + FdtException if val is NULL
>>> + """
>>> + # Normally a tuple is returned which contains the data and its length.
>>
>> Yeah.. since Python strings / bytestrings know their own length, why
>> do we need such a tuple?
>
> It's just what swig returns for functions like: const char *func(int *len)
>
> This is internal to the class so should not matter to users.
>
>>
>>> + # If we get just an integer error code, it means the function failed.
>>> + if not isinstance(val, list):
>>> + raise FdtException(val)
>>> + return val
>>> +
>>> +class Fdt:
>>> + """Device tree class, supporting all operations
>>> +
>>> + The Fdt object is created is created from a device tree binary file,
>>> + e.g. with something like:
>>> +
>>> + fdt = Fdt(open("filename.dtb").read())
>>> +
>>> + Operations can then be performed using the methods in this class. Each
>>> + method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
>>> +
>>> + Almost all methods raise a FdtException if an error occurs. The
>>> + following does not:
>>> +
>>> + string() - since it has no error checking
>>> +
>>> + To avoid this behaviour a 'quiet' version is provided for some functions.
>>> + This behaves as for the normal version except that it will not raise
>>> + an exception in the case of an FDT_ERR_NOTFOUND error: it will simply
>>> + return the -NOTFOUND error code or None.
>>> +
>>> + Separate classes are provided for nodes (Node) and properties (Prop).
>>> + """
>>> + def __init__(self, data):
>>> + self._fdt = bytearray(data)
>>> +
>>> + def string(self, offset):
>>> + """Get a string given its offset
>>> +
>>> + Args:
>>> + offset: FDT offset in big-endian format
>>> +
>>> + Returns:
>>> + string value at that offset
>>> + """
>>> + return fdt_string(self._fdt, fdt32_to_cpu(offset))
>>
>> ARGH, no. The offset argument is an integer, it should be treated as
>> a native integer, and the byteswap should absolutely not be here.
>
> Ah yes, oops.
>
>>
>>> + def path(self, path):
>>> + """Get the Node for a given path
>>> +
>>> + Args:
>>> + path: Path to the required node, e.g. '/node@3/subnode@1'
>>> +
>>> + Returns:
>>> + Node object for this node
>>> +
>>> + Raises
>>> + FdtException if the path is not valid
>>> + """
>>> + return Node(self, check_err(fdt_path_offset(self._fdt, path)))
>>
>> This interface is a mistake - more details on class Node below.
>
> [..]
>
>>> + def get_property(self, nodeoffset, name):
>>> + """Get a property given a node offset and the property name
>>> +
>>> + We cannot use fdt_get_property() here since it does not return the
>>> + offset. We prefer to create Node objects using the offset.
>>> +
>>> + Args:
>>> + nodeoffset: Offset of the node
>>> + name: Property name
>>> +
>>> + Returns:
>>> + Prop object found
>>> +
>>> + Raises:
>>> + FdtException on error such as no such property
>>> + """
>>> + poffset = self.first_property_offset(nodeoffset)
>>> + while True:
>>> + pdata = self.get_property_by_offset_internal(poffset)
>>> + if self.string(pdata.nameoff) == name:
>>> + return Prop(Node(self, nodeoffset), poffset)
>>> + poffset = self.next_property_offset(poffset)
>>
>> Wait.. what!? You're searching through the properties in Python?
>> Why not just using fdt_get_property() or fdt_getprop()?
>
> Because they do not tell me the offset-(see the comment in the
> function description above:
>
> We cannot use fdt_get_property() here since it does not return the
> offset. We prefer to create Node objects using the offset.
>
> If we want to have a Node object, then we probably want to store the
> offset of the node. Another option would be to add a function to
> libfdt which gets a property by name, but returns its offset, rather
> than a struct?
>
>>
>>> + def get_property_quiet(self, nodeoffset, name):
>>> + """Get a property given a node offset and the property name
>>> +
>>> + We cannot use fdt_get_property() here since it does not return the
>>> + offset. We prefer to create Node objects using the offset.
>>> +
>>> + Args:
>>> + nodeoffset: Offset of the node
>>> + name: Property name
>>> +
>>> + Returns:
>>> + Prop object found or None if there is no such property
>>> +
>>> + Raises:
>>> + FdtException on error
>>> + """
>>> + poffset = self.first_property_offset_quiet(nodeoffset)
>>> + while poffset >= 0:
>>> + pdata = self.get_property_by_offset_internal(poffset)
>>> + if self.string(pdata.nameoff) == name:
>>> + return Prop(Node(self, nodeoffset), poffset)
>>> + poffset = self.next_property_offset_quiet(poffset)
>>> + return None
>>> +
>>> + def first_subnode(self, nodeoffset):
>>> + return check_err(fdt_first_subnode(self._fdt, nodeoffset))
>>> +
>>> + def first_subnode_quiet(self, nodeoffset):
>>> + return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
>>> +
>>> + def next_subnode(self, nodeoffset):
>>> + return check_err(fdt_next_subnode(self._fdt, nodeoffset))
>>> +
>>> + def next_subnode_quiet(self, nodeoffset):
>>> + return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
>>> +
>>> + def totalsize(self):
>>> + return check_err(fdt_totalsize(self._fdt))
>>> +
>>> + def off_dt_struct(self):
>>> + return check_err(fdt_off_dt_struct(self._fdt))
>>> +
>>> + def pack(self):
>>> + return check_err(fdt_pack(self._fdt))
>>> +
>>> + def delprop(self, nodeoffset, prop_name):
>>> + return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
>>> +
>>> +
>>> +class Node:
>>
>> This can't work. The reason that so many libfdt functions are
>> insistently called whatever_offset(), rather than just get_whatever()
>> is to constantly remind the user that this is a actual byte offset
>> into a buffer of data. It's *not* a friendly stable node-handle, and
>> can become invalid when the fdt blob is altered.
>>
>> Attempting to put a class wrapper around it obscures that vital fact.
>> Treat node offsets as offsets everywhere, just encoded as plain old
>> python integers.
>
> My intent is to make the interface more pythonic, and I've been trying
> out various approaches. For read-only use this interface is nice.
> Consider this the high-water mark of this approach.
>
> But I can go back to something in between, where there is no Node or
> Prop. But please do check the code example in the cover letter and see
> if you are sure.
>
> With this approach we can do things like node.next() which is nice I think.
>
> Of course things can break if you start changing the tree while
> keeping objects around. So it isn't a perfect illusion. Hopefully it
> returns errors in that case..
>
> [..]
>
>>> +class Prop:
>>> + """A device-tree property
>>> +
>>> + This encapsulates a device-tree property and provides various operations
>>> + which can be performed on the property.
>>> +
>>> + Generally the standard constructor is used to create a Prop object. But
>>> + in the case where only the property offset is known (and not the Node
>>> + that holds the property), from_offset() can be used.
>>> + """
>>
>> TBH, I'm not really sold on the value of this class either. Is there
>> really any point dealing with these rather fragile property handles,
>> rather than simply returning bytestrings with the values directly? I
>> can't see it.
>
> It provides the ability to iterate, get the name (as well as the
> data), delete the property, etc.
>
>>
>>> + def __init__(self, node, offset, fdt=None):
>>> + self._node = node
>>> + self._offset = offset
>>> + self._fdt = node._fdt if node else fdt
>>> +
>>> + @classmethod
>>> + def from_offset(cls, fdt, prop_offset):
>>> + prop = cls(None, prop_offset, fdt)
>>> + prop._get_name_data()
>>> + return prop
>>> +
>>> + def next(self):
>>> + return Prop(self._node, check_err(fdt_next_property_offset(
>>> + self._fdt._fdt, self._offset)))
>>> +
>>> + def next_quiet(self):
>>> + val = check_err(fdt_next_property_offset(self._fdt._fdt, self._offset),
>>> + True)
>>> + if val < 0:
>>> + return None
>>> + return Prop(self._node, val)
>>> +
>>> + def _get_name_data(self):
>>> + pdata = fdt_get_property_by_offset(self._fdt._fdt, self._offset)
>>> + if not isinstance(pdata, list):
>>> + raise FdtException(pdata)
>>> + return self._fdt.string(pdata[0].nameoff), data(pdata[0])
>>> +
>>> + def name(self):
>>> + return self._get_name_data()[0]
>>> +
>>> + def data(self):
>>> + return self._get_name_data()[1]
>>> +
>>> + def delete(self):
>>> + if not self._node:
>>> + raise RuntimeError("Can't delete property offset %d of unknown node"
>>> + % self._offset)
>>> + self._fdt.delprop(self._node._offset, self.name())
>>> +%}
>
> [..]
>
>>> +
>>> +/*
>>> + * From here are the function definitions from libfdt.h, along with their
>>> + * exception-handling code.
>>> + */
>>
>> Uh.. why do you need to duplicate these rather than including libfdt.h?
>
> Hopefully I can do that, even with fdt_totalsize(). Will check.
>
>>
>>> +int fdt_path_offset(const void *fdt, const char *path);
>>> +
>>> +int fdt_first_property_offset(const void *fdt, int nodeoffset);
>>> +
>>> +int fdt_next_property_offset(const void *fdt, int offset);
>>> +
>>> +const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
>>> +
>>> +/* no exception handling, since this function has no error checking */
>>> +const char *fdt_string(const void *fdt, int stroffset);
>>> +
>>> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>>> + int offset, int *OUTPUT);
>>> +
>>> +/* no exception handling, this this function always returns a valid string */
>>> +const char *fdt_strerror(int errval);
>>> +
>>> +int fdt_first_subnode(const void *fdt, int offset);
>>> +
>>> +int fdt_next_subnode(const void *fdt, int offset);
>>> +
>>> +int fdt_delprop(void *fdt, int nodeoffset, const char *name);
>>> +
>>> +int fdt_pack(void *fdt);
>>> +
>>> +int fdt_totalsize(const void *fdt);
>>> +
>>> +int fdt_off_dt_struct(const void *fdt);
>>> diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
>>> new file mode 100644
>>> index 0000000..8f8618e
>>> --- /dev/null
>>> +++ b/pylibfdt/setup.py
>>> @@ -0,0 +1,34 @@
>>> +#!/usr/bin/env python
>>> +
>>> +"""
>>> +setup.py file for SWIG libfdt
>>> +"""
>>> +
>>> +from distutils.core import setup, Extension
>>> +import os
>>> +import sys
>>> +
>>> +progname = sys.argv[0]
>>> +cflags = sys.argv[1]
>>> +files = sys.argv[2:]
>>> +
>>> +if cflags:
>>> + cflags = [flag for flag in cflags.split(' ') if flag]
>>> +else:
>>> + cflags = None
>>> +
>>> +libfdt_module = Extension(
>>> + '_libfdt',
>>> + sources = files,
>>> + extra_compile_args = cflags
>>> +)
>>> +
>>> +sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
>>> +
>>> +setup (name = 'libfdt',
>>> + version = '0.1',
>>> + author = "SWIG Docs",
>>
>> Um.. that doesn't seem quite right.
>
> Nope, will fix.
>
>>
>>> + description = """Simple swig libfdt from docs""",
>>> + ext_modules = [libfdt_module],
>>> + py_modules = ["libfdt"],
>>> + )
>>
>> --
>> 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
>
> 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
