Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs

[Eric Auger]

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(_fatal, ...)
> 
>> +}
>> +parent_node = (char *)[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(_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, ) < 0) {
>> +error_setg(_fatal, "%s cannot lstat %s", __func__, 
>> tmpnam);
>> +}
>> +
>> +if (S_ISREG(st.st_mode)) {
>> +gchar *val;
>> +gsize len;
>> +
>> +if (!g_file_get_contents(tmpnam, , , NULL)) {
>> +error_setg(_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(_fdt_size);
>> +read_fstree(host_fdt, SYSFS_DT_BASEDIR);
>> +if (fdt_check_header(host_fdt)) {
>> +error_setg(_fatal,
>> +   "%s host device tree 

Re: [Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs

[Peter Maydell]

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"

> + *
> + * 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 *)[strlen(SYSFS_DT_BASEDIR)];

What causes us to need this cast to char* ?

> +
> +d = opendir(dirname);
> +if (!d) {
> +error_setg(_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, ) < 0) {
> +error_setg(_fatal, "%s cannot lstat %s", __func__, tmpnam);
> +}
> +
> +if (S_ISREG(st.st_mode)) {
> +gchar *val;
> +gsize len;
> +
> +if (!g_file_get_contents(tmpnam, , , NULL)) {
> +error_setg(_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(_fdt_size);
> +read_fstree(host_fdt, SYSFS_DT_BASEDIR);
> +if (fdt_check_header(host_fdt)) {
> +error_setg(_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 

[Qemu-devel] [PATCH v5 2/8] device_tree: introduce load_device_tree_from_sysfs

[Eric Auger]

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).
+ *
+ * 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 *)[strlen(SYSFS_DT_BASEDIR)];
+
+d = opendir(dirname);
+if (!d) {
+error_setg(_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, ) < 0) {
+error_setg(_fatal, "%s cannot lstat %s", __func__, tmpnam);
+}
+
+if (S_ISREG(st.st_mode)) {
+gchar *val;
+gsize len;
+
+if (!g_file_get_contents(tmpnam, , , NULL)) {
+error_setg(_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(_fdt_size);
+read_fstree(host_fdt, SYSFS_DT_BASEDIR);
+if (fdt_check_header(host_fdt)) {
+error_setg(_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);
-- 
1.9.1