[PATCH 0/4] Introduce Python bindings for libfdt

[PATCH 0/4] 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, to attract initial comments.


Simon Glass (4):
  Add an initial Python library for libfdt
  Add tests for pylibfdt
  Mention pylibfdt in the documentation
  RFC: Build pylibfdt as part of the normal build process

 Makefile                   |  16 +++-
 README                     |   6 ++
 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  21 ++++++
 pylibfdt/libfdt.swig       | 157 +++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 +++++++++
 tests/pylibfdt_tests.py    | 179 +++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh         |  19 ++++-
 8 files changed, 433 insertions(+), 2 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.8.0.rc3.226.g39d4020

[PATCH 1/4] 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>
---

 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  21 ++++++
 pylibfdt/libfdt.swig       | 157 +++++++++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 ++++++++++
 4 files changed, 215 insertions(+)
 create mode 100644 pylibfdt/.gitignore
 create mode 100644 pylibfdt/Makefile.pylibfdt
 create mode 100644 pylibfdt/libfdt.swig
 create mode 100644 pylibfdt/setup.py

diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
new file mode 100644
index 0000000..5e8c5e3
--- /dev/null
+++ b/pylibfdt/.gitignore
@@ -0,0 +1,3 @@
+libfdt.py
+libfdt.pyc
+libfdt_wrap.c
diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
new file mode 100644
index 0000000..fbdbca5
--- /dev/null
+++ b/pylibfdt/Makefile.pylibfdt
@@ -0,0 +1,21 @@
+# Makefile.pylibfdt
+#
+# This is not a complete Makefile of itself.  Instead, it is designed to
+# be easily embeddable into other systems of Makefiles.
+#
+
+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..91887da
--- /dev/null
+++ b/pylibfdt/libfdt.swig
@@ -0,0 +1,157 @@
+/*
+ * pylibfdt - Flat Device Tree manipulation in Python
+ * Copyright (C) 2016 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 %{
+
+def Raise(errnum):
+    raise ValueError('Error %s' % fdt_strerror(errnum))
+
+def Name(fdt, offset):
+    name, len = fdt_get_name(fdt, offset)
+    return name
+
+def String(fdt, offset):
+    offset = fdt32_to_cpu(offset)
+    name = fdt_string(fdt, offset)
+    return name
+
+def fdt32_to_cpu(val):
+    return struct.unpack("=I", struct.pack(">I", val))[0]
+
+def Data(prop):
+    set_prop(prop)
+    return get_prop_data()
+%}
+
+%include "typemaps.i"
+%include "cstring.i"
+
+%typemap(in) void* = char*;
+
+typedef int fdt32_t;
+
+struct fdt_property {
+        fdt32_t tag;
+        fdt32_t len;
+        fdt32_t nameoff;
+        char data[0];
+};
+
+/*
+ * This is a work-around since I'm not sure of a better way to copy out the
+ * contents of a string. This is used in dtoc/GetProps(). The intent is to
+ * pass in a pointer to a property and access the data field at the end of
+ * it. Ideally the Data() function above would be able to do this directly,
+ * but I'm not sure how to do that. Needs another look.
+ */
+#pragma SWIG nowarn=454
+%inline %{
+    static struct fdt_property *cur_prop;
+
+    void set_prop(struct fdt_property *prop) {
+        cur_prop = prop;
+    }
+%}
+
+%cstring_output_allocate_size(char **s, int *sz, free(*$1));
+%inline %{
+    void get_prop_data(char **s, int *sz) {
+        *sz = fdt32_to_cpu(cur_prop->len);
+        *s = (char *)malloc(*sz);
+        if (!*s)
+            *sz = 0;
+        else
+            memcpy(*s, cur_prop + 1, *sz);
+    }
+%}
+
+%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);
+}
+
+int fdt_path_offset(const void *fdt, const char *path);
+int fdt_first_property_offset(const void *fdt, int nodeoffset);
+int fdt_next_property_offset(const void *fdt, int offset);
+const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
+const char *fdt_string(const void *fdt, int stroffset);
+const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
+                                                      int offset,
+                                                      int *OUTPUT);
+const char *fdt_strerror(int errval);
+int fdt_first_subnode(const void *fdt, int offset);
+int fdt_next_subnode(const void *fdt, int offset);
+
+%typemap(in) (void *) {
+  if (!PyByteArray_Check($input)) {
+    SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname" "', argument "
+                       "$argnum"" of type '" "$type""'");
+  }
+  $1 = PyByteArray_AsString($input);
+}
+
+int fdt_delprop(void *fdt, int nodeoffset, const char *name);
+
+int fdt_pack(void *fdt);
+
+int fdt_totalsize(const void *fdt);
+int fdt_off_dt_struct(const void *fdt);
diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
new file mode 100644
index 0000000..8f8618e
--- /dev/null
+++ b/pylibfdt/setup.py
@@ -0,0 +1,34 @@
+#!/usr/bin/env python
+
+"""
+setup.py file for SWIG libfdt
+"""
+
+from distutils.core import setup, Extension
+import os
+import sys
+
+progname = sys.argv[0]
+cflags = sys.argv[1]
+files = sys.argv[2:]
+
+if cflags:
+    cflags = [flag for flag in cflags.split(' ') if flag]
+else:
+    cflags = None
+
+libfdt_module = Extension(
+    '_libfdt',
+    sources = files,
+    extra_compile_args =  cflags
+)
+
+sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
+
+setup (name = 'libfdt',
+       version = '0.1',
+       author      = "SWIG Docs",
+       description = """Simple swig libfdt from docs""",
+       ext_modules = [libfdt_module],
+       py_modules = ["libfdt"],
+       )
-- 
2.8.0.rc3.226.g39d4020

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

[David Gibson]

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

On Thu, Nov 17, 2016 at 03:53:22PM -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.

Nice idea.  Some nits in the details..

> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
>  pylibfdt/.gitignore        |   3 +
>  pylibfdt/Makefile.pylibfdt |  21 ++++++
>  pylibfdt/libfdt.swig       | 157 +++++++++++++++++++++++++++++++++++++++++++++
>  pylibfdt/setup.py          |  34 ++++++++++
>  4 files changed, 215 insertions(+)
>  create mode 100644 pylibfdt/.gitignore
>  create mode 100644 pylibfdt/Makefile.pylibfdt
>  create mode 100644 pylibfdt/libfdt.swig
>  create mode 100644 pylibfdt/setup.py
> 
> diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
> new file mode 100644
> index 0000000..5e8c5e3
> --- /dev/null
> +++ b/pylibfdt/.gitignore
> @@ -0,0 +1,3 @@
> +libfdt.py
> +libfdt.pyc
> +libfdt_wrap.c
> diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> new file mode 100644
> index 0000000..fbdbca5
> --- /dev/null
> +++ b/pylibfdt/Makefile.pylibfdt
> @@ -0,0 +1,21 @@
> +# Makefile.pylibfdt
> +#
> +# This is not a complete Makefile of itself.  Instead, it is designed to
> +# be easily embeddable into other systems of Makefiles.

So, Makefile.libfdt is explicitly designed for easily embedding libfdt
in other projecs - even ones with weird build environments, like
bootloaders.  It's less clear that that's valuable for Python
wrappers.

> +#
> +
> +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..91887da
> --- /dev/null
> +++ b/pylibfdt/libfdt.swig
> @@ -0,0 +1,157 @@
> +/*
> + * pylibfdt - Flat Device Tree manipulation in Python
> + * Copyright (C) 2016 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 %{
> +
> +def Raise(errnum):
> +    raise ValueError('Error %s' % fdt_strerror(errnum))

So, first, I believe this and several things below break normal Python
capitalization conventions for functions (capital-followed-by-lower
generally indicates a class).

More importantly, wouldn't it make more sense to have an Exception
subclass for libfdt errors, rather than shuffling everything through
ValueError?

> +def Name(fdt, offset):
> +    name, len = fdt_get_name(fdt, offset)
> +    return name
> +
> +def String(fdt, offset):
> +    offset = fdt32_to_cpu(offset)
> +    name = fdt_string(fdt, offset)
> +    return name
> +
> +def fdt32_to_cpu(val):
> +    return struct.unpack("=I", struct.pack(">I", val))[0]
> +
> +def Data(prop):
> +    set_prop(prop)
> +    return get_prop_data()
> +%}
> +
> +%include "typemaps.i"
> +%include "cstring.i"
> +
> +%typemap(in) void* = char*;
> +
> +typedef int fdt32_t;
> +
> +struct fdt_property {
> +        fdt32_t tag;
> +        fdt32_t len;
> +        fdt32_t nameoff;
> +        char data[0];
> +};

Can't you include this directly from fdt.h?  That header is guaranteed
never to have actual function definitions, just data structures and
constants.

> +/*
> + * This is a work-around since I'm not sure of a better way to copy out the
> + * contents of a string. This is used in dtoc/GetProps(). The intent is to
> + * pass in a pointer to a property and access the data field at the end of
> + * it. Ideally the Data() function above would be able to do this directly,
> + * but I'm not sure how to do that. Needs another look.
> + */
> +#pragma SWIG nowarn=454
> +%inline %{
> +    static struct fdt_property *cur_prop;
> +
> +    void set_prop(struct fdt_property *prop) {
> +        cur_prop = prop;
> +    }

Eugh... a global variable making this totally non thread safe.  You
really need a better solution.  Property values you should be able to
represent nicely as Python strings (since unlike C strings, those can
include embedded \0).  Uh.. I guess that's 'bytes' for Python3

Of course, in the other direction, node and property names can't
include \0, so you probably need some sort of exception if a Python
string which includes them is passed in.

> +%}
> +
> +%cstring_output_allocate_size(char **s, int *sz, free(*$1));
> +%inline %{
> +    void get_prop_data(char **s, int *sz) {
> +        *sz = fdt32_to_cpu(cur_prop->len);
> +        *s = (char *)malloc(*sz);
> +        if (!*s)
> +            *sz = 0;
> +        else
> +            memcpy(*s, cur_prop + 1, *sz);
> +    }
> +%}
> +
> +%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);
> +}
> +
> +int fdt_path_offset(const void *fdt, const char *path);
> +int fdt_first_property_offset(const void *fdt, int nodeoffset);
> +int fdt_next_property_offset(const void *fdt, int offset);
> +const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
> +const char *fdt_string(const void *fdt, int stroffset);
> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> +                                                      int offset,
> +                                                      int *OUTPUT);
> +const char *fdt_strerror(int errval);
> +int fdt_first_subnode(const void *fdt, int offset);
> +int fdt_next_subnode(const void *fdt, int offset);
> +
> +%typemap(in) (void *) {
> +  if (!PyByteArray_Check($input)) {
> +    SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname" "', argument "
> +                       "$argnum"" of type '" "$type""'");
> +  }
> +  $1 = PyByteArray_AsString($input);
> +}
> +
> +int fdt_delprop(void *fdt, int nodeoffset, const char *name);
> +
> +int fdt_pack(void *fdt);
> +
> +int fdt_totalsize(const void *fdt);
> +int fdt_off_dt_struct(const void *fdt);
> diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
> new file mode 100644
> index 0000000..8f8618e
> --- /dev/null
> +++ b/pylibfdt/setup.py
> @@ -0,0 +1,34 @@
> +#!/usr/bin/env python
> +
> +"""
> +setup.py file for SWIG libfdt
> +"""
> +
> +from distutils.core import setup, Extension
> +import os
> +import sys
> +
> +progname = sys.argv[0]
> +cflags = sys.argv[1]
> +files = sys.argv[2:]
> +
> +if cflags:
> +    cflags = [flag for flag in cflags.split(' ') if flag]
> +else:
> +    cflags = None
> +
> +libfdt_module = Extension(
> +    '_libfdt',
> +    sources = files,
> +    extra_compile_args =  cflags
> +)
> +
> +sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
> +
> +setup (name = 'libfdt',
> +       version = '0.1',
> +       author      = "SWIG Docs",
> +       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 1/4] Add an initial Python library for libfdt

[Simon Glass]

Hi David,

On 17 November 2016 at 17:36, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Nov 17, 2016 at 03:53:22PM -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.
>
> Nice idea.  Some nits in the details..
>
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>
>>  pylibfdt/.gitignore        |   3 +
>>  pylibfdt/Makefile.pylibfdt |  21 ++++++
>>  pylibfdt/libfdt.swig       | 157 +++++++++++++++++++++++++++++++++++++++++++++
>>  pylibfdt/setup.py          |  34 ++++++++++
>>  4 files changed, 215 insertions(+)
>>  create mode 100644 pylibfdt/.gitignore
>>  create mode 100644 pylibfdt/Makefile.pylibfdt
>>  create mode 100644 pylibfdt/libfdt.swig
>>  create mode 100644 pylibfdt/setup.py
>>
>> diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
>> new file mode 100644
>> index 0000000..5e8c5e3
>> --- /dev/null
>> +++ b/pylibfdt/.gitignore
>> @@ -0,0 +1,3 @@
>> +libfdt.py
>> +libfdt.pyc
>> +libfdt_wrap.c
>> diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
>> new file mode 100644
>> index 0000000..fbdbca5
>> --- /dev/null
>> +++ b/pylibfdt/Makefile.pylibfdt
>> @@ -0,0 +1,21 @@
>> +# Makefile.pylibfdt
>> +#
>> +# This is not a complete Makefile of itself.  Instead, it is designed to
>> +# be easily embeddable into other systems of Makefiles.
>
> So, Makefile.libfdt is explicitly designed for easily embedding libfdt
> in other projecs - even ones with weird build environments, like
> bootloaders.  It's less clear that that's valuable for Python
> wrappers.

OK, but what should i do instead?

Regards,
Simon

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

[David Gibson]

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

On Thu, Nov 24, 2016 at 11:08:00AM -0700, Simon Glass wrote:
> Hi David,
> 
> On 17 November 2016 at 17:36, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Nov 17, 2016 at 03:53:22PM -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.
> >
> > Nice idea.  Some nits in the details..
> >
> >>
> >> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> ---
> >>
> >>  pylibfdt/.gitignore        |   3 +
> >>  pylibfdt/Makefile.pylibfdt |  21 ++++++
> >>  pylibfdt/libfdt.swig       | 157 +++++++++++++++++++++++++++++++++++++++++++++
> >>  pylibfdt/setup.py          |  34 ++++++++++
> >>  4 files changed, 215 insertions(+)
> >>  create mode 100644 pylibfdt/.gitignore
> >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> >>  create mode 100644 pylibfdt/libfdt.swig
> >>  create mode 100644 pylibfdt/setup.py
> >>
> >> diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
> >> new file mode 100644
> >> index 0000000..5e8c5e3
> >> --- /dev/null
> >> +++ b/pylibfdt/.gitignore
> >> @@ -0,0 +1,3 @@
> >> +libfdt.py
> >> +libfdt.pyc
> >> +libfdt_wrap.c
> >> diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> >> new file mode 100644
> >> index 0000000..fbdbca5
> >> --- /dev/null
> >> +++ b/pylibfdt/Makefile.pylibfdt
> >> @@ -0,0 +1,21 @@
> >> +# Makefile.pylibfdt
> >> +#
> >> +# This is not a complete Makefile of itself.  Instead, it is designed to
> >> +# be easily embeddable into other systems of Makefiles.
> >
> > So, Makefile.libfdt is explicitly designed for easily embedding libfdt
> > in other projecs - even ones with weird build environments, like
> > bootloaders.  It's less clear that that's valuable for Python
> > wrappers.
> 
> OK, but what should i do instead?

Sorry, I just meant that that copied comment doesn't really make sense
in its entirity here.  Otherwise the makefile is 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 --]

[PATCH 2/4] Add tests for pylibfdt

[Simon Glass]

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

 tests/pylibfdt_tests.py | 179 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh      |  19 ++++-
 2 files changed, 197 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..c406ac9
--- /dev/null
+++ b/tests/pylibfdt_tests.py
@@ -0,0 +1,179 @@
+# pylibfdt - Tests for Flat Device Tree manipulation in Python
+# Copyright (C) 2016 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
+
+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 = self._ReadFdt('test_tree1.dtb')
+
+    def _ReadFdt(self, fname):
+        """Read a device tree file into a bytearray ready for use
+
+        Args:
+            fname: Filename to read from
+
+        Returns:
+            Fdt bytearray suitable for passing to libfdt functions
+        """
+        return bytearray(open(fname).read())
+
+    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 = libfdt.fdt_path_offset(self.fdt, node_path)
+        poffset = libfdt.fdt_first_property_offset(self.fdt, node)
+        while poffset >= 0:
+            pdata, plen = libfdt.fdt_get_property_by_offset(self.fdt, poffset)
+            prop_list.append(libfdt.String(self.fdt, pdata.nameoff))
+            poffset = libfdt.fdt_next_property_offset(self.fdt, poffset)
+        return prop_list
+
+    def testImport(self):
+        """Check that we can import the library correctly"""
+        self.assertEquals(type(libfdt), type(sys))
+
+    def testPathOffset(self):
+        """Check that we can find the offset of a node"""
+        self.assertEquals(libfdt.fdt_path_offset(self.fdt, '/'), 0)
+        self.assertEquals(libfdt.fdt_path_offset(self.fdt, '/subnode@1'), 124)
+
+    def testPropertyOffset(self):
+        """Walk through all the properties in the root node"""
+        self.assertEquals(libfdt.fdt_first_property_offset(self.fdt, 0), 8)
+        self.assertEquals(libfdt.fdt_next_property_offset(self.fdt, 8), 32)
+        self.assertEquals(libfdt.fdt_next_property_offset(self.fdt, 32), 48)
+        self.assertEquals(libfdt.fdt_next_property_offset(self.fdt, 48), 68)
+        self.assertEquals(libfdt.fdt_next_property_offset(self.fdt, 68), 92)
+        self.assertEquals(libfdt.fdt_next_property_offset(self.fdt, 92), 108)
+        self.assertEquals(libfdt.fdt_next_property_offset(self.fdt, 108), -1)
+
+    def testGetName(self):
+        """Check that we can get the name of a node"""
+        self.assertEquals(libfdt.fdt_get_name(self.fdt, 0), ['', 0])
+        node = libfdt.fdt_path_offset(self.fdt, '/subnode@1/subsubnode')
+        self.assertEquals(libfdt.fdt_get_name(self.fdt, node),
+                          ['subsubnode', 10])
+
+    def testGetString(self):
+        """Test that we can get a string from the string table"""
+        node = libfdt.fdt_path_offset(self.fdt, '/subnode@2')
+        poffset = libfdt.fdt_first_property_offset(self.fdt, node)
+        pdata, plen = libfdt.fdt_get_property_by_offset(self.fdt, poffset)
+        self.assertEquals(libfdt.String(self.fdt, pdata.nameoff), 'reg')
+
+    def testGetPropertyByOffset(self):
+        """Check that we can read the name and contents of a property"""
+        root = libfdt.fdt_path_offset(self.fdt, '/')
+        poffset = libfdt.fdt_first_property_offset(self.fdt, root)
+        pdata, plen = libfdt.fdt_get_property_by_offset(self.fdt, poffset)
+        self.assertEquals(libfdt.fdt32_to_cpu(pdata.tag), 3)
+        self.assertEquals(libfdt.fdt32_to_cpu(pdata.len), 11)
+        self.assertEquals(libfdt.String(self.fdt, pdata.nameoff), 'compatible')
+        self.assertEquals(libfdt.Data(pdata), 'test_tree1\0')
+
+    def testStrError(self):
+        """Check that we can get an error string"""
+        self.assertEquals(libfdt.fdt_strerror(-1), 'FDT_ERR_NOTFOUND')
+
+    def testFirstNextSubnode(self):
+        """Check that we can walk through subnodes"""
+        node_list = []
+        node = libfdt.fdt_first_subnode(self.fdt, 0)
+        while node >= 0:
+            node_list.append(libfdt.fdt_get_name(self.fdt, node)[0])
+            node = libfdt.fdt_next_subnode(self.fdt, node)
+        self.assertEquals(node_list, ['subnode@1', 'subnode@2'])
+
+    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 = libfdt.fdt_path_offset(self.fdt, '/%s' % node_name)
+        self.assertEquals(libfdt.fdt_delprop(self.fdt, 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(libfdt.fdt_totalsize(self.fdt), 693)
+        self.assertEquals(libfdt.fdt_off_dt_struct(self.fdt), 88)
+
+    def testPack(self):
+        """Test that we can pack the tree after deleting something"""
+        self.assertEquals(libfdt.fdt_totalsize(self.fdt), 693)
+        node = libfdt.fdt_path_offset(self.fdt, '/subnode@2')
+        self.assertEquals(libfdt.fdt_delprop(self.fdt, node, 'prop-int'), 0)
+        self.assertEquals(libfdt.fdt_totalsize(self.fdt), 693)
+        self.assertEquals(libfdt.fdt_pack(self.fdt), 0)
+        self.assertEquals(libfdt.fdt_totalsize(self.fdt), 677)
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index e4139dd..707702d 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -720,6 +720,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")
@@ -738,7 +752,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
@@ -767,6 +781,9 @@ for set in $TESTSETS; do
 	"fdtdump")
 	    fdtdump_tests
 	    ;;
+	"pylibfdt")
+	    pylibfdt_tests
+	    ;;
     esac
 done
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/4] Mention pylibfdt in the documentation

[Simon Glass]

Add a note about pylibfdt in the README.

Full documentation for pylibfdt will need to be provided also.

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

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

diff --git a/README b/README
index f92008f..dee77c8 100644
--- a/README
+++ b/README
@@ -7,6 +7,12 @@ DTC and LIBFDT are maintained by:
 David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
 Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
 
+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
+
+
 Mailing list
 ------------
 The following list is for discussion about dtc and libfdt implementation
-- 
2.8.0.rc3.226.g39d4020

[PATCH 4/4] RFC: Build pylibfdt as part of the normal build process

[Simon Glass]

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

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

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

diff --git a/Makefile b/Makefile
index 32dcfcf..4996cfd 100644
--- a/Makefile
+++ b/Makefile
@@ -115,7 +115,7 @@ BIN += fdtput
 
 SCRIPTS = dtdiff
 
-all: $(BIN) libfdt
+all: $(BIN) libfdt pylibfdt
 
 
 ifneq ($(DEPTARGETS),)
@@ -202,6 +202,19 @@ dist:
 	cat ../dtc-$(dtc_version).tar | \
 		gzip -9 > ../dtc-$(dtc_version).tar.gz
 
+
+#
+# Rules for pylibfdt
+#
+PYLIBFDT_srcdir = pylibfdt
+PYLIBFDT_objdir = pylibfdt
+
+include $(PYLIBFDT_srcdir)/Makefile.pylibfdt
+
+.PHONY: pylibfdt
+pylibfdt: $(PYLIBFDT_objdir)/_libfdt.so
+
+
 #
 # Release signing and uploading
 # This is for maintainer convenience, don't try this at home.
@@ -240,6 +253,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.8.0.rc3.226.g39d4020

[PATCH v3 0/4] 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, to attract initial comments.

The v3 series makes the binding more pythonic, allowing things like:

    fdt = libfdt.Fdt(open(fname).read())
    node = fdt.path('/subnode@1')
    print node.prop('compatible').data()
    subnode = node.first_subnode_quiet()
    while subnode:
        print subnode.name()
        subnode = subnode.next_subnode_quiet()

There are still some open question, particularly around naming.

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

Simon Glass (4):
  Add an initial Python library for libfdt
  Add tests for pylibfdt
  Mention pylibfdt in the documentation
  Build pylibfdt as part of the normal build process

 Makefile                   |  16 +-
 README                     |  33 +++
 pylibfdt/.gitignore        |   3 +
 pylibfdt/Makefile.pylibfdt |  18 ++
 pylibfdt/libfdt.swig       | 602 +++++++++++++++++++++++++++++++++++++++++++++
 pylibfdt/setup.py          |  34 +++
 tests/pylibfdt_tests.py    | 389 +++++++++++++++++++++++++++++
 tests/run_tests.sh         |  19 +-
 8 files changed, 1112 insertions(+), 2 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.8.0.rc3.226.g39d4020

[PATCH v3 1/4] 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 v3:
- Make the library more pythonic
- Add classes for Node and Prop along with methods
- Add an exception class
- Use Python to generate exeptions instead of SWIG

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

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

diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
new file mode 100644
index 0000000..5e8c5e3
--- /dev/null
+++ b/pylibfdt/.gitignore
@@ -0,0 +1,3 @@
+libfdt.py
+libfdt.pyc
+libfdt_wrap.c
diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
new file mode 100644
index 0000000..fa74dd2
--- /dev/null
+++ b/pylibfdt/Makefile.pylibfdt
@@ -0,0 +1,18 @@
+# Makefile.pylibfdt
+#
+
+PYLIBFDT_srcs = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_SRCS))
+WRAP = $(PYLIBFDT_objdir)/libfdt_wrap.c
+PYMODULE = $(PYLIBFDT_objdir)/_libfdt.so
+
+$(PYMODULE): $(PYLIBFDT_srcs) $(WRAP)
+	@$(VECHO) PYMOD $@
+	python $(PYLIBFDT_objdir)/setup.py "$(CPPFLAGS)" $^
+	mv _libfdt.so $(PYMODULE)
+
+$(WRAP): $(PYLIBFDT_srcdir)/libfdt.swig
+	@$(VECHO) SWIG $@
+	swig -python -o $@ $<
+
+PYLIBFDT_cleanfiles = libfdt_wrap.c libfdt.py libfdt.pyc
+PYLIBFDT_CLEANFILES = $(addprefix $(PYLIBFDT_objdir)/,$(PYLIBFDT_cleanfiles))
diff --git a/pylibfdt/libfdt.swig b/pylibfdt/libfdt.swig
new file mode 100644
index 0000000..461cdff
--- /dev/null
+++ b/pylibfdt/libfdt.swig
@@ -0,0 +1,602 @@
+/*
+ * pylibfdt - Flat Device Tree manipulation in Python
+ * Copyright (C) 2016 Google, Inc.
+ * Written by Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
+ *
+ * libfdt is dual licensed: you can use it either under the terms of
+ * the GPL, or the BSD license, at your option.
+ *
+ *  a) This library is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This library is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ *     You should have received a copy of the GNU General Public
+ *     License along with this library; if not, write to the Free
+ *     Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
+ *     MA 02110-1301 USA
+ *
+ * Alternatively,
+ *
+ *  b) Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *     1. Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *     2. Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ *     THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ *     CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ *     INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ *     MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ *     DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ *     CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *     SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ *     NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ *     LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ *     HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ *     CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ *     OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+%module libfdt
+
+%{
+#define SWIG_FILE_WITH_INIT
+#include "libfdt.h"
+%}
+
+%pythoncode %{
+
+import struct
+
+# Error codes, corresponding to FDT_ERR_... in libfdt.h
+(NOTFOUND,
+        EXISTS,
+        NOSPACE,
+        BADOFFSET,
+        BADPATH,
+        BADPHANDLE,
+        BADSTATE,
+        TRUNCATED,
+        BADMAGIC,
+        BADVERSION,
+        BADSTRUCTURE,
+        BADLAYOUT,
+        INTERNAL,
+        BADNCELLS,
+        BADVALUE,
+        BADOVERLAY) = range(1, 17)
+
+class FdtException(Exception):
+    """An exception caused by an error such as one of the codes above"""
+    def __init__(self, err):
+        self.err = err
+
+    def __str__(self):
+        return 'pylibfdt error %d: %s' % (self.err, fdt_strerror(self.err))
+
+def fdt32_to_cpu(val):
+    """Convert a device-tree cell value into a native integer"""
+    return struct.unpack("=I", struct.pack(">I", val))[0]
+
+def data(prop):
+    """Extract the data from a property
+
+    Args:
+        prop: Property structure, as returned by get_property_by_offset()
+
+    Returns:
+        The property data as a bytearray
+    """
+    buf = bytearray(fdt32_to_cpu(prop.len))
+    pylibfdt_copy_data(buf, prop)
+    return buf
+
+def strerror(fdt_err):
+    """Get the string for an error number
+
+    Args:
+        fdt_err: Error number (-ve)
+
+    Returns:
+        String containing the associated error
+    """
+    return fdt_strerror(fdt_err)
+
+def check_err(val, quiet=False):
+    """Raise an error if the return value is -ve
+
+    This is used to check for errors returned by libfdt C functions.
+
+    Args:
+        val: Return value from a libfdt function
+        quiet: True to ignore the NOTFOUND error, False to raise on all errors
+
+    Returns:
+        val if val >= 0
+
+    Raises
+        FdtException if val is < 0
+    """
+    if val < 0:
+        if not quiet or val != -NOTFOUND:
+            raise FdtException(val)
+    return val
+
+def check_err_null(val):
+    """Raise an error if the return value is NULL
+
+    This is used to check for a NULL return value from certain libfdt C
+    functions
+
+    Args:
+        val: Return value from a libfdt function
+        quiet: True to ignore the NOTFOUND error, False to raise on all errors
+
+    Returns:
+        val if val is not NULL
+
+    Raises
+        FdtException if val is NULL
+    """
+    # Normally a tuple is returned which contains the data and its length.
+    # If we get just an integer error code, it means the function failed.
+    if not isinstance(val, list):
+        raise FdtException(val)
+    return val
+
+class Fdt:
+    """Device tree class, supporting all operations
+
+    The Fdt object is created is created from a device tree binary file,
+    e.g. with something like:
+
+       fdt = Fdt(open("filename.dtb").read())
+
+    Operations can then be performed using the methods in this class. Each
+    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
+
+    Almost all methods raise a FdtException if an error occurs. The
+    following does not:
+
+        string() - since it has no error checking
+
+    To avoid this behaviour a 'quiet' version is provided for some functions.
+    This behaves as for the normal version except that it will not raise
+    an exception in the case of an FDT_ERR_NOTFOUND error: it will simply
+    return the -NOTFOUND error code or None.
+
+    Separate classes are provided for nodes (Node) and properties (Prop).
+    """
+    def __init__(self, data):
+        self._fdt = bytearray(data)
+
+    def string(self, offset):
+        """Get a string given its offset
+
+        Args:
+            offset: FDT offset in big-endian format
+
+        Returns:
+            string value at that offset
+        """
+        return fdt_string(self._fdt, fdt32_to_cpu(offset))
+
+    def path(self, path):
+        """Get the Node for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+
+        Returns:
+            Node object for this node
+
+        Raises
+            FdtException if the path is not valid
+        """
+        return Node(self, check_err(fdt_path_offset(self._fdt, path)))
+
+    def path_quiet(self, path):
+        """Get the Node for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+
+        Returns:
+            Node object for this node, or None if the path is not valid
+        """
+        val = check_err(fdt_path_offset(self._fdt, path), True)
+        if val < 0:
+            return None
+        return Node(self, val)
+
+    def path_offset(self, path):
+        """Get the offset for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+
+        Returns:
+            Node offset
+
+        Raises
+            FdtException if the path is not valid
+        """
+        return check_err(fdt_path_offset(self._fdt, path))
+
+    def path_offset_quiet(self, path):
+        """Get the offset for a given path
+
+        Args:
+            path: Path to the required node, e.g. '/node@3/subnode@1'
+
+        Returns:
+            Node offset, or -NOTFOUND if the path is not value
+
+        Raises
+            FdtException if any error occurs other than NOTFOUND
+        """
+        return check_err(fdt_path_offset(self._fdt, path), True)
+
+    def first_property_offset(self, nodeoffset):
+        """Get the offset of the first property in a node offset
+
+        Args:
+            nodeoffset: Offset to the node to check
+
+        Returns:
+            Offset of the first property
+
+        Raises
+            FdtException if the associated node has no properties, or some
+                other error occurred
+        """
+        return check_err(fdt_first_property_offset(self._fdt, nodeoffset))
+
+    def first_property_offset_quiet(self, nodeoffset):
+        """Get the offset of the first property in a node offset
+
+        Args:
+            nodeoffset: Offset to the node to check
+
+        Returns:
+            Offset of the first property, or -ENOTFOUND if the node has no
+                properties
+
+        Raises
+            FdtException if any other error occurs
+        """
+        return check_err(fdt_first_property_offset(self._fdt, nodeoffset), True)
+
+    # TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Comment the rest of the methods once everything
+    # is confirmed.
+    def next_property_offset(self, prop_offset):
+        return check_err(fdt_next_property_offset(self._fdt, prop_offset))
+
+    def next_property_offset_quiet(self, prop_offset):
+        return check_err(fdt_next_property_offset(self._fdt, prop_offset), True)
+
+    def get_name(self, nodeoffset):
+        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
+
+    def get_property_by_offset_internal(self, prop_offset):
+        """Obtains a property that can be examined
+
+        Args:
+            prop_offset: Offset of property (e.g. from first_property_offset())
+
+        Returns:
+            Property object with members:
+                tag: Big-endian device tree tag value
+                len: Big-endian property length
+                nameoff: Big-endian string offset for use with string()
+
+            Use data() on the return value to obtain the property value.
+
+        Raises:
+            FdtException on error (e.g. invalid prop_offset or device
+            tree format)
+        """
+        return check_err_null(fdt_get_property_by_offset(self._fdt,
+                                                         prop_offset))[0]
+
+    def get_property_by_offset(self, prop_offset):
+        return Prop.from_offset(self, prop_offset)
+
+    def get_property(self, nodeoffset, name):
+        """Get a property given a node offset and the property name
+
+        We cannot use fdt_get_property() here since it does not return the
+        offset. We prefer to create Node objects using the offset.
+
+        Args:
+            nodeoffset: Offset of the node
+            name: Property name
+
+        Returns:
+            Prop object found
+
+        Raises:
+            FdtException on error such as no such property
+        """
+        poffset = self.first_property_offset(nodeoffset)
+        while True:
+            pdata = self.get_property_by_offset_internal(poffset)
+            if self.string(pdata.nameoff) == name:
+                return Prop(Node(self, nodeoffset), poffset)
+            poffset = self.next_property_offset(poffset)
+
+    def get_property_quiet(self, nodeoffset, name):
+        """Get a property given a node offset and the property name
+
+        We cannot use fdt_get_property() here since it does not return the
+        offset. We prefer to create Node objects using the offset.
+
+        Args:
+            nodeoffset: Offset of the node
+            name: Property name
+
+        Returns:
+            Prop object found or None if there is no such property
+
+        Raises:
+            FdtException on error
+        """
+        poffset = self.first_property_offset_quiet(nodeoffset)
+        while poffset >= 0:
+            pdata = self.get_property_by_offset_internal(poffset)
+            if self.string(pdata.nameoff) == name:
+                return Prop(Node(self, nodeoffset), poffset)
+            poffset = self.next_property_offset_quiet(poffset)
+        return None
+
+    def first_subnode(self, nodeoffset):
+        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
+
+    def first_subnode_quiet(self, nodeoffset):
+        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
+
+    def next_subnode(self, nodeoffset):
+        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
+
+    def next_subnode_quiet(self, nodeoffset):
+        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
+
+    def totalsize(self):
+        return check_err(fdt_totalsize(self._fdt))
+
+    def off_dt_struct(self):
+        return check_err(fdt_off_dt_struct(self._fdt))
+
+    def pack(self):
+        return check_err(fdt_pack(self._fdt))
+
+    def delprop(self, nodeoffset, prop_name):
+        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
+
+
+class Node:
+    """A device tree node
+
+    This encapsulates a device-tree node and provides various operations
+    which can be performed on the node. In particular it is possible to find
+    subnodes and properties.
+    """
+    def __init__(self, fdt, offset):
+        self._fdt = fdt
+        self._offset = offset
+
+    def first_property(self):
+        return Prop(self, check_err(fdt_first_property_offset(self._fdt._fdt,
+                                                              self._offset)))
+
+    def first_property_quiet(self):
+        val = check_err(fdt_first_property_offset(self._fdt._fdt, self._offset),
+                        True)
+        if val < 0:
+            return None
+        return Prop(self, val)
+
+    def props(self):
+        props = []
+        prop = self.first_property_quiet()
+        while prop:
+            props.append(prop)
+            prop = prop.next_quiet()
+        return props
+
+    def first_subnode(self):
+        val = check_err(fdt_first_subnode(self._fdt._fdt, self._offset))
+        return Node(self._fdt, val)
+
+    def next_subnode(self):
+        val = check_err(fdt_next_subnode(self._fdt._fdt, self._offset))
+        return Node(self._fdt, val)
+
+    def first_subnode_quiet(self):
+        val = check_err(fdt_first_subnode(self._fdt._fdt, self._offset), True)
+        if val < 0:
+            return None
+        return Node(self._fdt, val)
+
+    def next_subnode_quiet(self):
+        val = check_err(fdt_next_subnode(self._fdt._fdt, self._offset), True)
+        if val < 0:
+            return None
+        return Node(self._fdt, val)
+
+    def name(self):
+        return check_err_null(fdt_get_name(self._fdt._fdt, self._offset))[0]
+
+    def prop(self, name):
+        return self._fdt.get_property(self._offset, name)
+
+
+class Prop:
+    """A device-tree property
+
+    This encapsulates a device-tree property and provides various operations
+    which can be performed on the property.
+
+    Generally the standard constructor is used to create a Prop object. But
+    in the case where only the property offset is known (and not the Node
+    that holds the property), from_offset() can be used.
+    """
+    def __init__(self, node, offset, fdt=None):
+        self._node = node
+        self._offset = offset
+        self._fdt = node._fdt if node else fdt
+
+    @classmethod
+    def from_offset(cls, fdt, prop_offset):
+        prop = cls(None, prop_offset, fdt)
+        prop._get_name_data()
+        return prop
+
+    def next(self):
+        return Prop(self._node, check_err(fdt_next_property_offset(
+                self._fdt._fdt, self._offset)))
+
+    def next_quiet(self):
+        val = check_err(fdt_next_property_offset(self._fdt._fdt, self._offset),
+                        True)
+        if val < 0:
+            return None
+        return Prop(self._node, val)
+
+    def _get_name_data(self):
+        pdata = fdt_get_property_by_offset(self._fdt._fdt, self._offset)
+        if not isinstance(pdata, list):
+            raise FdtException(pdata)
+        return self._fdt.string(pdata[0].nameoff), data(pdata[0])
+
+    def name(self):
+        return self._get_name_data()[0]
+
+    def data(self):
+        return self._get_name_data()[1]
+
+    def delete(self):
+        if not self._node:
+            raise RuntimeError("Can't delete property offset %d of unknown node"
+                               % self._offset)
+        self._fdt.delprop(self._node._offset, self.name())
+%}
+
+typedef int fdt32_t;
+
+%include "libfdt/fdt.h"
+
+%include "typemaps.i"
+
+%typemap(in) void * = char *;
+
+/*
+ * Unfortunately the defintiion of pybuffer_mutable_binary() in my Python
+ * version appears to be broken:
+ * pylibfdt/libfdt_wrap.c: In function ‘_wrap_pylibfdt_copy_data’:
+ * pylibfdt/libfdt_wrap.c:3603:22: error: ‘size’ undeclared (first use in this
+ * function)
+ *   arg2 = (size_t) (size/sizeof(char));
+ *
+ * This version works correctly.
+ */
+%define %mypybuffer_mutable_binary(TYPEMAP, SIZE)
+%typemap(in) (TYPEMAP, SIZE)(int res, Py_ssize_t size = 0, void *buf = 0)
+{
+	res = PyObject_AsWriteBuffer($input, &buf, &size);
+	if (res < 0) {
+		PyErr_Clear();
+		%argument_fail(res, "(TYPEMAP, SIZE)", $symname, $argnum);
+	}
+	$1 = ($1_ltype)buf;
+	$2 = ($2_ltype)(size1 / sizeof($*1_type));
+}
+%enddef
+
+/* This is used to copy property data into a bytearray */
+%mypybuffer_mutable_binary(char *str, size_t size);
+void pylibfdt_copy_data(char *str, size_t size,
+			const struct fdt_property *prop);
+
+/* Most functions don't change the device tree, so use a const void * */
+%typemap(in) (const void *) {
+    if (!PyByteArray_Check($input)) {
+        SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
+                            "', argument " "$argnum"" of type '" "$type""'");
+  }
+  $1 = (void *)PyByteArray_AsString($input);
+}
+
+/* Some functions do change the device tree, so use void * */
+%typemap(in) (void *) {
+    if (!PyByteArray_Check($input)) {
+        SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
+                            "', argument " "$argnum"" of type '" "$type""'");
+  }
+  $1 = PyByteArray_AsString($input);
+}
+
+%inline %{
+
+/**
+ * pylibfdt_copy_data() - Copy data from a property to the given buffer
+ *
+ * This is used by the data() function to place the contents of a property
+ * into a bytearray.
+ *
+ * @buf: Destination pointer (typically the start of the bytearray)
+ * @size: Number of bytes to copy (size of bytearray)
+ * @prop: Property to copy
+ */
+void pylibfdt_copy_data(char *buf, size_t size, const struct fdt_property *prop)
+{
+	memcpy(buf, prop + 1, size);
+}
+
+%}
+
+/*
+ * From here are the function definitions from libfdt.h, along with their
+ * exception-handling code.
+ */
+int fdt_path_offset(const void *fdt, const char *path);
+
+int fdt_first_property_offset(const void *fdt, int nodeoffset);
+
+int fdt_next_property_offset(const void *fdt, int offset);
+
+const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
+
+/* no exception handling, since this function has no error checking */
+const char *fdt_string(const void *fdt, int stroffset);
+
+const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
+                                                      int offset, int *OUTPUT);
+
+/* no exception handling, this this function always returns a valid string */
+const char *fdt_strerror(int errval);
+
+int fdt_first_subnode(const void *fdt, int offset);
+
+int fdt_next_subnode(const void *fdt, int offset);
+
+int fdt_delprop(void *fdt, int nodeoffset, const char *name);
+
+int fdt_pack(void *fdt);
+
+int fdt_totalsize(const void *fdt);
+
+int fdt_off_dt_struct(const void *fdt);
diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
new file mode 100644
index 0000000..8f8618e
--- /dev/null
+++ b/pylibfdt/setup.py
@@ -0,0 +1,34 @@
+#!/usr/bin/env python
+
+"""
+setup.py file for SWIG libfdt
+"""
+
+from distutils.core import setup, Extension
+import os
+import sys
+
+progname = sys.argv[0]
+cflags = sys.argv[1]
+files = sys.argv[2:]
+
+if cflags:
+    cflags = [flag for flag in cflags.split(' ') if flag]
+else:
+    cflags = None
+
+libfdt_module = Extension(
+    '_libfdt',
+    sources = files,
+    extra_compile_args =  cflags
+)
+
+sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
+
+setup (name = 'libfdt',
+       version = '0.1',
+       author      = "SWIG Docs",
+       description = """Simple swig libfdt from docs""",
+       ext_modules = [libfdt_module],
+       py_modules = ["libfdt"],
+       )
-- 
2.8.0.rc3.226.g39d4020

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

[David Gibson]

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

On Sat, Dec 03, 2016 at 05:48:07PM -0700, Simon Glass wrote:
> Add Python bindings for a bare-bones set of libfdt functions. These allow
> navigating the tree and reading node names and properties.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v3:
> - Make the library more pythonic
> - Add classes for Node and Prop along with methods
> - Add an exception class
> - Use Python to generate exeptions instead of SWIG
> 
> Changes in v2:
> - Add exceptions when functions return an error
> - Correct Python naming to following PEP8
> - Use a class to encapsulate the various methods
> - Include fdt.h instead of redefining struct fdt_property
> - Use bytearray to avoid the SWIG warning 454
> - Add comments
> 
>  pylibfdt/.gitignore        |   3 +
>  pylibfdt/Makefile.pylibfdt |  18 ++
>  pylibfdt/libfdt.swig       | 602 +++++++++++++++++++++++++++++++++++++++++++++
>  pylibfdt/setup.py          |  34 +++
>  4 files changed, 657 insertions(+)
>  create mode 100644 pylibfdt/.gitignore
>  create mode 100644 pylibfdt/Makefile.pylibfdt
>  create mode 100644 pylibfdt/libfdt.swig
>  create mode 100644 pylibfdt/setup.py
> 
> diff --git a/pylibfdt/.gitignore b/pylibfdt/.gitignore
> new file mode 100644
> index 0000000..5e8c5e3
> --- /dev/null
> +++ b/pylibfdt/.gitignore
> @@ -0,0 +1,3 @@
> +libfdt.py
> +libfdt.pyc
> +libfdt_wrap.c
> diff --git a/pylibfdt/Makefile.pylibfdt b/pylibfdt/Makefile.pylibfdt
> new file mode 100644
> index 0000000..fa74dd2
> --- /dev/null
> +++ b/pylibfdt/Makefile.pylibfdt
> @@ -0,0 +1,18 @@
> +# Makefile.pylibfdt
> +#
> +
> +PYLIBFDT_srcs = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_SRCS))
> +WRAP = $(PYLIBFDT_objdir)/libfdt_wrap.c
> +PYMODULE = $(PYLIBFDT_objdir)/_libfdt.so
> +
> +$(PYMODULE): $(PYLIBFDT_srcs) $(WRAP)
> +	@$(VECHO) PYMOD $@
> +	python $(PYLIBFDT_objdir)/setup.py "$(CPPFLAGS)" $^
> +	mv _libfdt.so $(PYMODULE)
> +
> +$(WRAP): $(PYLIBFDT_srcdir)/libfdt.swig
> +	@$(VECHO) SWIG $@
> +	swig -python -o $@ $<
> +
> +PYLIBFDT_cleanfiles = libfdt_wrap.c libfdt.py libfdt.pyc
> +PYLIBFDT_CLEANFILES = $(addprefix $(PYLIBFDT_objdir)/,$(PYLIBFDT_cleanfiles))
> diff --git a/pylibfdt/libfdt.swig b/pylibfdt/libfdt.swig
> new file mode 100644
> index 0000000..461cdff
> --- /dev/null
> +++ b/pylibfdt/libfdt.swig
> @@ -0,0 +1,602 @@
> +/*
> + * pylibfdt - Flat Device Tree manipulation in Python
> + * Copyright (C) 2016 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)

Pity we can't get swig to autogenerate those.

> +class FdtException(Exception):
> +    """An exception caused by an error such as one of the codes above"""
> +    def __init__(self, err):
> +        self.err = err
> +
> +    def __str__(self):
> +        return 'pylibfdt error %d: %s' % (self.err, fdt_strerror(self.err))
> +
> +def fdt32_to_cpu(val):
> +    """Convert a device-tree cell value into a native integer"""
> +    return struct.unpack("=I", struct.pack(">I", val))[0]
> +
> +def data(prop):
> +    """Extract the data from a property
> +
> +    Args:
> +        prop: Property structure, as returned by get_property_by_offset()
> +
> +    Returns:
> +        The property data as a bytearray
> +    """
> +    buf = bytearray(fdt32_to_cpu(prop.len))

prop.len should be a native value, not requiring byteswap.

> +    pylibfdt_copy_data(buf, prop)
> +    return buf

Urgh.. all this fdt_property() wrangling stuff is really ugly.  Any
chance we could just exclude it from the wrapper, providing only
fdt_getprop() interfaces which simply return a Python bytestring
directly?

> +def strerror(fdt_err):
> +    """Get the string for an error number
> +
> +    Args:
> +        fdt_err: Error number (-ve)
> +
> +    Returns:
> +        String containing the associated error
> +    """
> +    return fdt_strerror(fdt_err)
> +
> +def check_err(val, quiet=False):
> +    """Raise an error if the return value is -ve
> +
> +    This is used to check for errors returned by libfdt C functions.
> +
> +    Args:
> +        val: Return value from a libfdt function
> +        quiet: True to ignore the NOTFOUND error, False to raise on all errors
> +
> +    Returns:
> +        val if val >= 0
> +
> +    Raises
> +        FdtException if val is < 0
> +    """
> +    if val < 0:
> +        if not quiet or val != -NOTFOUND:
> +            raise FdtException(val)
> +    return val
> +
> +def check_err_null(val):
> +    """Raise an error if the return value is NULL
> +
> +    This is used to check for a NULL return value from certain libfdt C
> +    functions
> +
> +    Args:
> +        val: Return value from a libfdt function
> +        quiet: True to ignore the NOTFOUND error, False to raise on all errors
> +
> +    Returns:
> +        val if val is not NULL
> +
> +    Raises
> +        FdtException if val is NULL
> +    """
> +    # Normally a tuple is returned which contains the data and its length.

Yeah.. since Python strings / bytestrings know their own length, why
do we need such a tuple?

> +    # If we get just an integer error code, it means the function failed.
> +    if not isinstance(val, list):
> +        raise FdtException(val)
> +    return val
> +
> +class Fdt:
> +    """Device tree class, supporting all operations
> +
> +    The Fdt object is created is created from a device tree binary file,
> +    e.g. with something like:
> +
> +       fdt = Fdt(open("filename.dtb").read())
> +
> +    Operations can then be performed using the methods in this class. Each
> +    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
> +
> +    Almost all methods raise a FdtException if an error occurs. The
> +    following does not:
> +
> +        string() - since it has no error checking
> +
> +    To avoid this behaviour a 'quiet' version is provided for some functions.
> +    This behaves as for the normal version except that it will not raise
> +    an exception in the case of an FDT_ERR_NOTFOUND error: it will simply
> +    return the -NOTFOUND error code or None.
> +
> +    Separate classes are provided for nodes (Node) and properties (Prop).
> +    """
> +    def __init__(self, data):
> +        self._fdt = bytearray(data)
> +
> +    def string(self, offset):
> +        """Get a string given its offset
> +
> +        Args:
> +            offset: FDT offset in big-endian format
> +
> +        Returns:
> +            string value at that offset
> +        """
> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))

ARGH, no.  The offset argument is an integer, it should be treated as
a native integer, and the byteswap should absolutely not be here.

> +    def path(self, path):
> +        """Get the Node for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +
> +        Returns:
> +            Node object for this node
> +
> +        Raises
> +            FdtException if the path is not valid
> +        """
> +        return Node(self, check_err(fdt_path_offset(self._fdt, path)))

This interface is a mistake - more details on class Node below.

> +    def path_quiet(self, path):
> +        """Get the Node for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +
> +        Returns:
> +            Node object for this node, or None if the path is not valid
> +        """
> +        val = check_err(fdt_path_offset(self._fdt, path), True)
> +        if val < 0:
> +            return None
> +        return Node(self, val)
> +
> +    def path_offset(self, path):
> +        """Get the offset for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +
> +        Returns:
> +            Node offset
> +
> +        Raises
> +            FdtException if the path is not valid
> +        """
> +        return check_err(fdt_path_offset(self._fdt, path))
> +
> +    def path_offset_quiet(self, path):
> +        """Get the offset for a given path
> +
> +        Args:
> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> +
> +        Returns:
> +            Node offset, or -NOTFOUND if the path is not value
> +
> +        Raises
> +            FdtException if any error occurs other than NOTFOUND
> +        """
> +        return check_err(fdt_path_offset(self._fdt, path), True)
> +
> +    def first_property_offset(self, nodeoffset):
> +        """Get the offset of the first property in a node offset
> +
> +        Args:
> +            nodeoffset: Offset to the node to check
> +
> +        Returns:
> +            Offset of the first property
> +
> +        Raises
> +            FdtException if the associated node has no properties, or some
> +                other error occurred
> +        """
> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset))
> +
> +    def first_property_offset_quiet(self, nodeoffset):
> +        """Get the offset of the first property in a node offset
> +
> +        Args:
> +            nodeoffset: Offset to the node to check
> +
> +        Returns:
> +            Offset of the first property, or -ENOTFOUND if the node has no
> +                properties
> +
> +        Raises
> +            FdtException if any other error occurs
> +        """
> +        return check_err(fdt_first_property_offset(self._fdt, nodeoffset), True)
> +
> +    # TODO(sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org): Comment the rest of the methods once everything
> +    # is confirmed.
> +    def next_property_offset(self, prop_offset):
> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset))
> +
> +    def next_property_offset_quiet(self, prop_offset):
> +        return check_err(fdt_next_property_offset(self._fdt, prop_offset), True)
> +
> +    def get_name(self, nodeoffset):
> +        return check_err_null(fdt_get_name(self._fdt, nodeoffset))[0]
> +
> +    def get_property_by_offset_internal(self, prop_offset):
> +        """Obtains a property that can be examined
> +
> +        Args:
> +            prop_offset: Offset of property (e.g. from first_property_offset())
> +
> +        Returns:
> +            Property object with members:
> +                tag: Big-endian device tree tag value
> +                len: Big-endian property length
> +                nameoff: Big-endian string offset for use with string()
> +
> +            Use data() on the return value to obtain the property value.
> +
> +        Raises:
> +            FdtException on error (e.g. invalid prop_offset or device
> +            tree format)
> +        """
> +        return check_err_null(fdt_get_property_by_offset(self._fdt,
> +                                                         prop_offset))[0]
> +
> +    def get_property_by_offset(self, prop_offset):
> +        return Prop.from_offset(self, prop_offset)
> +
> +    def get_property(self, nodeoffset, name):
> +        """Get a property given a node offset and the property name
> +
> +        We cannot use fdt_get_property() here since it does not return the
> +        offset. We prefer to create Node objects using the offset.
> +
> +        Args:
> +            nodeoffset: Offset of the node
> +            name: Property name
> +
> +        Returns:
> +            Prop object found
> +
> +        Raises:
> +            FdtException on error such as no such property
> +        """
> +        poffset = self.first_property_offset(nodeoffset)
> +        while True:
> +            pdata = self.get_property_by_offset_internal(poffset)
> +            if self.string(pdata.nameoff) == name:
> +                return Prop(Node(self, nodeoffset), poffset)
> +            poffset = self.next_property_offset(poffset)

Wait.. what!?  You're searching through the properties in Python?
Why not just using fdt_get_property() or fdt_getprop()?

> +    def get_property_quiet(self, nodeoffset, name):
> +        """Get a property given a node offset and the property name
> +
> +        We cannot use fdt_get_property() here since it does not return the
> +        offset. We prefer to create Node objects using the offset.
> +
> +        Args:
> +            nodeoffset: Offset of the node
> +            name: Property name
> +
> +        Returns:
> +            Prop object found or None if there is no such property
> +
> +        Raises:
> +            FdtException on error
> +        """
> +        poffset = self.first_property_offset_quiet(nodeoffset)
> +        while poffset >= 0:
> +            pdata = self.get_property_by_offset_internal(poffset)
> +            if self.string(pdata.nameoff) == name:
> +                return Prop(Node(self, nodeoffset), poffset)
> +            poffset = self.next_property_offset_quiet(poffset)
> +        return None
> +
> +    def first_subnode(self, nodeoffset):
> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
> +
> +    def first_subnode_quiet(self, nodeoffset):
> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
> +
> +    def next_subnode(self, nodeoffset):
> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
> +
> +    def next_subnode_quiet(self, nodeoffset):
> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
> +
> +    def totalsize(self):
> +        return check_err(fdt_totalsize(self._fdt))
> +
> +    def off_dt_struct(self):
> +        return check_err(fdt_off_dt_struct(self._fdt))
> +
> +    def pack(self):
> +        return check_err(fdt_pack(self._fdt))
> +
> +    def delprop(self, nodeoffset, prop_name):
> +        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
> +
> +
> +class Node:

This can't work.  The reason that so many libfdt functions are
insistently called whatever_offset(), rather than just get_whatever()
is to constantly remind the user that this is a actual byte offset
into a buffer of data.  It's *not* a friendly stable node-handle, and
can become invalid when the fdt blob is altered.

Attempting to put a class wrapper around it obscures that vital fact.
Treat node offsets as offsets everywhere, just encoded as plain old
python integers.

> +    """A device tree node
> +
> +    This encapsulates a device-tree node and provides various operations
> +    which can be performed on the node. In particular it is possible to find
> +    subnodes and properties.
> +    """
> +    def __init__(self, fdt, offset):
> +        self._fdt = fdt
> +        self._offset = offset
> +
> +    def first_property(self):
> +        return Prop(self, check_err(fdt_first_property_offset(self._fdt._fdt,
> +                                                              self._offset)))
> +
> +    def first_property_quiet(self):
> +        val = check_err(fdt_first_property_offset(self._fdt._fdt, self._offset),
> +                        True)
> +        if val < 0:
> +            return None
> +        return Prop(self, val)
> +
> +    def props(self):
> +        props = []
> +        prop = self.first_property_quiet()
> +        while prop:
> +            props.append(prop)
> +            prop = prop.next_quiet()
> +        return props
> +
> +    def first_subnode(self):
> +        val = check_err(fdt_first_subnode(self._fdt._fdt, self._offset))
> +        return Node(self._fdt, val)
> +
> +    def next_subnode(self):
> +        val = check_err(fdt_next_subnode(self._fdt._fdt, self._offset))
> +        return Node(self._fdt, val)
> +
> +    def first_subnode_quiet(self):
> +        val = check_err(fdt_first_subnode(self._fdt._fdt, self._offset), True)
> +        if val < 0:
> +            return None
> +        return Node(self._fdt, val)
> +
> +    def next_subnode_quiet(self):
> +        val = check_err(fdt_next_subnode(self._fdt._fdt, self._offset), True)
> +        if val < 0:
> +            return None
> +        return Node(self._fdt, val)
> +
> +    def name(self):
> +        return check_err_null(fdt_get_name(self._fdt._fdt, self._offset))[0]
> +
> +    def prop(self, name):
> +        return self._fdt.get_property(self._offset, name)
> +
> +
> +class Prop:
> +    """A device-tree property
> +
> +    This encapsulates a device-tree property and provides various operations
> +    which can be performed on the property.
> +
> +    Generally the standard constructor is used to create a Prop object. But
> +    in the case where only the property offset is known (and not the Node
> +    that holds the property), from_offset() can be used.
> +    """

TBH, I'm not really sold on the value of this class either.  Is there
really any point dealing with these rather fragile property handles,
rather than simply returning bytestrings with the values directly?  I
can't see it.

> +    def __init__(self, node, offset, fdt=None):
> +        self._node = node
> +        self._offset = offset
> +        self._fdt = node._fdt if node else fdt
> +
> +    @classmethod
> +    def from_offset(cls, fdt, prop_offset):
> +        prop = cls(None, prop_offset, fdt)
> +        prop._get_name_data()
> +        return prop
> +
> +    def next(self):
> +        return Prop(self._node, check_err(fdt_next_property_offset(
> +                self._fdt._fdt, self._offset)))
> +
> +    def next_quiet(self):
> +        val = check_err(fdt_next_property_offset(self._fdt._fdt, self._offset),
> +                        True)
> +        if val < 0:
> +            return None
> +        return Prop(self._node, val)
> +
> +    def _get_name_data(self):
> +        pdata = fdt_get_property_by_offset(self._fdt._fdt, self._offset)
> +        if not isinstance(pdata, list):
> +            raise FdtException(pdata)
> +        return self._fdt.string(pdata[0].nameoff), data(pdata[0])
> +
> +    def name(self):
> +        return self._get_name_data()[0]
> +
> +    def data(self):
> +        return self._get_name_data()[1]
> +
> +    def delete(self):
> +        if not self._node:
> +            raise RuntimeError("Can't delete property offset %d of unknown node"
> +                               % self._offset)
> +        self._fdt.delprop(self._node._offset, self.name())
> +%}
> +
> +typedef int fdt32_t;
> +
> +%include "libfdt/fdt.h"
> +
> +%include "typemaps.i"
> +
> +%typemap(in) void * = char *;
> +
> +/*
> + * Unfortunately the defintiion of pybuffer_mutable_binary() in my Python
> + * version appears to be broken:
> + * pylibfdt/libfdt_wrap.c: In function ‘_wrap_pylibfdt_copy_data’:
> + * pylibfdt/libfdt_wrap.c:3603:22: error: ‘size’ undeclared (first use in this
> + * function)
> + *   arg2 = (size_t) (size/sizeof(char));
> + *
> + * This version works correctly.
> + */
> +%define %mypybuffer_mutable_binary(TYPEMAP, SIZE)
> +%typemap(in) (TYPEMAP, SIZE)(int res, Py_ssize_t size = 0, void *buf = 0)
> +{
> +	res = PyObject_AsWriteBuffer($input, &buf, &size);
> +	if (res < 0) {
> +		PyErr_Clear();
> +		%argument_fail(res, "(TYPEMAP, SIZE)", $symname, $argnum);
> +	}
> +	$1 = ($1_ltype)buf;
> +	$2 = ($2_ltype)(size1 / sizeof($*1_type));
> +}
> +%enddef
> +
> +/* This is used to copy property data into a bytearray */
> +%mypybuffer_mutable_binary(char *str, size_t size);
> +void pylibfdt_copy_data(char *str, size_t size,
> +			const struct fdt_property *prop);
> +
> +/* Most functions don't change the device tree, so use a const void * */
> +%typemap(in) (const void *) {
> +    if (!PyByteArray_Check($input)) {
> +        SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
> +                            "', argument " "$argnum"" of type '" "$type""'");
> +  }
> +  $1 = (void *)PyByteArray_AsString($input);
> +}
> +
> +/* Some functions do change the device tree, so use void * */
> +%typemap(in) (void *) {
> +    if (!PyByteArray_Check($input)) {
> +        SWIG_exception_fail(SWIG_TypeError, "in method '" "$symname"
> +                            "', argument " "$argnum"" of type '" "$type""'");
> +  }
> +  $1 = PyByteArray_AsString($input);
> +}
> +
> +%inline %{
> +
> +/**
> + * pylibfdt_copy_data() - Copy data from a property to the given buffer
> + *
> + * This is used by the data() function to place the contents of a property
> + * into a bytearray.
> + *
> + * @buf: Destination pointer (typically the start of the bytearray)
> + * @size: Number of bytes to copy (size of bytearray)
> + * @prop: Property to copy
> + */
> +void pylibfdt_copy_data(char *buf, size_t size, const struct fdt_property *prop)
> +{
> +	memcpy(buf, prop + 1, size);
> +}
> +
> +%}
> +
> +/*
> + * From here are the function definitions from libfdt.h, along with their
> + * exception-handling code.
> + */

Uh.. why do you need to duplicate these rather than including libfdt.h?

> +int fdt_path_offset(const void *fdt, const char *path);
> +
> +int fdt_first_property_offset(const void *fdt, int nodeoffset);
> +
> +int fdt_next_property_offset(const void *fdt, int offset);
> +
> +const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
> +
> +/* no exception handling, since this function has no error checking */
> +const char *fdt_string(const void *fdt, int stroffset);
> +
> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> +                                                      int offset, int *OUTPUT);
> +
> +/* no exception handling, this this function always returns a valid string */
> +const char *fdt_strerror(int errval);
> +
> +int fdt_first_subnode(const void *fdt, int offset);
> +
> +int fdt_next_subnode(const void *fdt, int offset);
> +
> +int fdt_delprop(void *fdt, int nodeoffset, const char *name);
> +
> +int fdt_pack(void *fdt);
> +
> +int fdt_totalsize(const void *fdt);
> +
> +int fdt_off_dt_struct(const void *fdt);
> diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
> new file mode 100644
> index 0000000..8f8618e
> --- /dev/null
> +++ b/pylibfdt/setup.py
> @@ -0,0 +1,34 @@
> +#!/usr/bin/env python
> +
> +"""
> +setup.py file for SWIG libfdt
> +"""
> +
> +from distutils.core import setup, Extension
> +import os
> +import sys
> +
> +progname = sys.argv[0]
> +cflags = sys.argv[1]
> +files = sys.argv[2:]
> +
> +if cflags:
> +    cflags = [flag for flag in cflags.split(' ') if flag]
> +else:
> +    cflags = None
> +
> +libfdt_module = Extension(
> +    '_libfdt',
> +    sources = files,
> +    extra_compile_args =  cflags
> +)
> +
> +sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
> +
> +setup (name = 'libfdt',
> +       version = '0.1',
> +       author      = "SWIG Docs",

Um.. that doesn't seem quite right.

> +       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 v3 1/4] Add an initial Python library for libfdt

[Simon Glass]

Hi David,

On 4 December 2016 at 21:35, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sat, Dec 03, 2016 at 05:48:07PM -0700, Simon Glass wrote:
>> Add Python bindings for a bare-bones set of libfdt functions. These allow
>> navigating the tree and reading node names and properties.
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>
>> Changes in v3:
>> - Make the library more pythonic
>> - Add classes for Node and Prop along with methods
>> - Add an exception class
>> - Use Python to generate exeptions instead of SWIG
>>
>> Changes in v2:
>> - Add exceptions when functions return an error
>> - Correct Python naming to following PEP8
>> - Use a class to encapsulate the various methods
>> - Include fdt.h instead of redefining struct fdt_property
>> - Use bytearray to avoid the SWIG warning 454
>> - Add comments
>>
>>  pylibfdt/.gitignore        |   3 +
>>  pylibfdt/Makefile.pylibfdt |  18 ++
>>  pylibfdt/libfdt.swig       | 602 +++++++++++++++++++++++++++++++++++++++++++++
>>  pylibfdt/setup.py          |  34 +++
>>  4 files changed, 657 insertions(+)
>>  create mode 100644 pylibfdt/.gitignore
>>  create mode 100644 pylibfdt/Makefile.pylibfdt
>>  create mode 100644 pylibfdt/libfdt.swig
>>  create mode 100644 pylibfdt/setup.py
>>
[..]

>> +# Error codes, corresponding to FDT_ERR_... in libfdt.h
>> +(NOTFOUND,
>> +        EXISTS,
>> +        NOSPACE,
>> +        BADOFFSET,
>> +        BADPATH,
>> +        BADPHANDLE,
>> +        BADSTATE,
>> +        TRUNCATED,
>> +        BADMAGIC,
>> +        BADVERSION,
>> +        BADSTRUCTURE,
>> +        BADLAYOUT,
>> +        INTERNAL,
>> +        BADNCELLS,
>> +        BADVALUE,
>> +        BADOVERLAY) = range(1, 17)
>
> Pity we can't get swig to autogenerate those.

Maybe we can if you want it to change to an enum?

>
>> +class FdtException(Exception):
>> +    """An exception caused by an error such as one of the codes above"""
>> +    def __init__(self, err):
>> +        self.err = err
>> +
>> +    def __str__(self):
>> +        return 'pylibfdt error %d: %s' % (self.err, fdt_strerror(self.err))
>> +
>> +def fdt32_to_cpu(val):
>> +    """Convert a device-tree cell value into a native integer"""
>> +    return struct.unpack("=I", struct.pack(">I", val))[0]
>> +
>> +def data(prop):
>> +    """Extract the data from a property
>> +
>> +    Args:
>> +        prop: Property structure, as returned by get_property_by_offset()
>> +
>> +    Returns:
>> +        The property data as a bytearray
>> +    """
>> +    buf = bytearray(fdt32_to_cpu(prop.len))
>
> prop.len should be a native value, not requiring byteswap.
>
>> +    pylibfdt_copy_data(buf, prop)
>> +    return buf
>
> Urgh.. all this fdt_property() wrangling stuff is really ugly.  Any
> chance we could just exclude it from the wrapper, providing only
> fdt_getprop() interfaces which simply return a Python bytestring
> directly?

I effectively have, by hiding it in here. Client code need never use this.

>
>> +def strerror(fdt_err):
>> +    """Get the string for an error number
>> +
>> +    Args:
>> +        fdt_err: Error number (-ve)
>> +
>> +    Returns:
>> +        String containing the associated error
>> +    """
>> +    return fdt_strerror(fdt_err)
>> +
>> +def check_err(val, quiet=False):
>> +    """Raise an error if the return value is -ve
>> +
>> +    This is used to check for errors returned by libfdt C functions.
>> +
>> +    Args:
>> +        val: Return value from a libfdt function
>> +        quiet: True to ignore the NOTFOUND error, False to raise on all errors
>> +
>> +    Returns:
>> +        val if val >= 0
>> +
>> +    Raises
>> +        FdtException if val is < 0
>> +    """
>> +    if val < 0:
>> +        if not quiet or val != -NOTFOUND:
>> +            raise FdtException(val)
>> +    return val
>> +
>> +def check_err_null(val):
>> +    """Raise an error if the return value is NULL
>> +
>> +    This is used to check for a NULL return value from certain libfdt C
>> +    functions
>> +
>> +    Args:
>> +        val: Return value from a libfdt function
>> +        quiet: True to ignore the NOTFOUND error, False to raise on all errors
>> +
>> +    Returns:
>> +        val if val is not NULL
>> +
>> +    Raises
>> +        FdtException if val is NULL
>> +    """
>> +    # Normally a tuple is returned which contains the data and its length.
>
> Yeah.. since Python strings / bytestrings know their own length, why
> do we need such a tuple?

It's just what swig returns for functions like: const char *func(int *len)

This is internal to the class so should not matter to users.

>
>> +    # If we get just an integer error code, it means the function failed.
>> +    if not isinstance(val, list):
>> +        raise FdtException(val)
>> +    return val
>> +
>> +class Fdt:
>> +    """Device tree class, supporting all operations
>> +
>> +    The Fdt object is created is created from a device tree binary file,
>> +    e.g. with something like:
>> +
>> +       fdt = Fdt(open("filename.dtb").read())
>> +
>> +    Operations can then be performed using the methods in this class. Each
>> +    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
>> +
>> +    Almost all methods raise a FdtException if an error occurs. The
>> +    following does not:
>> +
>> +        string() - since it has no error checking
>> +
>> +    To avoid this behaviour a 'quiet' version is provided for some functions.
>> +    This behaves as for the normal version except that it will not raise
>> +    an exception in the case of an FDT_ERR_NOTFOUND error: it will simply
>> +    return the -NOTFOUND error code or None.
>> +
>> +    Separate classes are provided for nodes (Node) and properties (Prop).
>> +    """
>> +    def __init__(self, data):
>> +        self._fdt = bytearray(data)
>> +
>> +    def string(self, offset):
>> +        """Get a string given its offset
>> +
>> +        Args:
>> +            offset: FDT offset in big-endian format
>> +
>> +        Returns:
>> +            string value at that offset
>> +        """
>> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
>
> ARGH, no.  The offset argument is an integer, it should be treated as
> a native integer, and the byteswap should absolutely not be here.

Ah yes, oops.

>
>> +    def path(self, path):
>> +        """Get the Node for a given path
>> +
>> +        Args:
>> +            path: Path to the required node, e.g. '/node@3/subnode@1'
>> +
>> +        Returns:
>> +            Node object for this node
>> +
>> +        Raises
>> +            FdtException if the path is not valid
>> +        """
>> +        return Node(self, check_err(fdt_path_offset(self._fdt, path)))
>
> This interface is a mistake - more details on class Node below.

[..]

>> +    def get_property(self, nodeoffset, name):
>> +        """Get a property given a node offset and the property name
>> +
>> +        We cannot use fdt_get_property() here since it does not return the
>> +        offset. We prefer to create Node objects using the offset.
>> +
>> +        Args:
>> +            nodeoffset: Offset of the node
>> +            name: Property name
>> +
>> +        Returns:
>> +            Prop object found
>> +
>> +        Raises:
>> +            FdtException on error such as no such property
>> +        """
>> +        poffset = self.first_property_offset(nodeoffset)
>> +        while True:
>> +            pdata = self.get_property_by_offset_internal(poffset)
>> +            if self.string(pdata.nameoff) == name:
>> +                return Prop(Node(self, nodeoffset), poffset)
>> +            poffset = self.next_property_offset(poffset)
>
> Wait.. what!?  You're searching through the properties in Python?
> Why not just using fdt_get_property() or fdt_getprop()?

Because they do not tell me the offset-(see the comment in the
function description above:

        We cannot use fdt_get_property() here since it does not return the
        offset. We prefer to create Node objects using the offset.

If we want to have a Node object, then we probably want to store the
offset of the node. Another option would be to add a function to
libfdt which gets a property by name, but returns its offset, rather
than a struct?

>
>> +    def get_property_quiet(self, nodeoffset, name):
>> +        """Get a property given a node offset and the property name
>> +
>> +        We cannot use fdt_get_property() here since it does not return the
>> +        offset. We prefer to create Node objects using the offset.
>> +
>> +        Args:
>> +            nodeoffset: Offset of the node
>> +            name: Property name
>> +
>> +        Returns:
>> +            Prop object found or None if there is no such property
>> +
>> +        Raises:
>> +            FdtException on error
>> +        """
>> +        poffset = self.first_property_offset_quiet(nodeoffset)
>> +        while poffset >= 0:
>> +            pdata = self.get_property_by_offset_internal(poffset)
>> +            if self.string(pdata.nameoff) == name:
>> +                return Prop(Node(self, nodeoffset), poffset)
>> +            poffset = self.next_property_offset_quiet(poffset)
>> +        return None
>> +
>> +    def first_subnode(self, nodeoffset):
>> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
>> +
>> +    def first_subnode_quiet(self, nodeoffset):
>> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
>> +
>> +    def next_subnode(self, nodeoffset):
>> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
>> +
>> +    def next_subnode_quiet(self, nodeoffset):
>> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
>> +
>> +    def totalsize(self):
>> +        return check_err(fdt_totalsize(self._fdt))
>> +
>> +    def off_dt_struct(self):
>> +        return check_err(fdt_off_dt_struct(self._fdt))
>> +
>> +    def pack(self):
>> +        return check_err(fdt_pack(self._fdt))
>> +
>> +    def delprop(self, nodeoffset, prop_name):
>> +        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
>> +
>> +
>> +class Node:
>
> This can't work.  The reason that so many libfdt functions are
> insistently called whatever_offset(), rather than just get_whatever()
> is to constantly remind the user that this is a actual byte offset
> into a buffer of data.  It's *not* a friendly stable node-handle, and
> can become invalid when the fdt blob is altered.
>
> Attempting to put a class wrapper around it obscures that vital fact.
> Treat node offsets as offsets everywhere, just encoded as plain old
> python integers.

My intent is to make the interface more pythonic, and I've been trying
out various approaches. For read-only use this interface is nice.
Consider this the high-water mark of this approach.

But I can go back to something in between, where there is no Node or
Prop. But please do check the code example in the cover letter and see
if you are sure.

With this approach we can do things like node.next() which is nice I think.

Of course things can break if you start changing the tree while
keeping objects around. So it isn't a perfect illusion. Hopefully it
returns errors in that case..

[..]

>> +class Prop:
>> +    """A device-tree property
>> +
>> +    This encapsulates a device-tree property and provides various operations
>> +    which can be performed on the property.
>> +
>> +    Generally the standard constructor is used to create a Prop object. But
>> +    in the case where only the property offset is known (and not the Node
>> +    that holds the property), from_offset() can be used.
>> +    """
>
> TBH, I'm not really sold on the value of this class either.  Is there
> really any point dealing with these rather fragile property handles,
> rather than simply returning bytestrings with the values directly?  I
> can't see it.

It provides the ability to iterate, get the name (as well as the
data), delete the property, etc.

>
>> +    def __init__(self, node, offset, fdt=None):
>> +        self._node = node
>> +        self._offset = offset
>> +        self._fdt = node._fdt if node else fdt
>> +
>> +    @classmethod
>> +    def from_offset(cls, fdt, prop_offset):
>> +        prop = cls(None, prop_offset, fdt)
>> +        prop._get_name_data()
>> +        return prop
>> +
>> +    def next(self):
>> +        return Prop(self._node, check_err(fdt_next_property_offset(
>> +                self._fdt._fdt, self._offset)))
>> +
>> +    def next_quiet(self):
>> +        val = check_err(fdt_next_property_offset(self._fdt._fdt, self._offset),
>> +                        True)
>> +        if val < 0:
>> +            return None
>> +        return Prop(self._node, val)
>> +
>> +    def _get_name_data(self):
>> +        pdata = fdt_get_property_by_offset(self._fdt._fdt, self._offset)
>> +        if not isinstance(pdata, list):
>> +            raise FdtException(pdata)
>> +        return self._fdt.string(pdata[0].nameoff), data(pdata[0])
>> +
>> +    def name(self):
>> +        return self._get_name_data()[0]
>> +
>> +    def data(self):
>> +        return self._get_name_data()[1]
>> +
>> +    def delete(self):
>> +        if not self._node:
>> +            raise RuntimeError("Can't delete property offset %d of unknown node"
>> +                               % self._offset)
>> +        self._fdt.delprop(self._node._offset, self.name())
>> +%}

[..]

>> +
>> +/*
>> + * From here are the function definitions from libfdt.h, along with their
>> + * exception-handling code.
>> + */
>
> Uh.. why do you need to duplicate these rather than including libfdt.h?

Hopefully I can do that, even with fdt_totalsize(). Will check.

>
>> +int fdt_path_offset(const void *fdt, const char *path);
>> +
>> +int fdt_first_property_offset(const void *fdt, int nodeoffset);
>> +
>> +int fdt_next_property_offset(const void *fdt, int offset);
>> +
>> +const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
>> +
>> +/* no exception handling, since this function has no error checking */
>> +const char *fdt_string(const void *fdt, int stroffset);
>> +
>> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>> +                                                      int offset, int *OUTPUT);
>> +
>> +/* no exception handling, this this function always returns a valid string */
>> +const char *fdt_strerror(int errval);
>> +
>> +int fdt_first_subnode(const void *fdt, int offset);
>> +
>> +int fdt_next_subnode(const void *fdt, int offset);
>> +
>> +int fdt_delprop(void *fdt, int nodeoffset, const char *name);
>> +
>> +int fdt_pack(void *fdt);
>> +
>> +int fdt_totalsize(const void *fdt);
>> +
>> +int fdt_off_dt_struct(const void *fdt);
>> diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
>> new file mode 100644
>> index 0000000..8f8618e
>> --- /dev/null
>> +++ b/pylibfdt/setup.py
>> @@ -0,0 +1,34 @@
>> +#!/usr/bin/env python
>> +
>> +"""
>> +setup.py file for SWIG libfdt
>> +"""
>> +
>> +from distutils.core import setup, Extension
>> +import os
>> +import sys
>> +
>> +progname = sys.argv[0]
>> +cflags = sys.argv[1]
>> +files = sys.argv[2:]
>> +
>> +if cflags:
>> +    cflags = [flag for flag in cflags.split(' ') if flag]
>> +else:
>> +    cflags = None
>> +
>> +libfdt_module = Extension(
>> +    '_libfdt',
>> +    sources = files,
>> +    extra_compile_args =  cflags
>> +)
>> +
>> +sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
>> +
>> +setup (name = 'libfdt',
>> +       version = '0.1',
>> +       author      = "SWIG Docs",
>
> Um.. that doesn't seem quite right.

Nope, will fix.

>
>> +       description = """Simple swig libfdt from docs""",
>> +       ext_modules = [libfdt_module],
>> +       py_modules = ["libfdt"],
>> +       )
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Regards,
Simon

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

[David Gibson]

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

On Sun, Dec 04, 2016 at 11:23:32PM -0700, Simon Glass wrote:
> Hi David,

Sorry for the slow reply.  I was away for a while, and then it just
fell off my radar.

> On 4 December 2016 at 21:35, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Sat, Dec 03, 2016 at 05:48:07PM -0700, Simon Glass wrote:
> >> Add Python bindings for a bare-bones set of libfdt functions. These allow
> >> navigating the tree and reading node names and properties.
> >>
> >> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> ---
> >>
> >> Changes in v3:
> >> - Make the library more pythonic
> >> - Add classes for Node and Prop along with methods
> >> - Add an exception class
> >> - Use Python to generate exeptions instead of SWIG
> >>
> >> Changes in v2:
> >> - Add exceptions when functions return an error
> >> - Correct Python naming to following PEP8
> >> - Use a class to encapsulate the various methods
> >> - Include fdt.h instead of redefining struct fdt_property
> >> - Use bytearray to avoid the SWIG warning 454
> >> - Add comments
> >>
> >>  pylibfdt/.gitignore        |   3 +
> >>  pylibfdt/Makefile.pylibfdt |  18 ++
> >>  pylibfdt/libfdt.swig       | 602 +++++++++++++++++++++++++++++++++++++++++++++
> >>  pylibfdt/setup.py          |  34 +++
> >>  4 files changed, 657 insertions(+)
> >>  create mode 100644 pylibfdt/.gitignore
> >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> >>  create mode 100644 pylibfdt/libfdt.swig
> >>  create mode 100644 pylibfdt/setup.py
> >>
> [..]
> 
> >> +# Error codes, corresponding to FDT_ERR_... in libfdt.h
> >> +(NOTFOUND,
> >> +        EXISTS,
> >> +        NOSPACE,
> >> +        BADOFFSET,
> >> +        BADPATH,
> >> +        BADPHANDLE,
> >> +        BADSTATE,
> >> +        TRUNCATED,
> >> +        BADMAGIC,
> >> +        BADVERSION,
> >> +        BADSTRUCTURE,
> >> +        BADLAYOUT,
> >> +        INTERNAL,
> >> +        BADNCELLS,
> >> +        BADVALUE,
> >> +        BADOVERLAY) = range(1, 17)
> >
> > Pity we can't get swig to autogenerate those.
> 
> Maybe we can if you want it to change to an enum?

Right.  Hmm, might be worth doing; I don't see any immediate reason
we'd need these constants at the cpp level.

> >> +class FdtException(Exception):
> >> +    """An exception caused by an error such as one of the codes above"""
> >> +    def __init__(self, err):
> >> +        self.err = err
> >> +
> >> +    def __str__(self):
> >> +        return 'pylibfdt error %d: %s' % (self.err, fdt_strerror(self.err))
> >> +
> >> +def fdt32_to_cpu(val):
> >> +    """Convert a device-tree cell value into a native integer"""
> >> +    return struct.unpack("=I", struct.pack(">I", val))[0]
> >> +
> >> +def data(prop):
> >> +    """Extract the data from a property
> >> +
> >> +    Args:
> >> +        prop: Property structure, as returned by get_property_by_offset()
> >> +
> >> +    Returns:
> >> +        The property data as a bytearray
> >> +    """
> >> +    buf = bytearray(fdt32_to_cpu(prop.len))
> >
> > prop.len should be a native value, not requiring byteswap.
> >
> >> +    pylibfdt_copy_data(buf, prop)
> >> +    return buf
> >
> > Urgh.. all this fdt_property() wrangling stuff is really ugly.  Any
> > chance we could just exclude it from the wrapper, providing only
> > fdt_getprop() interfaces which simply return a Python bytestring
> > directly?
> 
> I effectively have, by hiding it in here. Client code need never use this.

I don't see that that's the case.  At the very least they get this
prop object back from some functions and have to explicitly call
data() to get the actual value out.

I wonder if the problem here is that you're trying to write a faithful
wrapper for fdt_get_property().  I don't know that there's any point -
it's really intended for some internal usage and for performance in
some special cases that make sense in C.  For the Python wrapper I
think you could just leave it out and provide only fdt_getprop() -
that returns the property value directly rather than a complicated to
encode structure.

> >> +def strerror(fdt_err):
> >> +    """Get the string for an error number
> >> +
> >> +    Args:
> >> +        fdt_err: Error number (-ve)
> >> +
> >> +    Returns:
> >> +        String containing the associated error
> >> +    """
> >> +    return fdt_strerror(fdt_err)
> >> +
> >> +def check_err(val, quiet=False):
> >> +    """Raise an error if the return value is -ve
> >> +
> >> +    This is used to check for errors returned by libfdt C functions.
> >> +
> >> +    Args:
> >> +        val: Return value from a libfdt function
> >> +        quiet: True to ignore the NOTFOUND error, False to raise on all errors
> >> +
> >> +    Returns:
> >> +        val if val >= 0
> >> +
> >> +    Raises
> >> +        FdtException if val is < 0
> >> +    """
> >> +    if val < 0:
> >> +        if not quiet or val != -NOTFOUND:
> >> +            raise FdtException(val)
> >> +    return val
> >> +
> >> +def check_err_null(val):
> >> +    """Raise an error if the return value is NULL
> >> +
> >> +    This is used to check for a NULL return value from certain libfdt C
> >> +    functions
> >> +
> >> +    Args:
> >> +        val: Return value from a libfdt function
> >> +        quiet: True to ignore the NOTFOUND error, False to raise on all errors
> >> +
> >> +    Returns:
> >> +        val if val is not NULL
> >> +
> >> +    Raises
> >> +        FdtException if val is NULL
> >> +    """
> >> +    # Normally a tuple is returned which contains the data and its length.
> >
> > Yeah.. since Python strings / bytestrings know their own length, why
> > do we need such a tuple?
> 
> It's just what swig returns for functions like: const char *func(int *len)
> 
> This is internal to the class so should not matter to users.

Hm, ok.

> >> +    # If we get just an integer error code, it means the function failed.
> >> +    if not isinstance(val, list):
> >> +        raise FdtException(val)
> >> +    return val
> >> +
> >> +class Fdt:
> >> +    """Device tree class, supporting all operations
> >> +
> >> +    The Fdt object is created is created from a device tree binary file,
> >> +    e.g. with something like:
> >> +
> >> +       fdt = Fdt(open("filename.dtb").read())
> >> +
> >> +    Operations can then be performed using the methods in this class. Each
> >> +    method xxx(args...) corresponds to a libfdt function fdt_xxx(fdt, args...).
> >> +
> >> +    Almost all methods raise a FdtException if an error occurs. The
> >> +    following does not:
> >> +
> >> +        string() - since it has no error checking
> >> +
> >> +    To avoid this behaviour a 'quiet' version is provided for some functions.
> >> +    This behaves as for the normal version except that it will not raise
> >> +    an exception in the case of an FDT_ERR_NOTFOUND error: it will simply
> >> +    return the -NOTFOUND error code or None.
> >> +
> >> +    Separate classes are provided for nodes (Node) and properties (Prop).
> >> +    """
> >> +    def __init__(self, data):
> >> +        self._fdt = bytearray(data)
> >> +
> >> +    def string(self, offset):
> >> +        """Get a string given its offset
> >> +
> >> +        Args:
> >> +            offset: FDT offset in big-endian format
> >> +
> >> +        Returns:
> >> +            string value at that offset
> >> +        """
> >> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> >
> > ARGH, no.  The offset argument is an integer, it should be treated as
> > a native integer, and the byteswap should absolutely not be here.
> 
> Ah yes, oops.
> 
> >
> >> +    def path(self, path):
> >> +        """Get the Node for a given path
> >> +
> >> +        Args:
> >> +            path: Path to the required node, e.g. '/node@3/subnode@1'
> >> +
> >> +        Returns:
> >> +            Node object for this node
> >> +
> >> +        Raises
> >> +            FdtException if the path is not valid
> >> +        """
> >> +        return Node(self, check_err(fdt_path_offset(self._fdt, path)))
> >
> > This interface is a mistake - more details on class Node below.
> 
> [..]
> 
> >> +    def get_property(self, nodeoffset, name):
> >> +        """Get a property given a node offset and the property name
> >> +
> >> +        We cannot use fdt_get_property() here since it does not return the
> >> +        offset. We prefer to create Node objects using the offset.
> >> +
> >> +        Args:
> >> +            nodeoffset: Offset of the node
> >> +            name: Property name
> >> +
> >> +        Returns:
> >> +            Prop object found
> >> +
> >> +        Raises:
> >> +            FdtException on error such as no such property
> >> +        """
> >> +        poffset = self.first_property_offset(nodeoffset)
> >> +        while True:
> >> +            pdata = self.get_property_by_offset_internal(poffset)
> >> +            if self.string(pdata.nameoff) == name:
> >> +                return Prop(Node(self, nodeoffset), poffset)
> >> +            poffset = self.next_property_offset(poffset)
> >
> > Wait.. what!?  You're searching through the properties in Python?
> > Why not just using fdt_get_property() or fdt_getprop()?
> 
> Because they do not tell me the offset-(see the comment in the
> function description above:
> 
>         We cannot use fdt_get_property() here since it does not return the
>         offset. We prefer to create Node objects using the offset.
> 
> If we want to have a Node object, then we probably want to store the
> offset of the node. Another option would be to add a function to
> libfdt which gets a property by name, but returns its offset, rather
> than a struct?

So, most of the above is moot, because the Node object is a bad idea -
see below.

> >> +    def get_property_quiet(self, nodeoffset, name):
> >> +        """Get a property given a node offset and the property name
> >> +
> >> +        We cannot use fdt_get_property() here since it does not return the
> >> +        offset. We prefer to create Node objects using the offset.
> >> +
> >> +        Args:
> >> +            nodeoffset: Offset of the node
> >> +            name: Property name
> >> +
> >> +        Returns:
> >> +            Prop object found or None if there is no such property
> >> +
> >> +        Raises:
> >> +            FdtException on error
> >> +        """
> >> +        poffset = self.first_property_offset_quiet(nodeoffset)
> >> +        while poffset >= 0:
> >> +            pdata = self.get_property_by_offset_internal(poffset)
> >> +            if self.string(pdata.nameoff) == name:
> >> +                return Prop(Node(self, nodeoffset), poffset)
> >> +            poffset = self.next_property_offset_quiet(poffset)
> >> +        return None
> >> +
> >> +    def first_subnode(self, nodeoffset):
> >> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
> >> +
> >> +    def first_subnode_quiet(self, nodeoffset):
> >> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
> >> +
> >> +    def next_subnode(self, nodeoffset):
> >> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
> >> +
> >> +    def next_subnode_quiet(self, nodeoffset):
> >> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
> >> +
> >> +    def totalsize(self):
> >> +        return check_err(fdt_totalsize(self._fdt))
> >> +
> >> +    def off_dt_struct(self):
> >> +        return check_err(fdt_off_dt_struct(self._fdt))
> >> +
> >> +    def pack(self):
> >> +        return check_err(fdt_pack(self._fdt))
> >> +
> >> +    def delprop(self, nodeoffset, prop_name):
> >> +        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
> >> +
> >> +
> >> +class Node:
> >
> > This can't work.  The reason that so many libfdt functions are
> > insistently called whatever_offset(), rather than just get_whatever()
> > is to constantly remind the user that this is a actual byte offset
> > into a buffer of data.  It's *not* a friendly stable node-handle, and
> > can become invalid when the fdt blob is altered.
> >
> > Attempting to put a class wrapper around it obscures that vital fact.
> > Treat node offsets as offsets everywhere, just encoded as plain old
> > python integers.
> 
> My intent is to make the interface more pythonic, and I've been trying
> out various approaches. For read-only use this interface is nice.
> Consider this the high-water mark of this approach.

Ok.  So making the interface Pythonic is a reasonable goal - but not
when it obscures fundamental properties of behaviour.  It's simply not
possible to have a "stable" node (or property) handle with libfdt, so
we shouldn't make the Python interface appear to have one.

> But I can go back to something in between, where there is no Node or
> Prop. But please do check the code example in the cover letter and see
> if you are sure.

I am absoltely sure.  Node and Property objects are a bad idea.
Well.. Node objects definitely are.  Property objects could work, but
only if they're essentially just representing a (name, length, value)
tuple, *not* an offset into the actual tree.  i.e. such a property
object would be a copy of some data from the tree, not a handle to
something still within the tree.

> With this approach we can do things like node.next() which is nice I think.

Nice until you have a use case which breaks it, at which point it
becomes horribly confusing.

> Of course things can break if you start changing the tree while
> keeping objects around. So it isn't a perfect illusion. Hopefully it
> returns errors in that case..

It's not really possible to safely return errors in this case.
Usually you will get an error because your offset will point at
something bogus.  But it doesn't require that much bad luck for the
old offset to line up with something that's no longer the same thing
but looks vaguely sane, at which point you'll return valid looking but
wrong data.  You might hit an error condition later, or maybe you'll
just carry that bad data into the rest of your program.

With a bit more bad luck you could even end up with an infinite loop.

No, sorry, the illusion is sufficiently incomplete as to be more
dangerous than useful.

> [..]
> 
> >> +class Prop:
> >> +    """A device-tree property
> >> +
> >> +    This encapsulates a device-tree property and provides various operations
> >> +    which can be performed on the property.
> >> +
> >> +    Generally the standard constructor is used to create a Prop object. But
> >> +    in the case where only the property offset is known (and not the Node
> >> +    that holds the property), from_offset() can be used.
> >> +    """
> >
> > TBH, I'm not really sold on the value of this class either.  Is there
> > really any point dealing with these rather fragile property handles,
> > rather than simply returning bytestrings with the values directly?  I
> > can't see it.
> 
> It provides the ability to iterate, get the name (as well as the
> data), delete the property, etc.

Name is only relevant when iterating (otherwise you already knew the
name in order to get the property).  For iteration we could add an
interface to return a (name, value) tuple easily enough.  For
iteration itself I think we want something closer to the raw library
iterators - something working explicitly in terms of property offsets.

And I think we should only use those special considerations when doing
iteration.  For the common case of retrieving a property with a known
name, we should just have a simple wrapper on fdt_getprop() returning
a value directly, not messing around with objects and structures.

> >> +    def __init__(self, node, offset, fdt=None):
> >> +        self._node = node
> >> +        self._offset = offset
> >> +        self._fdt = node._fdt if node else fdt
> >> +
> >> +    @classmethod
> >> +    def from_offset(cls, fdt, prop_offset):
> >> +        prop = cls(None, prop_offset, fdt)
> >> +        prop._get_name_data()
> >> +        return prop
> >> +
> >> +    def next(self):
> >> +        return Prop(self._node, check_err(fdt_next_property_offset(
> >> +                self._fdt._fdt, self._offset)))
> >> +
> >> +    def next_quiet(self):
> >> +        val = check_err(fdt_next_property_offset(self._fdt._fdt, self._offset),
> >> +                        True)
> >> +        if val < 0:
> >> +            return None
> >> +        return Prop(self._node, val)
> >> +
> >> +    def _get_name_data(self):
> >> +        pdata = fdt_get_property_by_offset(self._fdt._fdt, self._offset)
> >> +        if not isinstance(pdata, list):
> >> +            raise FdtException(pdata)
> >> +        return self._fdt.string(pdata[0].nameoff), data(pdata[0])
> >> +
> >> +    def name(self):
> >> +        return self._get_name_data()[0]
> >> +
> >> +    def data(self):
> >> +        return self._get_name_data()[1]
> >> +
> >> +    def delete(self):
> >> +        if not self._node:
> >> +            raise RuntimeError("Can't delete property offset %d of unknown node"
> >> +                               % self._offset)
> >> +        self._fdt.delprop(self._node._offset, self.name())
> >> +%}
> 
> [..]
> 
> >> +
> >> +/*
> >> + * From here are the function definitions from libfdt.h, along with their
> >> + * exception-handling code.
> >> + */
> >
> > Uh.. why do you need to duplicate these rather than including libfdt.h?
> 
> Hopefully I can do that, even with fdt_totalsize(). Will check.
> 
> >
> >> +int fdt_path_offset(const void *fdt, const char *path);
> >> +
> >> +int fdt_first_property_offset(const void *fdt, int nodeoffset);
> >> +
> >> +int fdt_next_property_offset(const void *fdt, int offset);
> >> +
> >> +const char *fdt_get_name(const void *fdt, int nodeoffset, int *OUTPUT);
> >> +
> >> +/* no exception handling, since this function has no error checking */
> >> +const char *fdt_string(const void *fdt, int stroffset);
> >> +
> >> +const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
> >> +                                                      int offset, int *OUTPUT);
> >> +
> >> +/* no exception handling, this this function always returns a valid string */
> >> +const char *fdt_strerror(int errval);
> >> +
> >> +int fdt_first_subnode(const void *fdt, int offset);
> >> +
> >> +int fdt_next_subnode(const void *fdt, int offset);
> >> +
> >> +int fdt_delprop(void *fdt, int nodeoffset, const char *name);
> >> +
> >> +int fdt_pack(void *fdt);
> >> +
> >> +int fdt_totalsize(const void *fdt);
> >> +
> >> +int fdt_off_dt_struct(const void *fdt);
> >> diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
> >> new file mode 100644
> >> index 0000000..8f8618e
> >> --- /dev/null
> >> +++ b/pylibfdt/setup.py
> >> @@ -0,0 +1,34 @@
> >> +#!/usr/bin/env python
> >> +
> >> +"""
> >> +setup.py file for SWIG libfdt
> >> +"""
> >> +
> >> +from distutils.core import setup, Extension
> >> +import os
> >> +import sys
> >> +
> >> +progname = sys.argv[0]
> >> +cflags = sys.argv[1]
> >> +files = sys.argv[2:]
> >> +
> >> +if cflags:
> >> +    cflags = [flag for flag in cflags.split(' ') if flag]
> >> +else:
> >> +    cflags = None
> >> +
> >> +libfdt_module = Extension(
> >> +    '_libfdt',
> >> +    sources = files,
> >> +    extra_compile_args =  cflags
> >> +)
> >> +
> >> +sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
> >> +
> >> +setup (name = 'libfdt',
> >> +       version = '0.1',
> >> +       author      = "SWIG Docs",
> >
> > Um.. that doesn't seem quite right.
> 
> Nope, will fix.
> 
> >
> >> +       description = """Simple swig libfdt from docs""",
> >> +       ext_modules = [libfdt_module],
> >> +       py_modules = ["libfdt"],
> >> +       )
> >
> 
> Regards,
> Simon

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

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

[Simon Glass]

Hi David,

On Wednesday, 11 January 2017, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Sun, Dec 04, 2016 at 11:23:32PM -0700, Simon Glass wrote:
> > Hi David,
>
> Sorry for the slow reply.  I was away for a while, and then it just
> fell off my radar.

OK thanks. Have been on holiday for quite a while. This reply gave me
enough into to do v4.
>
> > On 4 December 2016 at 21:35, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > On Sat, Dec 03, 2016 at 05:48:07PM -0700, Simon Glass wrote:
> > >> Add Python bindings for a bare-bones set of libfdt functions. These allow
> > >> navigating the tree and reading node names and properties.
> > >>
> > >> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > >> ---
> > >>
> > >> Changes in v3:
> > >> - Make the library more pythonic
> > >> - Add classes for Node and Prop along with methods
> > >> - Add an exception class
> > >> - Use Python to generate exeptions instead of SWIG
> > >>
> > >> Changes in v2:
> > >> - Add exceptions when functions return an error
> > >> - Correct Python naming to following PEP8
> > >> - Use a class to encapsulate the various methods
> > >> - Include fdt.h instead of redefining struct fdt_property
> > >> - Use bytearray to avoid the SWIG warning 454
> > >> - Add comments
> > >>
> > >>  pylibfdt/.gitignore        |   3 +
> > >>  pylibfdt/Makefile.pylibfdt |  18 ++
> > >>  pylibfdt/libfdt.swig       | 602 +++++++++++++++++++++++++++++++++++++++++++++
> > >>  pylibfdt/setup.py          |  34 +++
> > >>  4 files changed, 657 insertions(+)
> > >>  create mode 100644 pylibfdt/.gitignore
> > >>  create mode 100644 pylibfdt/Makefile.pylibfdt
> > >>  create mode 100644 pylibfdt/libfdt.swig
> > >>  create mode 100644 pylibfdt/setup.py
> > >>
> > [..]
> >
> > >> +# Error codes, corresponding to FDT_ERR_... in libfdt.h
> > >> +(NOTFOUND,
> > >> +        EXISTS,
> > >> +        NOSPACE,
> > >> +        BADOFFSET,
> > >> +        BADPATH,
> > >> +        BADPHANDLE,
> > >> +        BADSTATE,
> > >> +        TRUNCATED,
> > >> +        BADMAGIC,
> > >> +        BADVERSION,
> > >> +        BADSTRUCTURE,
> > >> +        BADLAYOUT,
> > >> +        INTERNAL,
> > >> +        BADNCELLS,
> > >> +        BADVALUE,
> > >> +        BADOVERLAY) = range(1, 17)
> > >
> > > Pity we can't get swig to autogenerate those.
> >
> > Maybe we can if you want it to change to an enum?
>
> Right.  Hmm, might be worth doing; I don't see any immediate reason
> we'd need these constants at the cpp level.

OK, will take a look (in v5).

>
> > >> +class FdtException(Exception):
> > >> +    """An exception caused by an error such as one of the codes above"""
> > >> +    def __init__(self, err):
> > >> +        self.err = err
> > >> +
> > >> +    def __str__(self):
> > >> +        return 'pylibfdt error %d: %s' % (self.err, fdt_strerror(self.err))
> > >> +
> > >> +def fdt32_to_cpu(val):
> > >> +    """Convert a device-tree cell value into a native integer"""
> > >> +    return struct.unpack("=I", struct.pack(">I", val))[0]
> > >> +
> > >> +def data(prop):
> > >> +    """Extract the data from a property
> > >> +
> > >> +    Args:
> > >> +        prop: Property structure, as returned by get_property_by_offset()
> > >> +
> > >> +    Returns:
> > >> +        The property data as a bytearray
> > >> +    """
> > >> +    buf = bytearray(fdt32_to_cpu(prop.len))
> > >
> > > prop.len should be a native value, not requiring byteswap.
> > >
> > >> +    pylibfdt_copy_data(buf, prop)
> > >> +    return buf
> > >
> > > Urgh.. all this fdt_property() wrangling stuff is really ugly.  Any
> > > chance we could just exclude it from the wrapper, providing only
> > > fdt_getprop() interfaces which simply return a Python bytestring
> > > directly?
> >
> > I effectively have, by hiding it in here. Client code need never use this.
>
> I don't see that that's the case.  At the very least they get this
> prop object back from some functions and have to explicitly call
> data() to get the actual value out.
>
> I wonder if the problem here is that you're trying to write a faithful
> wrapper for fdt_get_property().  I don't know that there's any point -
> it's really intended for some internal usage and for performance in
> some special cases that make sense in C.  For the Python wrapper I
> think you could just leave it out and provide only fdt_getprop() -
> that returns the property value directly rather than a complicated to
> encode structure.

But what about fdt_get_property_by_offset() which needs both the
property name and its value? For v4, I've left this as is.

[...]

> > >> +    def string(self, offset):
> > >> +        """Get a string given its offset
> > >> +
> > >> +        Args:
> > >> +            offset: FDT offset in big-endian format
> > >> +
> > >> +        Returns:
> > >> +            string value at that offset
> > >> +        """
> > >> +        return fdt_string(self._fdt, fdt32_to_cpu(offset))
> > >
> > > ARGH, no.  The offset argument is an integer, it should be treated as
> > > a native integer, and the byteswap should absolutely not be here.
> >
> > Ah yes, oops.

I didn't change this as it is only used in tests. I think in fact it
might go away if we have a Prop object, so will look in v5 once I have
your thoughts on that.

[...]

>
> > >> +    def get_property_quiet(self, nodeoffset, name):
> > >> +        """Get a property given a node offset and the property name
> > >> +
> > >> +        We cannot use fdt_get_property() here since it does not return the
> > >> +        offset. We prefer to create Node objects using the offset.
> > >> +
> > >> +        Args:
> > >> +            nodeoffset: Offset of the node
> > >> +            name: Property name
> > >> +
> > >> +        Returns:
> > >> +            Prop object found or None if there is no such property
> > >> +
> > >> +        Raises:
> > >> +            FdtException on error
> > >> +        """
> > >> +        poffset = self.first_property_offset_quiet(nodeoffset)
> > >> +        while poffset >= 0:
> > >> +            pdata = self.get_property_by_offset_internal(poffset)
> > >> +            if self.string(pdata.nameoff) == name:
> > >> +                return Prop(Node(self, nodeoffset), poffset)
> > >> +            poffset = self.next_property_offset_quiet(poffset)
> > >> +        return None
> > >> +
> > >> +    def first_subnode(self, nodeoffset):
> > >> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset))
> > >> +
> > >> +    def first_subnode_quiet(self, nodeoffset):
> > >> +        return check_err(fdt_first_subnode(self._fdt, nodeoffset), True)
> > >> +
> > >> +    def next_subnode(self, nodeoffset):
> > >> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset))
> > >> +
> > >> +    def next_subnode_quiet(self, nodeoffset):
> > >> +        return check_err(fdt_next_subnode(self._fdt, nodeoffset), True)
> > >> +
> > >> +    def totalsize(self):
> > >> +        return check_err(fdt_totalsize(self._fdt))
> > >> +
> > >> +    def off_dt_struct(self):
> > >> +        return check_err(fdt_off_dt_struct(self._fdt))
> > >> +
> > >> +    def pack(self):
> > >> +        return check_err(fdt_pack(self._fdt))
> > >> +
> > >> +    def delprop(self, nodeoffset, prop_name):
> > >> +        return check_err(fdt_delprop(self._fdt, nodeoffset, prop_name))
> > >> +
> > >> +
> > >> +class Node:
> > >
> > > This can't work.  The reason that so many libfdt functions are
> > > insistently called whatever_offset(), rather than just get_whatever()
> > > is to constantly remind the user that this is a actual byte offset
> > > into a buffer of data.  It's *not* a friendly stable node-handle, and
> > > can become invalid when the fdt blob is altered.
> > >
> > > Attempting to put a class wrapper around it obscures that vital fact.
> > > Treat node offsets as offsets everywhere, just encoded as plain old
> > > python integers.
> >
> > My intent is to make the interface more pythonic, and I've been trying
> > out various approaches. For read-only use this interface is nice.
> > Consider this the high-water mark of this approach.
>
> Ok.  So making the interface Pythonic is a reasonable goal - but not
> when it obscures fundamental properties of behaviour.  It's simply not
> possible to have a "stable" node (or property) handle with libfdt, so
> we shouldn't make the Python interface appear to have one.
>
> > But I can go back to something in between, where there is no Node or
> > Prop. But please do check the code example in the cover letter and see
> > if you are sure.
>
> I am absoltely sure.  Node and Property objects are a bad idea.
> Well.. Node objects definitely are.  Property objects could work, but
> only if they're essentially just representing a (name, length, value)
> tuple, *not* an offset into the actual tree.  i.e. such a property
> object would be a copy of some data from the tree, not a handle to
> something still within the tree.
>
> > With this approach we can do things like node.next() which is nice I think.
>
> Nice until you have a use case which breaks it, at which point it
> becomes horribly confusing.
>
> > Of course things can break if you start changing the tree while
> > keeping objects around. So it isn't a perfect illusion. Hopefully it
> > returns errors in that case..
>
> It's not really possible to safely return errors in this case.
> Usually you will get an error because your offset will point at
> something bogus.  But it doesn't require that much bad luck for the
> old offset to line up with something that's no longer the same thing
> but looks vaguely sane, at which point you'll return valid looking but
> wrong data.  You might hit an error condition later, or maybe you'll
> just carry that bad data into the rest of your program.
>
> With a bit more bad luck you could even end up with an infinite loop.
>
> No, sorry, the illusion is sufficiently incomplete as to be more
> dangerous than useful.

OK, I dropped this in v4. Thanks for taking the time to go through
this in detail.

>
> > [..]
> >
> > >> +class Prop:
> > >> +    """A device-tree property
> > >> +
> > >> +    This encapsulates a device-tree property and provides various operations
> > >> +    which can be performed on the property.
> > >> +
> > >> +    Generally the standard constructor is used to create a Prop object. But
> > >> +    in the case where only the property offset is known (and not the Node
> > >> +    that holds the property), from_offset() can be used.
> > >> +    """
> > >
> > > TBH, I'm not really sold on the value of this class either.  Is there
> > > really any point dealing with these rather fragile property handles,
> > > rather than simply returning bytestrings with the values directly?  I
> > > can't see it.
> >
> > It provides the ability to iterate, get the name (as well as the
> > data), delete the property, etc.
>
> Name is only relevant when iterating (otherwise you already knew the
> name in order to get the property).  For iteration we could add an
> interface to return a (name, value) tuple easily enough.  For
> iteration itself I think we want something closer to the raw library
> iterators - something working explicitly in terms of property offsets.
>
> And I think we should only use those special considerations when doing
> iteration.  For the common case of retrieving a property with a known
> name, we should just have a simple wrapper on fdt_getprop() returning
> a value directly, not messing around with objects and structures.

OK I've done this in v4.

[...]

> > >> +
> > >> +/*
> > >> + * From here are the function definitions from libfdt.h, along with their
> > >> + * exception-handling code.
> > >> + */
> > >
> > > Uh.. why do you need to duplicate these rather than including libfdt.h?
> >
> > Hopefully I can do that, even with fdt_totalsize(). Will check.

Unfortunately I don't think I can, as they are #define macros.

[...]

> > >> diff --git a/pylibfdt/setup.py b/pylibfdt/setup.py
> > >> new file mode 100644
> > >> index 0000000..8f8618e
> > >> --- /dev/null
> > >> +++ b/pylibfdt/setup.py
> > >> @@ -0,0 +1,34 @@
> > >> +#!/usr/bin/env python
> > >> +
> > >> +"""
> > >> +setup.py file for SWIG libfdt
> > >> +"""
> > >> +
> > >> +from distutils.core import setup, Extension
> > >> +import os
> > >> +import sys
> > >> +
> > >> +progname = sys.argv[0]
> > >> +cflags = sys.argv[1]
> > >> +files = sys.argv[2:]
> > >> +
> > >> +if cflags:
> > >> +    cflags = [flag for flag in cflags.split(' ') if flag]
> > >> +else:
> > >> +    cflags = None
> > >> +
> > >> +libfdt_module = Extension(
> > >> +    '_libfdt',
> > >> +    sources = files,
> > >> +    extra_compile_args =  cflags
> > >> +)
> > >> +
> > >> +sys.argv = [progname, '--quiet', 'build_ext', '--inplace']
> > >> +
> > >> +setup (name = 'libfdt',
> > >> +       version = '0.1',
> > >> +       author      = "SWIG Docs",
> > >
> > > Um.. that doesn't seem quite right.
> >
> > Nope, will fix.

(in v5, as I forgot this time)

Regards,
Simon