[PATCH 0/4] Add new helpers to pylibfdt

[PATCH 0/4] Add new helpers to pylibfdt

[Luca Weiss]

Allow decoding properties as stringlist and {u,}int{32,64} arrays,
wrap fdt_get_path from the C library and add a useful helper
getprop_or_none that returns None instead of throwing an exception when
a property isn't found.

Luca Weiss (4):
  pylibfdt: add Property.as_stringlist()
  pylibfdt: add Property.as_*int*_array()
  pylibfdt: add FdtRo.get_path()
  pylibfdt: add FdtRo.getprop_or_none()

 pylibfdt/libfdt.i       | 53 +++++++++++++++++++++++++++++++++++++++++
 tests/pylibfdt_tests.py | 33 +++++++++++++++++++++++++
 tests/test_props.dts    |  4 ++++
 3 files changed, 90 insertions(+)

-- 
2.34.1

[PATCH 1/4] pylibfdt: add Property.as_stringlist()

[Luca Weiss]

Add a new method for decoding a string list property, useful for e.g.
the "reg-names" property.

Also add a test for the new method.

Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
---
 pylibfdt/libfdt.i       | 7 +++++++
 tests/pylibfdt_tests.py | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
index 9ccc57b..c81b504 100644
--- a/pylibfdt/libfdt.i
+++ b/pylibfdt/libfdt.i
@@ -724,6 +724,13 @@ class Property(bytearray):
             raise ValueError('Property contains embedded nul characters')
         return self[:-1].decode('utf-8')
 
+    def as_stringlist(self):
+        """Unicode is supported by decoding from UTF-8"""
+        if self[-1] != 0:
+            raise ValueError('Property lacks nul termination')
+        parts = self[:-1].split(b'\x00')
+        return list(map(lambda x: x.decode('utf-8'), parts))
+
 
 class FdtSw(FdtRo):
     """Software interface to create a device tree from scratch
diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
index 64b5bd1..7e3cc4c 100644
--- a/tests/pylibfdt_tests.py
+++ b/tests/pylibfdt_tests.py
@@ -382,6 +382,14 @@ class PyLibfdtBasicTests(unittest.TestCase):
                           self.get_prop("prop-uint64").as_uint64())
         self.assertEqual(-2, self.get_prop("prop-int64").as_int64())
 
+    def testGetStringlistProperties(self):
+        """Test that we can access properties as string list"""
+        node = self.fdt.path_offset('/subnode@1/subsubnode')
+        self.assertEqual(["subsubnode1", "subsubnode"],
+                         self.fdt.getprop(node, "compatible").as_stringlist())
+        self.assertEqual(["this is a placeholder string", "string2"],
+                         self.fdt.getprop(node, "placeholder").as_stringlist())
+
     def testReserveMap(self):
         """Test that we can access the memory reserve map"""
         self.assertEqual(2, self.fdt.num_mem_rsv())
-- 
2.34.1

[PATCH 2/4] pylibfdt: add Property.as_*int*_array()

[Luca Weiss]

Add new methods to handle decoding of int32, uint32, int64 and uint64
arrays.

Also add tests for the new methods.

Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
---
 pylibfdt/libfdt.i       | 15 +++++++++++++++
 tests/pylibfdt_tests.py | 11 +++++++++++
 tests/test_props.dts    |  4 ++++
 3 files changed, 30 insertions(+)

diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
index c81b504..ac70762 100644
--- a/pylibfdt/libfdt.i
+++ b/pylibfdt/libfdt.i
@@ -716,6 +716,21 @@ class Property(bytearray):
     def as_int64(self):
         return self.as_cell('q')
 
+    def as_list(self, fmt):
+        return list(map(lambda x: x[0], struct.iter_unpack('>' + fmt, self)))
+
+    def as_uint32_list(self):
+        return self.as_list('L')
+
+    def as_int32_list(self):
+        return self.as_list('l')
+
+    def as_uint64_list(self):
+        return self.as_list('Q')
+
+    def as_int64_list(self):
+        return self.as_list('q')
+
     def as_str(self):
         """Unicode is supported by decoding from UTF-8"""
         if self[-1] != 0:
diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
index 7e3cc4c..5479363 100644
--- a/tests/pylibfdt_tests.py
+++ b/tests/pylibfdt_tests.py
@@ -382,6 +382,17 @@ class PyLibfdtBasicTests(unittest.TestCase):
                           self.get_prop("prop-uint64").as_uint64())
         self.assertEqual(-2, self.get_prop("prop-int64").as_int64())
 
+    def testGetIntListProperties(self):
+        """Test that we can access properties as integer lists"""
+        self.assertEqual([128, -16, -2],
+                         self.get_prop("prop-int32-array").as_int32_list())
+        self.assertEqual([0x1, 0x98765432, 0xdeadbeef],
+                         self.get_prop("prop-uint32-array").as_uint32_list())
+        self.assertEqual([0x100000000, -2],
+                         self.get_prop("prop-int64-array").as_int64_list())
+        self.assertEqual([0x100000000, 0x1],
+                         self.get_prop("prop-uint64-array").as_uint64_list())
+
     def testGetStringlistProperties(self):
         """Test that we can access properties as string list"""
         node = self.fdt.path_offset('/subnode@1/subsubnode')
diff --git a/tests/test_props.dts b/tests/test_props.dts
index 7e59bd1..5089023 100644
--- a/tests/test_props.dts
+++ b/tests/test_props.dts
@@ -8,4 +8,8 @@
 	prop-hex64 = /bits/ 64 <0xdeadbeef01abcdef>;
 	prop-uint64 = /bits/ 64 <9223372036854775807>;
 	prop-int64 = /bits/ 64 <0xfffffffffffffffe>;
+	prop-int32-array = <128>, <(-16)>, <0xfffffffe>;
+	prop-uint32-array = <0x1>, <0x98765432>, <0xdeadbeef>;
+	prop-int64-array = /bits/ 64 <0x100000000 0xfffffffffffffffe>;
+	prop-uint64-array = /bits/ 64 <0x100000000 0x1>;
 };
-- 
2.34.1

[PATCH 3/4] pylibfdt: add FdtRo.get_path()

[Luca Weiss]

Add a new Python method wrapping fdt_get_path() from the C API.

Also add a test for the new method.

Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
---
 pylibfdt/libfdt.i       | 21 +++++++++++++++++++++
 tests/pylibfdt_tests.py | 12 ++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
index ac70762..5434d48 100644
--- a/pylibfdt/libfdt.i
+++ b/pylibfdt/libfdt.i
@@ -443,6 +443,22 @@ class FdtRo(object):
         """
         return fdt_get_alias(self._fdt, name)
 
+    def get_path(self, nodeoffset, size=128):
+        """Get the full path of a node
+
+        Args:
+            nodeoffset: Node offset to check
+
+        Returns:
+            Full path to the node
+
+        Raises:
+            FdtException if the path is longer than 'size' or other error occurs
+        """
+        ret, path = fdt_get_path(self._fdt, nodeoffset, size)
+        check_err(ret)
+        return path
+
     def parent_offset(self, nodeoffset, quiet=()):
         """Get the offset of a node's parent
 
@@ -1115,6 +1131,11 @@ typedef uint32_t fdt32_t;
         }
 }
 
+%include "cstring.i"
+
+%cstring_output_maxsize(char *buf, int buflen);
+int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen);
+
 /* We have both struct fdt_property and a function fdt_property() */
 %warnfilter(302) fdt_property;
 
diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
index 5479363..bcd3daa 100644
--- a/tests/pylibfdt_tests.py
+++ b/tests/pylibfdt_tests.py
@@ -348,6 +348,18 @@ class PyLibfdtBasicTests(unittest.TestCase):
         self.assertEqual("/subnode@1/subsubnode", self.fdt3.get_alias('ss1'))
         self.assertEqual("/subnode@1/subsubnode/subsubsubnode", self.fdt3.get_alias('sss1'))
 
+    def testGetPath(self):
+        """Test for the get_path() method"""
+        node = self.fdt.path_offset('/subnode@1')
+        node2 = self.fdt.path_offset('/subnode@1/subsubnode')
+        self.assertEqual("/subnode@1", self.fdt.get_path(node))
+        self.assertEqual("/subnode@1", self.fdt.get_path(node, 16))
+        self.assertEqual("/subnode@1/subsubnode", self.fdt.get_path(node2))
+
+        with self.assertRaises(FdtException) as e:
+            self.fdt.get_path(node2, 16)
+        self.assertEqual(e.exception.err, -libfdt.NOSPACE)
+
     def testParentOffset(self):
         """Test for the parent_offset() method"""
         self.assertEqual(-libfdt.NOTFOUND,
-- 
2.34.1

[PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Luca Weiss]

Add a new method that doesn't throw an exception when a property isn't
found but returns None instead.

Also add a test for the new method.

Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
---
 pylibfdt/libfdt.i       | 10 ++++++++++
 tests/pylibfdt_tests.py |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
index 5434d48..2c2fa5c 100644
--- a/pylibfdt/libfdt.i
+++ b/pylibfdt/libfdt.i
@@ -419,6 +419,16 @@ class FdtRo(object):
             return pdata
         return Property(prop_name, bytearray(pdata[0]))
 
+    def getprop_or_none(self, nodeoffset, prop_name):
+        """Get a property from a node, or None if it is not found
+
+        See getprop() for the arguments.
+        """
+        prop = self.getprop(nodeoffset, prop_name, [FDT_ERR_NOTFOUND])
+        if prop == -FDT_ERR_NOTFOUND:
+            return None
+        return prop
+
     def get_phandle(self, nodeoffset):
         """Get the phandle of a node
 
diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
index bcd3daa..152e925 100644
--- a/tests/pylibfdt_tests.py
+++ b/tests/pylibfdt_tests.py
@@ -219,6 +219,8 @@ class PyLibfdtBasicTests(unittest.TestCase):
             self.fdt.getprop(root, 'missing')
         self.assertEqual(e.exception.err, -libfdt.NOTFOUND)
 
+        self.assertIsNone(self.fdt.getprop_or_none(root, 'missing'))
+
         node = self.fdt.path_offset('/subnode@1/subsubnode')
         value = self.fdt.getprop(node, "compatible")
         self.assertEqual(value, b'subsubnode1\0subsubnode\0')
-- 
2.34.1

Re: [PATCH 1/4] pylibfdt: add Property.as_stringlist()

[David Gibson]

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

On Sat, Dec 25, 2021 at 02:25:55PM +0100, Luca Weiss wrote:
> Add a new method for decoding a string list property, useful for e.g.
> the "reg-names" property.
> 
> Also add a test for the new method.
> 
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>

Applied, thanks.

> ---
>  pylibfdt/libfdt.i       | 7 +++++++
>  tests/pylibfdt_tests.py | 8 ++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index 9ccc57b..c81b504 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -724,6 +724,13 @@ class Property(bytearray):
>              raise ValueError('Property contains embedded nul characters')
>          return self[:-1].decode('utf-8')
>  
> +    def as_stringlist(self):
> +        """Unicode is supported by decoding from UTF-8"""
> +        if self[-1] != 0:
> +            raise ValueError('Property lacks nul termination')
> +        parts = self[:-1].split(b'\x00')
> +        return list(map(lambda x: x.decode('utf-8'), parts))
> +
>  
>  class FdtSw(FdtRo):
>      """Software interface to create a device tree from scratch
> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> index 64b5bd1..7e3cc4c 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -382,6 +382,14 @@ class PyLibfdtBasicTests(unittest.TestCase):
>                            self.get_prop("prop-uint64").as_uint64())
>          self.assertEqual(-2, self.get_prop("prop-int64").as_int64())
>  
> +    def testGetStringlistProperties(self):
> +        """Test that we can access properties as string list"""
> +        node = self.fdt.path_offset('/subnode@1/subsubnode')
> +        self.assertEqual(["subsubnode1", "subsubnode"],
> +                         self.fdt.getprop(node, "compatible").as_stringlist())
> +        self.assertEqual(["this is a placeholder string", "string2"],
> +                         self.fdt.getprop(node, "placeholder").as_stringlist())
> +
>      def testReserveMap(self):
>          """Test that we can access the memory reserve map"""
>          self.assertEqual(2, self.fdt.num_mem_rsv())

-- 
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: 833 bytes --]

Re: [PATCH 2/4] pylibfdt: add Property.as_*int*_array()

[David Gibson]

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

On Sat, Dec 25, 2021 at 02:25:56PM +0100, Luca Weiss wrote:
> Add new methods to handle decoding of int32, uint32, int64 and uint64
> arrays.
> 
> Also add tests for the new methods.
> 
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>

Applied, thanks.

> ---
>  pylibfdt/libfdt.i       | 15 +++++++++++++++
>  tests/pylibfdt_tests.py | 11 +++++++++++
>  tests/test_props.dts    |  4 ++++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index c81b504..ac70762 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -716,6 +716,21 @@ class Property(bytearray):
>      def as_int64(self):
>          return self.as_cell('q')
>  
> +    def as_list(self, fmt):
> +        return list(map(lambda x: x[0], struct.iter_unpack('>' + fmt, self)))
> +
> +    def as_uint32_list(self):
> +        return self.as_list('L')
> +
> +    def as_int32_list(self):
> +        return self.as_list('l')
> +
> +    def as_uint64_list(self):
> +        return self.as_list('Q')
> +
> +    def as_int64_list(self):
> +        return self.as_list('q')
> +
>      def as_str(self):
>          """Unicode is supported by decoding from UTF-8"""
>          if self[-1] != 0:
> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> index 7e3cc4c..5479363 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -382,6 +382,17 @@ class PyLibfdtBasicTests(unittest.TestCase):
>                            self.get_prop("prop-uint64").as_uint64())
>          self.assertEqual(-2, self.get_prop("prop-int64").as_int64())
>  
> +    def testGetIntListProperties(self):
> +        """Test that we can access properties as integer lists"""
> +        self.assertEqual([128, -16, -2],
> +                         self.get_prop("prop-int32-array").as_int32_list())
> +        self.assertEqual([0x1, 0x98765432, 0xdeadbeef],
> +                         self.get_prop("prop-uint32-array").as_uint32_list())
> +        self.assertEqual([0x100000000, -2],
> +                         self.get_prop("prop-int64-array").as_int64_list())
> +        self.assertEqual([0x100000000, 0x1],
> +                         self.get_prop("prop-uint64-array").as_uint64_list())
> +
>      def testGetStringlistProperties(self):
>          """Test that we can access properties as string list"""
>          node = self.fdt.path_offset('/subnode@1/subsubnode')
> diff --git a/tests/test_props.dts b/tests/test_props.dts
> index 7e59bd1..5089023 100644
> --- a/tests/test_props.dts
> +++ b/tests/test_props.dts
> @@ -8,4 +8,8 @@
>  	prop-hex64 = /bits/ 64 <0xdeadbeef01abcdef>;
>  	prop-uint64 = /bits/ 64 <9223372036854775807>;
>  	prop-int64 = /bits/ 64 <0xfffffffffffffffe>;
> +	prop-int32-array = <128>, <(-16)>, <0xfffffffe>;
> +	prop-uint32-array = <0x1>, <0x98765432>, <0xdeadbeef>;
> +	prop-int64-array = /bits/ 64 <0x100000000 0xfffffffffffffffe>;
> +	prop-uint64-array = /bits/ 64 <0x100000000 0x1>;
>  };

-- 
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: 833 bytes --]

Re: [PATCH 3/4] pylibfdt: add FdtRo.get_path()

[David Gibson]

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

On Sat, Dec 25, 2021 at 02:25:57PM +0100, Luca Weiss wrote:
> Add a new Python method wrapping fdt_get_path() from the C API.
> 
> Also add a test for the new method.
> 
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> ---
>  pylibfdt/libfdt.i       | 21 +++++++++++++++++++++
>  tests/pylibfdt_tests.py | 12 ++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index ac70762..5434d48 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -443,6 +443,22 @@ class FdtRo(object):
>          """
>          return fdt_get_alias(self._fdt, name)
>  
> +    def get_path(self, nodeoffset, size=128):

Having an explicit max buffer size here makes for a very un-Pythonic
interface.  The python interface really needs to re-allocate and retry
automatically until it gets the full result.

Whether you implement that in Python code or in the C wrapper is up to
you.

> +        """Get the full path of a node
> +
> +        Args:
> +            nodeoffset: Node offset to check
> +
> +        Returns:
> +            Full path to the node
> +
> +        Raises:
> +            FdtException if the path is longer than 'size' or other error occurs
> +        """
> +        ret, path = fdt_get_path(self._fdt, nodeoffset, size)
> +        check_err(ret)
> +        return path
> +
>      def parent_offset(self, nodeoffset, quiet=()):
>          """Get the offset of a node's parent
>  
> @@ -1115,6 +1131,11 @@ typedef uint32_t fdt32_t;
>          }
>  }
>  
> +%include "cstring.i"
> +
> +%cstring_output_maxsize(char *buf, int buflen);
> +int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen);
> +
>  /* We have both struct fdt_property and a function fdt_property() */
>  %warnfilter(302) fdt_property;
>  
> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> index 5479363..bcd3daa 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -348,6 +348,18 @@ class PyLibfdtBasicTests(unittest.TestCase):
>          self.assertEqual("/subnode@1/subsubnode", self.fdt3.get_alias('ss1'))
>          self.assertEqual("/subnode@1/subsubnode/subsubsubnode", self.fdt3.get_alias('sss1'))
>  
> +    def testGetPath(self):
> +        """Test for the get_path() method"""
> +        node = self.fdt.path_offset('/subnode@1')
> +        node2 = self.fdt.path_offset('/subnode@1/subsubnode')
> +        self.assertEqual("/subnode@1", self.fdt.get_path(node))
> +        self.assertEqual("/subnode@1", self.fdt.get_path(node, 16))
> +        self.assertEqual("/subnode@1/subsubnode", self.fdt.get_path(node2))
> +
> +        with self.assertRaises(FdtException) as e:
> +            self.fdt.get_path(node2, 16)
> +        self.assertEqual(e.exception.err, -libfdt.NOSPACE)
> +
>      def testParentOffset(self):
>          """Test for the parent_offset() method"""
>          self.assertEqual(-libfdt.NOTFOUND,

-- 
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: 833 bytes --]

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[David Gibson]

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

On Sat, Dec 25, 2021 at 02:25:58PM +0100, Luca Weiss wrote:
> Add a new method that doesn't throw an exception when a property isn't
> found but returns None instead.
> 
> Also add a test for the new method.
> 
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>

I'd need a stronger rationale to apply this.  It's not really doing
anything that the existing getprop method can't, just presenting it in
a slightly different form.

> ---
>  pylibfdt/libfdt.i       | 10 ++++++++++
>  tests/pylibfdt_tests.py |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index 5434d48..2c2fa5c 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -419,6 +419,16 @@ class FdtRo(object):
>              return pdata
>          return Property(prop_name, bytearray(pdata[0]))
>  
> +    def getprop_or_none(self, nodeoffset, prop_name):
> +        """Get a property from a node, or None if it is not found
> +
> +        See getprop() for the arguments.
> +        """
> +        prop = self.getprop(nodeoffset, prop_name, [FDT_ERR_NOTFOUND])
> +        if prop == -FDT_ERR_NOTFOUND:
> +            return None
> +        return prop
> +
>      def get_phandle(self, nodeoffset):
>          """Get the phandle of a node
>  
> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> index bcd3daa..152e925 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -219,6 +219,8 @@ class PyLibfdtBasicTests(unittest.TestCase):
>              self.fdt.getprop(root, 'missing')
>          self.assertEqual(e.exception.err, -libfdt.NOTFOUND)
>  
> +        self.assertIsNone(self.fdt.getprop_or_none(root, 'missing'))
> +
>          node = self.fdt.path_offset('/subnode@1/subsubnode')
>          value = self.fdt.getprop(node, "compatible")
>          self.assertEqual(value, b'subsubnode1\0subsubnode\0')

-- 
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: 833 bytes --]

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Simon Glass]

Hi Luca,

On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
>
> Add a new method that doesn't throw an exception when a property isn't
> found but returns None instead.
>
> Also add a test for the new method.

You can use

getprop(node, prop, quiet=QUIET_NOTFOUND)

>
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> ---
>  pylibfdt/libfdt.i       | 10 ++++++++++
>  tests/pylibfdt_tests.py |  2 ++
>  2 files changed, 12 insertions(+)

Regards,
Simon

Re: [PATCH 1/4] pylibfdt: add Property.as_stringlist()

[Rob Herring]

On Sat, Dec 25, 2021 at 7:26 AM Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
>
> Add a new method for decoding a string list property, useful for e.g.
> the "reg-names" property.
>
> Also add a test for the new method.
>
> Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> ---
>  pylibfdt/libfdt.i       | 7 +++++++
>  tests/pylibfdt_tests.py | 8 ++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index 9ccc57b..c81b504 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -724,6 +724,13 @@ class Property(bytearray):
>              raise ValueError('Property contains embedded nul characters')
>          return self[:-1].decode('utf-8')
>
> +    def as_stringlist(self):
> +        """Unicode is supported by decoding from UTF-8"""
> +        if self[-1] != 0:
> +            raise ValueError('Property lacks nul termination')
> +        parts = self[:-1].split(b'\x00')
> +        return list(map(lambda x: x.decode('utf-8'), parts))

Doesn't this result in multiple decode() calls when a single one would work:

return data[:-1].decode(encoding='ascii').split('\0')

Rob

Re: [PATCH 1/4] pylibfdt: add Property.as_stringlist()

[David Gibson]

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

On Wed, Jan 05, 2022 at 04:48:31PM -0600, Rob Herring wrote:
> On Sat, Dec 25, 2021 at 7:26 AM Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> >
> > Add a new method for decoding a string list property, useful for e.g.
> > the "reg-names" property.
> >
> > Also add a test for the new method.
> >
> > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > ---
> >  pylibfdt/libfdt.i       | 7 +++++++
> >  tests/pylibfdt_tests.py | 8 ++++++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > index 9ccc57b..c81b504 100644
> > --- a/pylibfdt/libfdt.i
> > +++ b/pylibfdt/libfdt.i
> > @@ -724,6 +724,13 @@ class Property(bytearray):
> >              raise ValueError('Property contains embedded nul characters')
> >          return self[:-1].decode('utf-8')
> >
> > +    def as_stringlist(self):
> > +        """Unicode is supported by decoding from UTF-8"""
> > +        if self[-1] != 0:
> > +            raise ValueError('Property lacks nul termination')
> > +        parts = self[:-1].split(b'\x00')
> > +        return list(map(lambda x: x.decode('utf-8'), parts))
> 
> Doesn't this result in multiple decode() calls when a single one would work:
> 
> return data[:-1].decode(encoding='ascii').split('\0')

Uh.. I guess?  I feel like the split-then-decode makes more logical
sense, since it's splitting a bytestring, then decoding the pieces as
utf-8 strings.  That makes sense to me given that raw properties are
bytestrings and can included multiple different datatypes and
encodings in general.

In this specific case, decode-then-split would be fine as well, since
\u00000 works as a separator unambiguously, but it still seems
conceptually muddier to me.

-- 
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: 833 bytes --]

Re: [PATCH 1/4] pylibfdt: add Property.as_stringlist()

[Luca Weiss]

Hi,

On Dienstag, 18. Jänner 2022 11:08:22 CET David Gibson wrote:
> On Wed, Jan 05, 2022 at 04:48:31PM -0600, Rob Herring wrote:
> > On Sat, Dec 25, 2021 at 7:26 AM Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > Add a new method for decoding a string list property, useful for e.g.
> > > the "reg-names" property.
> > > 
> > > Also add a test for the new method.
> > > 
> > > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > > ---
> > > 
> > >  pylibfdt/libfdt.i       | 7 +++++++
> > >  tests/pylibfdt_tests.py | 8 ++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > > index 9ccc57b..c81b504 100644
> > > --- a/pylibfdt/libfdt.i
> > > +++ b/pylibfdt/libfdt.i
> > > 
> > > @@ -724,6 +724,13 @@ class Property(bytearray):
> > >              raise ValueError('Property contains embedded nul
> > >              characters')
> > >          
> > >          return self[:-1].decode('utf-8')
> > > 
> > > +    def as_stringlist(self):
> > > +        """Unicode is supported by decoding from UTF-8"""
> > > +        if self[-1] != 0:
> > > +            raise ValueError('Property lacks nul termination')
> > > +        parts = self[:-1].split(b'\x00')
> > > +        return list(map(lambda x: x.decode('utf-8'), parts))
> > 
> > Doesn't this result in multiple decode() calls when a single one would
> > work:
> > 
> > return data[:-1].decode(encoding='ascii').split('\0')
> 
> Uh.. I guess?  I feel like the split-then-decode makes more logical
> sense, since it's splitting a bytestring, then decoding the pieces as
> utf-8 strings.  That makes sense to me given that raw properties are
> bytestrings and can included multiple different datatypes and
> encodings in general.
> 
> In this specific case, decode-then-split would be fine as well, since
> \u00000 works as a separator unambiguously, but it still seems
> conceptually muddier to me.

The reason I made it this way was mostly because I didn't know you could have 
null bytes present in str.decode, I just remember horrible UnicodeDecodeErrors 
on invalid input from other projects.
If wanted I can make a patch changing to just one str.decode call as yes, it's 
surely more efficient than doing multiple. But it's also not like there will be 
100 parts of this property that's being decoded in a performance critical 
application so I think it's also okay like that.

Let me know what you think.
Regards
Luca

Re: [PATCH 1/4] pylibfdt: add Property.as_stringlist()

[David Gibson]

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

On Thu, Jan 20, 2022 at 08:24:31PM +0100, Luca Weiss wrote:
> Hi,
> 
> On Dienstag, 18. Jänner 2022 11:08:22 CET David Gibson wrote:
> > On Wed, Jan 05, 2022 at 04:48:31PM -0600, Rob Herring wrote:
> > > On Sat, Dec 25, 2021 at 7:26 AM Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > > Add a new method for decoding a string list property, useful for e.g.
> > > > the "reg-names" property.
> > > > 
> > > > Also add a test for the new method.
> > > > 
> > > > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > > > ---
> > > > 
> > > >  pylibfdt/libfdt.i       | 7 +++++++
> > > >  tests/pylibfdt_tests.py | 8 ++++++++
> > > >  2 files changed, 15 insertions(+)
> > > > 
> > > > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > > > index 9ccc57b..c81b504 100644
> > > > --- a/pylibfdt/libfdt.i
> > > > +++ b/pylibfdt/libfdt.i
> > > > 
> > > > @@ -724,6 +724,13 @@ class Property(bytearray):
> > > >              raise ValueError('Property contains embedded nul
> > > >              characters')
> > > >          
> > > >          return self[:-1].decode('utf-8')
> > > > 
> > > > +    def as_stringlist(self):
> > > > +        """Unicode is supported by decoding from UTF-8"""
> > > > +        if self[-1] != 0:
> > > > +            raise ValueError('Property lacks nul termination')
> > > > +        parts = self[:-1].split(b'\x00')
> > > > +        return list(map(lambda x: x.decode('utf-8'), parts))
> > > 
> > > Doesn't this result in multiple decode() calls when a single one would
> > > work:
> > > 
> > > return data[:-1].decode(encoding='ascii').split('\0')
> > 
> > Uh.. I guess?  I feel like the split-then-decode makes more logical
> > sense, since it's splitting a bytestring, then decoding the pieces as
> > utf-8 strings.  That makes sense to me given that raw properties are
> > bytestrings and can included multiple different datatypes and
> > encodings in general.
> > 
> > In this specific case, decode-then-split would be fine as well, since
> > \u00000 works as a separator unambiguously, but it still seems
> > conceptually muddier to me.
> 
> The reason I made it this way was mostly because I didn't know you could have 
> null bytes present in str.decode, I just remember horrible UnicodeDecodeErrors 
> on invalid input from other projects.
> If wanted I can make a patch changing to just one str.decode call as yes, it's 
> surely more efficient than doing multiple. But it's also not like there will be 
> 100 parts of this property that's being decoded in a performance critical 
> application so I think it's also okay like that.

I agree, and I tend to prefer it the way it is, unless there's a
compelling reason to change.

-- 
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: 833 bytes --]

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Luca Weiss]

Hi Simon,

On Dienstag, 28. Dezember 2021 09:34:57 CET Simon Glass wrote:
> Hi Luca,
> 
> On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > Add a new method that doesn't throw an exception when a property isn't
> > found but returns None instead.
> > 
> > Also add a test for the new method.
> 
> You can use
> 
> getprop(node, prop, quiet=QUIET_NOTFOUND)
> 

This returns -1 when not found which I found quite un-pythonic (not that 
pylibfdt is currently super pythonic ;) ).
Having None returned if not found is much nicer to use and much more clear to 
use

    if foobar is None:

 instead of having to use

    if foobar == -QUIET_NOTFOUND:


Are there any changes I can do for you to reconsider your position on this 
patch?

Regards
Luca

> > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > ---
> > 
> >  pylibfdt/libfdt.i       | 10 ++++++++++
> >  tests/pylibfdt_tests.py |  2 ++
> >  2 files changed, 12 insertions(+)
> 
> Regards,
> Simon

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Simon Glass]

Hi Luca,

On Sat, 22 Jan 2022 at 03:36, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
>
> Hi Simon,
>
> On Dienstag, 28. Dezember 2021 09:34:57 CET Simon Glass wrote:
> > Hi Luca,
> >
> > On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > Add a new method that doesn't throw an exception when a property isn't
> > > found but returns None instead.
> > >
> > > Also add a test for the new method.
> >
> > You can use
> >
> > getprop(node, prop, quiet=QUIET_NOTFOUND)
> >
>
> This returns -1 when not found which I found quite un-pythonic (not that
> pylibfdt is currently super pythonic ;) ).
> Having None returned if not found is much nicer to use and much more clear to
> use
>
>     if foobar is None:
>
>  instead of having to use
>
>     if foobar == -QUIET_NOTFOUND:
>
>
> Are there any changes I can do for you to reconsider your position on this
> patch?

I think I was put off by the mention of an exception in the commit
message. Really all you are doing is trying to make this function more
pythonic. I suppose we could change the getprop() function to return
None if there is no property (with QUIET_NOTFOUND supplied for the
'quiet' argument). That makes not-found special, but I suppose if the
error were anything else, e.g. -FDT_ERR_BADSTRUCTURE and
quiet=[BADSTRUCTURE] were passed, we could return None in that case
too?

I agree it is more pythonic.

David, what do you think?

Failing that I'm OK with this function, but with the commit message
updated to not mention exceptions, as we already have that mechanism,
so it is confusing.

BTW you can use QUIET_NOTFOUND instead of [FDT_ERR_NOTFOUND]

Regards,
Simon

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Luca Weiss]

Hi Simon,

On Montag, 24. Jänner 2022 18:57:45 CET Simon Glass wrote:
> Hi Luca,
> 
> On Sat, 22 Jan 2022 at 03:36, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > Hi Simon,
> > 
> > On Dienstag, 28. Dezember 2021 09:34:57 CET Simon Glass wrote:
> > > Hi Luca,
> > > 
> > > On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > > Add a new method that doesn't throw an exception when a property isn't
> > > > found but returns None instead.
> > > > 
> > > > Also add a test for the new method.
> > > 
> > > You can use
> > > 
> > > getprop(node, prop, quiet=QUIET_NOTFOUND)
> > 
> > This returns -1 when not found which I found quite un-pythonic (not that
> > pylibfdt is currently super pythonic ;) ).
> > Having None returned if not found is much nicer to use and much more clear
> > to use
> > 
> >     if foobar is None:
> >  instead of having to use
> >  
> >     if foobar == -QUIET_NOTFOUND:
> > Are there any changes I can do for you to reconsider your position on this
> > patch?
> 
> I think I was put off by the mention of an exception in the commit
> message. Really all you are doing is trying to make this function more
> pythonic. I suppose we could change the getprop() function to return
> None if there is no property (with QUIET_NOTFOUND supplied for the
> 'quiet' argument). That makes not-found special, but I suppose if the
> error were anything else, e.g. -FDT_ERR_BADSTRUCTURE and
> quiet=[BADSTRUCTURE] were passed, we could return None in that case
> too?
> 
> I agree it is more pythonic.

The "proper" way (in my opinion) would be to return the property or None, 
except for more serious errors like the BADSTRUCTURE one that just throws an 
exception; and hide all the C integer return values inside the wrapper.
And instead of passing quiet=[BADSTRUCTURE] you would do

try:
    myprop = fdt.getprop(foobar)
except FdtBadStructureError:
    foobar()

I understand it's probably not a good idea to change the existing function too 
much given current users where scripts depend on this behavior but this way 
it's definitely more approachable to an average Python user.

Regards
Luca

> 
> David, what do you think?
> 
> Failing that I'm OK with this function, but with the commit message
> updated to not mention exceptions, as we already have that mechanism,
> so it is confusing.
> 
> BTW you can use QUIET_NOTFOUND instead of [FDT_ERR_NOTFOUND]
> 
> Regards,
> Simon

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[Simon Glass]

Hi Luca,

On Mon, 24 Jan 2022 at 13:50, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
>
> Hi Simon,
>
> On Montag, 24. Jänner 2022 18:57:45 CET Simon Glass wrote:
> > Hi Luca,
> >
> > On Sat, 22 Jan 2022 at 03:36, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > Hi Simon,
> > >
> > > On Dienstag, 28. Dezember 2021 09:34:57 CET Simon Glass wrote:
> > > > Hi Luca,
> > > >
> > > > On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > > > Add a new method that doesn't throw an exception when a property isn't
> > > > > found but returns None instead.
> > > > >
> > > > > Also add a test for the new method.
> > > >
> > > > You can use
> > > >
> > > > getprop(node, prop, quiet=QUIET_NOTFOUND)
> > >
> > > This returns -1 when not found which I found quite un-pythonic (not that
> > > pylibfdt is currently super pythonic ;) ).
> > > Having None returned if not found is much nicer to use and much more clear
> > > to use
> > >
> > >     if foobar is None:
> > >  instead of having to use
> > >
> > >     if foobar == -QUIET_NOTFOUND:
> > > Are there any changes I can do for you to reconsider your position on this
> > > patch?
> >
> > I think I was put off by the mention of an exception in the commit
> > message. Really all you are doing is trying to make this function more
> > pythonic. I suppose we could change the getprop() function to return
> > None if there is no property (with QUIET_NOTFOUND supplied for the
> > 'quiet' argument). That makes not-found special, but I suppose if the
> > error were anything else, e.g. -FDT_ERR_BADSTRUCTURE and
> > quiet=[BADSTRUCTURE] were passed, we could return None in that case
> > too?
> >
> > I agree it is more pythonic.
>
> The "proper" way (in my opinion) would be to return the property or None,
> except for more serious errors like the BADSTRUCTURE one that just throws an
> exception; and hide all the C integer return values inside the wrapper.
> And instead of passing quiet=[BADSTRUCTURE] you would do

You could return None for the ones that are quietened, but then note
that you cannot recover the actual error. So in fact I think that is a
problem with what I proposed above, so in fact we should only return
None for -FDT_ERR_NOTFOUND, if even that.

>
> try:
>     myprop = fdt.getprop(foobar)
> except FdtBadStructureError:
>     foobar()
>
> I understand it's probably not a good idea to change the existing function too
> much given current users where scripts depend on this behavior but this way
> it's definitely more approachable to an average Python user.

Unless you are possibly expecting an error, e.g. -FDT_ERR_NOSPACE. It
is a pain to have to catch exceptions for what is normal operation. In
fact if I recall the Python sages recommend against things like that?

Regards,
Simon


> >
> > David, what do you think?
> >
> > Failing that I'm OK with this function, but with the commit message
> > updated to not mention exceptions, as we already have that mechanism,
> > so it is confusing.
> >
> > BTW you can use QUIET_NOTFOUND instead of [FDT_ERR_NOTFOUND]
> >
> > Regards,
> > Simon
>
>
>
>

Re: [PATCH 4/4] pylibfdt: add FdtRo.getprop_or_none()

[David Gibson]

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

On Sat, Jan 22, 2022 at 11:36:01AM +0100, Luca Weiss wrote:
> Hi Simon,
> 
> On Dienstag, 28. Dezember 2021 09:34:57 CET Simon Glass wrote:
> > Hi Luca,
> > 
> > On Sat, 25 Dec 2021 at 06:26, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> wrote:
> > > Add a new method that doesn't throw an exception when a property isn't
> > > found but returns None instead.
> > > 
> > > Also add a test for the new method.
> > 
> > You can use
> > 
> > getprop(node, prop, quiet=QUIET_NOTFOUND)
> > 
> 
> This returns -1 when not found which I found quite un-pythonic (not that 
> pylibfdt is currently super pythonic ;) ).
> Having None returned if not found is much nicer to use and much more clear to 
> use
> 
>     if foobar is None:
> 
>  instead of having to use
> 
>     if foobar == -QUIET_NOTFOUND:
> 
> 
> Are there any changes I can do for you to reconsider your position on this 
> patch?

So, you're absolutely right that the current interface is
non-pythonic.  The question is simply whether that ugliness is enough
to outweight the complexity of introducing an extra interface.

> 
> Regards
> Luca
> 
> > > Signed-off-by: Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org>
> > > ---
> > > 
> > >  pylibfdt/libfdt.i       | 10 ++++++++++
> > >  tests/pylibfdt_tests.py |  2 ++
> > >  2 files changed, 12 insertions(+)
> > 
> > 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: 833 bytes --]