| Message ID | 1450355367-14818-3-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 >
On 4 January 2016 at 17:37, Eric Auger <eric.auger@linaro.org> wrote: > Hi Peter, > On 12/18/2015 03:10 PM, Peter Maydell wrote: >> 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? I don't have a strong opinion, but I don't think this code is arm-specific, so hw/arm doesn't sound quite right. A CONFIG_LINUX ifdef might be simplest if there's no obvious other file to put this. thanks -- PMM
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> #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; + struct stat st; + const char *root_dir = "/sys/firmware/devicetree/base"; + 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]; + + 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; + } + + 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); + + 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); + } + 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);
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(+) -- 1.9.1