Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
---
 device_tree.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 52c3358..2b75905 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -379,8 +379,12 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char
*path)

     r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
     if (r == 0) {
-        error_report("%s: Couldn't get phandle for %s: %s", __func__,
-                     path, fdt_strerror(r));
+        error_report("%s: Node %s does not have a 'phandle'", __func__,
+                     path);
+        exit(1);
+    }
+    if (r == -1) {
+        error_report("%s: Couldn't get phandle for %s", __func__, path);
         exit(1);
     }

-- 
2.1.4

On Fri, May 4, 2018 at 7:37 PM, Jonathan Marler <johnnymarler@gmail.com> wrote:
> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
> ---
>  device_tree.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 52c3358..2b75905 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -379,8 +379,12 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char
> *path)
>
>      r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>      if (r == 0) {
> -        error_report("%s: Couldn't get phandle for %s: %s", __func__,
> -                     path, fdt_strerror(r));
> +        error_report("%s: Node %s does not have a 'phandle'", __func__,
> +                     path);
> +        exit(1);
> +    }
> +    if (r == -1) {
> +        error_report("%s: Couldn't get phandle for %s", __func__, path);

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

On 4 May 2018 at 19:37, Jonathan Marler <johnnymarler@gmail.com> wrote:
> Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>

Hi; thanks for this patch.

> ---
>  device_tree.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/device_tree.c b/device_tree.c
> index 52c3358..2b75905 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -379,8 +379,12 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char
> *path)
>
>      r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
>      if (r == 0) {
> -        error_report("%s: Couldn't get phandle for %s: %s", __func__,
> -                     path, fdt_strerror(r));
> +        error_report("%s: Node %s does not have a 'phandle'", __func__,
> +                     path);
> +        exit(1);
> +    }
> +    if (r == -1) {
> +        error_report("%s: Couldn't get phandle for %s", __func__, path);
>          exit(1);
>      }

Could you explain in what situation this is needed? The documentation
for fdt_get_phandle() says
 * returns:
 *      the phandle of the node at nodeoffset, on success (!= 0, != -1)
 *      0, if the node has no phandle, or another error occurs

which I interpret to mean that it is not possible for it to return -1.

thanks
-- PMM

On Mon, May 14, 2018 at 04:56:54PM +0100, Peter Maydell wrote:
> On 4 May 2018 at 19:37, Jonathan Marler <johnnymarler@gmail.com> wrote:
> > Signed-off-by: Jonathan Marler <johnnymarler@gmail.com>
> 
> Hi; thanks for this patch.
> 
> > ---
> >  device_tree.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/device_tree.c b/device_tree.c
> > index 52c3358..2b75905 100644
> > --- a/device_tree.c
> > +++ b/device_tree.c
> > @@ -379,8 +379,12 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char
> > *path)
> >
> >      r = fdt_get_phandle(fdt, findnode_nofail(fdt, path));
> >      if (r == 0) {
> > -        error_report("%s: Couldn't get phandle for %s: %s", __func__,
> > -                     path, fdt_strerror(r));
> > +        error_report("%s: Node %s does not have a 'phandle'", __func__,
> > +                     path);
> > +        exit(1);
> > +    }
> > +    if (r == -1) {
> > +        error_report("%s: Couldn't get phandle for %s", __func__, path);
> >          exit(1);
> >      }
> 
> Could you explain in what situation this is needed? The documentation
> for fdt_get_phandle() says
>  * returns:
>  *      the phandle of the node at nodeoffset, on success (!= 0, != -1)
>  *      0, if the node has no phandle, or another error occurs
> 
> which I interpret to mean that it is not possible for it to return -1.

I *think* that was the intention; it's a long time since I wrote it,
so I'm not sure.  However, looking at the implementation, that's not
strictly the case.

We certainly return 0 for any error we explicitly detect.  However, if
the node has a "phandle" property that's correctly formed but contains
-1, then we'll return -1.  Of course, a correct dt blob won't have
such a property, but then we do detect a bunch of other conditions
that a correct dt shouldn't have.

On balance, I'm thinking I should probably change libfdt to check for
a -1 phandle property and return 0 for that case, like all other error
cases.

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