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

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

On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> wrote:
> This function returns the host device tree blob from sysfs
> (/sys/firmware/devicetree/base). It uses a recursive function
> inspired from dtc read_fstree.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> RFC -> v1:
> - remove runtime dependency on dtc binary and introduce read_fstree
> ---
>  device_tree.c                | 102 +++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/device_tree.h |   1 +
>  2 files changed, 103 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index a9f5f8e..e556a99 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -17,6 +17,7 @@
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#include <dirent.h>

Does this code compile on non-Linux hosts? (You've put it in a file
which is built everywhere, but it's definitely semantically Linux
specific.)

>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -117,6 +118,107 @@ fail:
>      return NULL;
>  }
>
> +/**
> + * read_fstree: this function is inspired from dtc read_fstree
> + * @fdt: preallocated fdt blob buffer, to be populated
> + * @dirname: directory to scan under /sys/firmware/devicetree/base
> + * the search is recursive and the tree is search down to the
> + * leafs (property files).
> + *
> + * the function self-asserts in case of error
> + */
> +static void read_fstree(void *fdt, const char *dirname)
> +{
> +        DIR *d;
> +        struct dirent *de;

Indent here doesn't match QEMU coding style, which is four-space.

> +        struct stat st;
> +        const char *root_dir = "/sys/firmware/devicetree/base";

You use this string twice and its length once so it would be nice
to have it in a #define.

> +        char *parent_node;
> +
> +        if (strstr(dirname, root_dir) != dirname) {
> +            error_report("%s: %s must be searched within %s",
> +                         __func__, dirname, root_dir);
> +            exit(1);
> +        }
> +        parent_node = (char *)&dirname[29];

I think 29 here should be strlen(SYSFS_DT_BASEDIR) or whatever
you want to call it.

> +
> +        d = opendir(dirname);
> +        if (!d) {
> +                error_report("%s cannot open %s", __func__, dirname);
> +                exit(1);
> +        }
> +
> +        while ((de = readdir(d)) != NULL) {
> +                char *tmpnam;
> +
> +                if (!g_strcmp0(de->d_name, ".")
> +                    || !g_strcmp0(de->d_name, "..")) {
> +                        continue;
> +                }

If you used glib g_dir_open/g_dir_read_name/g_dir_close it would
automatically skip '.' and '..' for you, but I'm not sure the
benefit is enough to bother redoing this code now.

> +
> +                tmpnam = g_strjoin("/", dirname, de->d_name, NULL);
> +
> +                if (lstat(tmpnam, &st) < 0) {
> +                        error_report("%s cannot lstat %s", __func__, tmpnam);
> +                        exit(1);
> +                }
> +
> +                if (S_ISREG(st.st_mode)) {
> +                    int ret, size = st.st_size;
> +                    void *val = g_malloc0(size);
> +                    FILE *pfile;
> +
> +                    pfile = fopen(tmpnam, "r");
> +                    if (!pfile) {
> +                        error_report("%s cannot open %s", __func__, tmpnam);
> +                        exit(1);
> +                    }
> +                    ret = fread(val, 1, size, pfile);
> +                    if (ferror(pfile) || ret < size) {
> +                        error_report("%s fail reading %s", __func__, tmpnam);
> +                        exit(1);
> +                    }
> +                    fclose(pfile);

This looks like it's reimplementing g_file_get_contents().

> +
> +                    if (strlen(parent_node) > 0) {
> +                        qemu_fdt_setprop(fdt, parent_node,
> +                                         de->d_name, val, size);
> +                    } else {
> +                        qemu_fdt_setprop(fdt, "/", de->d_name, val, size);
> +                    }
> +                    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);
> +                        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, "/sys/firmware/devicetree/base");
> +    if (fdt_check_header(host_fdt)) {
> +        error_report("%s host device tree extracted into memory is invalid",
> +                     __func__);
> +        g_free(host_fdt);

Why do we exit-on-error for the errors inside read_fstree() but
plough on (returning a pointer to freed memory!) in this case?

> +    }
> +    return host_fdt;
> +}
> +
>  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..307e53d 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -16,6 +16,7 @@
>
>  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_setprop(void *fdt, const char *node_path,
>                       const char *property, const void *val, int size);
> --
> 1.9.1

thanks
-- PMM