From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43015) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQEH6-0000uY-GM for qemu-devel@nongnu.org; Mon, 01 Feb 2016 08:12:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQEH3-0002XT-9M for qemu-devel@nongnu.org; Mon, 01 Feb 2016 08:12:12 -0500 Received: from mail-wm0-x22b.google.com ([2a00:1450:400c:c09::22b]:36058) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQEH2-0002XH-KV for qemu-devel@nongnu.org; Mon, 01 Feb 2016 08:12:09 -0500 Received: by mail-wm0-x22b.google.com with SMTP id p63so69693613wmp.1 for ; Mon, 01 Feb 2016 05:12:08 -0800 (PST) References: <1453130204-655-1-git-send-email-eric.auger@linaro.org> <1453130204-655-3-git-send-email-eric.auger@linaro.org> From: Eric Auger Message-ID: <56AF5991.9030301@linaro.org> Date: Mon, 1 Feb 2016 14:11:45 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Baptiste Reynal , Thomas Huth , eric.auger@st.com, Patch Tracking , Peter Crosthwaite , QEMU Developers , Alex Williamson , qemu-arm , Suravee Suthikulpanit , Paolo Bonzini , thomas.lendacky@amd.com, =?UTF-8?Q?Alex_Benn=c3=a9e?= , Christoffer Dall , David Gibson Hi Peter, On 01/25/2016 03:13 PM, Peter Maydell wrote: > On 18 January 2016 at 15:16, Eric Auger 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 >> >> --- >> 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 >> #include >> #include >> +#ifdef CONFIG_LINUX >> +#include >> +#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 >