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

[Eric Auger <eric.auger@linaro.org>]

Hi Peter,
On 12/18/2015 03:10 PM, Peter Maydell wrote:
> 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.)

I struggled quite a lot while cross-compiling all dependencies for W32
(~ http://wiki.qemu.org/Hosts/W32).

Eventually device_tree.c compiles but there is a link issue since lstat
does not seem to be available with MinGW

But there is definitively a problem with hw/arm/sysbus-fdt.c which is
not compiling due to the inclusion of #include <linux/vfio.h>

So thanks for raising the concern.

With respect to read_fstree, what is your sugestion: shall I keep it in
device_tree.c while protecting it with a CONFIG_LINUX or is it better to
move it, for instance in hw/arm/sysbus-fdt.c?

> 
>>  #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.
OK
> 
>> +        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.
OK
> 
>> +        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.
OK
> 
>> +
>> +        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.
OK thanks for the hint
> 
>> +
>> +                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().
OK
> 
>> +
>> +                    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?
Yes I can do that. I was doing something similar as in load_device_tree

Best Regards

Eric
> 
>> +    }
>> +    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
>