Re: [Qemu-devel] [PATCH 3/6] device_tree: introduce qemu_fdt_node_path

[Peter Maydell <peter.maydell@linaro.org>]

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> This new helper routine returns the node path of a device
> referred to by its node name and compat string.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC -> v1:
> - improve error handling according to Alex' comments
> ---
>  device_tree.c                | 45 ++++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  3 +++
>  2 files changed, 48 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index e556a99..b5d7e0b 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path)
>      return offset;
>  }
>
> +/**
> + * qemu_fdt_node_path
> + *
> + * return the node path of a device, given its node name and its
> + * compat string
> + * fdt: pointer to the dt blob
> + * name: device node name
> + * compat: compatibility string of the device
> + *
> + * upon success, the path is output at node_path address
> + * returns 0 on success, < 0 on failure
> + */

Can we put the doc comment in the header file, since this is
a globally visible function? Also it would be nice to follow the
doc-comment syntax standards about marking up arguments with '@'
and so on.

> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                       char **node_path)
> +{
> +    int offset, len, ret;
> +    const char *iter_name;
> +    char path[256];

Rather than a fixed buffer size, we should check whether
fdt_get_path returns -FDT_ERR_NOSPACE and if so enlarge the
buffer and try again.

> +
> +    *node_path = NULL;
> +    offset = fdt_node_offset_by_compatible(fdt, -1, compat);
> +    while (offset != -FDT_ERR_NOTFOUND) {
> +        if (offset < 0) {
> +            continue;

I don't understand this continue -- if the fdt function returned any
error other than -FDT_ERR_NOTFOUND then this will cause us to go
into an infinite loop around this while(). Did you mean 'break' ?
(Though if you just want to break then fixing the while condition
would be better.)

> +        }
> +        iter_name = fdt_get_name(fdt, offset, &len);
> +        if (!iter_name) {
> +            continue;

This also seems like it ought to be a break, except you need to
set offset to the error code first (which fdt_get_name() will
have returned in 'len').

> +        }
> +
> +        if (!strncmp(iter_name, name, len)) {

Do we really want strncmp rather than strcmp here ? (ie
"find first node whose name has 'name' as a prefix" rather
than "find first node whose name matches 'name').

> +            goto found;
> +        }
> +        offset = fdt_node_offset_by_compatible(fdt, offset, compat);
> +    }
> +    return offset;
> +
> +found:
> +    ret = fdt_get_path(fdt, offset, path, 256);
> +    if (!ret) {
> +        *node_path = g_strdup(path);
> +    }
> +    return ret;
> +}
> +
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size)
>  {
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 307e53d..f9e6e6e 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  void *load_device_tree_from_sysfs(void);
>
> +int qemu_fdt_node_path(void *fdt, const char *name, char *compat,
> +                       char **node_path);
> +
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);
>  int qemu_fdt_setprop_cell(void *fdt, const char *node_path,
> --
> 1.9.1

thanks
-- PMM