[PATCH v4 0/5] Introduce Python bindings for libfdt

[PATCH v4 0/5] Introduce Python bindings for libfdt

[Simon Glass]

At present libfdt consists of only a C implementation. Many scripts are
written using Python so it useful to have Python bindings for libfdt.
Apparently this has never been attempted before, or if so I cannot find a
reference.

This series starts the process of adding this support, with just a
bare-bones set of methods.

The v4 series provides binding that can be used like this:

    fdt = libfdt.Fdt(open(fname).read())
    node = fdt.path_offset('/subnode@1')
    print fdt.get_prop(node, 'compatible')
    subnode = fdt.first_subnode_quiet(node)
    while subnode > 0:
        print fdt.get_name(subnode)
        subnode = fdt.next_subnode_quiet(subnode)

This version does not include a class for properties. That can be added
if it is felt to be useful, but I want to get some basic functionality
agreed first. The get_property_by_offset() function would likely benefit
from this.

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
- Drop tests that are no-longer applicable
- Add a get for getprop()
- Add new patch to adjust libfdt.h to work with swig

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
- Add some more tests

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
- Update tests for new pylibfdt
- Add a few more tests to increase coverage
- Add details on how to obtain full help and code coverage

Simon Glass (5):
  Add an initial Python library for libfdt
  Add tests for pylibfdt
  Mention pylibfdt in the documentation
  Adjust libfdt.h to work with swig
  Build pylibfdt as part of the normal build process

 Makefile                   |  16 +-
 README                     |  33 +++
 libfdt/libfdt.h            |   7 +-
 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  18 ++
 pylibfdt/libfdt.swig       | 565 +++++++++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 +++
 tests/pylibfdt_tests.py    | 275 ++++++++++++++++++++++
 tests/run_tests.sh         |  19 +-
 9 files changed, 967 insertions(+), 3 deletions(-)
 create mode 100644 pylibfdt/.gitignore
 create mode 100644 pylibfdt/Makefile.pylibfdt
 create mode 100644 pylibfdt/libfdt.swig
 create mode 100644 pylibfdt/setup.py
 create mode 100644 tests/pylibfdt_tests.py

-- 
2.11.0.483.g087da7b7c-goog

[PATCH v4 1/5] Add an initial Python library for libfdt

[Simon Glass]

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 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

 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  18 ++
 pylibfdt/libfdt.swig       | 565 +++++++++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 +++
 4 files changed, 620 insertions(+)
 create mode 100644 pylibfdt/.gitignore
 create mode 100644 pylibfdt/Makefile.pylibfdt
 create mode 100644 pylibfdt/libfdt.swig
 create mode 100644 pylibfdt/setup.py

diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
new file mode 100644
index 0000000..5e8c5e3
--- /dev/null
+++ b/pylibfdt/.gitignore
@@ -0,0 +1,3 @@
+libfdt.py
+libfdt.pyc
+libfdt_wrap.c
diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
new file mode 100644
index 0000000..fa74dd2
--- /dev/null
+++ b/pylibfdt/Makefile.pylibfdt
@@ -0,0 +1,18 @@
+# Makefile.pylibfdt
+#
+
+PYLIBFDT_srcs = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_SRCS))
+WRAP = $(PYLIBFDT_objdir)/libfdt_wrap.c
+PYMODULE = $(PYLIBFDT_objdir)/_libfdt.so
+
+$(PYMODULE): $(PYLIBFDT_srcs) $(WRAP)
+	@$(VECHO) PYMOD $@
+	python $(PYLIBFDT_objdir)/setup.py "$(CPPFLAGS)" $^
+	mv _libfdt.so $(PYMODULE)
+
+$(WRAP): $(PYLIBFDT_srcdir)/libfdt.swig
+	@$(VECHO) SWIG $@
+	swig -python -o $@ $<
+
+PYLIBFDT_cleanfiles = libfdt_wrap.c libfdt.py libfdt.pyc
+PYLIBFDT_CLEANFILES = $(addprefix $(PYLIBFDT_objdir)/,$(PYLIBFDT_cleanfiles))
diff --git a/pylibfdt/libfdt.swig b/pylibfdt/libfdt.swig
new file mode 100644
index 0000000..ac478d5
--- /dev/null
+++ b/pylibfdt/libfdt.swig
@@ -0,0 +1,565 @@
+/*
+ * pylibfdt - Flat Device Tree manipulation in Python
+ * Copyright (C) 2017 Google, Inc.
+ * Written by Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
+ *
+ * libfdt is dual licensed: you can use it either under the terms of
+ * the GPL, or the BSD license, at your option.
+ *
+ *  a) This library is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This library is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ *     You should have received a copy of the GNU General Public
+ *     License along with this library; if not, write to the Free
+ *     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+ *     MA 02110-1301 USA
+ *
+ * Alternatively,
+ *
+ *  b) Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *     1. Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *     2. Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ *     THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ *     CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ *     INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ *     MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ *     DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ *     CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *     SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ *     NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ *     LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ *     HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ *     CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ *     OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+%module libfdt
+
+%{
+#define SWIG_FILE_WITH_INIT
+#include "libfdt.h"
+%}
+
+%pythoncode %{
+
+import struct
+
+# 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)
+
+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
+
+    This is an internal function only.
+
+    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))
+    pylibfdt_copy_data(buf, prop)
+    return buf
+
+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 < 0
+    """
+    if val < 0:
+        if not quiet or val != -NOTFOUND:
+            raise FdtException(val)
+    return val
+
+def check_err_null(val, quiet=False):
+    """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 a list, None if not
+
+    Raises
+        FdtException if quiet is False and val indicates an error was
+           reported. If quiet if True then an FdtException is raised only if
+           the error is something other than -NOTFOUND.
+    """
+    # Normally a tuple is returned which contains the data and its length.
+    # If we get just an integer error code, it means the function failed.
+    if not isinstance(val, list):
+        if not quiet or val != -NOTFOUND:
+            raise FdtException(val)
+        return None,
+    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 an 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.
+    """
+    def __init__(self, data):
+        self._fdt = bytearray(data)
+
+    def string(self, offset):
+        """Get a string given its offset
+
+        This is an internal function.
+
+        Args:
+            offset: FDT offset in big-endian format
+
+        Returns:
+            string value at that offset
+        """
+        return fdt_string(self._fdt, fdt32_to_cpu(offset))
+
+    def path_offset(self, path):
+        """Get the offset for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+
+        Returns:
+            Node offset
+
+        Raises
+            FdtException if the path is not valid
+        """
+        return check_err(fdt_path_offset(self._fdt, path))
+
+    def path_offset_quiet(self, path):
+        """Get the offset for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+
+        Returns:
+            Node offset, or -NOTFOUND if the path is not value
+
+        Raises
+            FdtException if any error occurs other than NOTFOUND
+        """
+        return check_err(fdt_path_offset(self._fdt, path), True)
+
+    def first_property_offset(self, nodeoffset):
+        """Get the offset of the first property in a node offset
+
+        Args:
+            nodeoffset: Offset to the node to check
+
+        Returns:
+            Offset of the first property
+
+        Raises
+            FdtException if the associated node has no properties, or some
+                other error occurred
+        """
+        return check_err(fdt_first_property_offset(self._fdt, nodeoffset))
+
+    def first_property_offset_quiet(self, nodeoffset):
+        """Get the offset of the first property in a node offset
+
+        Args:
+            nodeoffset: Offset to the node to check
+
+        Returns:
+            Offset of the first property, or -NOTFOUND if the node has no
+                properties
+
+        Raises
+            FdtException if any other error occurs
+        """
+        return check_err(fdt_first_property_offset(self._fdt, nodeoffset), True)
+
+    def next_property_offset(self, prop_offset):
+        """Get the next property in a node
+
+        Args:
+            prop_offset: Offset of the previous property
+
+        Returns:
+            Offset of the next property
+
+        Raises:
+            FdtException if the associated node has no more properties, or
+                some other error occurred
+        """
+        return check_err(fdt_next_property_offset(self._fdt, prop_offset))
+
+    def next_property_offset_quiet(self, prop_offset):
+        """Get the next property in a node
+
+        Args:
+            prop_offset: Offset of the previous property
+
+        Returns:
+            Offset ot the next property, or -NOTFOUND if there are no more
+            properties
+
+        Raises:
+            FdtException if any other error occurs
+        """
+        return check_err(fdt_next_property_offset(self._fdt, prop_offset), True)
+
+    def get_name(self, nodeoffset):
+        """Get the name of a node
+
+        Args:
+            nodeoffset: Offset of node to check
+
+        Returns:
+            Node name
+
+        Raises:
+            FdtException on error (e.g. nodeoffset is invalid)
+        """
+        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
+
+    def get_property_by_offset(self, prop_offset):
+        """Obtains a property that can be examined
+
+        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property object instead,
+        with the data extracted from the Fdt.
+
+        Args:
+            prop_offset: Offset of property (e.g. from first_property_offset())
+
+        Returns:
+            Property object with members:
+                tag: Big-endian device tree tag value
+                len: Big-endian property length
+                nameoff: Big-endian string offset for use with string()
+
+            Use data() on the return value to obtain the property value.
+
+        Raises:
+            FdtException on error (e.g. invalid prop_offset or device
+            tree format)
+        """
+        return check_err_null(fdt_get_property_by_offset(self._fdt,
+                                                         prop_offset))[0]
+
+    def first_subnode(self, nodeoffset):
+        """Find the first subnode of a parent node
+
+        Args:
+            nodeoffset: Node offset of parent node
+
+        Returns:
+            The offset of the first subnode, if any
+
+        Raises:
+            FdtException if no subnode found or other error occurs
+        """
+        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
+
+    def first_subnode_quiet(self, nodeoffset):
+        """Find the first subnode of a node
+
+        Args:
+            nodeoffset: Node offset of parent node
+
+        Returns:
+            The offset of the first subnode, or -NOTFOUND if none
+
+        Raises:
+            FdtException on error (e.g. invalid nodeoffset)
+        """
+        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
+
+    def next_subnode(self, nodeoffset):
+        """Find the next subnode
+
+        Args:
+            nodeoffset: Node offset of previous subnode
+
+        Returns:
+            The offset of the next subnode, if any
+
+        Raises:
+            FdtException if no more subnode found or other error occurs
+        """
+        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
+
+    def next_subnode_quiet(self, nodeoffset):
+        """Find the next subnode
+
+        Args:
+            nodeoffset: Node offset of previous subnode
+
+        Returns:
+            The offset of the next subnode, or -NOTFOUND if none
+
+        Raises:
+            FdtException on error (e.g. invalid nodeoffset)
+        """
+        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
+
+    def totalsize(self):
+        """Return the total size of the device tree
+
+        Returns:
+            Total tree size in bytes
+
+        Raises:
+            FdtException if any error occurs
+        """
+        return check_err(fdt_totalsize(self._fdt))
+
+    def off_dt_struct(self):
+        """Return the start of the device tree struct area
+
+        Returns:
+            Start offset of struct area
+
+        Raises:
+            FdtException if any error occurs
+        """
+        return check_err(fdt_off_dt_struct(self._fdt))
+
+    def pack(self):
+        """Pack the device tree to remove unused space
+
+        This adjusts the tree in place.
+
+        Raises:
+            FdtException if any error occurs
+        """
+        return check_err(fdt_pack(self._fdt))
+
+    def delprop(self, nodeoffset, prop_name):
+        """Delete a property from a node
+
+        Args:
+            nodeoffset: Node offset containing property to delete
+            prop_name: Name of property to delete
+
+        Raises:
+            FdtError if the property does not exist, or another error occurs
+        """
+        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
+
+    def getprop(self, nodeoffset, prop_name):
+        """Get a property from a node
+
+        Args:
+            nodeoffset: Node offset containing property to get
+            prop_name: Name of property to get
+
+        Returns:
+            Value of property as a string
+
+        Raises:
+            FdtError if any error occurs (e.g. the property is not found)
+        """
+        return check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name))[0]
+
+    def getprop_quiet(self, nodeoffset, prop_name):
+        """Get a property from a node
+
+        Args:
+            nodeoffset: Node offset containing property to get
+            prop_name: Name of property to get
+
+        Returns:
+            Value of property as a string, or None if not found
+
+        Raises:
+            FdtError if an error occurs (e.g. nodeoffset is invalid)
+        """
+        return check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
+                              True)[0]
+%}
+
+%rename(fdt_property) fdt_property_func;
+
+typedef int fdt32_t;
+
+%include "libfdt/fdt.h"
+
+%include "typemaps.i"
+
+/*
+ * Unfortunately the defintiion of pybuffer_mutable_binary() in my Python
+ * version appears to be broken:
+ * pylibfdt/libfdt_wrap.c: In function ‘_wrap_pylibfdt_copy_data’:
+ * pylibfdt/libfdt_wrap.c:3603:22: error: ‘size’ undeclared (first use in this
+ * function)
+ *   arg2 = (size_t) (size/sizeof(char));
+ *
+ * This version works correctly.
+ */
+%define %mypybuffer_mutable_binary(TYPEMAP, SIZE)
+%typemap(in) (TYPEMAP, SIZE)(int res, Py_ssize_t size = 0, void *buf = 0)
+{
+	res = PyObject_AsWriteBuffer($input, &buf, &size);
+	if (res < 0) {
+		PyErr_Clear();
+		%argument_fail(res, "(TYPEMAP, SIZE)", $symname, $argnum);
+	}
+	$1 = ($1_ltype)buf;
+	$2 = ($2_ltype)(size1 / sizeof($*1_type));
+}
+%enddef
+
+/* This is used to copy property data into a bytearray */
+%mypybuffer_mutable_binary(char *str, size_t size);
+void pylibfdt_copy_data(char *str, size_t size,
+			const struct fdt_property *prop);
+
+/* Most functions don't change the device tree, so use a const void * */
+%typemap(in) (const void *) {
+	if (!PyByteArray_Check($input)) {
+		SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
+			"', argument " "$argnum"" of type '" "$type""'");
+	}
+	$1 = (void *)PyByteArray_AsString($input);
+}
+
+/* Some functions do change the device tree, so use void * */
+%typemap(in) (void *) {
+	if (!PyByteArray_Check($input)) {
+		SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
+			"', argument " "$argnum"" of type '" "$type""'");
+	}
+	$1 = PyByteArray_AsString($input);
+}
+
+%inline %{
+
+/**
+ * pylibfdt_copy_data() - Copy data from a property to the given buffer
+ *
+ * This is used by the data() function 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_data(char *buf, size_t size, const struct fdt_property *prop)
+{
+	memcpy(buf, prop + 1, size);
+}
+
+%}
+
+%apply int *OUTPUT { int *lenp };
+
+/* Handle a few special cases which conflict with the typemap below */
+const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
+const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
+				const char *name, int namelen, int *lenp);
+
+/* typemap used for fdt_getprop() */
+%typemap(out) (const void *) {
+	if (!$1)
+		$result = Py_None;
+	else
+		/* TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Can we avoid the 'arg4'? */
+		$result = Py_BuildValue("s#", $1, *arg4);
+}
+
+/* We have both struct fdt_property and a function fdt_property() */
+%warnfilter(302) fdt_property;
+
+%include <../libfdt/libfdt.h>
+
+/* These are macros in the header so have to be redefined here */
+int fdt_magic(const void *fdt);
+int fdt_totalsize(const void *fdt);
+int fdt_off_dt_struct(const void *fdt);
+int fdt_off_dt_strings(const void *fdt);
+int fdt_off_mem_rsvmap(const void *fdt);
+int fdt_version(const void *fdt);
+int fdt_last_comp_version(const void *fdt);
+int fdt_boot_cpuid_phys(const void *fdt);
+int fdt_size_dt_strings(const void *fdt);
+int fdt_size_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",
+       description = """Simple swig libfdt from docs""",
+       ext_modules = [libfdt_module],
+       py_modules = ["libfdt"],
+       )
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v4 2/5] Add tests for pylibfdt

[Simon Glass]

Add a set of tests to cover the functionality in pylibfdt.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v4:
- Drop tests that are no-longer applicable
- Add a get for getprop()

Changes in v3:
- Add some more tests

Changes in v2:
- Update tests for new pylibfdt
- Add a few more tests to increase coverage

 tests/pylibfdt_tests.py | 275 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh      |  19 +++-
 2 files changed, 293 insertions(+), 1 deletion(-)
 create mode 100644 tests/pylibfdt_tests.py

diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
new file mode 100644
index 0000000..66eb45b
--- /dev/null
+++ b/tests/pylibfdt_tests.py
@@ -0,0 +1,275 @@
+# pylibfdt - Tests for Flat Device Tree manipulation in Python
+# Copyright (C) 2017 Google, Inc.
+# Written by Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
+#
+# libfdt is dual licensed: you can use it either under the terms of
+# the GPL, or the BSD license, at your option.
+#
+#  a) This library is free software; you can redistribute it and/or
+#     modify it under the terms of the GNU General Public License as
+#     published by the Free Software Foundation; either version 2 of the
+#     License, or (at your option) any later version.
+#
+#     This library is distributed in the hope that it will be useful,
+#     but WITHOUT ANY WARRANTY; without even the implied warranty of
+#     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#     GNU General Public License for more details.
+#
+#     You should have received a copy of the GNU General Public
+#     License along with this library; if not, write to the Free
+#     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+#     MA 02110-1301 USA
+#
+# Alternatively,
+#
+#  b) Redistribution and use in source and binary forms, with or
+#     without modification, are permitted provided that the following
+#     conditions are met:
+#
+#     1. Redistributions of source code must retain the above
+#        copyright notice, this list of conditions and the following
+#        disclaimer.
+#     2. Redistributions in binary form must reproduce the above
+#        copyright notice, this list of conditions and the following
+#        disclaimer in the documentation and/or other materials
+#        provided with the distribution.
+#
+#     THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+#     CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+#     INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+#     MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+#     DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+#     CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#     SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+#     NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+#     LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+#     HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+#     CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+#     OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+#     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#
+
+import sys
+import unittest
+
+sys.path.append('../pylibfdt')
+import libfdt
+from libfdt import FdtException
+
+# Offsets of properties in the root node
+ROOT_PROPS = (8, 32, 48, 68, 92, 108)
+
+def get_err(err_code):
+    """Convert an error code into an error message
+
+    Args:
+        err_code: Error code value (FDT_ERR_...)
+
+    Returns:
+        String error code
+    """
+    return 'pylibfdt error %d: %s' % (-err_code, libfdt.fdt_strerror(-err_code))
+
+def _ReadFdt(fname):
+    """Read a device tree file into an Fdt object, ready for use
+
+    Args:
+        fname: Filename to read from
+
+    Returns:
+        Fdt bytearray suitable for passing to libfdt functions
+    """
+    return libfdt.Fdt(open(fname).read())
+
+class PyLibfdtTests(unittest.TestCase):
+    """Test class for pylibfdt
+
+    Properties:
+        fdt: Device tree file used for testing
+    """
+
+    def setUp(self):
+        """Read in the device tree we use for testing"""
+        self.fdt = _ReadFdt('test_tree1.dtb')
+
+    def GetPropList(self, node_path):
+        """Read a list of properties from a node
+
+        Args:
+            node_path: Full path to node, e.g. '/subnode@1/subsubnode'
+
+        Returns:
+            List of property names for that node, e.g. ['compatible', 'reg']
+        """
+        prop_list = []
+        node = self.fdt.path_offset(node_path)
+        poffset = self.fdt.first_property_offset_quiet(node)
+        while poffset > 0:
+            pdata = self.fdt.get_property_by_offset(poffset)
+            prop_list.append(self.fdt.string(pdata.nameoff))
+            poffset = self.fdt.next_property_offset_quiet(poffset)
+        return prop_list
+
+    def testImport(self):
+        """Check that we can import the library correctly"""
+        self.assertEquals(type(libfdt), type(sys))
+
+    def testPathOffset(self):
+        """Check that we can find the offset of a node"""
+        self.assertEquals(self.fdt.path_offset('/'), 0)
+        self.assertEquals(self.fdt.path_offset('/subnode@1'), 124)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.path_offset('/wibble')
+        self.assertEquals(self.fdt.path_offset_quiet('/wibble'),
+                          -libfdt.NOTFOUND)
+
+    def testPropertyOffset(self):
+        """Walk through all the properties in the root node"""
+        self.assertEquals(self.fdt.first_property_offset(0), ROOT_PROPS[0])
+        for pos in range(len(ROOT_PROPS) - 1):
+            self.assertEquals(self.fdt.next_property_offset(ROOT_PROPS[pos]),
+                              ROOT_PROPS[pos + 1])
+        self.assertEquals(self.fdt.next_property_offset_quiet(ROOT_PROPS[-1]),
+                          -libfdt.NOTFOUND)
+
+    def testPropertyOffsetExceptions(self):
+        """Check that exceptions are raised as expected"""
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.next_property_offset(108)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.next_property_offset(107)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.first_property_offset_quiet(107)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.next_property_offset_quiet(107)
+
+        node = self.fdt.path_offset('/subnode@1/ss1')
+        self.assertEquals(self.fdt.first_property_offset_quiet(node),
+                          -libfdt.NOTFOUND)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.first_property_offset(node)
+
+    def testGetName(self):
+        """Check that we can get the name of a node"""
+        self.assertEquals(self.fdt.get_name(0), '')
+        node = self.fdt.path_offset('/subnode@1/subsubnode')
+        self.assertEquals(self.fdt.get_name(node), 'subsubnode')
+
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.get_name(-2)
+
+    def testGetString(self):
+        """Test that we can get a string from the string table"""
+        node = self.fdt.path_offset('/subnode@2')
+        poffset = self.fdt.first_property_offset(node)
+        pdata = self.fdt.get_property_by_offset(poffset)
+        self.assertEquals(self.fdt.string(pdata.nameoff), 'reg')
+
+    def testGetPropertyByOffset(self):
+        """Check that we can read the name and contents of a property"""
+        root = self.fdt.path_offset('/')
+        poffset = self.fdt.first_property_offset(root)
+        pdata = self.fdt.get_property_by_offset(poffset)
+        self.assertEquals(libfdt.fdt32_to_cpu(pdata.tag), 3)
+        self.assertEquals(libfdt.fdt32_to_cpu(pdata.len), 11)
+        self.assertEquals(self.fdt.string(pdata.nameoff), 'compatible')
+        self.assertEquals(libfdt.data(pdata), 'test_tree1\0')
+
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.get_property_by_offset(-2)
+
+    def testGetPropertyByOffset(self):
+        """Check that we can read the details of a property"""
+        root = self.fdt.path_offset('/')
+        poffset = self.fdt.first_property_offset(root)
+        prop = self.fdt.get_property_by_offset(poffset)
+        self.assertEquals(self.fdt.string(prop.nameoff), 'compatible')
+        self.assertEquals(libfdt.data(prop), 'test_tree1\0')
+
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.get_property_by_offset(-2)
+
+    def testGetProp(self):
+        """Check that we can read the contents of a property by name"""
+        root = self.fdt.path_offset('/')
+        data = self.fdt.getprop(root, "compatible")
+        self.assertEquals(data, 'test_tree1\0')
+        self.assertEquals(None, self.fdt.getprop_quiet(root, 'missing'))
+
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.getprop(root, 'missing')
+
+        node = self.fdt.path_offset('/subnode@1/subsubnode')
+        data = self.fdt.getprop(node, "compatible")
+        self.assertEquals(data, 'subsubnode1\0subsubnode\0')
+
+    def testStrError(self):
+        """Check that we can get an error string"""
+        self.assertEquals(libfdt.strerror(-libfdt.NOTFOUND),
+                          'FDT_ERR_NOTFOUND')
+
+    def testFirstNextSubnodeOffset(self):
+        """Check that we can walk through subnodes"""
+        node_list = []
+        node = self.fdt.first_subnode_quiet(0)
+        while node >= 0:
+            node_list.append(self.fdt.get_name(node))
+            node = self.fdt.next_subnode_quiet(node)
+        self.assertEquals(node_list, ['subnode@1', 'subnode@2'])
+
+    def testFirstNextSubnodeOffsetExceptions(self):
+        """Check except handling for first/next subnode functions"""
+        node = self.fdt.path_offset_quiet('/subnode@1/subsubnode')
+        self.assertEquals(self.fdt.first_subnode_quiet(node), -libfdt.NOTFOUND)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.first_subnode(node)
+
+        node = self.fdt.path_offset_quiet('/subnode@1/ss1')
+        self.assertEquals(self.fdt.next_subnode_quiet(node), -libfdt.NOTFOUND)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.next_subnode(node)
+
+    def testDeleteProperty(self):
+        """Test that we can delete a property"""
+        node_name = '/subnode@1'
+        self.assertEquals(self.GetPropList(node_name),
+                          ['compatible', 'reg', 'prop-int'])
+        node = self.fdt.path_offset('/%s' % node_name)
+        self.assertEquals(self.fdt.delprop(node, 'reg'), 0)
+        self.assertEquals(self.GetPropList(node_name),
+                          ['compatible', 'prop-int'])
+
+    def testHeader(self):
+        """Test that we can access the header values"""
+        self.assertEquals(self.fdt.totalsize(), 693)
+        self.assertEquals(self.fdt.off_dt_struct(), 88)
+
+    def testPack(self):
+        """Test that we can pack the tree after deleting something"""
+        self.assertEquals(self.fdt.totalsize(), 693)
+        node = self.fdt.path_offset_quiet('/subnode@2')
+        self.assertEquals(self.fdt.delprop(node, 'prop-int'), 0)
+        self.assertEquals(self.fdt.totalsize(), 693)
+        self.assertEquals(self.fdt.pack(), 0)
+        self.assertEquals(self.fdt.totalsize(), 677)
+
+    def testBadPropertyOffset(self):
+        """Test that bad property offsets are detected"""
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.get_property_by_offset(13)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.first_property_offset(3)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.next_property_offset(3)
+
+    def testBadPathOffset(self):
+        """Test that bad path names are detected"""
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADPATH)):
+            self.fdt.path_offset('not-present')
+
+    def testEndian(self):
+        """Check that we can convert from FDT (big endian) to native endian"""
+        self.assertEquals(libfdt.fdt32_to_cpu(0x10000000), 0x10)
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index ed489db..144feb6 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -769,6 +769,20 @@ fdtdump_tests () {
     run_fdtdump_test fdtdump.dts
 }
 
+pylibfdt_tests () {
+    TMP=/tmp/tests.stderr.$$
+    python pylibfdt_tests.py 2> ${TMP}
+    result=$(head -1 ${TMP} | awk \
+        '{ for (i = 1; i <= length($0); i++) { \
+            result = substr($0,i,1); fail = fail + (result == "F"); \
+            ok = ok + (result == ".")}; } END { print fail,  ok, fail + ok}')
+
+    # Extract the test results and add them to our totals
+    tot_fail=$((tot_fail + $(echo $result | cut -d" " -f 1)))
+    tot_pass=$((tot_pass + $(echo $result | cut -d" " -f 2)))
+    tot_tests=$((tot_tests + $(echo $result | cut -d" " -f 3)))
+}
+
 while getopts "vt:me" ARG ; do
     case $ARG in
 	"v")
@@ -787,7 +801,7 @@ while getopts "vt:me" ARG ; do
 done
 
 if [ -z "$TESTSETS" ]; then
-    TESTSETS="libfdt utilfdt dtc dtbs_equal fdtget fdtput fdtdump"
+    TESTSETS="libfdt utilfdt dtc dtbs_equal fdtget fdtput fdtdump pylibfdt"
 fi
 
 # Make sure we don't have stale blobs lying around
@@ -816,6 +830,9 @@ for set in $TESTSETS; do
 	"fdtdump")
 	    fdtdump_tests
 	    ;;
+	"pylibfdt")
+	    pylibfdt_tests
+	    ;;
     esac
 done
 
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v4 3/5] Mention pylibfdt in the documentation

[Simon Glass]

Add a note about pylibfdt in the README.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2:
- Add details on how to obtain full help and code coverage

 README | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/README b/README
index f92008f..7191d1b 100644
--- a/README
+++ b/README
@@ -7,6 +7,39 @@ DTC and LIBFDT are maintained by:
 David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
 Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
 
+
+Python library
+--------------
+
+A Python library is also available. To build this you will need to install
+swig and Python development files. On Debian distributions:
+
+   sudo apt-get install swig python-dev
+
+The library provides an Fdt class which you can use like this:
+
+    fdt = _ReadFdt('test_tree1.dtb')
+    node = fdt.path_offset('/test-node')
+    prop = fdt.first_property_offset(node)
+    print 'Property name: %s' % fdt.string(prop.nameoff)
+    print 'Property data: %s' % fdt.data(prop.nameoff)
+
+You will find tests in tests/pylibfdt_tests.py showing how to use each
+method. Help is available using the Python help command, e.g.:
+
+    $ cd pylibfdt
+    $ python -c "import libfdt; help(libfdt)"
+
+If you add new features, please check code coverage:
+
+    $ sudo apt-get install python-pip python-pytest
+    $ sudo pip install coverage
+    $ cd tests
+    $ coverage run pylibfdt_tests.py
+    $ coverage html
+    # Open 'htmlcov/index.html' in your browser
+
+
 Mailing list
 ------------
 The following list is for discussion about dtc and libfdt implementation
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v4 4/5] Adjust libfdt.h to work with swig

[Simon Glass]

There are a few places where libfdt.h cannot be used as is with swig:

- macros like fdt_totalsize() have to be defined as C declarations
- fdt_offset_ptr() and fdt_getprop_namelen() need special treatment due to
    a TODO in the wrapper for fdt_getprop()

The second one can hopefully be resolved by someone with more knowledge of
SWIG than me.

Add #ifdefs to work around this problem.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v4:
- Add new patch to adjust libfdt.h to work with swig

Changes in v3: None
Changes in v2: None

 libfdt/libfdt.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index c69e918..2e78754 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -143,7 +143,9 @@
 /* Low-level functions (you probably don't need these)                */
 /**********************************************************************/
 
+#ifndef SWIG /* Use a special rule in libfdt.swig instead */
 const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
+#endif
 static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
 {
 	return (void *)(uintptr_t)fdt_offset_ptr(fdt, offset, checklen);
@@ -210,7 +212,7 @@ int fdt_next_subnode(const void *fdt, int offset);
 /**********************************************************************/
 /* General functions                                                  */
 /**********************************************************************/
-
+#ifndef SWIG /* Repeated in libfdt.swig (we cannot use macros) */
 #define fdt_get_header(fdt, field) \
 	(fdt32_to_cpu(((const struct fdt_header *)(fdt))->field))
 #define fdt_magic(fdt)			(fdt_get_header(fdt, magic))
@@ -223,6 +225,7 @@ int fdt_next_subnode(const void *fdt, int offset);
 #define fdt_boot_cpuid_phys(fdt)	(fdt_get_header(fdt, boot_cpuid_phys))
 #define fdt_size_dt_strings(fdt)	(fdt_get_header(fdt, size_dt_strings))
 #define fdt_size_dt_struct(fdt)		(fdt_get_header(fdt, size_dt_struct))
+#endif /* SWIG */
 
 #define __fdt_set_hdr(name) \
 	static inline void fdt_set_##name(void *fdt, uint32_t val) \
@@ -638,8 +641,10 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
  * Identical to fdt_getprop(), but only examine the first namelen
  * characters of name for matching the property name.
  */
+#ifndef SWIG /* Use a special rule in libfdt.swig instead */
 const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 				const char *name, int namelen, int *lenp);
+#endif
 static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
 					  const char *name, int namelen,
 					  int *lenp)
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v4 5/5] Build pylibfdt as part of the normal build process

[Simon Glass]

Possible this needs to be made optional. For now just hook it up.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 Makefile | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index ce05eba..c516d9f 100644
--- a/Makefile
+++ b/Makefile
@@ -115,7 +115,7 @@ BIN += fdtput
 
 SCRIPTS = dtdiff
 
-all: $(BIN) libfdt
+all: $(BIN) libfdt pylibfdt
 
 
 ifneq ($(DEPTARGETS),)
@@ -202,6 +202,19 @@ dist:
 	cat ../dtc-$(dtc_version).tar | \
 		gzip -9 > ../dtc-$(dtc_version).tar.gz
 
+
+#
+# Rules for pylibfdt
+#
+PYLIBFDT_srcdir = pylibfdt
+PYLIBFDT_objdir = pylibfdt
+
+include $(PYLIBFDT_srcdir)/Makefile.pylibfdt
+
+.PHONY: pylibfdt
+pylibfdt: $(PYLIBFDT_objdir)/_libfdt.so
+
+
 #
 # Release signing and uploading
 # This is for maintainer convenience, don't try this at home.
@@ -246,6 +259,7 @@ STD_CLEANFILES = *~ *.o *.$(SHAREDLIB_EXT) *.d *.a *.i *.s core a.out vgcore.* \
 clean: libfdt_clean tests_clean
 	@$(VECHO) CLEAN
 	rm -f $(STD_CLEANFILES)
+	rm -f $(PYLIBFDT_CLEANFILES)
 	rm -f $(VERSION_FILE)
 	rm -f $(BIN)
 	rm -f dtc-*.tar dtc-*.tar.sign dtc-*.tar.asc
-- 
2.11.0.483.g087da7b7c-goog

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 24687 bytes --]

On Sun, Feb 05, 2017 at 01:13:19PM -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 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
> 
>  pylibfdt/.gitignore        |   3 +
>  pylibfdt/Makefile.pylibfdt |  18 ++
>  pylibfdt/libfdt.swig       | 565 +++++++++++++++++++++++++++++++++++++++++++++
>  pylibfdt/setup.py          |  34 +++
>  4 files changed, 620 insertions(+)
>  create mode 100644 pylibfdt/.gitignore
>  create mode 100644 pylibfdt/Makefile.pylibfdt
>  create mode 100644 pylibfdt/libfdt.swig
>  create mode 100644 pylibfdt/setup.py
> 
> diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
> new file mode 100644
> index 0000000..5e8c5e3
> --- /dev/null
> +++ b/pylibfdt/.gitignore
> @@ -0,0 +1,3 @@
> +libfdt.py
> +libfdt.pyc
> +libfdt_wrap.c
> diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> new file mode 100644
> index 0000000..fa74dd2
> --- /dev/null
> +++ b/pylibfdt/Makefile.pylibfdt
> @@ -0,0 +1,18 @@
> +# Makefile.pylibfdt
> +#
> +
> +PYLIBFDT_srcs = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_SRCS))
> +WRAP = $(PYLIBFDT_objdir)/libfdt_wrap.c
> +PYMODULE = $(PYLIBFDT_objdir)/_libfdt.so
> +
> +$(PYMODULE): $(PYLIBFDT_srcs) $(WRAP)
> +	@$(VECHO) PYMOD $@
> +	python $(PYLIBFDT_objdir)/setup.py "$(CPPFLAGS)" $^
> +	mv _libfdt.so $(PYMODULE)
> +
> +$(WRAP): $(PYLIBFDT_srcdir)/libfdt.swig
> +	@$(VECHO) SWIG $@
> +	swig -python -o $@ $<
> +
> +PYLIBFDT_cleanfiles = libfdt_wrap.c libfdt.py libfdt.pyc
> +PYLIBFDT_CLEANFILES = $(addprefix $(PYLIBFDT_objdir)/,$(PYLIBFDT_cleanfiles))
> diff --git a/pylibfdt/libfdt.swig b/pylibfdt/libfdt.swig
> new file mode 100644
> index 0000000..ac478d5
> --- /dev/null
> +++ b/pylibfdt/libfdt.swig
> @@ -0,0 +1,565 @@
> +/*
> + * pylibfdt - Flat Device Tree manipulation in Python
> + * Copyright (C) 2017 Google, Inc.
> + * Written by Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> + *
> + * libfdt is dual licensed: you can use it either under the terms of
> + * the GPL, or the BSD license, at your option.
> + *
> + *  a) This library is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2 of the
> + *     License, or (at your option) any later version.
> + *
> + *     This library is distributed in the hope that it will be useful,
> + *     but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *     GNU General Public License for more details.
> + *
> + *     You should have received a copy of the GNU General Public
> + *     License along with this library; if not, write to the Free
> + *     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
> + *     MA 02110-1301 USA
> + *
> + * Alternatively,
> + *
> + *  b) Redistribution and use in source and binary forms, with or
> + *     without modification, are permitted provided that the following
> + *     conditions are met:
> + *
> + *     1. Redistributions of source code must retain the above
> + *        copyright notice, this list of conditions and the following
> + *        disclaimer.
> + *     2. Redistributions in binary form must reproduce the above
> + *        copyright notice, this list of conditions and the following
> + *        disclaimer in the documentation and/or other materials
> + *        provided with the distribution.
> + *
> + *     THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> + *     CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
> + *     INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> + *     MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + *     DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> + *     CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *     SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + *     NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + *     LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + *     HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + *     CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> + *     OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
> + *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +%module libfdt
> +
> +%{
> +#define SWIG_FILE_WITH_INIT
> +#include "libfdt.h"
> +%}
> +
> +%pythoncode %{
> +
> +import struct
> +
> +# 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)
> +
> +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
> +
> +    This is an internal function only.
> +
> +    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))
> +    pylibfdt_copy_data(buf, prop)
> +    return buf

AFAICT this data() function (and the C functions it uses) aren't
actually used any more.  I think you can remove them.

> +
> +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 < 0
> +    """
> +    if val < 0:
> +        if not quiet or val != -NOTFOUND:

I don't really like hardcoding the specialness of NOTFOUND here -
depending on the call, there could be different error codes which are
relatively harmless.

Instead of a quiet boolean, you could pass in a set (list, sequence,
whatever) of error codes to be considered quiet.

> +            raise FdtException(val)
> +    return val
> +
> +def check_err_null(val, quiet=False):
> +    """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 a list, None if not
> +
> +    Raises
> +        FdtException if quiet is False and val indicates an error was
> +           reported. If quiet if True then an FdtException is raised only if
> +           the error is something other than -NOTFOUND.
> +    """
> +    # Normally a tuple is returned which contains the data and its length.
> +    # If we get just an integer error code, it means the function failed.
> +    if not isinstance(val, list):
> +        if not quiet or val != -NOTFOUND:
> +            raise FdtException(val)
> +        return None,
> +    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())

I noticed when playing with this that doing Fdt("filename.dtb") will
appear to succeed, but then (of course) fail with BADMAGIC as soon as
you do anything.  Might be an idea to at least do a magic number check
in the constructor.

> +
> +    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 an 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.
> +    """
> +    def __init__(self, data):
> +        self._fdt = bytearray(data)
> +
> +    def string(self, offset):
> +        """Get a string given its offset
> +
> +        This is an internal function.
> +
> +        Args:
> +            offset: FDT offset in big-endian format
> +
> +        Returns:
> +            string value at that offset
> +        """
> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> +
> +    def path_offset(self, path):
> +        """Get the offset for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +
> +        Returns:
> +            Node offset
> +
> +        Raises
> +            FdtException if the path is not valid
> +        """
> +        return check_err(fdt_path_offset(self._fdt, path))
> +
> +    def path_offset_quiet(self, path):
> +        """Get the offset for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +
> +        Returns:
> +            Node offset, or -NOTFOUND if the path is not value
> +
> +        Raises
> +            FdtException if any error occurs other than NOTFOUND
> +        """
> +        return check_err(fdt_path_offset(self._fdt, path), True)

Ugh.  I don't much like the idea of making quiet versions of a bunch
of entry points.  I think we need to pick one:
    a) Just return error codes (not very Pythonic, but you can't have
       everything)
    b) Always raise exceptions, and the user has to try: except: to
       quiet then (creating different error classes for the different
       codes could make this easier).

> +    def first_property_offset(self, nodeoffset):
> +        """Get the offset of the first property in a node offset
> +
> +        Args:
> +            nodeoffset: Offset to the node to check
> +
> +        Returns:
> +            Offset of the first property
> +
> +        Raises
> +            FdtException if the associated node has no properties, or some
> +                other error occurred
> +        """
> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset))
> +
> +    def first_property_offset_quiet(self, nodeoffset):
> +        """Get the offset of the first property in a node offset
> +
> +        Args:
> +            nodeoffset: Offset to the node to check
> +
> +        Returns:
> +            Offset of the first property, or -NOTFOUND if the node has no
> +                properties
> +
> +        Raises
> +            FdtException if any other error occurs
> +        """
> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset), True)
> +
> +    def next_property_offset(self, prop_offset):
> +        """Get the next property in a node
> +
> +        Args:
> +            prop_offset: Offset of the previous property
> +
> +        Returns:
> +            Offset of the next property
> +
> +        Raises:
> +            FdtException if the associated node has no more properties, or
> +                some other error occurred
> +        """
> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset))
> +
> +    def next_property_offset_quiet(self, prop_offset):
> +        """Get the next property in a node
> +
> +        Args:
> +            prop_offset: Offset of the previous property
> +
> +        Returns:
> +            Offset ot the next property, or -NOTFOUND if there are no more
> +            properties
> +
> +        Raises:
> +            FdtException if any other error occurs
> +        """
> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset), True)
> +
> +    def get_name(self, nodeoffset):
> +        """Get the name of a node
> +
> +        Args:
> +            nodeoffset: Offset of node to check
> +
> +        Returns:
> +            Node name
> +
> +        Raises:
> +            FdtException on error (e.g. nodeoffset is invalid)
> +        """
> +        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
> +
> +    def get_property_by_offset(self, prop_offset):
> +        """Obtains a property that can be examined
> +
> +        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property object instead,
> +        with the data extracted from the Fdt.

AFAICT you're already copying the property value out of the fdt as a
bytestring / bytearray.  I don't see what a property object would
encode apart from that bytestring.

> +        Args:
> +            prop_offset: Offset of property (e.g. from first_property_offset())
> +
> +        Returns:
> +            Property object with members:
> +                tag: Big-endian device tree tag value
> +                len: Big-endian property length
> +                nameoff: Big-endian string offset for use with string()
> +
> +            Use data() on the return value to obtain the property value.
> +
> +        Raises:
> +            FdtException on error (e.g. invalid prop_offset or device
> +            tree format)
> +        """
> +        return check_err_null(fdt_get_property_by_offset(self._fdt,
> +                                                         prop_offset))[0]
> +
> +    def first_subnode(self, nodeoffset):
> +        """Find the first subnode of a parent node
> +
> +        Args:
> +            nodeoffset: Node offset of parent node
> +
> +        Returns:
> +            The offset of the first subnode, if any
> +
> +        Raises:
> +            FdtException if no subnode found or other error occurs
> +        """
> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
> +
> +    def first_subnode_quiet(self, nodeoffset):
> +        """Find the first subnode of a node
> +
> +        Args:
> +            nodeoffset: Node offset of parent node
> +
> +        Returns:
> +            The offset of the first subnode, or -NOTFOUND if none
> +
> +        Raises:
> +            FdtException on error (e.g. invalid nodeoffset)
> +        """
> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
> +
> +    def next_subnode(self, nodeoffset):
> +        """Find the next subnode
> +
> +        Args:
> +            nodeoffset: Node offset of previous subnode
> +
> +        Returns:
> +            The offset of the next subnode, if any
> +
> +        Raises:
> +            FdtException if no more subnode found or other error occurs
> +        """
> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
> +
> +    def next_subnode_quiet(self, nodeoffset):
> +        """Find the next subnode
> +
> +        Args:
> +            nodeoffset: Node offset of previous subnode
> +
> +        Returns:
> +            The offset of the next subnode, or -NOTFOUND if none
> +
> +        Raises:
> +            FdtException on error (e.g. invalid nodeoffset)
> +        """
> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
> +
> +    def totalsize(self):
> +        """Return the total size of the device tree
> +
> +        Returns:
> +            Total tree size in bytes
> +
> +        Raises:
> +            FdtException if any error occurs
> +        """
> +        return check_err(fdt_totalsize(self._fdt))
> +
> +    def off_dt_struct(self):
> +        """Return the start of the device tree struct area
> +
> +        Returns:
> +            Start offset of struct area
> +
> +        Raises:
> +            FdtException if any error occurs
> +        """
> +        return check_err(fdt_off_dt_struct(self._fdt))
> +
> +    def pack(self):
> +        """Pack the device tree to remove unused space
> +
> +        This adjusts the tree in place.
> +
> +        Raises:
> +            FdtException if any error occurs
> +        """
> +        return check_err(fdt_pack(self._fdt))
> +
> +    def delprop(self, nodeoffset, prop_name):
> +        """Delete a property from a node
> +
> +        Args:
> +            nodeoffset: Node offset containing property to delete
> +            prop_name: Name of property to delete
> +
> +        Raises:
> +            FdtError if the property does not exist, or another error occurs
> +        """
> +        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
> +
> +    def getprop(self, nodeoffset, prop_name):
> +        """Get a property from a node
> +
> +        Args:
> +            nodeoffset: Node offset containing property to get
> +            prop_name: Name of property to get
> +
> +        Returns:
> +            Value of property as a string
> +
> +        Raises:
> +            FdtError if any error occurs (e.g. the property is not found)
> +        """
> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name))[0]
> +
> +    def getprop_quiet(self, nodeoffset, prop_name):
> +        """Get a property from a node
> +
> +        Args:
> +            nodeoffset: Node offset containing property to get
> +            prop_name: Name of property to get
> +
> +        Returns:
> +            Value of property as a string, or None if not found
> +
> +        Raises:
> +            FdtError if an error occurs (e.g. nodeoffset is invalid)
> +        """
> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
> +                              True)[0]


There are a bunch of missing things here.  e.g. fdt_subnode_offset(),
off_dt_strings(), fdt_parent_offset(), ...

> +%}
> +
> +%rename(fdt_property) fdt_property_func;
> +
> +typedef int fdt32_t;
> +
> +%include "libfdt/fdt.h"
> +
> +%include "typemaps.i"
> +
> +/*
> + * Unfortunately the defintiion of pybuffer_mutable_binary() in my Python
> + * version appears to be broken:
> + * pylibfdt/libfdt_wrap.c: In function ‘_wrap_pylibfdt_copy_data’:
> + * pylibfdt/libfdt_wrap.c:3603:22: error: ‘size’ undeclared (first use in this
> + * function)
> + *   arg2 = (size_t) (size/sizeof(char));
> + *
> + * This version works correctly.
> + */
> +%define %mypybuffer_mutable_binary(TYPEMAP, SIZE)
> +%typemap(in) (TYPEMAP, SIZE)(int res, Py_ssize_t size = 0, void *buf = 0)
> +{
> +	res = PyObject_AsWriteBuffer($input, &buf, &size);
> +	if (res < 0) {
> +		PyErr_Clear();
> +		%argument_fail(res, "(TYPEMAP, SIZE)", $symname, $argnum);
> +	}
> +	$1 = ($1_ltype)buf;
> +	$2 = ($2_ltype)(size1 / sizeof($*1_type));
> +}
> +%enddef
> +
> +/* This is used to copy property data into a bytearray */
> +%mypybuffer_mutable_binary(char *str, size_t size);
> +void pylibfdt_copy_data(char *str, size_t size,
> +			const struct fdt_property *prop);
> +
> +/* Most functions don't change the device tree, so use a const void * */
> +%typemap(in) (const void *) {
> +	if (!PyByteArray_Check($input)) {
> +		SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
> +			"', argument " "$argnum"" of type '" "$type""'");
> +	}
> +	$1 = (void *)PyByteArray_AsString($input);
> +}
> +
> +/* Some functions do change the device tree, so use void * */
> +%typemap(in) (void *) {
> +	if (!PyByteArray_Check($input)) {
> +		SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
> +			"', argument " "$argnum"" of type '" "$type""'");
> +	}
> +	$1 = PyByteArray_AsString($input);
> +}
> +
> +%inline %{
> +
> +/**
> + * pylibfdt_copy_data() - Copy data from a property to the given buffer
> + *
> + * This is used by the data() function 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_data(char *buf, size_t size, const struct fdt_property *prop)
> +{
> +	memcpy(buf, prop + 1, size);
> +}
> +
> +%}
> +
> +%apply int *OUTPUT { int *lenp };
> +
> +/* Handle a few special cases which conflict with the typemap below */
> +const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
> +const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
> +				const char *name, int namelen, int *lenp);
> +
> +/* typemap used for fdt_getprop() */
> +%typemap(out) (const void *) {
> +	if (!$1)
> +		$result = Py_None;
> +	else
> +		/* TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Can we avoid the 'arg4'? */
> +		$result = Py_BuildValue("s#", $1, *arg4);
> +}
> +
> +/* We have both struct fdt_property and a function fdt_property() */
> +%warnfilter(302) fdt_property;
> +
> +%include <../libfdt/libfdt.h>
> +
> +/* These are macros in the header so have to be redefined here */
> +int fdt_magic(const void *fdt);
> +int fdt_totalsize(const void *fdt);
> +int fdt_off_dt_struct(const void *fdt);
> +int fdt_off_dt_strings(const void *fdt);
> +int fdt_off_mem_rsvmap(const void *fdt);
> +int fdt_version(const void *fdt);
> +int fdt_last_comp_version(const void *fdt);
> +int fdt_boot_cpuid_phys(const void *fdt);
> +int fdt_size_dt_strings(const void *fdt);
> +int fdt_size_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",
> +       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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 2/5] Add tests for pylibfdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 15393 bytes --]

On Sun, Feb 05, 2017 at 01:13:20PM -0700, Simon Glass wrote:
> Add a set of tests to cover the functionality in pylibfdt.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

I'd suggest as a base writing Python-equivalent tests for everything
in tree1_tests().

> ---
> 
> Changes in v4:
> - Drop tests that are no-longer applicable
> - Add a get for getprop()
> 
> Changes in v3:
> - Add some more tests
> 
> Changes in v2:
> - Update tests for new pylibfdt
> - Add a few more tests to increase coverage
> 
>  tests/pylibfdt_tests.py | 275 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/run_tests.sh      |  19 +++-
>  2 files changed, 293 insertions(+), 1 deletion(-)
>  create mode 100644 tests/pylibfdt_tests.py
> 
> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> new file mode 100644
> index 0000000..66eb45b
> --- /dev/null
> +++ b/tests/pylibfdt_tests.py
> @@ -0,0 +1,275 @@
> +# pylibfdt - Tests for Flat Device Tree manipulation in Python
> +# Copyright (C) 2017 Google, Inc.
> +# Written by Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> +#
> +# libfdt is dual licensed: you can use it either under the terms of
> +# the GPL, or the BSD license, at your option.
> +#
> +#  a) This library is free software; you can redistribute it and/or
> +#     modify it under the terms of the GNU General Public License as
> +#     published by the Free Software Foundation; either version 2 of the
> +#     License, or (at your option) any later version.
> +#
> +#     This library is distributed in the hope that it will be useful,
> +#     but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +#     GNU General Public License for more details.
> +#
> +#     You should have received a copy of the GNU General Public
> +#     License along with this library; if not, write to the Free
> +#     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
> +#     MA 02110-1301 USA
> +#
> +# Alternatively,
> +#
> +#  b) Redistribution and use in source and binary forms, with or
> +#     without modification, are permitted provided that the following
> +#     conditions are met:
> +#
> +#     1. Redistributions of source code must retain the above
> +#        copyright notice, this list of conditions and the following
> +#        disclaimer.
> +#     2. Redistributions in binary form must reproduce the above
> +#        copyright notice, this list of conditions and the following
> +#        disclaimer in the documentation and/or other materials
> +#        provided with the distribution.
> +#
> +#     THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> +#     CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
> +#     INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> +#     MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +#     DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> +#     CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +#     SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> +#     NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +#     LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> +#     HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> +#     CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> +#     OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
> +#     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +#
> +
> +import sys
> +import unittest
> +
> +sys.path.append('../pylibfdt')
> +import libfdt
> +from libfdt import FdtException
> +
> +# Offsets of properties in the root node
> +ROOT_PROPS = (8, 32, 48, 68, 92, 108)
> +
> +def get_err(err_code):
> +    """Convert an error code into an error message
> +
> +    Args:
> +        err_code: Error code value (FDT_ERR_...)
> +
> +    Returns:
> +        String error code
> +    """
> +    return 'pylibfdt error %d: %s' % (-err_code, libfdt.fdt_strerror(-err_code))
> +
> +def _ReadFdt(fname):
> +    """Read a device tree file into an Fdt object, ready for use
> +
> +    Args:
> +        fname: Filename to read from
> +
> +    Returns:
> +        Fdt bytearray suitable for passing to libfdt functions
> +    """
> +    return libfdt.Fdt(open(fname).read())
> +
> +class PyLibfdtTests(unittest.TestCase):
> +    """Test class for pylibfdt
> +
> +    Properties:
> +        fdt: Device tree file used for testing
> +    """
> +
> +    def setUp(self):
> +        """Read in the device tree we use for testing"""
> +        self.fdt = _ReadFdt('test_tree1.dtb')
> +
> +    def GetPropList(self, node_path):
> +        """Read a list of properties from a node
> +
> +        Args:
> +            node_path: Full path to node, e.g. '/subnode@1/subsubnode'
> +
> +        Returns:
> +            List of property names for that node, e.g. ['compatible', 'reg']
> +        """
> +        prop_list = []
> +        node = self.fdt.path_offset(node_path)
> +        poffset = self.fdt.first_property_offset_quiet(node)
> +        while poffset > 0:
> +            pdata = self.fdt.get_property_by_offset(poffset)
> +            prop_list.append(self.fdt.string(pdata.nameoff))
> +            poffset = self.fdt.next_property_offset_quiet(poffset)
> +        return prop_list
> +
> +    def testImport(self):
> +        """Check that we can import the library correctly"""
> +        self.assertEquals(type(libfdt), type(sys))
> +
> +    def testPathOffset(self):
> +        """Check that we can find the offset of a node"""
> +        self.assertEquals(self.fdt.path_offset('/'), 0)
> +        self.assertEquals(self.fdt.path_offset('/subnode@1'), 124)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.path_offset('/wibble')
> +        self.assertEquals(self.fdt.path_offset_quiet('/wibble'),
> +                          -libfdt.NOTFOUND)
> +
> +    def testPropertyOffset(self):
> +        """Walk through all the properties in the root node"""
> +        self.assertEquals(self.fdt.first_property_offset(0), ROOT_PROPS[0])
> +        for pos in range(len(ROOT_PROPS) - 1):
> +            self.assertEquals(self.fdt.next_property_offset(ROOT_PROPS[pos]),
> +                              ROOT_PROPS[pos + 1])
> +        self.assertEquals(self.fdt.next_property_offset_quiet(ROOT_PROPS[-1]),
> +                          -libfdt.NOTFOUND)
> +
> +    def testPropertyOffsetExceptions(self):
> +        """Check that exceptions are raised as expected"""
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.next_property_offset(108)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.next_property_offset(107)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.first_property_offset_quiet(107)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.next_property_offset_quiet(107)
> +
> +        node = self.fdt.path_offset('/subnode@1/ss1')
> +        self.assertEquals(self.fdt.first_property_offset_quiet(node),
> +                          -libfdt.NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.first_property_offset(node)
> +
> +    def testGetName(self):
> +        """Check that we can get the name of a node"""
> +        self.assertEquals(self.fdt.get_name(0), '')
> +        node = self.fdt.path_offset('/subnode@1/subsubnode')
> +        self.assertEquals(self.fdt.get_name(node), 'subsubnode')
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.get_name(-2)
> +
> +    def testGetString(self):
> +        """Test that we can get a string from the string table"""
> +        node = self.fdt.path_offset('/subnode@2')
> +        poffset = self.fdt.first_property_offset(node)
> +        pdata = self.fdt.get_property_by_offset(poffset)
> +        self.assertEquals(self.fdt.string(pdata.nameoff), 'reg')
> +
> +    def testGetPropertyByOffset(self):
> +        """Check that we can read the name and contents of a property"""
> +        root = self.fdt.path_offset('/')
> +        poffset = self.fdt.first_property_offset(root)
> +        pdata = self.fdt.get_property_by_offset(poffset)
> +        self.assertEquals(libfdt.fdt32_to_cpu(pdata.tag), 3)
> +        self.assertEquals(libfdt.fdt32_to_cpu(pdata.len), 11)
> +        self.assertEquals(self.fdt.string(pdata.nameoff), 'compatible')
> +        self.assertEquals(libfdt.data(pdata), 'test_tree1\0')
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.get_property_by_offset(-2)
> +
> +    def testGetPropertyByOffset(self):
> +        """Check that we can read the details of a property"""
> +        root = self.fdt.path_offset('/')
> +        poffset = self.fdt.first_property_offset(root)
> +        prop = self.fdt.get_property_by_offset(poffset)
> +        self.assertEquals(self.fdt.string(prop.nameoff), 'compatible')
> +        self.assertEquals(libfdt.data(prop), 'test_tree1\0')
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.get_property_by_offset(-2)
> +
> +    def testGetProp(self):
> +        """Check that we can read the contents of a property by name"""
> +        root = self.fdt.path_offset('/')
> +        data = self.fdt.getprop(root, "compatible")
> +        self.assertEquals(data, 'test_tree1\0')
> +        self.assertEquals(None, self.fdt.getprop_quiet(root, 'missing'))
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.getprop(root, 'missing')
> +
> +        node = self.fdt.path_offset('/subnode@1/subsubnode')
> +        data = self.fdt.getprop(node, "compatible")
> +        self.assertEquals(data, 'subsubnode1\0subsubnode\0')
> +
> +    def testStrError(self):
> +        """Check that we can get an error string"""
> +        self.assertEquals(libfdt.strerror(-libfdt.NOTFOUND),
> +                          'FDT_ERR_NOTFOUND')
> +
> +    def testFirstNextSubnodeOffset(self):
> +        """Check that we can walk through subnodes"""
> +        node_list = []
> +        node = self.fdt.first_subnode_quiet(0)
> +        while node >= 0:
> +            node_list.append(self.fdt.get_name(node))
> +            node = self.fdt.next_subnode_quiet(node)
> +        self.assertEquals(node_list, ['subnode@1', 'subnode@2'])
> +
> +    def testFirstNextSubnodeOffsetExceptions(self):
> +        """Check except handling for first/next subnode functions"""
> +        node = self.fdt.path_offset_quiet('/subnode@1/subsubnode')
> +        self.assertEquals(self.fdt.first_subnode_quiet(node), -libfdt.NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.first_subnode(node)
> +
> +        node = self.fdt.path_offset_quiet('/subnode@1/ss1')
> +        self.assertEquals(self.fdt.next_subnode_quiet(node), -libfdt.NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.next_subnode(node)
> +
> +    def testDeleteProperty(self):
> +        """Test that we can delete a property"""
> +        node_name = '/subnode@1'
> +        self.assertEquals(self.GetPropList(node_name),
> +                          ['compatible', 'reg', 'prop-int'])
> +        node = self.fdt.path_offset('/%s' % node_name)
> +        self.assertEquals(self.fdt.delprop(node, 'reg'), 0)
> +        self.assertEquals(self.GetPropList(node_name),
> +                          ['compatible', 'prop-int'])
> +
> +    def testHeader(self):
> +        """Test that we can access the header values"""
> +        self.assertEquals(self.fdt.totalsize(), 693)
> +        self.assertEquals(self.fdt.off_dt_struct(), 88)
> +
> +    def testPack(self):
> +        """Test that we can pack the tree after deleting something"""
> +        self.assertEquals(self.fdt.totalsize(), 693)
> +        node = self.fdt.path_offset_quiet('/subnode@2')
> +        self.assertEquals(self.fdt.delprop(node, 'prop-int'), 0)
> +        self.assertEquals(self.fdt.totalsize(), 693)
> +        self.assertEquals(self.fdt.pack(), 0)
> +        self.assertEquals(self.fdt.totalsize(), 677)
> +
> +    def testBadPropertyOffset(self):
> +        """Test that bad property offsets are detected"""
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.get_property_by_offset(13)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.first_property_offset(3)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.next_property_offset(3)
> +
> +    def testBadPathOffset(self):
> +        """Test that bad path names are detected"""
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADPATH)):
> +            self.fdt.path_offset('not-present')
> +
> +    def testEndian(self):
> +        """Check that we can convert from FDT (big endian) to native endian"""
> +        self.assertEquals(libfdt.fdt32_to_cpu(0x10000000), 0x10)
> +
> +if __name__ == "__main__":
> +    unittest.main()
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index ed489db..144feb6 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -769,6 +769,20 @@ fdtdump_tests () {
>      run_fdtdump_test fdtdump.dts
>  }
>  
> +pylibfdt_tests () {
> +    TMP=/tmp/tests.stderr.$$
> +    python pylibfdt_tests.py 2> ${TMP}
> +    result=$(head -1 ${TMP} | awk \
> +        '{ for (i = 1; i <= length($0); i++) { \
> +            result = substr($0,i,1); fail = fail + (result == "F"); \
> +            ok = ok + (result == ".")}; } END { print fail,  ok, fail + ok}')
> +
> +    # Extract the test results and add them to our totals
> +    tot_fail=$((tot_fail + $(echo $result | cut -d" " -f 1)))
> +    tot_pass=$((tot_pass + $(echo $result | cut -d" " -f 2)))
> +    tot_tests=$((tot_tests + $(echo $result | cut -d" " -f 3)))
> +}
> +
>  while getopts "vt:me" ARG ; do
>      case $ARG in
>  	"v")
> @@ -787,7 +801,7 @@ while getopts "vt:me" ARG ; do
>  done
>  
>  if [ -z "$TESTSETS" ]; then
> -    TESTSETS="libfdt utilfdt dtc dtbs_equal fdtget fdtput fdtdump"
> +    TESTSETS="libfdt utilfdt dtc dtbs_equal fdtget fdtput fdtdump pylibfdt"
>  fi
>  
>  # Make sure we don't have stale blobs lying around
> @@ -816,6 +830,9 @@ for set in $TESTSETS; do
>  	"fdtdump")
>  	    fdtdump_tests
>  	    ;;
> +	"pylibfdt")
> +	    pylibfdt_tests
> +	    ;;
>      esac
>  done
>  

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 3/5] Mention pylibfdt in the documentation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

On Sun, Feb 05, 2017 at 01:13:21PM -0700, Simon Glass wrote:
> Add a note about pylibfdt in the README.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Add details on how to obtain full help and code coverage
> 
>  README | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/README b/README
> index f92008f..7191d1b 100644
> --- a/README
> +++ b/README
> @@ -7,6 +7,39 @@ DTC and LIBFDT are maintained by:
>  David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>  Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
>  
> +
> +Python library
> +--------------
> +
> +A Python library is also available. To build this you will need to install
> +swig and Python development files. On Debian distributions:
> +
> +   sudo apt-get install swig python-dev
> +
> +The library provides an Fdt class which you can use like this:
> +
> +    fdt = _ReadFdt('test_tree1.dtb')

This seems to be using the test wrapper _ReadFdt(0 rather than the fdt
class proper.

> +    node = fdt.path_offset('/test-node')
> +    prop = fdt.first_property_offset(node)
> +    print 'Property name: %s' % fdt.string(prop.nameoff)
> +    print 'Property data: %s' % fdt.data(prop.nameoff)

I think this is not quite up to date with the current version.

In addition, I think the example would be more informative if you
showed an interactive session, demonstrating how the offsets and
property values are encoded as Python ints and strings.


> +You will find tests in tests/pylibfdt_tests.py showing how to use each
> +method. Help is available using the Python help command, e.g.:
> +
> +    $ cd pylibfdt
> +    $ python -c "import libfdt; help(libfdt)"
> +
> +If you add new features, please check code coverage:
> +
> +    $ sudo apt-get install python-pip python-pytest
> +    $ sudo pip install coverage
> +    $ cd tests
> +    $ coverage run pylibfdt_tests.py
> +    $ coverage html
> +    # Open 'htmlcov/index.html' in your browser
> +
> +
>  Mailing list
>  ------------
>  The following list is for discussion about dtc and libfdt implementation

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 4/5] Adjust libfdt.h to work with swig

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3747 bytes --]

On Sun, Feb 05, 2017 at 01:13:22PM -0700, Simon Glass wrote:
> There are a few places where libfdt.h cannot be used as is with swig:
> 
> - macros like fdt_totalsize() have to be defined as C declarations
> - fdt_offset_ptr() and fdt_getprop_namelen() need special treatment due to
>     a TODO in the wrapper for fdt_getprop()

fdt_offset_ptr() is a very low-level function which should be rarely
needed outside libfdt internals, even in C.  I think it makes sense to
exclude it from the Python interface.

fdt_getprop_namelen() is primarily to make certain things easier in C
where copying and slicing up strings can be quite painful.  Since
that's trivial in Python, I don't think there's any need to expose the
namelen() functions in Python.

> 
> The second one can hopefully be resolved by someone with more knowledge of
> SWIG than me.
> 
> Add #ifdefs to work around this problem.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v4:
> - Add new patch to adjust libfdt.h to work with swig
> 
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/libfdt.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index c69e918..2e78754 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -143,7 +143,9 @@
>  /* Low-level functions (you probably don't need these)                */
>  /**********************************************************************/
>  
> +#ifndef SWIG /* Use a special rule in libfdt.swig instead */
>  const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
> +#endif
>  static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
>  {
>  	return (void *)(uintptr_t)fdt_offset_ptr(fdt, offset, checklen);
> @@ -210,7 +212,7 @@ int fdt_next_subnode(const void *fdt, int offset);
>  /**********************************************************************/
>  /* General functions                                                  */
>  /**********************************************************************/
> -
> +#ifndef SWIG /* Repeated in libfdt.swig (we cannot use macros) */

You need to duplicate them in libfdt.swig, certainly, but why do you
need to explicitly cut them out here?  Isn't swig smart enough to know
it can't do anything with these?

>  #define fdt_get_header(fdt, field) \
>  	(fdt32_to_cpu(((const struct fdt_header *)(fdt))->field))
>  #define fdt_magic(fdt)			(fdt_get_header(fdt, magic))
> @@ -223,6 +225,7 @@ int fdt_next_subnode(const void *fdt, int offset);
>  #define fdt_boot_cpuid_phys(fdt)	(fdt_get_header(fdt, boot_cpuid_phys))
>  #define fdt_size_dt_strings(fdt)	(fdt_get_header(fdt, size_dt_strings))
>  #define fdt_size_dt_struct(fdt)		(fdt_get_header(fdt, size_dt_struct))
> +#endif /* SWIG */
>  
>  #define __fdt_set_hdr(name) \
>  	static inline void fdt_set_##name(void *fdt, uint32_t val) \
> @@ -638,8 +641,10 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>   * Identical to fdt_getprop(), but only examine the first namelen
>   * characters of name for matching the property name.
>   */
> +#ifndef SWIG /* Use a special rule in libfdt.swig instead */
>  const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>  				const char *name, int namelen, int *lenp);
> +#endif
>  static inline void *fdt_getprop_namelen_w(void *fdt, int nodeoffset,
>  					  const char *name, int namelen,
>  					  int *lenp)

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

On Sun, Feb 05, 2017 at 01:13:19PM -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 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
> 
>  pylibfdt/.gitignore        |   3 +
>  pylibfdt/Makefile.pylibfdt |  18 ++
>  pylibfdt/libfdt.swig       | 565 +++++++++++++++++++++++++++++++++++++++++++++
>  pylibfdt/setup.py          |  34 +++
>  4 files changed, 620 insertions(+)
>  create mode 100644 pylibfdt/.gitignore
>  create mode 100644 pylibfdt/Makefile.pylibfdt
>  create mode 100644 pylibfdt/libfdt.swig
>  create mode 100644 pylibfdt/setup.py
> 
> diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
> new file mode 100644
> index 0000000..5e8c5e3
> --- /dev/null
> +++ b/pylibfdt/.gitignore
> @@ -0,0 +1,3 @@
> +libfdt.py
> +libfdt.pyc
> +libfdt_wrap.c
> diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> new file mode 100644
> index 0000000..fa74dd2
> --- /dev/null
> +++ b/pylibfdt/Makefile.pylibfdt
> @@ -0,0 +1,18 @@
> +# Makefile.pylibfdt
> +#
> +
> +PYLIBFDT_srcs = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_SRCS))
> +WRAP = $(PYLIBFDT_objdir)/libfdt_wrap.c
> +PYMODULE = $(PYLIBFDT_objdir)/_libfdt.so
> +
> +$(PYMODULE): $(PYLIBFDT_srcs) $(WRAP)
> +	@$(VECHO) PYMOD $@
> +	python $(PYLIBFDT_objdir)/setup.py "$(CPPFLAGS)" $^
> +	mv _libfdt.so $(PYMODULE)
> +
> +$(WRAP): $(PYLIBFDT_srcdir)/libfdt.swig
> +	@$(VECHO) SWIG $@
> +	swig -python -o $@ $<

Forgot to mention this before: please wrap this in a $(SWIG) make
variable.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 0/5] Introduce Python bindings for libfdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Sun, Feb 05, 2017 at 01:13:18PM -0700, Simon Glass wrote:
> At present libfdt consists of only a C implementation. Many scripts are
> written using Python so it useful to have Python bindings for libfdt.
> Apparently this has never been attempted before, or if so I cannot find a
> reference.
> 
> This series starts the process of adding this support, with just a
> bare-bones set of methods.
> 
> The v4 series provides binding that can be used like this:
> 
>     fdt = libfdt.Fdt(open(fname).read())
>     node = fdt.path_offset('/subnode@1')
>     print fdt.get_prop(node, 'compatible')
>     subnode = fdt.first_subnode_quiet(node)
>     while subnode > 0:
>         print fdt.get_name(subnode)
>         subnode = fdt.next_subnode_quiet(subnode)
> 
> This version does not include a class for properties. That can be added
> if it is felt to be useful, but I want to get some basic functionality
> agreed first. The get_property_by_offset() function would likely benefit
> from this.

Sorry I've taken so long to look at this, I've finally given it a
review.

Fwiw, I ran into someone at linux.conf.au who also seemed to be rather
interested in a Python libfdt wrapper.

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 0/5] Introduce Python bindings for libfdt

[Simon Glass]

Hi David,

On 9 February 2017 at 22:05, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Feb 05, 2017 at 01:13:18PM -0700, Simon Glass wrote:
>> At present libfdt consists of only a C implementation. Many scripts are
>> written using Python so it useful to have Python bindings for libfdt.
>> Apparently this has never been attempted before, or if so I cannot find a
>> reference.
>>
>> This series starts the process of adding this support, with just a
>> bare-bones set of methods.
>>
>> The v4 series provides binding that can be used like this:
>>
>>     fdt = libfdt.Fdt(open(fname).read())
>>     node = fdt.path_offset('/subnode@1')
>>     print fdt.get_prop(node, 'compatible')
>>     subnode = fdt.first_subnode_quiet(node)
>>     while subnode > 0:
>>         print fdt.get_name(subnode)
>>         subnode = fdt.next_subnode_quiet(subnode)
>>
>> This version does not include a class for properties. That can be added
>> if it is felt to be useful, but I want to get some basic functionality
>> agreed first. The get_property_by_offset() function would likely benefit
>> from this.
>
> Sorry I've taken so long to look at this, I've finally given it a
> review.

That's OK. Thanks for looking. I'll try to spin this quickly.

>
> Fwiw, I ran into someone at linux.conf.au who also seemed to be rather
> interested in a Python libfdt wrapper.

OK good - there was someone on the U-Boot list also.

Regards,
Simo

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[Simon Glass]

Hi David,

On 9 February 2017 at 21:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Feb 05, 2017 at 01:13:19PM -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 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
>>
>>  pylibfdt/.gitignore        |   3 +
>>  pylibfdt/Makefile.pylibfdt |  18 ++
>>  pylibfdt/libfdt.swig       | 565 +++++++++++++++++++++++++++++++++++++++++++++
>>  pylibfdt/setup.py          |  34 +++
>>  4 files changed, 620 insertions(+)
>>  create mode 100644 pylibfdt/.gitignore
>>  create mode 100644 pylibfdt/Makefile.pylibfdt
>>  create mode 100644 pylibfdt/libfdt.swig
>>  create mode 100644 pylibfdt/setup.py
>>
[..]

>> +def data(prop):
>> +    """Extract the data from a property
>> +
>> +    This is an internal function only.
>> +
>> +    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))
>> +    pylibfdt_copy_data(buf, prop)
>> +    return buf
>
> AFAICT this data() function (and the C functions it uses) aren't
> actually used any more.  I think you can remove them.

It is used in tests, and needs to be used to process the return value
from fdt_get_property_by_offset(). You ask later what we need from
that function other than the property contents. We need the property
name.

So we could create a Prop class (again) with the name and data in it,
and have get_property_by_offset return it. Of course this means that
scanning the tree requires allocating space for the property contents
all over again, when you use it or not, but this is Python.

>
>> +
>> +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 < 0
>> +    """
>> +    if val < 0:
>> +        if not quiet or val != -NOTFOUND:
>
> I don't really like hardcoding the specialness of NOTFOUND here -
> depending on the call, there could be different error codes which are
> relatively harmless.
>
> Instead of a quiet boolean, you could pass in a set (list, sequence,
> whatever) of error codes to be considered quiet.

OK I can do that, except that later you suggest that the whole idea is
wrong. See below.

>
>> +            raise FdtException(val)
>> +    return val
>> +
>> +def check_err_null(val, quiet=False):
>> +    """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 a list, None if not
>> +
>> +    Raises
>> +        FdtException if quiet is False and val indicates an error was
>> +           reported. If quiet if True then an FdtException is raised only if
>> +           the error is something other than -NOTFOUND.
>> +    """
>> +    # Normally a tuple is returned which contains the data and its length.
>> +    # If we get just an integer error code, it means the function failed.
>> +    if not isinstance(val, list):
>> +        if not quiet or val != -NOTFOUND:
>> +            raise FdtException(val)
>> +        return None,
>> +    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())
>
> I noticed when playing with this that doing Fdt("filename.dtb") will
> appear to succeed, but then (of course) fail with BADMAGIC as soon as
> you do anything.  Might be an idea to at least do a magic number check
> in the constructor.

OK I'll add that.

>
>> +
>> +    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 an 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.
>> +    """
>> +    def __init__(self, data):
>> +        self._fdt = bytearray(data)
>> +
>> +    def string(self, offset):
>> +        """Get a string given its offset
>> +
>> +        This is an internal function.
>> +
>> +        Args:
>> +            offset: FDT offset in big-endian format
>> +
>> +        Returns:
>> +            string value at that offset
>> +        """
>> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
>> +
>> +    def path_offset(self, path):
>> +        """Get the offset for a given path
>> +
>> +        Args:
>> +            path: Path to the required node, e.g. '/node@3/subnode@1'
>> +
>> +        Returns:
>> +            Node offset
>> +
>> +        Raises
>> +            FdtException if the path is not valid
>> +        """
>> +        return check_err(fdt_path_offset(self._fdt, path))
>> +
>> +    def path_offset_quiet(self, path):
>> +        """Get the offset for a given path
>> +
>> +        Args:
>> +            path: Path to the required node, e.g. '/node@3/subnode@1'
>> +
>> +        Returns:
>> +            Node offset, or -NOTFOUND if the path is not value
>> +
>> +        Raises
>> +            FdtException if any error occurs other than NOTFOUND
>> +        """
>> +        return check_err(fdt_path_offset(self._fdt, path), True)
>
> Ugh.  I don't much like the idea of making quiet versions of a bunch
> of entry points.  I think we need to pick one:
>     a) Just return error codes (not very Pythonic, but you can't have
>        everything)
>     b) Always raise exceptions, and the user has to try: except: to
>        quiet then (creating different error classes for the different
>        codes could make this easier).

I don't like it either, but on the other hand, you did ask for exceptions.

I'm happy to go back to a) if that suits you.

Re b) this is really ugly IMO. Exceptions shouldn't be used in normal
program flow - e.g. they can appear in logs and cause confusion. Also
the syntax is not great.

The reason for the 'quiet' function is that it handles 99% of the
cases where you get an error return from a libfdt function. Also it
means that you don't have to check return values most of the time, but
still catch 'bad' things like out of space, bad offset, etc. without
blundering onwards.

As I say I don't like having 'quiet' versions of the functions. There
is one other option I can think of, which is to add an additional
argument to each function like this:

   def path_offset(self, path, quiet=False):

Then you can add 'quiet=True' to avoid the exception. Then you use the
same function name, and most of the time you get exceptions when there
are problems, but it's easy to avoid when you know there is a chance
an error will be returned.

If we go that way, then you are probably going to ask what is so
special about -NOTFOUND.

So we could make quiet a list (or perhaps Set) of exceptions, like this:

def path_offset(self, path, quiet=None)   # or perhaps quiet=[]

and have callers do:

fdt.path_offset("/", quiet=[NOTFOUND])

and I suppose we could define a constant for [NOTFOUND] also.

That might be better all round?

>
>> +    def first_property_offset(self, nodeoffset):
>> +        """Get the offset of the first property in a node offset
>> +
>> +        Args:
>> +            nodeoffset: Offset to the node to check
>> +
>> +        Returns:
>> +            Offset of the first property
>> +
>> +        Raises
>> +            FdtException if the associated node has no properties, or some
>> +                other error occurred
>> +        """
>> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset))
>> +
>> +    def first_property_offset_quiet(self, nodeoffset):
>> +        """Get the offset of the first property in a node offset
>> +
>> +        Args:
>> +            nodeoffset: Offset to the node to check
>> +
>> +        Returns:
>> +            Offset of the first property, or -NOTFOUND if the node has no
>> +                properties
>> +
>> +        Raises
>> +            FdtException if any other error occurs
>> +        """
>> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset), True)
>> +
>> +    def next_property_offset(self, prop_offset):
>> +        """Get the next property in a node
>> +
>> +        Args:
>> +            prop_offset: Offset of the previous property
>> +
>> +        Returns:
>> +            Offset of the next property
>> +
>> +        Raises:
>> +            FdtException if the associated node has no more properties, or
>> +                some other error occurred
>> +        """
>> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset))
>> +
>> +    def next_property_offset_quiet(self, prop_offset):
>> +        """Get the next property in a node
>> +
>> +        Args:
>> +            prop_offset: Offset of the previous property
>> +
>> +        Returns:
>> +            Offset ot the next property, or -NOTFOUND if there are no more
>> +            properties
>> +
>> +        Raises:
>> +            FdtException if any other error occurs
>> +        """
>> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset), True)
>> +
>> +    def get_name(self, nodeoffset):
>> +        """Get the name of a node
>> +
>> +        Args:
>> +            nodeoffset: Offset of node to check
>> +
>> +        Returns:
>> +            Node name
>> +
>> +        Raises:
>> +            FdtException on error (e.g. nodeoffset is invalid)
>> +        """
>> +        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
>> +
>> +    def get_property_by_offset(self, prop_offset):
>> +        """Obtains a property that can be examined
>> +
>> +        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property object instead,
>> +        with the data extracted from the Fdt.
>
> AFAICT you're already copying the property value out of the fdt as a
> bytestring / bytearray.  I don't see what a property object would
> encode apart from that bytestring.

The property name.

[...]

>> +    def getprop_quiet(self, nodeoffset, prop_name):
>> +        """Get a property from a node
>> +
>> +        Args:
>> +            nodeoffset: Node offset containing property to get
>> +            prop_name: Name of property to get
>> +
>> +        Returns:
>> +            Value of property as a string, or None if not found
>> +
>> +        Raises:
>> +            FdtError if an error occurs (e.g. nodeoffset is invalid)
>> +        """
>> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
>> +                              True)[0]
>
>
> There are a bunch of missing things here.  e.g. fdt_subnode_offset(),
> off_dt_strings(), fdt_parent_offset(), ...

Yes, I've only made a start. I don't want to implement the entire
thing right away! I want to get something basic agreed and accepted.
Then we can build on it. Refactoring everything as we iterate towards
a good binding takes time, and the functions I have chosen are fairly
representative of common uses.

[..]

Regards,
Simon

Re: [PATCH v4 2/5] Add tests for pylibfdt

[Simon Glass]

Hi David,

On 9 February 2017 at 21:56, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Feb 05, 2017 at 01:13:20PM -0700, Simon Glass wrote:
>> Add a set of tests to cover the functionality in pylibfdt.
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> I'd suggest as a base writing Python-equivalent tests for everything
> in tree1_tests().
>

OK I'll go through and do that (for the functions I've chosen to
implement) once we have the swig interface set.

[..]

Regards,
Simon

Re: [PATCH v4 3/5] Mention pylibfdt in the documentation

[Simon Glass]

Hi David,

On 9 February 2017 at 21:58, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Feb 05, 2017 at 01:13:21PM -0700, Simon Glass wrote:
>> Add a note about pylibfdt in the README.
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2:
>> - Add details on how to obtain full help and code coverage
>>
>>  README | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/README b/README
>> index f92008f..7191d1b 100644
>> --- a/README
>> +++ b/README
>> @@ -7,6 +7,39 @@ DTC and LIBFDT are maintained by:
>>  David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>>  Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
>>
>> +
>> +Python library
>> +--------------
>> +
>> +A Python library is also available. To build this you will need to install
>> +swig and Python development files. On Debian distributions:
>> +
>> +   sudo apt-get install swig python-dev
>> +
>> +The library provides an Fdt class which you can use like this:
>> +
>> +    fdt = _ReadFdt('test_tree1.dtb')
>
> This seems to be using the test wrapper _ReadFdt(0 rather than the fdt
> class proper.
>
>> +    node = fdt.path_offset('/test-node')
>> +    prop = fdt.first_property_offset(node)
>> +    print 'Property name: %s' % fdt.string(prop.nameoff)
>> +    print 'Property data: %s' % fdt.data(prop.nameoff)
>
> I think this is not quite up to date with the current version.
>
> In addition, I think the example would be more informative if you
> showed an interactive session, demonstrating how the offsets and
> property values are encoded as Python ints and strings.

OK, sounds good. I will revisit it once the interface is nailed down.
I can also expand this with a few more examples.

Regards,
Simon

Re: [PATCH v4 4/5] Adjust libfdt.h to work with swig

[Simon Glass]

Hi David,

On 9 February 2017 at 22:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Feb 05, 2017 at 01:13:22PM -0700, Simon Glass wrote:
>> There are a few places where libfdt.h cannot be used as is with swig:
>>
>> - macros like fdt_totalsize() have to be defined as C declarations
>> - fdt_offset_ptr() and fdt_getprop_namelen() need special treatment due to
>>     a TODO in the wrapper for fdt_getprop()
>
> fdt_offset_ptr() is a very low-level function which should be rarely
> needed outside libfdt internals, even in C.  I think it makes sense to
> exclude it from the Python interface.

Sounds good.

>
> fdt_getprop_namelen() is primarily to make certain things easier in C
> where copying and slicing up strings can be quite painful.  Since
> that's trivial in Python, I don't think there's any need to expose the
> namelen() functions in Python.

Yes that's a good point, will drop it.

>
>>
>> The second one can hopefully be resolved by someone with more knowledge of
>> SWIG than me.
>>
>> Add #ifdefs to work around this problem.
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>
>> Changes in v4:
>> - Add new patch to adjust libfdt.h to work with swig
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  libfdt/libfdt.h | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
>> index c69e918..2e78754 100644
>> --- a/libfdt/libfdt.h
>> +++ b/libfdt/libfdt.h
>> @@ -143,7 +143,9 @@
>>  /* Low-level functions (you probably don't need these)                */
>>  /**********************************************************************/
>>
>> +#ifndef SWIG /* Use a special rule in libfdt.swig instead */
>>  const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
>> +#endif
>>  static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
>>  {
>>       return (void *)(uintptr_t)fdt_offset_ptr(fdt, offset, checklen);
>> @@ -210,7 +212,7 @@ int fdt_next_subnode(const void *fdt, int offset);
>>  /**********************************************************************/
>>  /* General functions                                                  */
>>  /**********************************************************************/
>> -
>> +#ifndef SWIG /* Repeated in libfdt.swig (we cannot use macros) */
>
> You need to duplicate them in libfdt.swig, certainly, but why do you
> need to explicitly cut them out here?  Isn't swig smart enough to know
> it can't do anything with these?

Because the #define stuffs up my declarations. But it's easy to put
the declarations first to avoid that problem, so I'll do that.

[..]

Regards,
Simon

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 15969 bytes --]

On Fri, Feb 10, 2017 at 11:39:21AM -0700, Simon Glass wrote:
> Hi David,
> 
> On 9 February 2017 at 21:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Sun, Feb 05, 2017 at 01:13:19PM -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 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
> >>
> >>  pylibfdt/.gitignore        |   3 +
> >>  pylibfdt/Makefile.pylibfdt |  18 ++
> >>  pylibfdt/libfdt.swig       | 565 +++++++++++++++++++++++++++++++++++++++++++++
> >>  pylibfdt/setup.py          |  34 +++
> >>  4 files changed, 620 insertions(+)
> >>  create mode 100644 pylibfdt/.gitignore
> >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> >>  create mode 100644 pylibfdt/libfdt.swig
> >>  create mode 100644 pylibfdt/setup.py
> >>
> [..]
> 
> >> +def data(prop):
> >> +    """Extract the data from a property
> >> +
> >> +    This is an internal function only.
> >> +
> >> +    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))
> >> +    pylibfdt_copy_data(buf, prop)
> >> +    return buf
> >
> > AFAICT this data() function (and the C functions it uses) aren't
> > actually used any more.  I think you can remove them.
> 
> It is used in tests, and needs to be used to process the return value
> from fdt_get_property_by_offset(). You ask later what we need from
> that function other than the property contents. We need the property
> name.

Ok.. so why can't you just return a tuple of two bytestrings?

> So we could create a Prop class (again) with the name and data in it,
> and have get_property_by_offset return it. Of course this means that
> scanning the tree requires allocating space for the property contents
> all over again, when you use it or not, but this is Python.

Yeah, I think trying to avoid lots of allocations when working in
Python is not really going to fly.

> 
> >
> >> +
> >> +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 < 0
> >> +    """
> >> +    if val < 0:
> >> +        if not quiet or val != -NOTFOUND:
> >
> > I don't really like hardcoding the specialness of NOTFOUND here -
> > depending on the call, there could be different error codes which are
> > relatively harmless.
> >
> > Instead of a quiet boolean, you could pass in a set (list, sequence,
> > whatever) of error codes to be considered quiet.
> 
> OK I can do that, except that later you suggest that the whole idea is
> wrong. See below.
> 
> >
> >> +            raise FdtException(val)
> >> +    return val
> >> +
> >> +def check_err_null(val, quiet=False):
> >> +    """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 a list, None if not
> >> +
> >> +    Raises
> >> +        FdtException if quiet is False and val indicates an error was
> >> +           reported. If quiet if True then an FdtException is raised only if
> >> +           the error is something other than -NOTFOUND.
> >> +    """
> >> +    # Normally a tuple is returned which contains the data and its length.
> >> +    # If we get just an integer error code, it means the function failed.
> >> +    if not isinstance(val, list):
> >> +        if not quiet or val != -NOTFOUND:
> >> +            raise FdtException(val)
> >> +        return None,
> >> +    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())
> >
> > I noticed when playing with this that doing Fdt("filename.dtb") will
> > appear to succeed, but then (of course) fail with BADMAGIC as soon as
> > you do anything.  Might be an idea to at least do a magic number check
> > in the constructor.
> 
> OK I'll add that.
> 
> >
> >> +
> >> +    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 an 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.
> >> +    """
> >> +    def __init__(self, data):
> >> +        self._fdt = bytearray(data)
> >> +
> >> +    def string(self, offset):
> >> +        """Get a string given its offset
> >> +
> >> +        This is an internal function.
> >> +
> >> +        Args:
> >> +            offset: FDT offset in big-endian format
> >> +
> >> +        Returns:
> >> +            string value at that offset
> >> +        """
> >> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> >> +
> >> +    def path_offset(self, path):
> >> +        """Get the offset for a given path
> >> +
> >> +        Args:
> >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> >> +
> >> +        Returns:
> >> +            Node offset
> >> +
> >> +        Raises
> >> +            FdtException if the path is not valid
> >> +        """
> >> +        return check_err(fdt_path_offset(self._fdt, path))
> >> +
> >> +    def path_offset_quiet(self, path):
> >> +        """Get the offset for a given path
> >> +
> >> +        Args:
> >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> >> +
> >> +        Returns:
> >> +            Node offset, or -NOTFOUND if the path is not value
> >> +
> >> +        Raises
> >> +            FdtException if any error occurs other than NOTFOUND
> >> +        """
> >> +        return check_err(fdt_path_offset(self._fdt, path), True)
> >
> > Ugh.  I don't much like the idea of making quiet versions of a bunch
> > of entry points.  I think we need to pick one:
> >     a) Just return error codes (not very Pythonic, but you can't have
> >        everything)
> >     b) Always raise exceptions, and the user has to try: except: to
> >        quiet then (creating different error classes for the different
> >        codes could make this easier).
> 
> I don't like it either, but on the other hand, you did ask for exceptions.
> 
> I'm happy to go back to a) if that suits you.
> 
> Re b) this is really ugly IMO. Exceptions shouldn't be used in normal
> program flow - e.g. they can appear in logs and cause confusion. Also
> the syntax is not great.

Yeah, I guess I agree.  Although Python does essentially use this
approach internally for iterators, it's mostly hidden by the syntactic
sugar.

> The reason for the 'quiet' function is that it handles 99% of the
> cases where you get an error return from a libfdt function. Also it
> means that you don't have to check return values most of the time, but
> still catch 'bad' things like out of space, bad offset, etc. without
> blundering onwards.
> 
> As I say I don't like having 'quiet' versions of the functions. There
> is one other option I can think of, which is to add an additional
> argument to each function like this:
> 
>    def path_offset(self, path, quiet=False):
> 
> Then you can add 'quiet=True' to avoid the exception. Then you use the
> same function name, and most of the time you get exceptions when there
> are problems, but it's easy to avoid when you know there is a chance
> an error will be returned.

> If we go that way, then you are probably going to ask what is so
> special about -NOTFOUND.
> 
> So we could make quiet a list (or perhaps Set) of exceptions, like this:
> 
> def path_offset(self, path, quiet=None)   # or perhaps quiet=[]
> 
> and have callers do:
> 
> fdt.path_offset("/", quiet=[NOTFOUND])
> 
> and I suppose we could define a constant for [NOTFOUND] also.

Yes, this looks better than the other alternatives I think - it's
basically an extension of what I suggested for check_err().  I think
you can exploit duck typing here to let the user use a list or set as
they please (basically just allow anything supporting the 'in'
operator).

> That might be better all round?

Yes, I think so.  The remaining question, of course, is whether to a)
have the quiet set default to nothing, or b) default to the "usually
quiet" errors (NOTFOUND for most functions).  (b) will be less verbose
for common use cases, but (a) reduces invisible magic which is
generally a good goal.

> >
> >> +    def first_property_offset(self, nodeoffset):
> >> +        """Get the offset of the first property in a node offset
> >> +
> >> +        Args:
> >> +            nodeoffset: Offset to the node to check
> >> +
> >> +        Returns:
> >> +            Offset of the first property
> >> +
> >> +        Raises
> >> +            FdtException if the associated node has no properties, or some
> >> +                other error occurred
> >> +        """
> >> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset))
> >> +
> >> +    def first_property_offset_quiet(self, nodeoffset):
> >> +        """Get the offset of the first property in a node offset
> >> +
> >> +        Args:
> >> +            nodeoffset: Offset to the node to check
> >> +
> >> +        Returns:
> >> +            Offset of the first property, or -NOTFOUND if the node has no
> >> +                properties
> >> +
> >> +        Raises
> >> +            FdtException if any other error occurs
> >> +        """
> >> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset), True)
> >> +
> >> +    def next_property_offset(self, prop_offset):
> >> +        """Get the next property in a node
> >> +
> >> +        Args:
> >> +            prop_offset: Offset of the previous property
> >> +
> >> +        Returns:
> >> +            Offset of the next property
> >> +
> >> +        Raises:
> >> +            FdtException if the associated node has no more properties, or
> >> +                some other error occurred
> >> +        """
> >> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset))
> >> +
> >> +    def next_property_offset_quiet(self, prop_offset):
> >> +        """Get the next property in a node
> >> +
> >> +        Args:
> >> +            prop_offset: Offset of the previous property
> >> +
> >> +        Returns:
> >> +            Offset ot the next property, or -NOTFOUND if there are no more
> >> +            properties
> >> +
> >> +        Raises:
> >> +            FdtException if any other error occurs
> >> +        """
> >> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset), True)
> >> +
> >> +    def get_name(self, nodeoffset):
> >> +        """Get the name of a node
> >> +
> >> +        Args:
> >> +            nodeoffset: Offset of node to check
> >> +
> >> +        Returns:
> >> +            Node name
> >> +
> >> +        Raises:
> >> +            FdtException on error (e.g. nodeoffset is invalid)
> >> +        """
> >> +        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
> >> +
> >> +    def get_property_by_offset(self, prop_offset):
> >> +        """Obtains a property that can be examined
> >> +
> >> +        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property object instead,
> >> +        with the data extracted from the Fdt.
> >
> > AFAICT you're already copying the property value out of the fdt as a
> > bytestring / bytearray.  I don't see what a property object would
> > encode apart from that bytestring.
> 
> The property name.

Hm, true.  Alright, how about this: reintroduce a Property object, and
also a version of the get_property() vs. getprop() variants which are
in the C library.  get_property() and get_property_by_offset() will
return Property objects which, to be clear, are *copies* of a property
name & value, not a handle to a property within the tree.  getprop()
will return just the value bytestring().

Property should probably be derived from collections.namedtuple, since
it is basically just a wrapper on a (name, value) tuple.

> 
> [...]
> 
> >> +    def getprop_quiet(self, nodeoffset, prop_name):
> >> +        """Get a property from a node
> >> +
> >> +        Args:
> >> +            nodeoffset: Node offset containing property to get
> >> +            prop_name: Name of property to get
> >> +
> >> +        Returns:
> >> +            Value of property as a string, or None if not found
> >> +
> >> +        Raises:
> >> +            FdtError if an error occurs (e.g. nodeoffset is invalid)
> >> +        """
> >> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
> >> +                              True)[0]
> >
> >
> > There are a bunch of missing things here.  e.g. fdt_subnode_offset(),
> > off_dt_strings(), fdt_parent_offset(), ...
> 
> Yes, I've only made a start. I don't want to implement the entire
> thing right away! I want to get something basic agreed and accepted.
> Then we can build on it. Refactoring everything as we iterate towards
> a good binding takes time, and the functions I have chosen are fairly
> representative of common uses.

Ok, fair enough.

> 
> [..]
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[Ulrich Langenbach]

Hi David, hi Simon,

Am Montag, 13. Februar 2017, 16:20:22 CET schrieb David Gibson:
> On Fri, Feb 10, 2017 at 11:39:21AM -0700, Simon Glass wrote:
> > Hi David,
> > 
> > On 9 February 2017 at 21:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 
wrote:
> > > On Sun, Feb 05, 2017 at 01:13:19PM -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 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
> > >> 
> > >>  pylibfdt/.gitignore        |   3 +
> > >>  pylibfdt/Makefile.pylibfdt |  18 ++
> > >>  pylibfdt/libfdt.swig       | 565
> > >>  +++++++++++++++++++++++++++++++++++++++++++++ pylibfdt/setup.py      
> > >>     |  34 +++
> > >>  4 files changed, 620 insertions(+)
> > >>  create mode 100644 pylibfdt/.gitignore
> > >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> > >>  create mode 100644 pylibfdt/libfdt.swig
> > >>  create mode 100644 pylibfdt/setup.py
> > 
> > [..]
> > 
> > >> +def data(prop):
> > >> +    """Extract the data from a property
> > >> +
> > >> +    This is an internal function only.
> > >> +
> > >> +    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))
> > >> +    pylibfdt_copy_data(buf, prop)
> > >> +    return buf
> > > 
> > > AFAICT this data() function (and the C functions it uses) aren't
> > > actually used any more.  I think you can remove them.
> > 
> > It is used in tests, and needs to be used to process the return value
> > from fdt_get_property_by_offset(). You ask later what we need from
> > that function other than the property contents. We need the property
> > name.
> 
> Ok.. so why can't you just return a tuple of two bytestrings?
> 
> > So we could create a Prop class (again) with the name and data in it,
> > and have get_property_by_offset return it. Of course this means that
> > scanning the tree requires allocating space for the property contents
> > all over again, when you use it or not, but this is Python.
> 
> Yeah, I think trying to avoid lots of allocations when working in
> Python is not really going to fly.
> 
> > >> +
> > >> +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 < 0
> > >> +    """
> > >> +    if val < 0:
> > > 
> > >> +        if not quiet or val != -NOTFOUND:
> > > I don't really like hardcoding the specialness of NOTFOUND here -
> > > depending on the call, there could be different error codes which are
> > > relatively harmless.
> > > 
> > > Instead of a quiet boolean, you could pass in a set (list, sequence,
> > > whatever) of error codes to be considered quiet.
> > 
> > OK I can do that, except that later you suggest that the whole idea is
> > wrong. See below.
> > 
> > >> +            raise FdtException(val)
> > >> +    return val
> > >> +
> > >> +def check_err_null(val, quiet=False):
> > >> +    """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 a list, None if not
> > >> +
> > >> +    Raises
> > >> +        FdtException if quiet is False and val indicates an error was
> > >> +           reported. If quiet if True then an FdtException is raised
> > >> only if +           the error is something other than -NOTFOUND.
> > >> +    """
> > >> +    # Normally a tuple is returned which contains the data and its
> > >> length.
> > >> +    # If we get just an integer error code, it means the function
> > >> failed.
> > >> +    if not isinstance(val, list):
> > >> +        if not quiet or val != -NOTFOUND:
> > >> +            raise FdtException(val)
> > >> +        return None,
> > >> +    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())
> > > 
> > > I noticed when playing with this that doing Fdt("filename.dtb") will
> > > appear to succeed, but then (of course) fail with BADMAGIC as soon as
> > > you do anything.  Might be an idea to at least do a magic number check
> > > in the constructor.
in general this will fail once we start thinking of creating files, e.g. due to 
rearchitecting a DT into multiple ones with includes or creating new DTs.
However, with taking r/w modes into account the check can be skipped when the 
file is opened for writing or deferred to a later read call as it is just 
now...

> > 
> > OK I'll add that.
> > 
> > >> +
> > >> +    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 an 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.
> > >> +    """
> > >> +    def __init__(self, data):
> > >> +        self._fdt = bytearray(data)
> > >> +
> > >> +    def string(self, offset):
> > >> +        """Get a string given its offset
> > >> +
> > >> +        This is an internal function.
> > >> +
> > >> +        Args:
> > >> +            offset: FDT offset in big-endian format
> > >> +
> > >> +        Returns:
> > >> +            string value at that offset
> > >> +        """
> > >> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> > >> +
> > >> +    def path_offset(self, path):
> > >> +        """Get the offset for a given path
> > >> +
> > >> +        Args:
> > >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> > >> +
> > >> +        Returns:
> > >> +            Node offset
> > >> +
> > >> +        Raises
> > >> +            FdtException if the path is not valid
> > >> +        """
> > >> +        return check_err(fdt_path_offset(self._fdt, path))
> > >> +
> > >> +    def path_offset_quiet(self, path):
> > >> +        """Get the offset for a given path
> > >> +
> > >> +        Args:
> > >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> > >> +
> > >> +        Returns:
> > >> +            Node offset, or -NOTFOUND if the path is not value
> > >> +
> > >> +        Raises
> > >> +            FdtException if any error occurs other than NOTFOUND
> > >> +        """
> > >> +        return check_err(fdt_path_offset(self._fdt, path), True)
> > > 
> > > Ugh.  I don't much like the idea of making quiet versions of a bunch
> > > 
> > > of entry points.  I think we need to pick one:
> > >     a) Just return error codes (not very Pythonic, but you can't have
> > >     
> > >        everything)
> > >     
> > >     b) Always raise exceptions, and the user has to try: except: to
> > >     
> > >        quiet then (creating different error classes for the different
> > >        codes could make this easier).
> > 
> > I don't like it either, but on the other hand, you did ask for exceptions.
> > 
> > I'm happy to go back to a) if that suits you.
> > 
> > Re b) this is really ugly IMO. Exceptions shouldn't be used in normal
> > program flow - e.g. they can appear in logs and cause confusion. Also
> > the syntax is not great.
> 
> Yeah, I guess I agree.  Although Python does essentially use this
> approach internally for iterators, it's mostly hidden by the syntactic
> sugar.
> 
> > The reason for the 'quiet' function is that it handles 99% of the
> > cases where you get an error return from a libfdt function. Also it
> > means that you don't have to check return values most of the time, but
> > still catch 'bad' things like out of space, bad offset, etc. without
> > blundering onwards.
> > 
> > As I say I don't like having 'quiet' versions of the functions. There
> > is one other option I can think of, which is to add an additional
> > 
> > argument to each function like this:
> >    def path_offset(self, path, quiet=False):
> > Then you can add 'quiet=True' to avoid the exception. Then you use the
> > same function name, and most of the time you get exceptions when there
> > are problems, but it's easy to avoid when you know there is a chance
> > an error will be returned.
> > 
> > If we go that way, then you are probably going to ask what is so
> > special about -NOTFOUND.
> > 
> > So we could make quiet a list (or perhaps Set) of exceptions, like this:
> > 
> > def path_offset(self, path, quiet=None)   # or perhaps quiet=[]
> > 
> > and have callers do:
> > 
> > fdt.path_offset("/", quiet=[NOTFOUND])
> > 
> > and I suppose we could define a constant for [NOTFOUND] also.
> 
> Yes, this looks better than the other alternatives I think - it's
> basically an extension of what I suggested for check_err().  I think
> you can exploit duck typing here to let the user use a list or set as
> they please (basically just allow anything supporting the 'in'
> operator).
> 
> > That might be better all round?
> 
> Yes, I think so.  The remaining question, of course, is whether to a)
> have the quiet set default to nothing, or b) default to the "usually
> quiet" errors (NOTFOUND for most functions).  (b) will be less verbose
> for common use cases, but (a) reduces invisible magic which is
> generally a good goal.
I like the idea of hiding the not so important errors. I would also throw in a 
little bit different approach to this by adding a property to the Fdt object 
used to control this. It can be configured by the user during instantiation or 
later on. This just keeps the interfaces of the single function(s) cleaner.
However I am not sure how this is usually handled, so that we could just 
follow already agreed best practices.

> > >> +    def first_property_offset(self, nodeoffset):
> > >> +        """Get the offset of the first property in a node offset
> > >> +
> > >> +        Args:
> > >> +            nodeoffset: Offset to the node to check
> > >> +
> > >> +        Returns:
> > >> +            Offset of the first property
> > >> +
> > >> +        Raises
> > >> +            FdtException if the associated node has no properties, or
> > >> some
> > >> +                other error occurred
> > >> +        """
> > >> +        return check_err(fdt_first_property_offset(self._fdt,
> > >> nodeoffset))
> > >> +
> > >> +    def first_property_offset_quiet(self, nodeoffset):
> > >> +        """Get the offset of the first property in a node offset
> > >> +
> > >> +        Args:
> > >> +            nodeoffset: Offset to the node to check
> > >> +
> > >> +        Returns:
> > >> +            Offset of the first property, or -NOTFOUND if the node has
> > >> no
> > >> +                properties
> > >> +
> > >> +        Raises
> > >> +            FdtException if any other error occurs
> > >> +        """
> > >> +        return check_err(fdt_first_property_offset(self._fdt,
> > >> nodeoffset), True) +
> > >> +    def next_property_offset(self, prop_offset):
> > >> +        """Get the next property in a node
> > >> +
> > >> +        Args:
> > >> +            prop_offset: Offset of the previous property
> > >> +
> > >> +        Returns:
> > >> +            Offset of the next property
> > >> +
> > >> +        Raises:
> > >> +            FdtException if the associated node has no more
> > >> properties, or
> > >> +                some other error occurred
> > >> +        """
> > >> +        return check_err(fdt_next_property_offset(self._fdt,
> > >> prop_offset))
> > >> +
> > >> +    def next_property_offset_quiet(self, prop_offset):
> > >> +        """Get the next property in a node
> > >> +
> > >> +        Args:
> > >> +            prop_offset: Offset of the previous property
> > >> +
> > >> +        Returns:
> > >> +            Offset ot the next property, or -NOTFOUND if there are no
> > >> more
> > >> +            properties
> > >> +
> > >> +        Raises:
> > >> +            FdtException if any other error occurs
> > >> +        """
> > >> +        return check_err(fdt_next_property_offset(self._fdt,
> > >> prop_offset), True) +
> > >> +    def get_name(self, nodeoffset):
> > >> +        """Get the name of a node
> > >> +
> > >> +        Args:
> > >> +            nodeoffset: Offset of node to check
> > >> +
> > >> +        Returns:
> > >> +            Node name
> > >> +
> > >> +        Raises:
> > >> +            FdtException on error (e.g. nodeoffset is invalid)
> > >> +        """
> > >> +        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
> > >> +
> > >> +    def get_property_by_offset(self, prop_offset):
> > >> +        """Obtains a property that can be examined
> > >> +
> > >> +        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property object
> > >> instead, +        with the data extracted from the Fdt.
> > > 
> > > AFAICT you're already copying the property value out of the fdt as a
> > > bytestring / bytearray.  I don't see what a property object would
> > > encode apart from that bytestring.
> > 
> > The property name.
> 
> Hm, true.  Alright, how about this: reintroduce a Property object, and
> also a version of the get_property() vs. getprop() variants which are
> in the C library.  get_property() and get_property_by_offset() will
> return Property objects which, to be clear, are *copies* of a property
> name & value, not a handle to a property within the tree.  getprop()
> will return just the value bytestring().
> 
> Property should probably be derived from collections.namedtuple, since
> it is basically just a wrapper on a (name, value) tuple.
> 
> > [...]
> > 
> > >> +    def getprop_quiet(self, nodeoffset, prop_name):
> > >> +        """Get a property from a node
> > >> +
> > >> +        Args:
> > >> +            nodeoffset: Node offset containing property to get
> > >> +            prop_name: Name of property to get
> > >> +
> > >> +        Returns:
> > >> +            Value of property as a string, or None if not found
> > >> +
> > >> +        Raises:
> > >> +            FdtError if an error occurs (e.g. nodeoffset is invalid)
> > >> +        """
> > >> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset,
> > >> prop_name), +                              True)[0]
> > > 
> > > There are a bunch of missing things here.  e.g. fdt_subnode_offset(),
> > > off_dt_strings(), fdt_parent_offset(), ...
> > 
> > Yes, I've only made a start. I don't want to implement the entire
> > thing right away! I want to get something basic agreed and accepted.
> > Then we can build on it. Refactoring everything as we iterate towards
> > a good binding takes time, and the functions I have chosen are fairly
> > representative of common uses.
> 
> Ok, fair enough.
> 
> > [..]
> > 
> > Regards,
> > Simon
> >
Regards,
Ulrich

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 19620 bytes --]

On Wed, Feb 15, 2017 at 07:55:01PM +0100, Ulrich Langenbach wrote:
> Hi David, hi Simon,
> 
> Am Montag, 13. Februar 2017, 16:20:22 CET schrieb David Gibson:
> > On Fri, Feb 10, 2017 at 11:39:21AM -0700, Simon Glass wrote:
> > > Hi David,
> > > 
> > > On 9 February 2017 at 21:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> 
> wrote:
> > > > On Sun, Feb 05, 2017 at 01:13:19PM -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 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
> > > >> 
> > > >>  pylibfdt/.gitignore        |   3 +
> > > >>  pylibfdt/Makefile.pylibfdt |  18 ++
> > > >>  pylibfdt/libfdt.swig       | 565
> > > >>  +++++++++++++++++++++++++++++++++++++++++++++ pylibfdt/setup.py      
> > > >>     |  34 +++
> > > >>  4 files changed, 620 insertions(+)
> > > >>  create mode 100644 pylibfdt/.gitignore
> > > >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> > > >>  create mode 100644 pylibfdt/libfdt.swig
> > > >>  create mode 100644 pylibfdt/setup.py
> > > 
> > > [..]
> > > 
> > > >> +def data(prop):
> > > >> +    """Extract the data from a property
> > > >> +
> > > >> +    This is an internal function only.
> > > >> +
> > > >> +    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))
> > > >> +    pylibfdt_copy_data(buf, prop)
> > > >> +    return buf
> > > > 
> > > > AFAICT this data() function (and the C functions it uses) aren't
> > > > actually used any more.  I think you can remove them.
> > > 
> > > It is used in tests, and needs to be used to process the return value
> > > from fdt_get_property_by_offset(). You ask later what we need from
> > > that function other than the property contents. We need the property
> > > name.
> > 
> > Ok.. so why can't you just return a tuple of two bytestrings?
> > 
> > > So we could create a Prop class (again) with the name and data in it,
> > > and have get_property_by_offset return it. Of course this means that
> > > scanning the tree requires allocating space for the property contents
> > > all over again, when you use it or not, but this is Python.
> > 
> > Yeah, I think trying to avoid lots of allocations when working in
> > Python is not really going to fly.
> > 
> > > >> +
> > > >> +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 < 0
> > > >> +    """
> > > >> +    if val < 0:
> > > > 
> > > >> +        if not quiet or val != -NOTFOUND:
> > > > I don't really like hardcoding the specialness of NOTFOUND here -
> > > > depending on the call, there could be different error codes which are
> > > > relatively harmless.
> > > > 
> > > > Instead of a quiet boolean, you could pass in a set (list, sequence,
> > > > whatever) of error codes to be considered quiet.
> > > 
> > > OK I can do that, except that later you suggest that the whole idea is
> > > wrong. See below.
> > > 
> > > >> +            raise FdtException(val)
> > > >> +    return val
> > > >> +
> > > >> +def check_err_null(val, quiet=False):
> > > >> +    """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 a list, None if not
> > > >> +
> > > >> +    Raises
> > > >> +        FdtException if quiet is False and val indicates an error was
> > > >> +           reported. If quiet if True then an FdtException is raised
> > > >> only if +           the error is something other than -NOTFOUND.
> > > >> +    """
> > > >> +    # Normally a tuple is returned which contains the data and its
> > > >> length.
> > > >> +    # If we get just an integer error code, it means the function
> > > >> failed.
> > > >> +    if not isinstance(val, list):
> > > >> +        if not quiet or val != -NOTFOUND:
> > > >> +            raise FdtException(val)
> > > >> +        return None,
> > > >> +    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())
> > > > 
> > > > I noticed when playing with this that doing Fdt("filename.dtb") will
> > > > appear to succeed, but then (of course) fail with BADMAGIC as soon as
> > > > you do anything.  Might be an idea to at least do a magic number check
> > > > in the constructor.
> in general this will fail once we start thinking of creating files, e.g. due to 
> rearchitecting a DT into multiple ones with includes or creating new DTs.
> However, with taking r/w modes into account the check can be skipped when the 
> file is opened for writing or deferred to a later read call as it is just 
> now...

Hrm, not if we're smart about it.  I'd envisage several ways of
invoking the Fdt constructor.  One like this would read in a file and
do a basic verification.  We'd also need variants for creating a new
device tree.  We'd need one variant for creating a tree using the
sequential write functions, which would call fdt_create() in the
constructor.  Then we'd need another for creating a fresh tree using
the random access read-write functions, which would call
fdt_create_empty_tree() in the constructor.

> > > 
> > > OK I'll add that.
> > > 
> > > >> +
> > > >> +    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 an 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.
> > > >> +    """
> > > >> +    def __init__(self, data):
> > > >> +        self._fdt = bytearray(data)
> > > >> +
> > > >> +    def string(self, offset):
> > > >> +        """Get a string given its offset
> > > >> +
> > > >> +        This is an internal function.
> > > >> +
> > > >> +        Args:
> > > >> +            offset: FDT offset in big-endian format
> > > >> +
> > > >> +        Returns:
> > > >> +            string value at that offset
> > > >> +        """
> > > >> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> > > >> +
> > > >> +    def path_offset(self, path):
> > > >> +        """Get the offset for a given path
> > > >> +
> > > >> +        Args:
> > > >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> > > >> +
> > > >> +        Returns:
> > > >> +            Node offset
> > > >> +
> > > >> +        Raises
> > > >> +            FdtException if the path is not valid
> > > >> +        """
> > > >> +        return check_err(fdt_path_offset(self._fdt, path))
> > > >> +
> > > >> +    def path_offset_quiet(self, path):
> > > >> +        """Get the offset for a given path
> > > >> +
> > > >> +        Args:
> > > >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> > > >> +
> > > >> +        Returns:
> > > >> +            Node offset, or -NOTFOUND if the path is not value
> > > >> +
> > > >> +        Raises
> > > >> +            FdtException if any error occurs other than NOTFOUND
> > > >> +        """
> > > >> +        return check_err(fdt_path_offset(self._fdt, path), True)
> > > > 
> > > > Ugh.  I don't much like the idea of making quiet versions of a bunch
> > > > 
> > > > of entry points.  I think we need to pick one:
> > > >     a) Just return error codes (not very Pythonic, but you can't have
> > > >     
> > > >        everything)
> > > >     
> > > >     b) Always raise exceptions, and the user has to try: except: to
> > > >     
> > > >        quiet then (creating different error classes for the different
> > > >        codes could make this easier).
> > > 
> > > I don't like it either, but on the other hand, you did ask for exceptions.
> > > 
> > > I'm happy to go back to a) if that suits you.
> > > 
> > > Re b) this is really ugly IMO. Exceptions shouldn't be used in normal
> > > program flow - e.g. they can appear in logs and cause confusion. Also
> > > the syntax is not great.
> > 
> > Yeah, I guess I agree.  Although Python does essentially use this
> > approach internally for iterators, it's mostly hidden by the syntactic
> > sugar.
> > 
> > > The reason for the 'quiet' function is that it handles 99% of the
> > > cases where you get an error return from a libfdt function. Also it
> > > means that you don't have to check return values most of the time, but
> > > still catch 'bad' things like out of space, bad offset, etc. without
> > > blundering onwards.
> > > 
> > > As I say I don't like having 'quiet' versions of the functions. There
> > > is one other option I can think of, which is to add an additional
> > > 
> > > argument to each function like this:
> > >    def path_offset(self, path, quiet=False):
> > > Then you can add 'quiet=True' to avoid the exception. Then you use the
> > > same function name, and most of the time you get exceptions when there
> > > are problems, but it's easy to avoid when you know there is a chance
> > > an error will be returned.
> > > 
> > > If we go that way, then you are probably going to ask what is so
> > > special about -NOTFOUND.
> > > 
> > > So we could make quiet a list (or perhaps Set) of exceptions, like this:
> > > 
> > > def path_offset(self, path, quiet=None)   # or perhaps quiet=[]
> > > 
> > > and have callers do:
> > > 
> > > fdt.path_offset("/", quiet=[NOTFOUND])
> > > 
> > > and I suppose we could define a constant for [NOTFOUND] also.
> > 
> > Yes, this looks better than the other alternatives I think - it's
> > basically an extension of what I suggested for check_err().  I think
> > you can exploit duck typing here to let the user use a list or set as
> > they please (basically just allow anything supporting the 'in'
> > operator).
> > 
> > > That might be better all round?
> > 
> > Yes, I think so.  The remaining question, of course, is whether to a)
> > have the quiet set default to nothing, or b) default to the "usually
> > quiet" errors (NOTFOUND for most functions).  (b) will be less verbose
> > for common use cases, but (a) reduces invisible magic which is
> > generally a good goal.
> I like the idea of hiding the not so important errors. I would also throw in a 
> little bit different approach to this by adding a property to the Fdt object 
> used to control this. It can be configured by the user during instantiation or 
> later on. This just keeps the interfaces of the single function(s) cleaner.
> However I am not sure how this is usually handled, so that we could just 
> follow already agreed best practices.

I dislike the idea of putting this into the overall object, because
the obvious choices for "quiet" errors varies with the function you're
using.  NOTFOUND is by far the most common, but EXISTS could also be a
non serious error in a number of circumstances.  When we start adding
write functions, NOSPACE becomes extremely common.

> 
> > > >> +    def first_property_offset(self, nodeoffset):
> > > >> +        """Get the offset of the first property in a node offset
> > > >> +
> > > >> +        Args:
> > > >> +            nodeoffset: Offset to the node to check
> > > >> +
> > > >> +        Returns:
> > > >> +            Offset of the first property
> > > >> +
> > > >> +        Raises
> > > >> +            FdtException if the associated node has no properties, or
> > > >> some
> > > >> +                other error occurred
> > > >> +        """
> > > >> +        return check_err(fdt_first_property_offset(self._fdt,
> > > >> nodeoffset))
> > > >> +
> > > >> +    def first_property_offset_quiet(self, nodeoffset):
> > > >> +        """Get the offset of the first property in a node offset
> > > >> +
> > > >> +        Args:
> > > >> +            nodeoffset: Offset to the node to check
> > > >> +
> > > >> +        Returns:
> > > >> +            Offset of the first property, or -NOTFOUND if the node has
> > > >> no
> > > >> +                properties
> > > >> +
> > > >> +        Raises
> > > >> +            FdtException if any other error occurs
> > > >> +        """
> > > >> +        return check_err(fdt_first_property_offset(self._fdt,
> > > >> nodeoffset), True) +
> > > >> +    def next_property_offset(self, prop_offset):
> > > >> +        """Get the next property in a node
> > > >> +
> > > >> +        Args:
> > > >> +            prop_offset: Offset of the previous property
> > > >> +
> > > >> +        Returns:
> > > >> +            Offset of the next property
> > > >> +
> > > >> +        Raises:
> > > >> +            FdtException if the associated node has no more
> > > >> properties, or
> > > >> +                some other error occurred
> > > >> +        """
> > > >> +        return check_err(fdt_next_property_offset(self._fdt,
> > > >> prop_offset))
> > > >> +
> > > >> +    def next_property_offset_quiet(self, prop_offset):
> > > >> +        """Get the next property in a node
> > > >> +
> > > >> +        Args:
> > > >> +            prop_offset: Offset of the previous property
> > > >> +
> > > >> +        Returns:
> > > >> +            Offset ot the next property, or -NOTFOUND if there are no
> > > >> more
> > > >> +            properties
> > > >> +
> > > >> +        Raises:
> > > >> +            FdtException if any other error occurs
> > > >> +        """
> > > >> +        return check_err(fdt_next_property_offset(self._fdt,
> > > >> prop_offset), True) +
> > > >> +    def get_name(self, nodeoffset):
> > > >> +        """Get the name of a node
> > > >> +
> > > >> +        Args:
> > > >> +            nodeoffset: Offset of node to check
> > > >> +
> > > >> +        Returns:
> > > >> +            Node name
> > > >> +
> > > >> +        Raises:
> > > >> +            FdtException on error (e.g. nodeoffset is invalid)
> > > >> +        """
> > > >> +        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
> > > >> +
> > > >> +    def get_property_by_offset(self, prop_offset):
> > > >> +        """Obtains a property that can be examined
> > > >> +
> > > >> +        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property object
> > > >> instead, +        with the data extracted from the Fdt.
> > > > 
> > > > AFAICT you're already copying the property value out of the fdt as a
> > > > bytestring / bytearray.  I don't see what a property object would
> > > > encode apart from that bytestring.
> > > 
> > > The property name.
> > 
> > Hm, true.  Alright, how about this: reintroduce a Property object, and
> > also a version of the get_property() vs. getprop() variants which are
> > in the C library.  get_property() and get_property_by_offset() will
> > return Property objects which, to be clear, are *copies* of a property
> > name & value, not a handle to a property within the tree.  getprop()
> > will return just the value bytestring().
> > 
> > Property should probably be derived from collections.namedtuple, since
> > it is basically just a wrapper on a (name, value) tuple.
> > 
> > > [...]
> > > 
> > > >> +    def getprop_quiet(self, nodeoffset, prop_name):
> > > >> +        """Get a property from a node
> > > >> +
> > > >> +        Args:
> > > >> +            nodeoffset: Node offset containing property to get
> > > >> +            prop_name: Name of property to get
> > > >> +
> > > >> +        Returns:
> > > >> +            Value of property as a string, or None if not found
> > > >> +
> > > >> +        Raises:
> > > >> +            FdtError if an error occurs (e.g. nodeoffset is invalid)
> > > >> +        """
> > > >> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset,
> > > >> prop_name), +                              True)[0]
> > > > 
> > > > There are a bunch of missing things here.  e.g. fdt_subnode_offset(),
> > > > off_dt_strings(), fdt_parent_offset(), ...
> > > 
> > > Yes, I've only made a start. I don't want to implement the entire
> > > thing right away! I want to get something basic agreed and accepted.
> > > Then we can build on it. Refactoring everything as we iterate towards
> > > a good binding takes time, and the functions I have chosen are fairly
> > > representative of common uses.
> > 
> > Ok, fair enough.
> > 
> > > [..]
> > > 
> > > Regards,
> > > Simon
> > >
> Regards,
> Ulrich
> 

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/5] Add an initial Python library for libfdt

[Ulrich Langenbach]

Hi,

Am Donnerstag, 16. Februar 2017, 13:11:23 CET schrieb David Gibson:
> On Wed, Feb 15, 2017 at 07:55:01PM +0100, Ulrich Langenbach wrote:
> > Hi David, hi Simon,
> > 
> > Am Montag, 13. Februar 2017, 16:20:22 CET schrieb David Gibson:
> > > On Fri, Feb 10, 2017 at 11:39:21AM -0700, Simon Glass wrote:
> > > > Hi David,
> > > > 
> > > > On 9 February 2017 at 21:37, David Gibson
> > > > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> > 
> > wrote:
> > > > > On Sun, Feb 05, 2017 at 01:13:19PM -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 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
> > > > >> 
> > > > >>  pylibfdt/.gitignore        |   3 +
> > > > >>  pylibfdt/Makefile.pylibfdt |  18 ++
> > > > >>  pylibfdt/libfdt.swig       | 565
> > > > >>  +++++++++++++++++++++++++++++++++++++++++++++ pylibfdt/setup.py
> > > > >>  
> > > > >>     |  34 +++
> > > > >>  
> > > > >>  4 files changed, 620 insertions(+)
> > > > >>  create mode 100644 pylibfdt/.gitignore
> > > > >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> > > > >>  create mode 100644 pylibfdt/libfdt.swig
> > > > >>  create mode 100644 pylibfdt/setup.py
> > > > 
> > > > [..]
> > > > 
> > > > >> +def data(prop):
> > > > >> +    """Extract the data from a property
> > > > >> +
> > > > >> +    This is an internal function only.
> > > > >> +
> > > > >> +    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))
> > > > >> +    pylibfdt_copy_data(buf, prop)
> > > > >> +    return buf
> > > > > 
> > > > > AFAICT this data() function (and the C functions it uses) aren't
> > > > > actually used any more.  I think you can remove them.
> > > > 
> > > > It is used in tests, and needs to be used to process the return value
> > > > from fdt_get_property_by_offset(). You ask later what we need from
> > > > that function other than the property contents. We need the property
> > > > name.
> > > 
> > > Ok.. so why can't you just return a tuple of two bytestrings?
> > > 
> > > > So we could create a Prop class (again) with the name and data in it,
> > > > and have get_property_by_offset return it. Of course this means that
> > > > scanning the tree requires allocating space for the property contents
> > > > all over again, when you use it or not, but this is Python.
> > > 
> > > Yeah, I think trying to avoid lots of allocations when working in
> > > Python is not really going to fly.
> > > 
> > > > >> +
> > > > >> +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 < 0
> > > > >> +    """
> > > > >> +    if val < 0:
> > > > > 
> > > > >> +        if not quiet or val != -NOTFOUND:
> > > > > I don't really like hardcoding the specialness of NOTFOUND here -
> > > > > depending on the call, there could be different error codes which
> > > > > are
> > > > > relatively harmless.
> > > > > 
> > > > > Instead of a quiet boolean, you could pass in a set (list, sequence,
> > > > > whatever) of error codes to be considered quiet.
> > > > 
> > > > OK I can do that, except that later you suggest that the whole idea is
> > > > wrong. See below.
> > > > 
> > > > >> +            raise FdtException(val)
> > > > >> +    return val
> > > > >> +
> > > > >> +def check_err_null(val, quiet=False):
> > > > >> +    """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 a list, None if not
> > > > >> +
> > > > >> +    Raises
> > > > >> +        FdtException if quiet is False and val indicates an error
> > > > >> was
> > > > >> +           reported. If quiet if True then an FdtException is
> > > > >> raised
> > > > >> only if +           the error is something other than -NOTFOUND.
> > > > >> +    """
> > > > >> +    # Normally a tuple is returned which contains the data and its
> > > > >> length.
> > > > >> +    # If we get just an integer error code, it means the function
> > > > >> failed.
> > > > >> +    if not isinstance(val, list):
> > > > >> +        if not quiet or val != -NOTFOUND:
> > > > >> +            raise FdtException(val)
> > > > >> +        return None,
> > > > >> +    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())
> > > > > 
> > > > > I noticed when playing with this that doing Fdt("filename.dtb") will
> > > > > appear to succeed, but then (of course) fail with BADMAGIC as soon
> > > > > as
> > > > > you do anything.  Might be an idea to at least do a magic number
> > > > > check
> > > > > in the constructor.
> > 
> > in general this will fail once we start thinking of creating files, e.g.
> > due to rearchitecting a DT into multiple ones with includes or creating
> > new DTs. However, with taking r/w modes into account the check can be
> > skipped when the file is opened for writing or deferred to a later read
> > call as it is just now...
> 
> Hrm, not if we're smart about it.  I'd envisage several ways of
> invoking the Fdt constructor.  One like this would read in a file and
> do a basic verification.  We'd also need variants for creating a new
> device tree.  We'd need one variant for creating a tree using the
> sequential write functions, which would call fdt_create() in the
> constructor.  Then we'd need another for creating a fresh tree using
> the random access read-write functions, which would call
> fdt_create_empty_tree() in the constructor.

Ok, that sounds good to me.

> > > > OK I'll add that.
> > > > 
> > > > >> +
> > > > >> +    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 an 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.
> > > > >> +    """
> > > > >> +    def __init__(self, data):
> > > > >> +        self._fdt = bytearray(data)
> > > > >> +
> > > > >> +    def string(self, offset):
> > > > >> +        """Get a string given its offset
> > > > >> +
> > > > >> +        This is an internal function.
> > > > >> +
> > > > >> +        Args:
> > > > >> +            offset: FDT offset in big-endian format
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            string value at that offset
> > > > >> +        """
> > > > >> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> > > > >> +
> > > > >> +    def path_offset(self, path):
> > > > >> +        """Get the offset for a given path
> > > > >> +
> > > > >> +        Args:
> > > > >> +            path: Path to the required node, e.g.
> > > > >> '/node@3/subnode@1'
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Node offset
> > > > >> +
> > > > >> +        Raises
> > > > >> +            FdtException if the path is not valid
> > > > >> +        """
> > > > >> +        return check_err(fdt_path_offset(self._fdt, path))
> > > > >> +
> > > > >> +    def path_offset_quiet(self, path):
> > > > >> +        """Get the offset for a given path
> > > > >> +
> > > > >> +        Args:
> > > > >> +            path: Path to the required node, e.g.
> > > > >> '/node@3/subnode@1'
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Node offset, or -NOTFOUND if the path is not value
> > > > >> +
> > > > >> +        Raises
> > > > >> +            FdtException if any error occurs other than NOTFOUND
> > > > >> +        """
> > > > >> +        return check_err(fdt_path_offset(self._fdt, path), True)
> > > > > 
> > > > > Ugh.  I don't much like the idea of making quiet versions of a bunch
> > > > > 
> > > > > of entry points.  I think we need to pick one:
> > > > >     a) Just return error codes (not very Pythonic, but you can't
> > > > >     have
> > > > >     
> > > > >        everything)
> > > > >     
> > > > >     b) Always raise exceptions, and the user has to try: except: to
> > > > >     
> > > > >        quiet then (creating different error classes for the
> > > > >        different
> > > > >        codes could make this easier).
> > > > 
> > > > I don't like it either, but on the other hand, you did ask for
> > > > exceptions.
> > > > 
> > > > I'm happy to go back to a) if that suits you.
> > > > 
> > > > Re b) this is really ugly IMO. Exceptions shouldn't be used in normal
> > > > program flow - e.g. they can appear in logs and cause confusion. Also
> > > > the syntax is not great.
> > > 
> > > Yeah, I guess I agree.  Although Python does essentially use this
> > > approach internally for iterators, it's mostly hidden by the syntactic
> > > sugar.
> > > 
> > > > The reason for the 'quiet' function is that it handles 99% of the
> > > > cases where you get an error return from a libfdt function. Also it
> > > > means that you don't have to check return values most of the time, but
> > > > still catch 'bad' things like out of space, bad offset, etc. without
> > > > blundering onwards.
> > > > 
> > > > As I say I don't like having 'quiet' versions of the functions. There
> > > > is one other option I can think of, which is to add an additional
> > > > 
> > > > argument to each function like this:
> > > >    def path_offset(self, path, quiet=False):
> > > > Then you can add 'quiet=True' to avoid the exception. Then you use the
> > > > same function name, and most of the time you get exceptions when there
> > > > are problems, but it's easy to avoid when you know there is a chance
> > > > an error will be returned.
> > > > 
> > > > If we go that way, then you are probably going to ask what is so
> > > > special about -NOTFOUND.
> > > > 
> > > > So we could make quiet a list (or perhaps Set) of exceptions, like
> > > > this:
> > > > 
> > > > def path_offset(self, path, quiet=None)   # or perhaps quiet=[]
> > > > 
> > > > and have callers do:
> > > > 
> > > > fdt.path_offset("/", quiet=[NOTFOUND])
> > > > 
> > > > and I suppose we could define a constant for [NOTFOUND] also.
> > > 
> > > Yes, this looks better than the other alternatives I think - it's
> > > basically an extension of what I suggested for check_err().  I think
> > > you can exploit duck typing here to let the user use a list or set as
> > > they please (basically just allow anything supporting the 'in'
> > > operator).
> > > 
> > > > That might be better all round?
> > > 
> > > Yes, I think so.  The remaining question, of course, is whether to a)
> > > have the quiet set default to nothing, or b) default to the "usually
> > > quiet" errors (NOTFOUND for most functions).  (b) will be less verbose
> > > for common use cases, but (a) reduces invisible magic which is
> > > generally a good goal.
> > 
> > I like the idea of hiding the not so important errors. I would also throw
> > in a little bit different approach to this by adding a property to the
> > Fdt object used to control this. It can be configured by the user during
> > instantiation or later on. This just keeps the interfaces of the single
> > function(s) cleaner. However I am not sure how this is usually handled,
> > so that we could just follow already agreed best practices.
> 
> I dislike the idea of putting this into the overall object, because
> the obvious choices for "quiet" errors varies with the function you're
> using.  NOTFOUND is by far the most common, but EXISTS could also be a
> non serious error in a number of circumstances.  When we start adding
> write functions, NOSPACE becomes extremely common.

I understand that this definitely is a design trade-off for flexibility. I was 
just thinking that a lot of people will probably use that lib in a single 
project for a single use case and then they could adjust the libs behavior for 
that instance globally. We could also enable both approaches, by setting the 
default of the functions to None and use a global setting in that case. This 
would enable users to set both, global default behavior and local specific 
behavior.
However that could make usage even more complicated than easier.

> > > > >> +    def first_property_offset(self, nodeoffset):
> > > > >> +        """Get the offset of the first property in a node offset
> > > > >> +
> > > > >> +        Args:
> > > > >> +            nodeoffset: Offset to the node to check
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Offset of the first property
> > > > >> +
> > > > >> +        Raises
> > > > >> +            FdtException if the associated node has no properties,
> > > > >> or
> > > > >> some
> > > > >> +                other error occurred
> > > > >> +        """
> > > > >> +        return check_err(fdt_first_property_offset(self._fdt,
> > > > >> nodeoffset))
> > > > >> +
> > > > >> +    def first_property_offset_quiet(self, nodeoffset):
> > > > >> +        """Get the offset of the first property in a node offset
> > > > >> +
> > > > >> +        Args:
> > > > >> +            nodeoffset: Offset to the node to check
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Offset of the first property, or -NOTFOUND if the node
> > > > >> has
> > > > >> no
> > > > >> +                properties
> > > > >> +
> > > > >> +        Raises
> > > > >> +            FdtException if any other error occurs
> > > > >> +        """
> > > > >> +        return check_err(fdt_first_property_offset(self._fdt,
> > > > >> nodeoffset), True) +
> > > > >> +    def next_property_offset(self, prop_offset):
> > > > >> +        """Get the next property in a node
> > > > >> +
> > > > >> +        Args:
> > > > >> +            prop_offset: Offset of the previous property
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Offset of the next property
> > > > >> +
> > > > >> +        Raises:
> > > > >> +            FdtException if the associated node has no more
> > > > >> properties, or
> > > > >> +                some other error occurred
> > > > >> +        """
> > > > >> +        return check_err(fdt_next_property_offset(self._fdt,
> > > > >> prop_offset))
> > > > >> +
> > > > >> +    def next_property_offset_quiet(self, prop_offset):
> > > > >> +        """Get the next property in a node
> > > > >> +
> > > > >> +        Args:
> > > > >> +            prop_offset: Offset of the previous property
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Offset ot the next property, or -NOTFOUND if there are
> > > > >> no
> > > > >> more
> > > > >> +            properties
> > > > >> +
> > > > >> +        Raises:
> > > > >> +            FdtException if any other error occurs
> > > > >> +        """
> > > > >> +        return check_err(fdt_next_property_offset(self._fdt,
> > > > >> prop_offset), True) +
> > > > >> +    def get_name(self, nodeoffset):
> > > > >> +        """Get the name of a node
> > > > >> +
> > > > >> +        Args:
> > > > >> +            nodeoffset: Offset of node to check
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Node name
> > > > >> +
> > > > >> +        Raises:
> > > > >> +            FdtException on error (e.g. nodeoffset is invalid)
> > > > >> +        """
> > > > >> +        return check_err_null(fdt_get_name(self._fdt,
> > > > >> nodeoffset))[0]
> > > > >> +
> > > > >> +    def get_property_by_offset(self, prop_offset):
> > > > >> +        """Obtains a property that can be examined
> > > > >> +
> > > > >> +        TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Consider returning a property
> > > > >> object
> > > > >> instead, +        with the data extracted from the Fdt.
> > > > > 
> > > > > AFAICT you're already copying the property value out of the fdt as a
> > > > > bytestring / bytearray.  I don't see what a property object would
> > > > > encode apart from that bytestring.
> > > > 
> > > > The property name.
> > > 
> > > Hm, true.  Alright, how about this: reintroduce a Property object, and
> > > also a version of the get_property() vs. getprop() variants which are
> > > in the C library.  get_property() and get_property_by_offset() will
> > > return Property objects which, to be clear, are *copies* of a property
> > > name & value, not a handle to a property within the tree.  getprop()
> > > will return just the value bytestring().
> > > 
> > > Property should probably be derived from collections.namedtuple, since
> > > it is basically just a wrapper on a (name, value) tuple.
> > > 
> > > > [...]
> > > > 
> > > > >> +    def getprop_quiet(self, nodeoffset, prop_name):
> > > > >> +        """Get a property from a node
> > > > >> +
> > > > >> +        Args:
> > > > >> +            nodeoffset: Node offset containing property to get
> > > > >> +            prop_name: Name of property to get
> > > > >> +
> > > > >> +        Returns:
> > > > >> +            Value of property as a string, or None if not found
> > > > >> +
> > > > >> +        Raises:
> > > > >> +            FdtError if an error occurs (e.g. nodeoffset is
> > > > >> invalid)
> > > > >> +        """
> > > > >> +        return check_err_null(fdt_getprop(self._fdt, nodeoffset,
> > > > >> prop_name), +                              True)[0]
> > > > > 
> > > > > There are a bunch of missing things here.  e.g.
> > > > > fdt_subnode_offset(),
> > > > > off_dt_strings(), fdt_parent_offset(), ...
> > > > 
> > > > Yes, I've only made a start. I don't want to implement the entire
> > > > thing right away! I want to get something basic agreed and accepted.
> > > > Then we can build on it. Refactoring everything as we iterate towards
> > > > a good binding takes time, and the functions I have chosen are fairly
> > > > representative of common uses.
> > > 
> > > Ok, fair enough.
> > > 
> > > > [..]
> > > > 
> > > > Regards,
> > > > Simon
> > 
> > Regards,
> > Ulrich

Regards,
Ulrich