| Message ID | 1453130204-655-3-git-send-email-eric.auger@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> 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 <eric.auger@linaro.org> > > --- > 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
Hi Peter, On 01/25/2016 03:13 PM, Peter Maydell wrote: > On 18 January 2016 at 15:16, Eric Auger <eric.auger@linaro.org> 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 <eric.auger@linaro.org> >> >> --- >> 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" OK > >> + * >> + * the function self-asserts in case of error > > "asserts" OK > >> + */ >> +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? replaced with error_setg(&error_fatal, ...) > >> + } >> + parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)]; > > What causes us to need this cast to char* ? I changed parent_node to a const char * instead of 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? ok > >> + 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? sure Thanks Eric > >> +#endif >> >> int qemu_fdt_setprop(void *fdt, const char *node_path, >> const char *property, const void *val, int size); >> -- >> 1.9.1 > > thanks > -- PMM >
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). + * + * 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 = 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); + } + parent_node = (char *)&dirname[strlen(SYSFS_DT_BASEDIR)]; + + d = opendir(dirname); + if (!d) { + error_setg(&error_fatal, "%s cannot open %s", __func__, dirname); + } + + 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); + 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__); + } + 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); +#endif 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 (/proc/device-tree). It uses a recursive function inspired from dtc read_fstree. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- 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(+) -- 1.9.1