Re: [PATCH v3 1/4] Add an initial Python library for libfdt

[Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

Hi David,

On 4 December 2016 at 23:23, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Hi David,
>
> On 4 December 2016 at 21:35, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> ---
>>>
>>> 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