[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] device_tree: introduce load_device_tree_fro
From: |
Eric Auger |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] device_tree: introduce load_device_tree_from_sysfs |
Date: |
Mon, 4 Jan 2016 18:37:52 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
Hi Peter,
On 12/18/2015 03:10 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <address@hidden> 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 <address@hidden>
>>
>> ---
>>
>> 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
>
- Re: [Qemu-devel] [PATCH 2/6] device_tree: introduce load_device_tree_from_sysfs,
Eric Auger <=