Re: [PATCH v2 2/2] pylibfdt: Allow reading integer values from properties

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Thu, Aug 31, 2017 at 04:42:00AM -0600, Simon Glass wrote:
> Extend the Properties class with some functions to read a single integer
> property. Add a new getprop_obj() function to return a Property object
> instead of the raw data.
> 
> This suggested approach can be extended to handle other types, as well as
> arrays.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Sorry again I've taken so long to respond.

This way of doing it really seems awkward to me.  You're introducing a
clunky variant getter just to wrap the bytearray in an object,
essentially just to get conversion methods.

It seems to be simpler to just have as_cell() and whatnot as plain
functions that go straight from bytestring/bytearray to an int or
whatever.


Longer term, I did have an idea that might resolve the
propery-as-object or property-as-bytestring conundrum a bit more
satisfactorily.  But, I'm not entirely sure how you'd go about
implementing it with swig in the picture: it should be possible in
(modern) Python to make a Property object that's a sub-class of the
(byte)string type.  That way it can be used directly as though it were
a bytestring, but you can also add properties to do extra stuff.

[Which raises a minor detail, most of the functions which return
bytearray()s now should probably be returning strings - there doesn't
seem to be a reason to return a mutable object]
> ---
> 
> Changes in v2: None
> 
>  pylibfdt/libfdt.i       | 35 +++++++++++++++++++++++++++++++++++
>  tests/pylibfdt_tests.py | 12 ++++++++++++
>  tests/run_tests.sh      |  1 +
>  tests/test_props.dts    | 11 +++++++++++
>  4 files changed, 59 insertions(+)
>  create mode 100644 tests/test_props.dts
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index f8e3a2c..4d60f90 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -376,6 +376,26 @@ class Fdt:
>              return pdata
>          return bytearray(pdata[0])
>  
> +    def getprop_obj(self, nodeoffset, prop_name, quiet=()):
> +        """Get a property from a node as a Property object
> +
> +        Args:
> +            nodeoffset: Node offset containing property to get
> +            prop_name: Name of property to get
> +            quiet: Errors to ignore (empty to raise on all errors)
> +
> +        Returns:
> +            Property object, or None if not found
> +
> +        Raises:
> +            FdtError if any error occurs (e.g. the property is not found)
> +        """
> +        pdata = check_err_null(fdt_getprop(self._fdt, nodeoffset, prop_name),
> +                               quiet)
> +        if isinstance(pdata, (int)):
> +            return None
> +        return Property(prop_name, bytearray(pdata[0]))
> +
>      def get_phandle(self, nodeoffset):
>          """Get the phandle of a node
>  
> @@ -432,6 +452,21 @@ class Property:
>      def __init__(self, name, value):
>          self.name = name
>          self.value = value
> +
> +    def to_cell(self, fmt):
> +        return struct.unpack('>' + fmt, self.value)[0]
> +
> +    def to_uint32(self):
> +        return self.to_cell('L')
> +
> +    def to_int32(self):
> +        return self.to_cell('l')
> +
> +    def to_uint64(self):
> +        return self.to_cell('Q')
> +
> +    def to_int64(self):
> +        return self.to_cell('q')
>  %}
>  
>  %rename(fdt_property) fdt_property_func;
> diff --git a/tests/pylibfdt_tests.py b/tests/pylibfdt_tests.py
> index 0ec0f38..fc5210b 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -89,6 +89,7 @@ class PyLibfdtTests(unittest.TestCase):
>      def setUp(self):
>          """Read in the device tree we use for testing"""
>          self.fdt = _ReadFdt('test_tree1.dtb')
> +        self.fdt2 = _ReadFdt('test_props.dtb')
>  
>      def GetPropList(self, node_path):
>          """Read a list of properties from a node
> @@ -330,6 +331,17 @@ class PyLibfdtTests(unittest.TestCase):
>          node2 = self.fdt.path_offset('/subnode@2/subsubnode@0')
>          self.assertEquals(node2, self.fdt.node_offset_by_phandle(0x2001))
>  
> +    def get_prop(self, name):
> +        return self.fdt2.getprop_obj(0, name)
> +
> +    def testGetIntProperties(self):
> +        """Test that we can access properties as integers"""
> +        self.assertEquals(0xdeadbeef, self.get_prop("prop-hex32").to_uint32())
> +        self.assertEquals(123, self.get_prop("prop-uint32").to_uint32())
> +        self.assertEquals(-2, self.get_prop("prop-int32").to_int32())
> +        self.assertEquals(9223372036854775807,
> +                          self.get_prop("prop-uint64").to_uint64())
> +        self.assertEquals(-2, self.get_prop("prop-int64").to_int64())
>  
>  if __name__ == "__main__":
>      unittest.main()
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index fa7b2f7..441e773 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -809,6 +809,7 @@ fdtoverlay_tests() {
>  }
>  
>  pylibfdt_tests () {
> +    run_dtc_test -I dts -O dtb -o test_props.dtb test_props.dts
>      TMP=/tmp/tests.stderr.$$
>      python pylibfdt_tests.py -v 2> $TMP
>  
> diff --git a/tests/test_props.dts b/tests/test_props.dts
> new file mode 100644
> index 0000000..7e59bd1
> --- /dev/null
> +++ b/tests/test_props.dts
> @@ -0,0 +1,11 @@
> +/dts-v1/;
> +
> +/ {
> +	compatible = "test_props";
> +	prop-hex32 = <0xdeadbeef>;
> +	prop-uint32 = <123>;
> +	prop-int32 = <0xfffffffe>;
> +	prop-hex64 = /bits/ 64 <0xdeadbeef01abcdef>;
> +	prop-uint64 = /bits/ 64 <9223372036854775807>;
> +	prop-int64 = /bits/ 64 <0xfffffffffffffffe>;
> +};

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