[PATCH v2] pylibfdt: add FdtRo.get_path()

[PATCH v2] 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>
---
Changes in v2:
- dynamically increase size of string buffer until it fits.
  The values are chosen relatively arbitrary, and terminates at lengths
  over than 4096 as I don't expect paths to be longer and there should
  be an exit condition to not eat up RAM in case of some bug.

 pylibfdt/libfdt.i       | 27 +++++++++++++++++++++++++++
 tests/pylibfdt_tests.py |  7 +++++++
 2 files changed, 34 insertions(+)

diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
index ac70762..d103bb6 100644
--- a/pylibfdt/libfdt.i
+++ b/pylibfdt/libfdt.i
@@ -443,6 +443,28 @@ class FdtRo(object):
         """
         return fdt_get_alias(self._fdt, name)
 
+    def get_path(self, nodeoffset):
+        """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
+        """
+        size = 64
+        while size < 4096:
+            ret, path = fdt_get_path(self._fdt, nodeoffset, size)
+            if ret == -NOSPACE:
+                size += 64
+                continue
+            check_err(ret)
+            return path
+        raise ValueError('Node path is unexpectedly long')
+
     def parent_offset(self, nodeoffset, quiet=()):
         """Get the offset of a node's parent
 
@@ -1115,6 +1137,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..2335a73 100644
--- a/tests/pylibfdt_tests.py
+++ b/tests/pylibfdt_tests.py
@@ -348,6 +348,13 @@ 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/subsubnode", self.fdt.get_path(node2))
+
     def testParentOffset(self):
         """Test for the parent_offset() method"""
         self.assertEqual(-libfdt.NOTFOUND,
-- 
2.34.1

Re: [PATCH v2] pylibfdt: add FdtRo.get_path()

[Simon Glass]

Hi Luca,

On Sat, 22 Jan 2022 at 03:59, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> 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>
> ---
> Changes in v2:
> - dynamically increase size of string buffer until it fits.
>   The values are chosen relatively arbitrary, and terminates at lengths
>   over than 4096 as I don't expect paths to be longer and there should
>   be an exit condition to not eat up RAM in case of some bug.
>
>  pylibfdt/libfdt.i       | 27 +++++++++++++++++++++++++++
>  tests/pylibfdt_tests.py |  7 +++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index ac70762..d103bb6 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -443,6 +443,28 @@ class FdtRo(object):
>          """
>          return fdt_get_alias(self._fdt, name)
>
> +    def get_path(self, nodeoffset):
> +        """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
> +        """
> +        size = 64
> +        while size < 4096:
> +            ret, path = fdt_get_path(self._fdt, nodeoffset, size)
> +            if ret == -NOSPACE:
> +                size += 64
> +                continue
> +            check_err(ret)
> +            return path
> +        raise ValueError('Node path is unexpectedly long')

Do you think it would hurt to go with 4096 immediately? Python is not
the most efficient language so it should not matter. You can also copy
it to another object of the correct size, perhaps, to avoid memory
wastage.

Also can you please explain how swig works here? I'm a bit lost as to
the magic for calling fdt_get_path().

> +
>      def parent_offset(self, nodeoffset, quiet=()):
>          """Get the offset of a node's parent
>
> @@ -1115,6 +1137,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..2335a73 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -348,6 +348,13 @@ 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/subsubnode", self.fdt.get_path(node2))
> +
>      def testParentOffset(self):
>          """Test for the parent_offset() method"""
>          self.assertEqual(-libfdt.NOTFOUND,
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2] pylibfdt: add FdtRo.get_path()

[Luca Weiss]

Hi Simon,

On Montag, 24. Jänner 2022 18:57:47 CET Simon Glass wrote:
> Hi Luca,
> 
> On Sat, 22 Jan 2022 at 03:59, Luca Weiss <luca-IfPCFPJWly+lVyrhU4qvOw@public.gmane.org> 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>
> > ---
> > Changes in v2:
> > - dynamically increase size of string buffer until it fits.
> > 
> >   The values are chosen relatively arbitrary, and terminates at lengths
> >   over than 4096 as I don't expect paths to be longer and there should
> >   be an exit condition to not eat up RAM in case of some bug.
> >  
> >  pylibfdt/libfdt.i       | 27 +++++++++++++++++++++++++++
> >  tests/pylibfdt_tests.py |  7 +++++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > index ac70762..d103bb6 100644
> > --- a/pylibfdt/libfdt.i
> > +++ b/pylibfdt/libfdt.i
> > 
> > @@ -443,6 +443,28 @@ class FdtRo(object):
> >          """
> >          return fdt_get_alias(self._fdt, name)
> > 
> > +    def get_path(self, nodeoffset):
> > +        """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 +        """
> > +        size = 64
> > +        while size < 4096:
> > +            ret, path = fdt_get_path(self._fdt, nodeoffset, size)
> > +            if ret == -NOSPACE:
> > +                size += 64
> > +                continue
> > +            check_err(ret)
> > +            return path
> > +        raise ValueError('Node path is unexpectedly long')
> 
> Do you think it would hurt to go with 4096 immediately? Python is not
> the most efficient language so it should not matter. You can also copy
> it to another object of the correct size, perhaps, to avoid memory
> wastage.

The returned string won't be of $size but of the actual string size, swig doc 
mentions of the buffer size being used to dynamically allocate memory on heap 
but the result will be returned as string.
Probably it's not a problem to start with a bigger size, but maybe 4096 is a 
bit overkill for a normal use case? Paths (at least in Linux dts files) are 
normally relatively short so 512 or 1024 feels better for this.

Which values do you think are appropriate here? I made this loop because of 
David's reply to v1: 
https://www.spinics.net/lists/devicetree-compiler/msg03857.html

> Also can you please explain how swig works here? I'm a bit lost as to
> the magic for calling fdt_get_path().

It's quite well described in the doc (ctrl-f for "cstring_output_maxsize")
http://www.swig.org/Doc4.0/SWIGDocumentation.html
I don't think I've worked with swig before so I found this on stackoverflow or 
somewhere, but this function definitely seems to be made for this use case.

As far as I understand it, it just allocates a buffer of x bytes, does the C 
call with the temporary buffer and then converts the result into a proper 
Python object and discards the buffer.

Regards
Luca

Re: [PATCH v2] pylibfdt: add FdtRo.get_path()

[David Gibson]

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

On Sat, Jan 22, 2022 at 11:56:54AM +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>
> ---
> Changes in v2:
> - dynamically increase size of string buffer until it fits.
>   The values are chosen relatively arbitrary, and terminates at lengths
>   over than 4096 as I don't expect paths to be longer and there should
>   be an exit condition to not eat up RAM in case of some bug.
> 
>  pylibfdt/libfdt.i       | 27 +++++++++++++++++++++++++++
>  tests/pylibfdt_tests.py |  7 +++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> index ac70762..d103bb6 100644
> --- a/pylibfdt/libfdt.i
> +++ b/pylibfdt/libfdt.i
> @@ -443,6 +443,28 @@ class FdtRo(object):
>          """
>          return fdt_get_alias(self._fdt, name)
>  
> +    def get_path(self, nodeoffset):
> +        """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
> +        """
> +        size = 64
> +        while size < 4096:
> +            ret, path = fdt_get_path(self._fdt, nodeoffset, size)
> +            if ret == -NOSPACE:
> +                size += 64
> +                continue
> +            check_err(ret)
> +            return path
> +        raise ValueError('Node path is unexpectedly long')

Ah... sorry, this isn't quite what I had in mind.  The idea of
resizing the buffer is to avoid having an arbitrary limit, but you
still have the 4096 byte arbitrary limit here.  As Simon suggests, 4k
would probably be a good *starting* point, rather than an ending
point.  Not only are repeated calls expensive in Python, but
fdt_get_path() itself is also quite expensive, since it needs to scan
the dtb from the start.  I'd also suggest doubling the buffer size on
each attempt, rather than increasing linearly.


>      def parent_offset(self, nodeoffset, quiet=()):
>          """Get the offset of a node's parent
>  
> @@ -1115,6 +1137,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..2335a73 100644
> --- a/tests/pylibfdt_tests.py
> +++ b/tests/pylibfdt_tests.py
> @@ -348,6 +348,13 @@ 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/subsubnode", self.fdt.get_path(node2))
> +
>      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 v2] pylibfdt: add FdtRo.get_path()

[Luca Weiss]

Hi David,

On Dienstag, 25. Jänner 2022 05:48:34 CET David Gibson wrote:
> On Sat, Jan 22, 2022 at 11:56:54AM +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>
> > ---
> > Changes in v2:
> > - dynamically increase size of string buffer until it fits.
> > 
> >   The values are chosen relatively arbitrary, and terminates at lengths
> >   over than 4096 as I don't expect paths to be longer and there should
> >   be an exit condition to not eat up RAM in case of some bug.
> >  
> >  pylibfdt/libfdt.i       | 27 +++++++++++++++++++++++++++
> >  tests/pylibfdt_tests.py |  7 +++++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > index ac70762..d103bb6 100644
> > --- a/pylibfdt/libfdt.i
> > +++ b/pylibfdt/libfdt.i
> > 
> > @@ -443,6 +443,28 @@ class FdtRo(object):
> >          """
> >          return fdt_get_alias(self._fdt, name)
> > 
> > +    def get_path(self, nodeoffset):
> > +        """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 +        """
> > +        size = 64
> > +        while size < 4096:
> > +            ret, path = fdt_get_path(self._fdt, nodeoffset, size)
> > +            if ret == -NOSPACE:
> > +                size += 64
> > +                continue
> > +            check_err(ret)
> > +            return path
> > +        raise ValueError('Node path is unexpectedly long')
> 
> Ah... sorry, this isn't quite what I had in mind.  The idea of
> resizing the buffer is to avoid having an arbitrary limit, but you
> still have the 4096 byte arbitrary limit here.  As Simon suggests, 4k
> would probably be a good *starting* point, rather than an ending
> point.  Not only are repeated calls expensive in Python, but
> fdt_get_path() itself is also quite expensive, since it needs to scan
> the dtb from the start.  I'd also suggest doubling the buffer size on
> each attempt, rather than increasing linearly.

I do think that there should be a limit *somewhere* instead of hitting OOM if 
there would be some bug in the underlying implementation or the file provided? 
I'm fine with limiting it at 100MB or whatever but I would rather have a limit 
there than not have it.

Once this is clarified I'll make a v3 of this patch.

Regards
Luca

> 
> >      def parent_offset(self, nodeoffset, quiet=()):
> >          """Get the offset of a node's parent
> > 
> > @@ -1115,6 +1137,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..2335a73 100644
> > --- a/tests/pylibfdt_tests.py
> > +++ b/tests/pylibfdt_tests.py
> > 
> > @@ -348,6 +348,13 @@ 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/subsubnode",
> > self.fdt.get_path(node2)) +
> > 
> >      def testParentOffset(self):
> >          """Test for the parent_offset() method"""
> >          self.assertEqual(-libfdt.NOTFOUND,

Re: [PATCH v2] pylibfdt: add FdtRo.get_path()

[David Gibson]

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

On Tue, Jan 25, 2022 at 07:36:17PM +0100, Luca Weiss wrote:
> Hi David,
> 
> On Dienstag, 25. Jänner 2022 05:48:34 CET David Gibson wrote:
> > On Sat, Jan 22, 2022 at 11:56:54AM +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>
> > > ---
> > > Changes in v2:
> > > - dynamically increase size of string buffer until it fits.
> > > 
> > >   The values are chosen relatively arbitrary, and terminates at lengths
> > >   over than 4096 as I don't expect paths to be longer and there should
> > >   be an exit condition to not eat up RAM in case of some bug.
> > >  
> > >  pylibfdt/libfdt.i       | 27 +++++++++++++++++++++++++++
> > >  tests/pylibfdt_tests.py |  7 +++++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i
> > > index ac70762..d103bb6 100644
> > > --- a/pylibfdt/libfdt.i
> > > +++ b/pylibfdt/libfdt.i
> > > 
> > > @@ -443,6 +443,28 @@ class FdtRo(object):
> > >          """
> > >          return fdt_get_alias(self._fdt, name)
> > > 
> > > +    def get_path(self, nodeoffset):
> > > +        """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 +        """
> > > +        size = 64
> > > +        while size < 4096:
> > > +            ret, path = fdt_get_path(self._fdt, nodeoffset, size)
> > > +            if ret == -NOSPACE:
> > > +                size += 64
> > > +                continue
> > > +            check_err(ret)
> > > +            return path
> > > +        raise ValueError('Node path is unexpectedly long')
> > 
> > Ah... sorry, this isn't quite what I had in mind.  The idea of
> > resizing the buffer is to avoid having an arbitrary limit, but you
> > still have the 4096 byte arbitrary limit here.  As Simon suggests, 4k
> > would probably be a good *starting* point, rather than an ending
> > point.  Not only are repeated calls expensive in Python, but
> > fdt_get_path() itself is also quite expensive, since it needs to scan
> > the dtb from the start.  I'd also suggest doubling the buffer size on
> > each attempt, rather than increasing linearly.
> 
> I do think that there should be a limit *somewhere* instead of hitting OOM if 
> there would be some bug in the underlying implementation or the file provided? 
> I'm fine with limiting it at 100MB or whatever but I would rather have a limit 
> there than not have it.

Well, it can never exceed the size of the whole dtb, structurally, so
I don't think it's really a problem.

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