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

[PATCH v5 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 v5 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(node, quiet=[libfdt.NOTFOUND])
    while subnode > 0:
        print fdt.get_name(subnode)
        subnode = fdt.next_subnode(subnode, quiet=[libfdt.NOTFOUND])

This version includes a simple class for properties.

Changes in v5:
- Use a 'quiet' parameter instead of quiet versions of functions
- Add a Property object to hold a property's name and value
- Drop the data() and string() functions which are not needed now
- Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
- Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
- Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
- Use $(SWIG) to call swig from the Makefile
- Review function comments
- Adjust tests to match new swig bindings
- Use an interactive session to demonstrate pylibfdt
- Mention that more work remains
- Update commit message
- Drop #ifdef around fdt_get_header() macros
- Fix 'possible' typo

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                   |  17 +-
 README                     |  43 +++++
 libfdt/libfdt.h            |   5 +-
 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  18 ++
 pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 ++++
 tests/pylibfdt_tests.py    | 267 ++++++++++++++++++++++++++
 tests/run_tests.sh         |  19 +-
 9 files changed, 868 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 v5 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 v5:
- Use a 'quiet' parameter instead of quiet versions of functions
- Add a Property object to hold a property's name and value
- Drop the data() and string() functions which are not needed now
- Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
- Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
- Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
- Use $(SWIG) to call swig from the Makefile
- Review function comments

Changes in v4:
- Make the library less pythonic to avoid a shaky illusion
- Drop classes for Node and Prop, along with associated methods
- Include libfdt.h instead of repeating it
- Add support for fdt_getprop()
- Bring in all libfdt functions (but Python support is missing for many)
- Add full comments for Python methods

Changes in v3:
- Make the library more pythonic
- Add classes for Node and Prop along with methods
- Add an exception class
- Use Python to generate exeptions instead of SWIG

Changes in v2:
- Add exceptions when functions return an error
- Correct Python naming to following PEP8
- Use a class to encapsulate the various methods
- Include fdt.h instead of redefining struct fdt_property
- Use bytearray to avoid the SWIG warning 454
- Add comments

 Makefile                   |   1 +
 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  18 ++
 pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 ++++
 5 files changed, 521 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/Makefile b/Makefile
index ce05eba..1c48210 100644
--- a/Makefile
+++ b/Makefile
@@ -22,6 +22,7 @@ CFLAGS = -g -Os -fPIC -Werror $(WARNINGS)
 
 BISON = bison
 LEX = flex
+SWIG = swig
 
 INSTALL = /usr/bin/install
 DESTDIR =
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..0c0b390
--- /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..d538b3e
--- /dev/null
+++ b/pylibfdt/libfdt.swig
@@ -0,0 +1,465 @@
+/*
+ * 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)
+
+# Pass this as the 'quiet' parameter to return -ENOTFOUND on NOTFOUND errors,
+# instead of raising an exception.
+QUIET_NOTFOUND = [NOTFOUND]
+
+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 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=[]):
+    """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: Errors to ignore (empty to raise on all errors)
+
+    Returns:
+        val if val >= 0
+
+    Raises
+        FdtException if val < 0
+    """
+    if val < 0:
+        if -val not in quiet:
+            raise FdtException(val)
+    return val
+
+def check_err_null(val, quiet=[]):
+    """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: Errors to ignore (empty to raise on all errors)
+
+    Returns:
+        val if val is a list, None if not
+
+    Raises
+        FdtException if val indicates an error was reported and the error
+        is not in @quiet.
+    """
+    # 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 -val not in quiet:
+            raise FdtException(val)
+    return val
+
+class Fdt:
+    """Device tree class, supporting all operations
+
+    The Fdt object is created is created from a device tree binary file,
+    e.g. with something like:
+
+       fdt = Fdt(open("filename.dtb").read())
+
+    Operations can then be performed using the methods in this class. Each
+    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
+
+    All methods raise an FdtException if an error occurs. To avoid this
+    behaviour a 'quiet' parameter is provided for some functions. This
+    defaults to empty, but you can pass a list of errors that you expect.
+    If one of these errors occurs, the function will return an error number
+    (e.g. -NOTFOUND).
+    """
+    def __init__(self, data):
+        self._fdt = bytearray(data)
+        check_err(fdt_check_header(self._fdt));
+
+    def path_offset(self, path, quiet=[]):
+        """Get the offset for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Node offset
+
+        Raises
+            FdtException if the path is not valid or not found
+        """
+        return check_err(fdt_path_offset(self._fdt, path), quiet)
+
+    def first_property_offset(self, nodeoffset, quiet=[]):
+        """Get the offset of the first property in a node offset
+
+        Args:
+            nodeoffset: Offset to the node to check
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        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),
+                         quiet)
+
+    def next_property_offset(self, prop_offset, quiet=[]):
+        """Get the next property in a node
+
+        Args:
+            prop_offset: Offset of the previous property
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        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),
+                         quiet)
+
+    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, quiet=[]):
+        """Obtains a property that can be examined
+
+        Args:
+            prop_offset: Offset of property (e.g. from first_property_offset())
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Property object, or None if not found
+
+        Raises:
+            FdtException on error (e.g. invalid prop_offset or device
+            tree format)
+        """
+        pdata = check_err_null(
+                fdt_get_property_by_offset(self._fdt, prop_offset), quiet)
+        if isinstance(pdata, (int)):
+            return pdata
+        return Property(self, pdata[0])
+
+    def first_subnode(self, nodeoffset, quiet=[]):
+        """Find the first subnode of a parent node
+
+        Args:
+            nodeoffset: Node offset of parent node
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        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), quiet)
+
+    def next_subnode(self, nodeoffset, quiet=[]):
+        """Find the next subnode
+
+        Args:
+            nodeoffset: Node offset of previous subnode
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        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), quiet)
+
+    def totalsize(self):
+        """Return the total size of the device tree
+
+        Returns:
+            Total tree size in bytes
+        """
+        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
+        """
+        return check_err(fdt_off_dt_struct(self._fdt))
+
+    def pack(self, quiet=[]):
+        """Pack the device tree to remove unused space
+
+        This adjusts the tree in place.
+
+        Args:
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Raises:
+            FdtException if any error occurs
+        """
+        return check_err(fdt_pack(self._fdt), quiet)
+
+    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, quiet=[]):
+        """Get a property from a node
+
+        Args:
+            nodeoffset: Node offset containing property to get
+            prop_name: Name of property to get
+            quiet: Errors to ignore (empty to raise on all errors)
+
+        Returns:
+            Value of property as a string, or -ve error number
+
+        Raises:
+            FdtError if any error occurs (e.g. the property is not found)
+        """
+        pdata = check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
+                               quiet)
+        if isinstance(pdata, (int)):
+            return pdata
+        return pdata[0]
+
+
+class Property:
+    """Holds a device tree property name and value.
+
+    This holds a copy of a property taken from the device tree. It does not
+    reference the device tree, so if anything changes in the device tree,
+    a Property object will remain valid.
+
+    Properties:
+        name: Property name
+        value: Proper value as a bytearray
+    """
+    def __init__(self, fdt, pdata):
+        self.name = fdt_string(fdt._fdt, fdt32_to_cpu(pdata.nameoff))
+        self.value = bytearray(fdt32_to_cpu(pdata.len))
+        pylibfdt_copy_value(self.value, pdata)
+%}
+
+%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_value’:
+ * 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_value(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_value() - Copy value from a property to the given buffer
+ *
+ * This is used by the Property class to place the contents of a property
+ * into a bytearray.
+ *
+ * @buf: Destination pointer (typically the start of the bytearray)
+ * @size: Number of bytes to copy (size of bytearray)
+ * @prop: Property to copy
+ */
+void pylibfdt_copy_value(char *buf, size_t size, const struct fdt_property *prop)
+{
+	memcpy(buf, prop + 1, size);
+}
+
+%}
+
+%apply int *OUTPUT { 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;
+
+/* 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);
+
+%include <../libfdt/libfdt.h>
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

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

[David Gibson]

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

On Tue, Feb 14, 2017 at 08:51:56PM -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 v5:
> - Use a 'quiet' parameter instead of quiet versions of functions
> - Add a Property object to hold a property's name and value
> - Drop the data() and string() functions which are not needed now
> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
> - Use $(SWIG) to call swig from the Makefile
> - Review function comments
> 
> Changes in v4:
> - Make the library less pythonic to avoid a shaky illusion
> - Drop classes for Node and Prop, along with associated methods
> - Include libfdt.h instead of repeating it
> - Add support for fdt_getprop()
> - Bring in all libfdt functions (but Python support is missing for many)
> - Add full comments for Python methods
> 
> Changes in v3:
> - Make the library more pythonic
> - Add classes for Node and Prop along with methods
> - Add an exception class
> - Use Python to generate exeptions instead of SWIG
> 
> Changes in v2:
> - Add exceptions when functions return an error
> - Correct Python naming to following PEP8
> - Use a class to encapsulate the various methods
> - Include fdt.h instead of redefining struct fdt_property
> - Use bytearray to avoid the SWIG warning 454
> - Add comments
> 
>  Makefile                   |   1 +
>  pylibfdt/.gitignore        |   3 +
>  pylibfdt/Makefile.pylibfdt |  18 ++
>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
>  pylibfdt/setup.py          |  34 ++++
>  5 files changed, 521 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/Makefile b/Makefile
> index ce05eba..1c48210 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -22,6 +22,7 @@ CFLAGS = -g -Os -fPIC -Werror $(WARNINGS)
>  
>  BISON = bison
>  LEX = flex
> +SWIG = swig
>  
>  INSTALL = /usr/bin/install
>  DESTDIR =
> 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..0c0b390
> --- /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..d538b3e
> --- /dev/null
> +++ b/pylibfdt/libfdt.swig
> @@ -0,0 +1,465 @@
> +/*
> + * 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)
> +
> +# Pass this as the 'quiet' parameter to return -ENOTFOUND on NOTFOUND errors,
> +# instead of raising an exception.
> +QUIET_NOTFOUND = [NOTFOUND]
> +
> +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 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=[]):
> +    """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: Errors to ignore (empty to raise on all errors)
> +
> +    Returns:
> +        val if val >= 0
> +
> +    Raises
> +        FdtException if val < 0
> +    """
> +    if val < 0:
> +        if -val not in quiet:
> +            raise FdtException(val)
> +    return val
> +
> +def check_err_null(val, quiet=[]):
> +    """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: Errors to ignore (empty to raise on all errors)
> +
> +    Returns:
> +        val if val is a list, None if not
> +
> +    Raises
> +        FdtException if val indicates an error was reported and the error
> +        is not in @quiet.
> +    """
> +    # Normally a tuple is returned which contains the data and its length.

Is it a tuple returned..?

> +    # If we get just an integer error code, it means the function failed.
> +    if not isinstance(val, list):

..or a list?  Seems like either the comment or the code must be
incorrect here.

Come to that, what is it tells swig to map fdt_propery_by_offset() and
the like to the pair of values?  How does it know how to detect the
error cases?

From the usage, I take it that in the success case val[0] is a string
or bytearray containing the relevant chunk of data.  Remind me what
the second value is?

> +        if -val not in quiet:
> +            raise FdtException(val)
> +    return val
> +
> +class Fdt:
> +    """Device tree class, supporting all operations
> +
> +    The Fdt object is created is created from a device tree binary file,
> +    e.g. with something like:
> +
> +       fdt = Fdt(open("filename.dtb").read())
> +
> +    Operations can then be performed using the methods in this class. Each
> +    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
> +
> +    All methods raise an FdtException if an error occurs. To avoid this
> +    behaviour a 'quiet' parameter is provided for some functions. This
> +    defaults to empty, but you can pass a list of errors that you expect.
> +    If one of these errors occurs, the function will return an error number
> +    (e.g. -NOTFOUND).
> +    """
> +    def __init__(self, data):
> +        self._fdt = bytearray(data)
> +        check_err(fdt_check_header(self._fdt));
> +
> +    def path_offset(self, path, quiet=[]):
> +        """Get the offset for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Node offset
> +
> +        Raises
> +            FdtException if the path is not valid or not found
> +        """
> +        return check_err(fdt_path_offset(self._fdt, path), quiet)
> +
> +    def first_property_offset(self, nodeoffset, quiet=[]):

I'd suggest changing the default values from an empty list to an empty
tuple ().  Might be slightly more efficient, but more importantly,
since a tuple is immutable it avoids any possibility of a reference to
the the default value somehow escaping and getting modified with
baffling results.

> +        """Get the offset of the first property in a node offset
> +
> +        Args:
> +            nodeoffset: Offset to the node to check
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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),
> +                         quiet)
> +
> +    def next_property_offset(self, prop_offset, quiet=[]):
> +        """Get the next property in a node
> +
> +        Args:
> +            prop_offset: Offset of the previous property
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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),
> +                         quiet)
> +
> +    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, quiet=[]):
> +        """Obtains a property that can be examined
> +
> +        Args:
> +            prop_offset: Offset of property (e.g. from first_property_offset())
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Property object, or None if not found
> +
> +        Raises:
> +            FdtException on error (e.g. invalid prop_offset or device
> +            tree format)
> +        """
> +        pdata = check_err_null(
> +                fdt_get_property_by_offset(self._fdt, prop_offset), quiet)
> +        if isinstance(pdata, (int)):
> +            return pdata
> +        return Property(self, pdata[0])
> +
> +    def first_subnode(self, nodeoffset, quiet=[]):
> +        """Find the first subnode of a parent node
> +
> +        Args:
> +            nodeoffset: Node offset of parent node
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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), quiet)
> +
> +    def next_subnode(self, nodeoffset, quiet=[]):
> +        """Find the next subnode
> +
> +        Args:
> +            nodeoffset: Node offset of previous subnode
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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), quiet)
> +
> +    def totalsize(self):
> +        """Return the total size of the device tree
> +
> +        Returns:
> +            Total tree size in bytes
> +        """
> +        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
> +        """
> +        return check_err(fdt_off_dt_struct(self._fdt))
> +
> +    def pack(self, quiet=[]):
> +        """Pack the device tree to remove unused space
> +
> +        This adjusts the tree in place.
> +
> +        Args:
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Raises:
> +            FdtException if any error occurs
> +        """
> +        return check_err(fdt_pack(self._fdt), quiet)
> +
> +    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, quiet=[]):
> +        """Get a property from a node
> +
> +        Args:
> +            nodeoffset: Node offset containing property to get
> +            prop_name: Name of property to get
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Value of property as a string, or -ve error number

Minor detail: for easier support for Python3 in the future, I suggest
you construct these "strings" as bytes() objects rather than
explicitly Python strings.  In Python2 they're the same thing, but in
Python3 bytes() is an immutable bytearray, but string() is a Unicode
string.

> +
> +        Raises:
> +            FdtError if any error occurs (e.g. the property is not found)
> +        """
> +        pdata = check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
> +                               quiet)
> +        if isinstance(pdata, (int)):
> +            return pdata
> +        return pdata[0]



> +
> +
> +class Property:
> +    """Holds a device tree property name and value.
> +
> +    This holds a copy of a property taken from the device tree. It does not
> +    reference the device tree, so if anything changes in the device tree,
> +    a Property object will remain valid.
> +
> +    Properties:
> +        name: Property name
> +        value: Proper value as a bytearray
> +    """
> +    def __init__(self, fdt, pdata):
> +        self.name = fdt_string(fdt._fdt, fdt32_to_cpu(pdata.nameoff))
> +        self.value = bytearray(fdt32_to_cpu(pdata.len))
> +        pylibfdt_copy_value(self.value, pdata)

Hm.  The Property object is now just a container for name & value.  So
I think it's a bit weird to do the unwrapping from its constructor.  I
think the construction should just take name and value strings (or
bytearrays), and the caller constructing it should do the unwrapping
calls.

> +%}
> +
> +%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_value’:
> + * 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_value(char *str, size_t size,
> +			const struct fdt_property *prop);

This still seems convoluted to me.  Maybe I'm misunderstanding
something about SWIG.  But instead of using pylibfdt_copy_value() as
an intermediary, couldn't you create a custom typemap that directly
maps struct fdt_property *prop (as an outbound value) to a Python
tuple, doing the copy right there.

Oh.. also if you do need this function, use 'uint8_t *val' instead
of 'char *str' to reinforce the fact that this is a bytestring not a
C-style string we're dealing with.

> +/* 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_value() - Copy value from a property to the given buffer
> + *
> + * This is used by the Property class to place the contents of a property
> + * into a bytearray.
> + *
> + * @buf: Destination pointer (typically the start of the bytearray)
> + * @size: Number of bytes to copy (size of bytearray)
> + * @prop: Property to copy
> + */
> +void pylibfdt_copy_value(char *buf, size_t size, const struct fdt_property *prop)
> +{
> +	memcpy(buf, prop + 1, size);
> +}
> +
> +%}
> +
> +%apply int *OUTPUT { 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);
> +}

Since you already have a custom typemap here for getprop(), couldn't
you drop the now unnecessary length (because Python strings know their
length) here, instead of doing it from the Python side?

> +
> +/* We have both struct fdt_property and a function fdt_property() */
> +%warnfilter(302) fdt_property;
> +
> +/* 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);
> +
> +%include <../libfdt/libfdt.h>
> 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 v5 1/5] Add an initial Python library for libfdt

[Simon Glass]

Hi David,

On 14 February 2017 at 22:29, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Feb 14, 2017 at 08:51:56PM -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 v5:
>> - Use a 'quiet' parameter instead of quiet versions of functions
>> - Add a Property object to hold a property's name and value
>> - Drop the data() and string() functions which are not needed now
>> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
>> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
>> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
>> - Use $(SWIG) to call swig from the Makefile
>> - Review function comments
>>
>> Changes in v4:
>> - Make the library less pythonic to avoid a shaky illusion
>> - Drop classes for Node and Prop, along with associated methods
>> - Include libfdt.h instead of repeating it
>> - Add support for fdt_getprop()
>> - Bring in all libfdt functions (but Python support is missing for many)
>> - Add full comments for Python methods
>>
>> Changes in v3:
>> - Make the library more pythonic
>> - Add classes for Node and Prop along with methods
>> - Add an exception class
>> - Use Python to generate exeptions instead of SWIG
>>
>> Changes in v2:
>> - Add exceptions when functions return an error
>> - Correct Python naming to following PEP8
>> - Use a class to encapsulate the various methods
>> - Include fdt.h instead of redefining struct fdt_property
>> - Use bytearray to avoid the SWIG warning 454
>> - Add comments
>>
>>  Makefile                   |   1 +
>>  pylibfdt/.gitignore        |   3 +
>>  pylibfdt/Makefile.pylibfdt |  18 ++
>>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
>>  pylibfdt/setup.py          |  34 ++++
>>  5 files changed, 521 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 check_err_null(val, quiet=[]):
>> +    """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: Errors to ignore (empty to raise on all errors)
>> +
>> +    Returns:
>> +        val if val is a list, None if not
>> +
>> +    Raises
>> +        FdtException if val indicates an error was reported and the error
>> +        is not in @quiet.
>> +    """
>> +    # Normally a tuple is returned which contains the data and its length.
>
> Is it a tuple returned..?
>
>> +    # If we get just an integer error code, it means the function failed.
>> +    if not isinstance(val, list):
>
> ..or a list?  Seems like either the comment or the code must be
> incorrect here.

OK.

>
> Come to that, what is it tells swig to map fdt_propery_by_offset() and
> the like to the pair of values?  How does it know how to detect the
> error cases?
>
> From the usage, I take it that in the success case val[0] is a string
> or bytearray containing the relevant chunk of data.  Remind me what
> the second value is?

It's the length, or error number. The final arg to
fdt_get_property_by_offset() is int *len.

[..]


>> +
>> +/* This is used to copy property data into a bytearray */
>> +%mypybuffer_mutable_binary(char *str, size_t size);
>> +void pylibfdt_copy_value(char *str, size_t size,
>> +                     const struct fdt_property *prop);
>
> This still seems convoluted to me.  Maybe I'm misunderstanding
> something about SWIG.  But instead of using pylibfdt_copy_value() as
> an intermediary, couldn't you create a custom typemap that directly
> maps struct fdt_property *prop (as an outbound value) to a Python
> tuple, doing the copy right there.

Possibly, but here I am trying to use a standard typemap to do this
and it would involve changing the error handling.

The other problem is that the output typemap would need access to the
device tree (fdt input parameter) in order to look up the name of the
property.

I suspect a full-custom function could do this, but then it would need
to written separately for each of the functions that returns struct
fdt_property.

Also my enthusiasm for more SWIG wrangling is waning faster than a
schooner at the races...perhaps someone other than myself can plot a
course to a better solution here?

>
> Oh.. also if you do need this function, use 'uint8_t *val' instead
> of 'char *str' to reinforce the fact that this is a bytestring not a
> C-style string we're dealing with.

OK.

[...]

>> +
>> +%apply int *OUTPUT { 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);
>> +}
>
> Since you already have a custom typemap here for getprop(), couldn't
> you drop the now unnecessary length (because Python strings know their
> length) here, instead of doing it from the Python side?

The length is necessary if it is an error number (the last parameter
to fdt_getprop()).

Regards,
Simon

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

[David Gibson]

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

On Thu, Feb 16, 2017 at 09:48:22PM -0700, Simon Glass wrote:
> Hi David,
> 
> On 14 February 2017 at 22:29, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Tue, Feb 14, 2017 at 08:51:56PM -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 v5:
> >> - Use a 'quiet' parameter instead of quiet versions of functions
> >> - Add a Property object to hold a property's name and value
> >> - Drop the data() and string() functions which are not needed now
> >> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
> >> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
> >> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
> >> - Use $(SWIG) to call swig from the Makefile
> >> - Review function comments
> >>
> >> Changes in v4:
> >> - Make the library less pythonic to avoid a shaky illusion
> >> - Drop classes for Node and Prop, along with associated methods
> >> - Include libfdt.h instead of repeating it
> >> - Add support for fdt_getprop()
> >> - Bring in all libfdt functions (but Python support is missing for many)
> >> - Add full comments for Python methods
> >>
> >> Changes in v3:
> >> - Make the library more pythonic
> >> - Add classes for Node and Prop along with methods
> >> - Add an exception class
> >> - Use Python to generate exeptions instead of SWIG
> >>
> >> Changes in v2:
> >> - Add exceptions when functions return an error
> >> - Correct Python naming to following PEP8
> >> - Use a class to encapsulate the various methods
> >> - Include fdt.h instead of redefining struct fdt_property
> >> - Use bytearray to avoid the SWIG warning 454
> >> - Add comments
> >>
> >>  Makefile                   |   1 +
> >>  pylibfdt/.gitignore        |   3 +
> >>  pylibfdt/Makefile.pylibfdt |  18 ++
> >>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
> >>  pylibfdt/setup.py          |  34 ++++
> >>  5 files changed, 521 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 check_err_null(val, quiet=[]):
> >> +    """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: Errors to ignore (empty to raise on all errors)
> >> +
> >> +    Returns:
> >> +        val if val is a list, None if not
> >> +
> >> +    Raises
> >> +        FdtException if val indicates an error was reported and the error
> >> +        is not in @quiet.
> >> +    """
> >> +    # Normally a tuple is returned which contains the data and its length.
> >
> > Is it a tuple returned..?
> >
> >> +    # If we get just an integer error code, it means the function failed.
> >> +    if not isinstance(val, list):
> >
> > ..or a list?  Seems like either the comment or the code must be
> > incorrect here.
> 
> OK.
> 
> >
> > Come to that, what is it tells swig to map fdt_propery_by_offset() and
> > the like to the pair of values?  How does it know how to detect the
> > error cases?
> >
> > From the usage, I take it that in the success case val[0] is a string
> > or bytearray containing the relevant chunk of data.  Remind me what
> > the second value is?
> 
> It's the length, or error number. The final arg to
> fdt_get_property_by_offset() is int *len.

Ok.  Why is it sometimes a tuple and sometimes a bare error value
then?  I would have expect it to always return a 2-tuple, the first
being the return value (maybe NULL/None) and the second being the
len/err value returned by reference.

I guess that's a swig question rather than  question for you, though.

> [..]
> 
> 
> >> +
> >> +/* This is used to copy property data into a bytearray */
> >> +%mypybuffer_mutable_binary(char *str, size_t size);
> >> +void pylibfdt_copy_value(char *str, size_t size,
> >> +                     const struct fdt_property *prop);
> >
> > This still seems convoluted to me.  Maybe I'm misunderstanding
> > something about SWIG.  But instead of using pylibfdt_copy_value() as
> > an intermediary, couldn't you create a custom typemap that directly
> > maps struct fdt_property *prop (as an outbound value) to a Python
> > tuple, doing the copy right there.
> 
> Possibly, but here I am trying to use a standard typemap to do this
> and it would involve changing the error handling.

Hm, ok.

> The other problem is that the output typemap would need access to the
> device tree (fdt input parameter) in order to look up the name of the
> property.

Ah.. yes.  But you could have the "raw" function return a tuple with
the name offset and value, then just look up the value in Python
code using fdt_string().

> I suspect a full-custom function could do this, but then it would need
> to written separately for each of the functions that returns struct
> fdt_property.
> 
> Also my enthusiasm for more SWIG wrangling is waning faster than a
> schooner at the races...perhaps someone other than myself can plot a
> course to a better solution here?

Heh, fair enough.

> 
> >
> > Oh.. also if you do need this function, use 'uint8_t *val' instead
> > of 'char *str' to reinforce the fact that this is a bytestring not a
> > C-style string we're dealing with.
> 
> OK.
> 
> [...]
> 
> >> +
> >> +%apply int *OUTPUT { 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);
> >> +}
> >
> > Since you already have a custom typemap here for getprop(), couldn't
> > you drop the now unnecessary length (because Python strings know their
> > length) here, instead of doing it from the Python side?
> 
> The length is necessary if it is an error number (the last parameter
> to fdt_getprop()).

Ah, of course.

-- 
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 v5 1/5] Add an initial Python library for libfdt

[Simon Glass]

Hi David,

On 19 February 2017 at 20:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Feb 16, 2017 at 09:48:22PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> On 14 February 2017 at 22:29, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Tue, Feb 14, 2017 at 08:51:56PM -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 v5:
>> >> - Use a 'quiet' parameter instead of quiet versions of functions
>> >> - Add a Property object to hold a property's name and value
>> >> - Drop the data() and string() functions which are not needed now
>> >> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
>> >> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
>> >> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
>> >> - Use $(SWIG) to call swig from the Makefile
>> >> - Review function comments
>> >>
>> >> Changes in v4:
>> >> - Make the library less pythonic to avoid a shaky illusion
>> >> - Drop classes for Node and Prop, along with associated methods
>> >> - Include libfdt.h instead of repeating it
>> >> - Add support for fdt_getprop()
>> >> - Bring in all libfdt functions (but Python support is missing for many)
>> >> - Add full comments for Python methods
>> >>
>> >> Changes in v3:
>> >> - Make the library more pythonic
>> >> - Add classes for Node and Prop along with methods
>> >> - Add an exception class
>> >> - Use Python to generate exeptions instead of SWIG
>> >>
>> >> Changes in v2:
>> >> - Add exceptions when functions return an error
>> >> - Correct Python naming to following PEP8
>> >> - Use a class to encapsulate the various methods
>> >> - Include fdt.h instead of redefining struct fdt_property
>> >> - Use bytearray to avoid the SWIG warning 454
>> >> - Add comments
>> >>
>> >>  Makefile                   |   1 +
>> >>  pylibfdt/.gitignore        |   3 +
>> >>  pylibfdt/Makefile.pylibfdt |  18 ++
>> >>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
>> >>  pylibfdt/setup.py          |  34 ++++
>> >>  5 files changed, 521 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 check_err_null(val, quiet=[]):
>> >> +    """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: Errors to ignore (empty to raise on all errors)
>> >> +
>> >> +    Returns:
>> >> +        val if val is a list, None if not
>> >> +
>> >> +    Raises
>> >> +        FdtException if val indicates an error was reported and the error
>> >> +        is not in @quiet.
>> >> +    """
>> >> +    # Normally a tuple is returned which contains the data and its length.
>> >
>> > Is it a tuple returned..?
>> >
>> >> +    # If we get just an integer error code, it means the function failed.
>> >> +    if not isinstance(val, list):
>> >
>> > ..or a list?  Seems like either the comment or the code must be
>> > incorrect here.
>>
>> OK.
>>
>> >
>> > Come to that, what is it tells swig to map fdt_propery_by_offset() and
>> > the like to the pair of values?  How does it know how to detect the
>> > error cases?
>> >
>> > From the usage, I take it that in the success case val[0] is a string
>> > or bytearray containing the relevant chunk of data.  Remind me what
>> > the second value is?
>>
>> It's the length, or error number. The final arg to
>> fdt_get_property_by_offset() is int *len.
>
> Ok.  Why is it sometimes a tuple and sometimes a bare error value
> then?  I would have expect it to always return a 2-tuple, the first
> being the return value (maybe NULL/None) and the second being the
> len/err value returned by reference.
>
> I guess that's a swig question rather than  question for you, though.

Well it's just the way that it seems to work. I think it makes some
sense in that we don't want to return a NULL pointer into Python. But
I agree it is a bit odd and it took me a while to get used to it.

Here is the generated code. I'm not sure what SWIG_IsTmpObj() does exactly...

  result = (struct fdt_property *)fdt_get_property_by_offset((void
const *)arg1,arg2,arg3);
  resultobj = SWIG_NewPointerObj(SWIG_as_voidptr(result),
SWIGTYPE_p_fdt_property, 0 |  0 );
  if (SWIG_IsTmpObj(res3)) {
    resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_From_int((*arg3)));
  } else {
    int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN |  0 ) :  0 ;
    resultobj = SWIG_Python_AppendOutput(resultobj,
SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_int, new_flags));
  }

Regards,
Simon

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

[David Gibson]

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

On Tue, Feb 21, 2017 at 11:07:58AM -0700, Simon Glass wrote:
> Hi David,
> 
> On 19 February 2017 at 20:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Feb 16, 2017 at 09:48:22PM -0700, Simon Glass wrote:
> >> Hi David,
> >>
> >> On 14 February 2017 at 22:29, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >> > On Tue, Feb 14, 2017 at 08:51:56PM -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 v5:
> >> >> - Use a 'quiet' parameter instead of quiet versions of functions
> >> >> - Add a Property object to hold a property's name and value
> >> >> - Drop the data() and string() functions which are not needed now
> >> >> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
> >> >> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
> >> >> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
> >> >> - Use $(SWIG) to call swig from the Makefile
> >> >> - Review function comments
> >> >>
> >> >> Changes in v4:
> >> >> - Make the library less pythonic to avoid a shaky illusion
> >> >> - Drop classes for Node and Prop, along with associated methods
> >> >> - Include libfdt.h instead of repeating it
> >> >> - Add support for fdt_getprop()
> >> >> - Bring in all libfdt functions (but Python support is missing for many)
> >> >> - Add full comments for Python methods
> >> >>
> >> >> Changes in v3:
> >> >> - Make the library more pythonic
> >> >> - Add classes for Node and Prop along with methods
> >> >> - Add an exception class
> >> >> - Use Python to generate exeptions instead of SWIG
> >> >>
> >> >> Changes in v2:
> >> >> - Add exceptions when functions return an error
> >> >> - Correct Python naming to following PEP8
> >> >> - Use a class to encapsulate the various methods
> >> >> - Include fdt.h instead of redefining struct fdt_property
> >> >> - Use bytearray to avoid the SWIG warning 454
> >> >> - Add comments
> >> >>
> >> >>  Makefile                   |   1 +
> >> >>  pylibfdt/.gitignore        |   3 +
> >> >>  pylibfdt/Makefile.pylibfdt |  18 ++
> >> >>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
> >> >>  pylibfdt/setup.py          |  34 ++++
> >> >>  5 files changed, 521 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 check_err_null(val, quiet=[]):
> >> >> +    """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: Errors to ignore (empty to raise on all errors)
> >> >> +
> >> >> +    Returns:
> >> >> +        val if val is a list, None if not
> >> >> +
> >> >> +    Raises
> >> >> +        FdtException if val indicates an error was reported and the error
> >> >> +        is not in @quiet.
> >> >> +    """
> >> >> +    # Normally a tuple is returned which contains the data and its length.
> >> >
> >> > Is it a tuple returned..?
> >> >
> >> >> +    # If we get just an integer error code, it means the function failed.
> >> >> +    if not isinstance(val, list):
> >> >
> >> > ..or a list?  Seems like either the comment or the code must be
> >> > incorrect here.
> >>
> >> OK.
> >>
> >> >
> >> > Come to that, what is it tells swig to map fdt_propery_by_offset() and
> >> > the like to the pair of values?  How does it know how to detect the
> >> > error cases?
> >> >
> >> > From the usage, I take it that in the success case val[0] is a string
> >> > or bytearray containing the relevant chunk of data.  Remind me what
> >> > the second value is?
> >>
> >> It's the length, or error number. The final arg to
> >> fdt_get_property_by_offset() is int *len.
> >
> > Ok.  Why is it sometimes a tuple and sometimes a bare error value
> > then?  I would have expect it to always return a 2-tuple, the first
> > being the return value (maybe NULL/None) and the second being the
> > len/err value returned by reference.
> >
> > I guess that's a swig question rather than  question for you, though.
> 
> Well it's just the way that it seems to work. I think it makes some
> sense in that we don't want to return a NULL pointer into Python. But
> I agree it is a bit odd and it took me a while to get used to it.

I suppose so.  Although None seems a pretty obvious way to translate
the NULL as well.

> Here is the generated code. I'm not sure what SWIG_IsTmpObj() does exactly...
> 
>   result = (struct fdt_property *)fdt_get_property_by_offset((void
> const *)arg1,arg2,arg3);
>   resultobj = SWIG_NewPointerObj(SWIG_as_voidptr(result),
> SWIGTYPE_p_fdt_property, 0 |  0 );
>   if (SWIG_IsTmpObj(res3)) {
>     resultobj = SWIG_Python_AppendOutput(resultobj, SWIG_From_int((*arg3)));
>   } else {
>     int new_flags = SWIG_IsNewObj(res3) ? (SWIG_POINTER_OWN |  0 ) :  0 ;
>     resultobj = SWIG_Python_AppendOutput(resultobj,
> SWIG_NewPointerObj((void*)(arg3), SWIGTYPE_p_int, new_flags));
>   }

Thanks for the info.

-- 
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 v5 1/5] Add an initial Python library for libfdt

[David Gibson]

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

On Tue, Feb 14, 2017 at 08:51:56PM -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 v5:
> - Use a 'quiet' parameter instead of quiet versions of functions
> - Add a Property object to hold a property's name and value
> - Drop the data() and string() functions which are not needed now
> - Rename pylibfdt_copy_data() tp pylibfdt_copy_value()
> - Change order of libfdt.h inclusion to avoid #ifdef around libfdt macros
> - Drop fdt_offset_ptr() and fdt_getprop_namelen() from the swig interface
> - Use $(SWIG) to call swig from the Makefile
> - Review function comments
> 
> Changes in v4:
> - Make the library less pythonic to avoid a shaky illusion
> - Drop classes for Node and Prop, along with associated methods
> - Include libfdt.h instead of repeating it
> - Add support for fdt_getprop()
> - Bring in all libfdt functions (but Python support is missing for many)
> - Add full comments for Python methods
> 
> Changes in v3:
> - Make the library more pythonic
> - Add classes for Node and Prop along with methods
> - Add an exception class
> - Use Python to generate exeptions instead of SWIG
> 
> Changes in v2:
> - Add exceptions when functions return an error
> - Correct Python naming to following PEP8
> - Use a class to encapsulate the various methods
> - Include fdt.h instead of redefining struct fdt_property
> - Use bytearray to avoid the SWIG warning 454
> - Add comments
> 
>  Makefile                   |   1 +
>  pylibfdt/.gitignore        |   3 +
>  pylibfdt/Makefile.pylibfdt |  18 ++
>  pylibfdt/libfdt.swig       | 465 +++++++++++++++++++++++++++++++++++++++++++++
>  pylibfdt/setup.py          |  34 ++++
>  5 files changed, 521 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/Makefile b/Makefile
> index ce05eba..1c48210 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -22,6 +22,7 @@ CFLAGS = -g -Os -fPIC -Werror $(WARNINGS)
>  
>  BISON = bison
>  LEX = flex
> +SWIG = swig
>  
>  INSTALL = /usr/bin/install
>  DESTDIR =
> 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..0c0b390
> --- /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..d538b3e
> --- /dev/null
> +++ b/pylibfdt/libfdt.swig
> @@ -0,0 +1,465 @@
> +/*
> + * 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)
> +
> +# Pass this as the 'quiet' parameter to return -ENOTFOUND on NOTFOUND errors,
> +# instead of raising an exception.
> +QUIET_NOTFOUND = [NOTFOUND]
> +
> +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]

Sorry, missed this on the first pass.  I dislike this function,
because it presumes that some bytes in the fdt have already been
converted into a Python integer, with the wrong endianness.  If a
value makes it into a Python int (as opposed to a string or bytes
object) it should already have been endian converted.

> +
> +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=[]):
> +    """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: Errors to ignore (empty to raise on all errors)
> +
> +    Returns:
> +        val if val >= 0
> +
> +    Raises
> +        FdtException if val < 0
> +    """
> +    if val < 0:
> +        if -val not in quiet:
> +            raise FdtException(val)
> +    return val
> +
> +def check_err_null(val, quiet=[]):
> +    """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: Errors to ignore (empty to raise on all errors)
> +
> +    Returns:
> +        val if val is a list, None if not
> +
> +    Raises
> +        FdtException if val indicates an error was reported and the error
> +        is not in @quiet.
> +    """
> +    # 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 -val not in quiet:
> +            raise FdtException(val)
> +    return val
> +
> +class Fdt:
> +    """Device tree class, supporting all operations
> +
> +    The Fdt object is created is created from a device tree binary file,
> +    e.g. with something like:
> +
> +       fdt = Fdt(open("filename.dtb").read())
> +
> +    Operations can then be performed using the methods in this class. Each
> +    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
> +
> +    All methods raise an FdtException if an error occurs. To avoid this
> +    behaviour a 'quiet' parameter is provided for some functions. This
> +    defaults to empty, but you can pass a list of errors that you expect.
> +    If one of these errors occurs, the function will return an error number
> +    (e.g. -NOTFOUND).
> +    """
> +    def __init__(self, data):
> +        self._fdt = bytearray(data)
> +        check_err(fdt_check_header(self._fdt));
> +
> +    def path_offset(self, path, quiet=[]):
> +        """Get the offset for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Node offset
> +
> +        Raises
> +            FdtException if the path is not valid or not found
> +        """
> +        return check_err(fdt_path_offset(self._fdt, path), quiet)
> +
> +    def first_property_offset(self, nodeoffset, quiet=[]):
> +        """Get the offset of the first property in a node offset
> +
> +        Args:
> +            nodeoffset: Offset to the node to check
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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),
> +                         quiet)
> +
> +    def next_property_offset(self, prop_offset, quiet=[]):
> +        """Get the next property in a node
> +
> +        Args:
> +            prop_offset: Offset of the previous property
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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),
> +                         quiet)
> +
> +    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, quiet=[]):
> +        """Obtains a property that can be examined
> +
> +        Args:
> +            prop_offset: Offset of property (e.g. from first_property_offset())
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Property object, or None if not found
> +
> +        Raises:
> +            FdtException on error (e.g. invalid prop_offset or device
> +            tree format)
> +        """
> +        pdata = check_err_null(
> +                fdt_get_property_by_offset(self._fdt, prop_offset), quiet)
> +        if isinstance(pdata, (int)):
> +            return pdata
> +        return Property(self, pdata[0])
> +
> +    def first_subnode(self, nodeoffset, quiet=[]):
> +        """Find the first subnode of a parent node
> +
> +        Args:
> +            nodeoffset: Node offset of parent node
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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), quiet)
> +
> +    def next_subnode(self, nodeoffset, quiet=[]):
> +        """Find the next subnode
> +
> +        Args:
> +            nodeoffset: Node offset of previous subnode
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        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), quiet)
> +
> +    def totalsize(self):
> +        """Return the total size of the device tree
> +
> +        Returns:
> +            Total tree size in bytes
> +        """
> +        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
> +        """
> +        return check_err(fdt_off_dt_struct(self._fdt))
> +
> +    def pack(self, quiet=[]):
> +        """Pack the device tree to remove unused space
> +
> +        This adjusts the tree in place.
> +
> +        Args:
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Raises:
> +            FdtException if any error occurs
> +        """
> +        return check_err(fdt_pack(self._fdt), quiet)
> +
> +    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, quiet=[]):
> +        """Get a property from a node
> +
> +        Args:
> +            nodeoffset: Node offset containing property to get
> +            prop_name: Name of property to get
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Value of property as a string, or -ve error number
> +
> +        Raises:
> +            FdtError if any error occurs (e.g. the property is not found)
> +        """
> +        pdata = check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
> +                               quiet)
> +        if isinstance(pdata, (int)):
> +            return pdata
> +        return pdata[0]
> +
> +
> +class Property:
> +    """Holds a device tree property name and value.
> +
> +    This holds a copy of a property taken from the device tree. It does not
> +    reference the device tree, so if anything changes in the device tree,
> +    a Property object will remain valid.
> +
> +    Properties:
> +        name: Property name
> +        value: Proper value as a bytearray
> +    """
> +    def __init__(self, fdt, pdata):
> +        self.name = fdt_string(fdt._fdt, fdt32_to_cpu(pdata.nameoff))
> +        self.value = bytearray(fdt32_to_cpu(pdata.len))
> +        pylibfdt_copy_value(self.value, pdata)
> +%}
> +
> +%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_value’:
> + * 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_value(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_value() - Copy value from a property to the given buffer
> + *
> + * This is used by the Property class to place the contents of a property
> + * into a bytearray.
> + *
> + * @buf: Destination pointer (typically the start of the bytearray)
> + * @size: Number of bytes to copy (size of bytearray)
> + * @prop: Property to copy
> + */
> +void pylibfdt_copy_value(char *buf, size_t size, const struct fdt_property *prop)
> +{
> +	memcpy(buf, prop + 1, size);
> +}
> +
> +%}
> +
> +%apply int *OUTPUT { 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;
> +
> +/* 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);
> +
> +%include <../libfdt/libfdt.h>
> 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 --]

[PATCH v5 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 v5:
- Adjust tests to match new swig bindings

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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh      |  19 +++-
 2 files changed, 285 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..eac2544
--- /dev/null
+++ b/tests/pylibfdt_tests.py
@@ -0,0 +1,267 @@
+# 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, QUIET_NOTFOUND
+
+# 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(node, QUIET_NOTFOUND)
+        while poffset > 0:
+            prop = self.fdt.get_property_by_offset(poffset)
+            prop_list.append(prop.name)
+            poffset = self.fdt.next_property_offset(poffset, QUIET_NOTFOUND)
+        return prop_list
+
+    def testImport(self):
+        """Check that we can import the library correctly"""
+        self.assertEquals(type(libfdt), type(sys))
+
+    def testBadFdt(self):
+        """Check that a filename provided accidentally is not accepted"""
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADMAGIC)):
+            fdt = libfdt.Fdt('a string')
+
+    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('/wibble', QUIET_NOTFOUND),
+                          -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(ROOT_PROPS[-1],
+                                                        QUIET_NOTFOUND),
+                          -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(107, QUIET_NOTFOUND)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
+
+        node = self.fdt.path_offset('/subnode@1/ss1')
+        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
+                          -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 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)
+        prop = self.fdt.get_property_by_offset(poffset)
+        self.assertEquals(prop.name, 'compatible')
+        self.assertEquals(prop.value, 'test_tree1\0')
+
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
+            self.fdt.get_property_by_offset(-2)
+        self.assertEquals(
+                -libfdt.BADOFFSET,
+                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
+
+    def testGetProp(self):
+        """Check that we can read the contents of a property by name"""
+        root = self.fdt.path_offset('/')
+        value = self.fdt.getprop(root, "compatible")
+        self.assertEquals(value, 'test_tree1\0')
+        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
+                                                             QUIET_NOTFOUND))
+
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.getprop(root, 'missing')
+
+        node = self.fdt.path_offset('/subnode@1/subsubnode')
+        value = self.fdt.getprop(node, "compatible")
+        self.assertEquals(value, '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(0, QUIET_NOTFOUND)
+        while node >= 0:
+            node_list.append(self.fdt.get_name(node))
+            node = self.fdt.next_subnode(node, QUIET_NOTFOUND)
+        self.assertEquals(node_list, ['subnode@1', 'subnode@2'])
+
+    def testFirstNextSubnodeOffsetExceptions(self):
+        """Check except handling for first/next subnode functions"""
+        node = self.fdt.path_offset('/subnode@1/subsubnode', QUIET_NOTFOUND)
+        self.assertEquals(self.fdt.first_subnode(node, QUIET_NOTFOUND),
+                          -libfdt.NOTFOUND)
+        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
+            self.fdt.first_subnode(node)
+
+        node = self.fdt.path_offset('/subnode@1/ss1', QUIET_NOTFOUND)
+        self.assertEquals(self.fdt.next_subnode(node, QUIET_NOTFOUND),
+                          -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('/subnode@2', QUIET_NOTFOUND)
+        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

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

[David Gibson]

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

On Tue, Feb 14, 2017 at 08:51:57PM -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>
> ---
> 
> Changes in v5:
> - Adjust tests to match new swig bindings
> 
> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/run_tests.sh      |  19 +++-
>  2 files changed, 285 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..eac2544
> --- /dev/null
> +++ b/tests/pylibfdt_tests.py
> @@ -0,0 +1,267 @@
> +# 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, QUIET_NOTFOUND
> +
> +# 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(node, QUIET_NOTFOUND)
> +        while poffset > 0:
> +            prop = self.fdt.get_property_by_offset(poffset)
> +            prop_list.append(prop.name)
> +            poffset = self.fdt.next_property_offset(poffset, QUIET_NOTFOUND)
> +        return prop_list
> +
> +    def testImport(self):
> +        """Check that we can import the library correctly"""
> +        self.assertEquals(type(libfdt), type(sys))
> +
> +    def testBadFdt(self):
> +        """Check that a filename provided accidentally is not accepted"""
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADMAGIC)):
> +            fdt = libfdt.Fdt('a string')
> +
> +    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)

This test is potentially fragile.  Eventually it would be nice to be
able to run the Python tests expecting test_tree1 on any of the copies
of test_tree1 we generate.  Those are required to be semantically
identicaly (including node/property order) to test_tree1.dtb.
However, some versions won't preserve exact offsets - for example
there's a sequence of tests where we insert additional nops in the
encoding to test handling of that.  That's why tests/path_offset.c,
for example, checks the behaviour of path_offset() against
subnode_offset() and knowing what property and node names are supposed
to be present, rather than against explicit known offsets.

> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.path_offset('/wibble')
> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
> +                          -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(ROOT_PROPS[-1],
> +                                                        QUIET_NOTFOUND),
> +                          -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)

Same issue here.

> +        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(107, QUIET_NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
> +
> +        node = self.fdt.path_offset('/subnode@1/ss1')
> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
> +                          -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 testGetPropertyByOffset(self):
> +        """Check that we can read the name and contents of a property"""
> +        root = self.fdt.path_offset('/')

No point to this - offset of / is always 0.  If you want to test that
happens, make it a separate testcase.

> +        poffset = self.fdt.first_property_offset(root)
> +        prop = self.fdt.get_property_by_offset(poffset)
> +        self.assertEquals(prop.name, 'compatible')
> +        self.assertEquals(prop.value, 'test_tree1\0')
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.get_property_by_offset(-2)
> +        self.assertEquals(
> +                -libfdt.BADOFFSET,
> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
> +
> +    def testGetProp(self):
> +        """Check that we can read the contents of a property by name"""
> +        root = self.fdt.path_offset('/')
> +        value = self.fdt.getprop(root, "compatible")
> +        self.assertEquals(value, 'test_tree1\0')
> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
> +                                                             QUIET_NOTFOUND))

For testing, it might be useful to add a special value you can set the
quiet parameter to to make all errors quiet.

> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.getprop(root, 'missing')
> +
> +        node = self.fdt.path_offset('/subnode@1/subsubnode')
> +        value = self.fdt.getprop(node, "compatible")
> +        self.assertEquals(value, '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(0, QUIET_NOTFOUND)
> +        while node >= 0:
> +            node_list.append(self.fdt.get_name(node))
> +            node = self.fdt.next_subnode(node, QUIET_NOTFOUND)
> +        self.assertEquals(node_list, ['subnode@1', 'subnode@2'])
> +
> +    def testFirstNextSubnodeOffsetExceptions(self):
> +        """Check except handling for first/next subnode functions"""
> +        node = self.fdt.path_offset('/subnode@1/subsubnode', QUIET_NOTFOUND)
> +        self.assertEquals(self.fdt.first_subnode(node, QUIET_NOTFOUND),
> +                          -libfdt.NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.first_subnode(node)
> +
> +        node = self.fdt.path_offset('/subnode@1/ss1', QUIET_NOTFOUND)
> +        self.assertEquals(self.fdt.next_subnode(node, QUIET_NOTFOUND),
> +                          -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)

Potentially fragile again.  Here you could instead check against len()
of the actual fdt buffer read from the file.

> +        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('/subnode@2', QUIET_NOTFOUND)
> +        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}')

Any way you could do this without awk?  I'd prefer not to add another
build dependency.

> +
> +    # 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 v5 2/5] Add tests for pylibfdt

[Simon Glass]

Hi David,

On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Feb 14, 2017 at 08:51:57PM -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>
>> ---
>>
>> Changes in v5:
>> - Adjust tests to match new swig bindings
>>
>> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/run_tests.sh      |  19 +++-
>>  2 files changed, 285 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/pylibfdt_tests.py
>>

[..]

>> +    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)
>
> This test is potentially fragile.  Eventually it would be nice to be
> able to run the Python tests expecting test_tree1 on any of the copies
> of test_tree1 we generate.  Those are required to be semantically
> identicaly (including node/property order) to test_tree1.dtb.
> However, some versions won't preserve exact offsets - for example
> there's a sequence of tests where we insert additional nops in the
> encoding to test handling of that.  That's why tests/path_offset.c,
> for example, checks the behaviour of path_offset() against
> subnode_offset() and knowing what property and node names are supposed
> to be present, rather than against explicit known offsets.I'm

Yes it is fragile, will check it's >0 which should be safe.

Re the tests, I feel we should try to avoid testing all the same
things as the C code, when we could just test the interface. But it
might be easier just to duplicate the tests you as say.

>
>> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
>> +            self.fdt.path_offset('/wibble')
>> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
>> +                          -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(ROOT_PROPS[-1],
>> +                                                        QUIET_NOTFOUND),
>> +                          -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)
>
> Same issue here.

OK, I can just drop this one.

>
>> +        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(107, QUIET_NOTFOUND)
>> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
>> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
>> +
>> +        node = self.fdt.path_offset('/subnode@1/ss1')
>> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
>> +                          -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 testGetPropertyByOffset(self):
>> +        """Check that we can read the name and contents of a property"""
>> +        root = self.fdt.path_offset('/')
>
> No point to this - offset of / is always 0.  If you want to test that
> happens, make it a separate testcase.

I already have it above so will drop this.

>
>> +        poffset = self.fdt.first_property_offset(root)
>> +        prop = self.fdt.get_property_by_offset(poffset)
>> +        self.assertEquals(prop.name, 'compatible')
>> +        self.assertEquals(prop.value, 'test_tree1\0')
>> +
>> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
>> +            self.fdt.get_property_by_offset(-2)
>> +        self.assertEquals(
>> +                -libfdt.BADOFFSET,
>> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
>> +
>> +    def testGetProp(self):
>> +        """Check that we can read the contents of a property by name"""
>> +        root = self.fdt.path_offset('/')
>> +        value = self.fdt.getprop(root, "compatible")
>> +        self.assertEquals(value, 'test_tree1\0')
>> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
>> +                                                             QUIET_NOTFOUND))
>
> For testing, it might be useful to add a special value you can set the
> quiet parameter to to make all errors quiet.

Isn't that what I did?

Regards,
Simon

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

[David Gibson]

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

On Thu, Feb 16, 2017 at 09:48:28PM -0700, Simon Glass wrote:
> Hi David,
> 
> On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Tue, Feb 14, 2017 at 08:51:57PM -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>
> >> ---
> >>
> >> Changes in v5:
> >> - Adjust tests to match new swig bindings
> >>
> >> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/run_tests.sh      |  19 +++-
> >>  2 files changed, 285 insertions(+), 1 deletion(-)
> >>  create mode 100644 tests/pylibfdt_tests.py
> >>
> 
> [..]
> 
> >> +    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)
> >
> > This test is potentially fragile.  Eventually it would be nice to be
> > able to run the Python tests expecting test_tree1 on any of the copies
> > of test_tree1 we generate.  Those are required to be semantically
> > identicaly (including node/property order) to test_tree1.dtb.
> > However, some versions won't preserve exact offsets - for example
> > there's a sequence of tests where we insert additional nops in the
> > encoding to test handling of that.  That's why tests/path_offset.c,
> > for example, checks the behaviour of path_offset() against
> > subnode_offset() and knowing what property and node names are supposed
> > to be present, rather than against explicit known offsets.I'm
> 
> Yes it is fragile, will check it's >0 which should be safe.

Ok.

> Re the tests, I feel we should try to avoid testing all the same
> things as the C code, when we could just test the interface. But it
> might be easier just to duplicate the tests you as say.

I think so.  The set of tree1 tests is a good model, because it's
already a reasonably thorough test model of the basic libfdt
interfaces.

> >
> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> >> +            self.fdt.path_offset('/wibble')
> >> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
> >> +                          -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(ROOT_PROPS[-1],
> >> +                                                        QUIET_NOTFOUND),
> >> +                          -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)
> >
> > Same issue here.
> 
> OK, I can just drop this one.
> 
> >
> >> +        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(107, QUIET_NOTFOUND)
> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
> >> +
> >> +        node = self.fdt.path_offset('/subnode@1/ss1')
> >> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
> >> +                          -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 testGetPropertyByOffset(self):
> >> +        """Check that we can read the name and contents of a property"""
> >> +        root = self.fdt.path_offset('/')
> >
> > No point to this - offset of / is always 0.  If you want to test that
> > happens, make it a separate testcase.
> 
> I already have it above so will drop this.
> 
> >
> >> +        poffset = self.fdt.first_property_offset(root)
> >> +        prop = self.fdt.get_property_by_offset(poffset)
> >> +        self.assertEquals(prop.name, 'compatible')
> >> +        self.assertEquals(prop.value, 'test_tree1\0')
> >> +
> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> +            self.fdt.get_property_by_offset(-2)
> >> +        self.assertEquals(
> >> +                -libfdt.BADOFFSET,
> >> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
> >> +
> >> +    def testGetProp(self):
> >> +        """Check that we can read the contents of a property by name"""
> >> +        root = self.fdt.path_offset('/')
> >> +        value = self.fdt.getprop(root, "compatible")
> >> +        self.assertEquals(value, 'test_tree1\0')
> >> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
> >> +                                                             QUIET_NOTFOUND))
> >
> > For testing, it might be useful to add a special value you can set the
> > quiet parameter to to make all errors quiet.
> 
> Isn't that what I did?

Sorry, I wasn't very clear.  What I mean is that if you want to treat
*all* errors as quiet, at the moment you have to do
	self.whatever(..., quiet=[NOTFOUND, EXISTS, NOSPACE,...])

I was suggesting an extension to check_err() so you can instead say
'quiet=SOMETHING' where SOMETHING is a special value, and it will
interpret that as making all errors quiet.

-- 
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 v5 2/5] Add tests for pylibfdt

[Simon Glass]

Hi David,

On 19 February 2017 at 20:42, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Feb 16, 2017 at 09:48:28PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Tue, Feb 14, 2017 at 08:51:57PM -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>
>> >> ---
>> >>
>> >> Changes in v5:
>> >> - Adjust tests to match new swig bindings
>> >>
>> >> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/run_tests.sh      |  19 +++-
>> >>  2 files changed, 285 insertions(+), 1 deletion(-)
>> >>  create mode 100644 tests/pylibfdt_tests.py
>> >>
>>
>> [..]
>>
>> >> +    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)
>> >
>> > This test is potentially fragile.  Eventually it would be nice to be
>> > able to run the Python tests expecting test_tree1 on any of the copies
>> > of test_tree1 we generate.  Those are required to be semantically
>> > identicaly (including node/property order) to test_tree1.dtb.
>> > However, some versions won't preserve exact offsets - for example
>> > there's a sequence of tests where we insert additional nops in the
>> > encoding to test handling of that.  That's why tests/path_offset.c,
>> > for example, checks the behaviour of path_offset() against
>> > subnode_offset() and knowing what property and node names are supposed
>> > to be present, rather than against explicit known offsets.I'm
>>
>> Yes it is fragile, will check it's >0 which should be safe.
>
> Ok.
>
>> Re the tests, I feel we should try to avoid testing all the same
>> things as the C code, when we could just test the interface. But it
>> might be easier just to duplicate the tests you as say.
>
> I think so.  The set of tree1 tests is a good model, because it's
> already a reasonably thorough test model of the basic libfdt
> interfaces.
>
>> >
>> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
>> >> +            self.fdt.path_offset('/wibble')
>> >> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
>> >> +                          -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(ROOT_PROPS[-1],
>> >> +                                                        QUIET_NOTFOUND),
>> >> +                          -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)
>> >
>> > Same issue here.
>>
>> OK, I can just drop this one.
>>
>> >
>> >> +        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(107, QUIET_NOTFOUND)
>> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
>> >> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
>> >> +
>> >> +        node = self.fdt.path_offset('/subnode@1/ss1')
>> >> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
>> >> +                          -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 testGetPropertyByOffset(self):
>> >> +        """Check that we can read the name and contents of a property"""
>> >> +        root = self.fdt.path_offset('/')
>> >
>> > No point to this - offset of / is always 0.  If you want to test that
>> > happens, make it a separate testcase.
>>
>> I already have it above so will drop this.
>>
>> >
>> >> +        poffset = self.fdt.first_property_offset(root)
>> >> +        prop = self.fdt.get_property_by_offset(poffset)
>> >> +        self.assertEquals(prop.name, 'compatible')
>> >> +        self.assertEquals(prop.value, 'test_tree1\0')
>> >> +
>> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
>> >> +            self.fdt.get_property_by_offset(-2)
>> >> +        self.assertEquals(
>> >> +                -libfdt.BADOFFSET,
>> >> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
>> >> +
>> >> +    def testGetProp(self):
>> >> +        """Check that we can read the contents of a property by name"""
>> >> +        root = self.fdt.path_offset('/')
>> >> +        value = self.fdt.getprop(root, "compatible")
>> >> +        self.assertEquals(value, 'test_tree1\0')
>> >> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
>> >> +                                                             QUIET_NOTFOUND))
>> >
>> > For testing, it might be useful to add a special value you can set the
>> > quiet parameter to to make all errors quiet.
>>
>> Isn't that what I did?
>
> Sorry, I wasn't very clear.  What I mean is that if you want to treat
> *all* errors as quiet, at the moment you have to do
>         self.whatever(..., quiet=[NOTFOUND, EXISTS, NOSPACE,...])
>
> I was suggesting an extension to check_err() so you can instead say
> 'quiet=SOMETHING' where SOMETHING is a special value, and it will
> interpret that as making all errors quiet.

OK I see. Could SOMETHING be

QUIET_ALL = [NOTFOUND, EXISTS, all other errors]

or are you wanting to take the dynamic typing further and use an
integer or something?

Regards,
Simon

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

[David Gibson]

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

On Tue, Feb 21, 2017 at 11:08:11AM -0700, Simon Glass wrote:
> Hi David,
> 
> On 19 February 2017 at 20:42, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Feb 16, 2017 at 09:48:28PM -0700, Simon Glass wrote:
> >> Hi David,
> >>
> >> On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >> > On Tue, Feb 14, 2017 at 08:51:57PM -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>
> >> >> ---
> >> >>
> >> >> Changes in v5:
> >> >> - Adjust tests to match new swig bindings
> >> >>
> >> >> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  tests/run_tests.sh      |  19 +++-
> >> >>  2 files changed, 285 insertions(+), 1 deletion(-)
> >> >>  create mode 100644 tests/pylibfdt_tests.py
> >> >>
> >>
> >> [..]
> >>
> >> >> +    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)
> >> >
> >> > This test is potentially fragile.  Eventually it would be nice to be
> >> > able to run the Python tests expecting test_tree1 on any of the copies
> >> > of test_tree1 we generate.  Those are required to be semantically
> >> > identicaly (including node/property order) to test_tree1.dtb.
> >> > However, some versions won't preserve exact offsets - for example
> >> > there's a sequence of tests where we insert additional nops in the
> >> > encoding to test handling of that.  That's why tests/path_offset.c,
> >> > for example, checks the behaviour of path_offset() against
> >> > subnode_offset() and knowing what property and node names are supposed
> >> > to be present, rather than against explicit known offsets.I'm
> >>
> >> Yes it is fragile, will check it's >0 which should be safe.
> >
> > Ok.
> >
> >> Re the tests, I feel we should try to avoid testing all the same
> >> things as the C code, when we could just test the interface. But it
> >> might be easier just to duplicate the tests you as say.
> >
> > I think so.  The set of tree1 tests is a good model, because it's
> > already a reasonably thorough test model of the basic libfdt
> > interfaces.
> >
> >> >
> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> >> >> +            self.fdt.path_offset('/wibble')
> >> >> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
> >> >> +                          -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(ROOT_PROPS[-1],
> >> >> +                                                        QUIET_NOTFOUND),
> >> >> +                          -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)
> >> >
> >> > Same issue here.
> >>
> >> OK, I can just drop this one.
> >>
> >> >
> >> >> +        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(107, QUIET_NOTFOUND)
> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
> >> >> +
> >> >> +        node = self.fdt.path_offset('/subnode@1/ss1')
> >> >> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
> >> >> +                          -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 testGetPropertyByOffset(self):
> >> >> +        """Check that we can read the name and contents of a property"""
> >> >> +        root = self.fdt.path_offset('/')
> >> >
> >> > No point to this - offset of / is always 0.  If you want to test that
> >> > happens, make it a separate testcase.
> >>
> >> I already have it above so will drop this.
> >>
> >> >
> >> >> +        poffset = self.fdt.first_property_offset(root)
> >> >> +        prop = self.fdt.get_property_by_offset(poffset)
> >> >> +        self.assertEquals(prop.name, 'compatible')
> >> >> +        self.assertEquals(prop.value, 'test_tree1\0')
> >> >> +
> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> +            self.fdt.get_property_by_offset(-2)
> >> >> +        self.assertEquals(
> >> >> +                -libfdt.BADOFFSET,
> >> >> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
> >> >> +
> >> >> +    def testGetProp(self):
> >> >> +        """Check that we can read the contents of a property by name"""
> >> >> +        root = self.fdt.path_offset('/')
> >> >> +        value = self.fdt.getprop(root, "compatible")
> >> >> +        self.assertEquals(value, 'test_tree1\0')
> >> >> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
> >> >> +                                                             QUIET_NOTFOUND))
> >> >
> >> > For testing, it might be useful to add a special value you can set the
> >> > quiet parameter to to make all errors quiet.
> >>
> >> Isn't that what I did?
> >
> > Sorry, I wasn't very clear.  What I mean is that if you want to treat
> > *all* errors as quiet, at the moment you have to do
> >         self.whatever(..., quiet=[NOTFOUND, EXISTS, NOSPACE,...])
> >
> > I was suggesting an extension to check_err() so you can instead say
> > 'quiet=SOMETHING' where SOMETHING is a special value, and it will
> > interpret that as making all errors quiet.
> 
> OK I see. Could SOMETHING be
> 
> QUIET_ALL = [NOTFOUND, EXISTS, all other errors]
> 
> or are you wanting to take the dynamic typing further and use an
> integer or something?

I was thinking of exploiting the dynamic typing (probably using a
special value, not an int, maybe 'True').  But really, either would be
fine.

-- 
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 v5 2/5] Add tests for pylibfdt

[Simon Glass]

Hi David,

On 21 February 2017 at 19:11, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Feb 21, 2017 at 11:08:11AM -0700, Simon Glass wrote:
>> Hi David,
>>
>> On 19 February 2017 at 20:42, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Thu, Feb 16, 2017 at 09:48:28PM -0700, Simon Glass wrote:
>> >> Hi David,
>> >>
>> >> On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> >> > On Tue, Feb 14, 2017 at 08:51:57PM -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>
>> >> >> ---
>> >> >>
>> >> >> Changes in v5:
>> >> >> - Adjust tests to match new swig bindings
>> >> >>
>> >> >> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >>  tests/run_tests.sh      |  19 +++-
>> >> >>  2 files changed, 285 insertions(+), 1 deletion(-)
>> >> >>  create mode 100644 tests/pylibfdt_tests.py
>> >> >>
>> >>
>> >> [..]
>> >>
>> >> >> +    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)
>> >> >
>> >> > This test is potentially fragile.  Eventually it would be nice to be
>> >> > able to run the Python tests expecting test_tree1 on any of the copies
>> >> > of test_tree1 we generate.  Those are required to be semantically
>> >> > identicaly (including node/property order) to test_tree1.dtb.
>> >> > However, some versions won't preserve exact offsets - for example
>> >> > there's a sequence of tests where we insert additional nops in the
>> >> > encoding to test handling of that.  That's why tests/path_offset.c,
>> >> > for example, checks the behaviour of path_offset() against
>> >> > subnode_offset() and knowing what property and node names are supposed
>> >> > to be present, rather than against explicit known offsets.I'm
>> >>
>> >> Yes it is fragile, will check it's >0 which should be safe.
>> >
>> > Ok.
>> >
>> >> Re the tests, I feel we should try to avoid testing all the same
>> >> things as the C code, when we could just test the interface. But it
>> >> might be easier just to duplicate the tests you as say.
>> >
>> > I think so.  The set of tree1 tests is a good model, because it's
>> > already a reasonably thorough test model of the basic libfdt
>> > interfaces.
>> >
>> >> >
>> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
>> >> >> +            self.fdt.path_offset('/wibble')
>> >> >> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
>> >> >> +                          -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(ROOT_PROPS[-1],
>> >> >> +                                                        QUIET_NOTFOUND),
>> >> >> +                          -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)
>> >> >
>> >> > Same issue here.
>> >>
>> >> OK, I can just drop this one.
>> >>
>> >> >
>> >> >> +        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(107, QUIET_NOTFOUND)
>> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
>> >> >> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
>> >> >> +
>> >> >> +        node = self.fdt.path_offset('/subnode@1/ss1')
>> >> >> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
>> >> >> +                          -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 testGetPropertyByOffset(self):
>> >> >> +        """Check that we can read the name and contents of a property"""
>> >> >> +        root = self.fdt.path_offset('/')
>> >> >
>> >> > No point to this - offset of / is always 0.  If you want to test that
>> >> > happens, make it a separate testcase.
>> >>
>> >> I already have it above so will drop this.
>> >>
>> >> >
>> >> >> +        poffset = self.fdt.first_property_offset(root)
>> >> >> +        prop = self.fdt.get_property_by_offset(poffset)
>> >> >> +        self.assertEquals(prop.name, 'compatible')
>> >> >> +        self.assertEquals(prop.value, 'test_tree1\0')
>> >> >> +
>> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
>> >> >> +            self.fdt.get_property_by_offset(-2)
>> >> >> +        self.assertEquals(
>> >> >> +                -libfdt.BADOFFSET,
>> >> >> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
>> >> >> +
>> >> >> +    def testGetProp(self):
>> >> >> +        """Check that we can read the contents of a property by name"""
>> >> >> +        root = self.fdt.path_offset('/')
>> >> >> +        value = self.fdt.getprop(root, "compatible")
>> >> >> +        self.assertEquals(value, 'test_tree1\0')
>> >> >> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
>> >> >> +                                                             QUIET_NOTFOUND))
>> >> >
>> >> > For testing, it might be useful to add a special value you can set the
>> >> > quiet parameter to to make all errors quiet.
>> >>
>> >> Isn't that what I did?
>> >
>> > Sorry, I wasn't very clear.  What I mean is that if you want to treat
>> > *all* errors as quiet, at the moment you have to do
>> >         self.whatever(..., quiet=[NOTFOUND, EXISTS, NOSPACE,...])
>> >
>> > I was suggesting an extension to check_err() so you can instead say
>> > 'quiet=SOMETHING' where SOMETHING is a special value, and it will
>> > interpret that as making all errors quiet.
>>
>> OK I see. Could SOMETHING be
>>
>> QUIET_ALL = [NOTFOUND, EXISTS, all other errors]
>>
>> or are you wanting to take the dynamic typing further and use an
>> integer or something?
>
> I was thinking of exploiting the dynamic typing (probably using a
> special value, not an int, maybe 'True').  But really, either would be
> fine.

Well I'll go with the tuple for consistency (i.e. to avoid a special
case). It's an internal detail so we can always change if it feels
wrong.

Regards,
Simon

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

[David Gibson]

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

On Tue, Feb 21, 2017 at 09:33:01PM -0700, Simon Glass wrote:
> Hi David,
> 
> On 21 February 2017 at 19:11, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Tue, Feb 21, 2017 at 11:08:11AM -0700, Simon Glass wrote:
> >> Hi David,
> >>
> >> On 19 February 2017 at 20:42, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >> > On Thu, Feb 16, 2017 at 09:48:28PM -0700, Simon Glass wrote:
> >> >> Hi David,
> >> >>
> >> >> On 14 February 2017 at 22:44, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6vUpdFzICT1y@public.gmane.orgd.au> wrote:
> >> >> > On Tue, Feb 14, 2017 at 08:51:57PM -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>
> >> >> >> ---
> >> >> >>
> >> >> >> Changes in v5:
> >> >> >> - Adjust tests to match new swig bindings
> >> >> >>
> >> >> >> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >>  tests/run_tests.sh      |  19 +++-
> >> >> >>  2 files changed, 285 insertions(+), 1 deletion(-)
> >> >> >>  create mode 100644 tests/pylibfdt_tests.py
> >> >> >>
> >> >>
> >> >> [..]
> >> >>
> >> >> >> +    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)
> >> >> >
> >> >> > This test is potentially fragile.  Eventually it would be nice to be
> >> >> > able to run the Python tests expecting test_tree1 on any of the copies
> >> >> > of test_tree1 we generate.  Those are required to be semantically
> >> >> > identicaly (including node/property order) to test_tree1.dtb.
> >> >> > However, some versions won't preserve exact offsets - for example
> >> >> > there's a sequence of tests where we insert additional nops in the
> >> >> > encoding to test handling of that.  That's why tests/path_offset.c,
> >> >> > for example, checks the behaviour of path_offset() against
> >> >> > subnode_offset() and knowing what property and node names are supposed
> >> >> > to be present, rather than against explicit known offsets.I'm
> >> >>
> >> >> Yes it is fragile, will check it's >0 which should be safe.
> >> >
> >> > Ok.
> >> >
> >> >> Re the tests, I feel we should try to avoid testing all the same
> >> >> things as the C code, when we could just test the interface. But it
> >> >> might be easier just to duplicate the tests you as say.
> >> >
> >> > I think so.  The set of tree1 tests is a good model, because it's
> >> > already a reasonably thorough test model of the basic libfdt
> >> > interfaces.
> >> >
> >> >> >
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> >> >> >> +            self.fdt.path_offset('/wibble')
> >> >> >> +        self.assertEquals(self.fdt.path_offset('/wibble', QUIET_NOTFOUND),
> >> >> >> +                          -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(ROOT_PROPS[-1],
> >> >> >> +                                                        QUIET_NOTFOUND),
> >> >> >> +                          -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)
> >> >> >
> >> >> > Same issue here.
> >> >>
> >> >> OK, I can just drop this one.
> >> >>
> >> >> >
> >> >> >> +        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(107, QUIET_NOTFOUND)
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
> >> >> >> +
> >> >> >> +        node = self.fdt.path_offset('/subnode@1/ss1')
> >> >> >> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
> >> >> >> +                          -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 testGetPropertyByOffset(self):
> >> >> >> +        """Check that we can read the name and contents of a property"""
> >> >> >> +        root = self.fdt.path_offset('/')
> >> >> >
> >> >> > No point to this - offset of / is always 0.  If you want to test that
> >> >> > happens, make it a separate testcase.
> >> >>
> >> >> I already have it above so will drop this.
> >> >>
> >> >> >
> >> >> >> +        poffset = self.fdt.first_property_offset(root)
> >> >> >> +        prop = self.fdt.get_property_by_offset(poffset)
> >> >> >> +        self.assertEquals(prop.name, 'compatible')
> >> >> >> +        self.assertEquals(prop.value, 'test_tree1\0')
> >> >> >> +
> >> >> >> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> >> >> >> +            self.fdt.get_property_by_offset(-2)
> >> >> >> +        self.assertEquals(
> >> >> >> +                -libfdt.BADOFFSET,
> >> >> >> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
> >> >> >> +
> >> >> >> +    def testGetProp(self):
> >> >> >> +        """Check that we can read the contents of a property by name"""
> >> >> >> +        root = self.fdt.path_offset('/')
> >> >> >> +        value = self.fdt.getprop(root, "compatible")
> >> >> >> +        self.assertEquals(value, 'test_tree1\0')
> >> >> >> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
> >> >> >> +                                                             QUIET_NOTFOUND))
> >> >> >
> >> >> > For testing, it might be useful to add a special value you can set the
> >> >> > quiet parameter to to make all errors quiet.
> >> >>
> >> >> Isn't that what I did?
> >> >
> >> > Sorry, I wasn't very clear.  What I mean is that if you want to treat
> >> > *all* errors as quiet, at the moment you have to do
> >> >         self.whatever(..., quiet=[NOTFOUND, EXISTS, NOSPACE,...])
> >> >
> >> > I was suggesting an extension to check_err() so you can instead say
> >> > 'quiet=SOMETHING' where SOMETHING is a special value, and it will
> >> > interpret that as making all errors quiet.
> >>
> >> OK I see. Could SOMETHING be
> >>
> >> QUIET_ALL = [NOTFOUND, EXISTS, all other errors]
> >>
> >> or are you wanting to take the dynamic typing further and use an
> >> integer or something?
> >
> > I was thinking of exploiting the dynamic typing (probably using a
> > special value, not an int, maybe 'True').  But really, either would be
> > fine.
> 
> Well I'll go with the tuple for consistency (i.e. to avoid a special
> case). It's an internal detail so we can always change if it feels
> wrong.

Yes, that makes sense.

-- 
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 v5 2/5] Add tests for pylibfdt

[David Gibson]

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

On Tue, Feb 14, 2017 at 08:51:57PM -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>
> ---
> 
> Changes in v5:
> - Adjust tests to match new swig bindings
> 
> 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 | 267 ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/run_tests.sh      |  19 +++-
>  2 files changed, 285 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..eac2544
> --- /dev/null
> +++ b/tests/pylibfdt_tests.py
> @@ -0,0 +1,267 @@
> +# 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, QUIET_NOTFOUND
> +
> +# 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(node, QUIET_NOTFOUND)
> +        while poffset > 0:
> +            prop = self.fdt.get_property_by_offset(poffset)
> +            prop_list.append(prop.name)
> +            poffset = self.fdt.next_property_offset(poffset, QUIET_NOTFOUND)
> +        return prop_list
> +
> +    def testImport(self):
> +        """Check that we can import the library correctly"""
> +        self.assertEquals(type(libfdt), type(sys))
> +
> +    def testBadFdt(self):
> +        """Check that a filename provided accidentally is not accepted"""
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADMAGIC)):
> +            fdt = libfdt.Fdt('a string')
> +
> +    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('/wibble', QUIET_NOTFOUND),
> +                          -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(ROOT_PROPS[-1],
> +                                                        QUIET_NOTFOUND),
> +                          -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(107, QUIET_NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.next_property_offset(107, QUIET_NOTFOUND)
> +
> +        node = self.fdt.path_offset('/subnode@1/ss1')
> +        self.assertEquals(self.fdt.first_property_offset(node, QUIET_NOTFOUND),
> +                          -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 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)
> +        prop = self.fdt.get_property_by_offset(poffset)
> +        self.assertEquals(prop.name, 'compatible')
> +        self.assertEquals(prop.value, 'test_tree1\0')
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.BADOFFSET)):
> +            self.fdt.get_property_by_offset(-2)
> +        self.assertEquals(
> +                -libfdt.BADOFFSET,
> +                self.fdt.get_property_by_offset(-2, [libfdt.BADOFFSET]))
> +
> +    def testGetProp(self):
> +        """Check that we can read the contents of a property by name"""
> +        root = self.fdt.path_offset('/')
> +        value = self.fdt.getprop(root, "compatible")
> +        self.assertEquals(value, 'test_tree1\0')
> +        self.assertEquals(-libfdt.NOTFOUND, self.fdt.getprop(root, 'missing',
> +                                                             QUIET_NOTFOUND))
> +
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.getprop(root, 'missing')
> +
> +        node = self.fdt.path_offset('/subnode@1/subsubnode')
> +        value = self.fdt.getprop(node, "compatible")
> +        self.assertEquals(value, '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(0, QUIET_NOTFOUND)
> +        while node >= 0:
> +            node_list.append(self.fdt.get_name(node))
> +            node = self.fdt.next_subnode(node, QUIET_NOTFOUND)
> +        self.assertEquals(node_list, ['subnode@1', 'subnode@2'])
> +
> +    def testFirstNextSubnodeOffsetExceptions(self):
> +        """Check except handling for first/next subnode functions"""
> +        node = self.fdt.path_offset('/subnode@1/subsubnode', QUIET_NOTFOUND)
> +        self.assertEquals(self.fdt.first_subnode(node, QUIET_NOTFOUND),
> +                          -libfdt.NOTFOUND)
> +        with self.assertRaisesRegexp(FdtException, get_err(libfdt.NOTFOUND)):
> +            self.fdt.first_subnode(node)
> +
> +        node = self.fdt.path_offset('/subnode@1/ss1', QUIET_NOTFOUND)
> +        self.assertEquals(self.fdt.next_subnode(node, QUIET_NOTFOUND),
> +                          -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('/subnode@2', QUIET_NOTFOUND)
> +        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)

This test will fail on a natively big endian system, because
fdt32_to_cpu() will become an identity function.

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

[PATCH v5 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 v5:
- Use an interactive session to demonstrate pylibfdt
- Mention that more work remains

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

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

diff --git a/README b/README
index f92008f..d4157ad 100644
--- a/README
+++ b/README
@@ -7,6 +7,49 @@ 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:
+
+$ PYTHONPATH=../pylibfdt python
+>>> import libfdt
+>>> fdt = libfdt.Fdt(open('test_tree1.dtb').read())
+>>> node = fdt.path_offset('/subnode@1')
+>>> prop_offset = fdt.first_property_offset(node)
+>>> prop = fdt.get_property_by_offset(prop_offset)
+>>> print '%s=%s' % (prop.name, prop.value)
+compatible=subnode1
+>>> node2 = fdt.path_offset('/')
+>>> print fdt.getprop(node2, 'compatible')
+test_tree1
+
+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
+
+
+More work remains to support all of libfdt, including access to numeric
+values.
+
+
 Mailing list
 ------------
 The following list is for discussion about dtc and libfdt implementation
-- 
2.11.0.483.g087da7b7c-goog

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

[David Gibson]

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

On Tue, Feb 14, 2017 at 08:51:58PM -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 v5:
> - Use an interactive session to demonstrate pylibfdt
> - Mention that more work remains
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2:
> - Add details on how to obtain full help and code coverage
> 
>  README | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/README b/README
> index f92008f..d4157ad 100644
> --- a/README
> +++ b/README
> @@ -7,6 +7,49 @@ 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:
> +
> +$ PYTHONPATH=../pylibfdt python
> +>>> import libfdt
> +>>> fdt = libfdt.Fdt(open('test_tree1.dtb').read())
> +>>> node = fdt.path_offset('/subnode@1')

I'd suggest adding some prints of the node values to show that they're
integers, rather than some exotic handle.

> +>>> prop_offset = fdt.first_property_offset(node)
> +>>> prop = fdt.get_property_by_offset(prop_offset)
> +>>> print '%s=%s' % (prop.name, prop.value)
> +compatible=subnode1

And maybe consider %r instead of %s for the values, to show the
included '\0' etc.

> +>>> node2 = fdt.path_offset('/')
> +>>> print fdt.getprop(node2, 'compatible')
> +test_tree1
> +
> +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
> +
> +
> +More work remains to support all of libfdt, including access to numeric
> +values.
> +
> +
>  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 --]

[PATCH v5 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(). However they are not useful to
    Python so can be removed

Add #ifdefs to work around these problem.

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

Changes in v5:
- Update commit message
- Drop #ifdef around fdt_get_header() macros

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index c69e918..2aa8ff6 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -143,7 +143,9 @@
 /* Low-level functions (you probably don't need these)                */
 /**********************************************************************/
 
+#ifndef SWIG /* This functions are not useful in Python */
 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,6 @@ int fdt_next_subnode(const void *fdt, int offset);
 /**********************************************************************/
 /* General functions                                                  */
 /**********************************************************************/
-
 #define fdt_get_header(fdt, field) \
 	(fdt32_to_cpu(((const struct fdt_header *)(fdt))->field))
 #define fdt_magic(fdt)			(fdt_get_header(fdt, magic))
@@ -638,8 +639,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 /* Not available in Python */
 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 v5 5/5] Build pylibfdt as part of the normal build process

[Simon Glass]

Possibly 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 v5:
- Fix 'possible' typo

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 1c48210..4adba10 100644
--- a/Makefile
+++ b/Makefile
@@ -116,7 +116,7 @@ BIN += fdtput
 
 SCRIPTS = dtdiff
 
-all: $(BIN) libfdt
+all: $(BIN) libfdt pylibfdt
 
 
 ifneq ($(DEPTARGETS),)
@@ -203,6 +203,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.
@@ -247,6 +260,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