[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v5 2/8] device_tree: introduce load_device_tree_fr
From: |
Eric Auger |
Subject: |
Re: [Qemu-arm] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs |
Date: |
Mon, 1 Feb 2016 14:11:45 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
Hi Peter,
On 01/25/2016 03:13 PM, Peter Maydell wrote:
> On 18 January 2016 at 15:16, Eric Auger <address@hidden> 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 <address@hidden>
>>
>> ---
>> 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
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-arm] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs,
Eric Auger <=