Re: [Qemu-devel] [RFC PATCH 2/6] device_tree: add qemu_fdt_add_path

[Andrew Jones <drjones@redhat.com>]

On Mon, Jul 23, 2018 at 02:42:54PM +0200, Igor Mammedov wrote:
> On Wed,  4 Jul 2018 14:49:19 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> > also recursively adds any missing parent nodes.
> 
> Probably add here why new helper is need?

OK

> 
> > 
> > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  device_tree.c                | 24 ++++++++++++++++++++++++
> >  include/sysemu/device_tree.h |  1 +
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/device_tree.c b/device_tree.c
> > index 6d9c9726f66c..ad570a4dbe3a 100644
> > --- a/device_tree.c
> > +++ b/device_tree.c
> > @@ -520,6 +520,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
> >      return retval;
> >  }
> >  
> > +int qemu_fdt_add_path(void *fdt, const char *path)
> > +{
> > +    char *parent;
> > +    int offset;
> > +
> > +    offset = fdt_path_offset(fdt, path);
> > +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> > +        error_report("%s Couldn't find node %s: %s", __func__, path,
> > +                     fdt_strerror(offset));
> > +        exit(1);
> > +    }
> > +
> > +    if (offset != -FDT_ERR_NOTFOUND) {
> > +        return offset;
> > +    }
> > +
> > +    parent = g_strdup(path);
> > +    strrchr(parent, '/')[0] = '\0';
> > +    qemu_fdt_add_path(fdt, parent);
> can it be implemented without recursion?

It can, but it gets ugly quick, as we need to walk forward, instead of
backward, building the path node by node, instead of letting the recursion
do its magic. g_strsplit() and g_strjoinv() help, but it's still not very
nice. This file already uses recursion in read_fstree() and the recursion
of this function is bounded by the number of /'s in the input path. I'd
rather leave this function recursive, unless people feel strongly about
it. I should add a sanity check for the root '/' though.

> 
> > +    g_free(parent);
> > +
> > +    return qemu_fdt_add_subnode(fdt, path);
> > +}
> > +
> >  void qemu_fdt_dumpdtb(void *fdt, int size)
> >  {
> >      const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
> > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> > index c16fd69bc0b1..d62fc873a3ea 100644
> > --- a/include/sysemu/device_tree.h
> > +++ b/include/sysemu/device_tree.h
> > @@ -101,6 +101,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
> >  uint32_t qemu_fdt_alloc_phandle(void *fdt);
> >  int qemu_fdt_nop_node(void *fdt, const char *node_path);
> >  int qemu_fdt_add_subnode(void *fdt, const char *name);
> 
> /**
>  * qemu_fdt_add_path: ....
>  ...
> */
> 
> It would be nice to have a doc comment here

OK

Thanks,
drew

> 
> > +int qemu_fdt_add_path(void *fdt, const char *path);
> >  
> >  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
> >      do {                                                                      \
> 
>