Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs

[Peter Maydell]

On 18 January 2016 at 15:16, Eric Auger <address@hidden> wrote:
> This function returns the host device tree blob from sysfs
> (/proc/device-tree). It uses a recursive function inspired
> from dtc read_fstree.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
> v1 -> v2:
> - do not implement/expose read_fstree and load_device_tree_from_sysfs
>   if CONFIG_LINUX is not defined (lstat is not implemeted in mingw)
> - correct indentation in read_fstree
> - use /proc/device-tree symlink instead of /sys/firmware/devicetree/base
>   path (kernel.org/doc/Documentation/ABI/testing/sysfs-firmware-ofw)
> - use g_file_get_contents in read_fstree
> - introduce SYSFS_DT_BASEDIR macro and use strlen
> - exit on error in load_device_tree_from_sysfs
> - user error_setg
>
> RFC -> v1:
> - remove runtime dependency on dtc binary and introduce read_fstree
> ---
>  device_tree.c                | 100 
> +++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |   3 ++
>  2 files changed, 103 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..b262c2d 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -17,6 +17,9 @@
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#ifdef CONFIG_LINUX
> +#include <dirent.h>
> +#endif
>
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -117,6 +120,103 @@ fail:
>      return NULL;
>  }
>
> +#ifdef CONFIG_LINUX
> +
> +#define SYSFS_DT_BASEDIR "/proc/device-tree"
> +
> +/**
> + * read_fstree: this function is inspired from dtc read_fstree
> + * @fdt: preallocated fdt blob buffer, to be populated
> + * @dirname: directory to scan under SYSFS_DT_BASEDIR
> + * the search is recursive and the tree is searched down to the
> + * leafs (property files).

"leaves"

> + *
> + * the function self-asserts in case of error

"asserts"

> + */
> +static void read_fstree(void *fdt, const char *dirname)
> +{
> +    DIR *d;
> +    struct dirent *de;
> +    struct stat st;
> +    const char *root_dir = SYSFS_DT_BASEDIR;
> +    char *parent_node;
> +
> +    if (strstr(dirname, root_dir) != dirname) {
> +        error_report("%s: %s must be searched within %s",
> +                     __func__, dirname, root_dir);
> +        exit(1);

Why does this one error_report and exit but other errors below use
error_setg?

> +    }
> +    parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)];

What causes us to need this cast to char* ?

> +
> +    d = opendir(dirname);
> +    if (!d) {
> +        error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);

You need to return here (and similarly to bail out properly
in the other error paths below).

> +    }
> +
> +    while ((de = readdir(d)) != NULL) {
> +        char *tmpnam;
> +
> +        if (!g_strcmp0(de->d_name, ".")
> +            || !g_strcmp0(de->d_name, "..")) {
> +            continue;
> +        }
> +
> +        tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
> +
> +        if (lstat(tmpnam, &st) < 0) {
> +            error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
> +        }
> +
> +        if (S_ISREG(st.st_mode)) {
> +            gchar *val;
> +            gsize len;
> +
> +            if (!g_file_get_contents(tmpnam, &val, &len, NULL)) {
> +                error_setg(&error_fatal, "%s not able to extract info from 
> %s",
> +                           __func__, tmpnam);
> +            }
> +
> +            if (strlen(parent_node) > 0) {
> +                qemu_fdt_setprop(fdt, parent_node,
> +                                 de->d_name, val, len);
> +            } else {
> +                qemu_fdt_setprop(fdt, "/", de->d_name, val, len);
> +            }
> +            g_free(val);
> +        } else if (S_ISDIR(st.st_mode)) {
> +            char *node_name;
> +
> +            node_name = g_strdup_printf("%s/%s",
> +                                        parent_node, de->d_name);

I don't mind whether we use g_strjoin("/",...) or g_strdup_printf("%s/%s", ...)
to glue together strings with a '/' between them, but can we not use
both methods in the same function, please?

> +            qemu_fdt_add_subnode(fdt, node_name);
> +            g_free(node_name);
> +            read_fstree(fdt, tmpnam);
> +        }
> +
> +        g_free(tmpnam);
> +    }
> +
> +    closedir(d);
> +}
> +
> +/* load_device_tree_from_sysfs: extract the dt blob from host sysfs */
> +void *load_device_tree_from_sysfs(void)
> +{
> +    void *host_fdt;
> +    int host_fdt_size;
> +
> +    host_fdt = create_device_tree(&host_fdt_size);
> +    read_fstree(host_fdt, SYSFS_DT_BASEDIR);
> +    if (fdt_check_header(host_fdt)) {
> +        error_setg(&error_fatal,
> +                   "%s host device tree extracted into memory is invalid",
> +                   __func__);

Should we free the created device tree and return NULL here?

> +    }
> +    return host_fdt;
> +}
> +
> +#endif /* CONFIG_LINUX */
> +
>  static int findnode_nofail(void *fdt, const char *node_path)
>  {
>      int offset;
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 359e143..fdf25a4 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -16,6 +16,9 @@
>
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
> +#ifdef CONFIG_LINUX
> +void *load_device_tree_from_sysfs(void);

Can we have a doc comment for a new global function, please?

> +#endif
>
>  int qemu_fdt_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);
> --
> 1.9.1

thanks
-- PMM