qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/3] qapi/error: converts error_setg(&error_fatal) to error_report() + exit()
@ 2018-06-21 11:40 Philippe Mathieu-Daudé
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 1/3] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-21 11:40 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Philippe Mathieu-Daudé, qemu-devel, Eric Blake

Hi,

This series converts error_setg(&error_fatal) to error_report() + exit() as
suggested by the "qapi/error.h" documentation.

This reduce Coverity and Clang static analyzer positive falses.

See http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07585.html:

    On 07/24/2017 04:52 PM, Eric Blake wrote:
    That's a shame.  Rather, we should patch this file (and others) to avoid
    all the inconsistent uses of error_setg(&error_*), to comply with the
    error.h documentation.

Since v2:
- added R-b, A-b (Markus, John, David)
- fixed incorrect update of sysbus-fdt patch between v1->v2 (Markus)

Since v1:
- patch #1: use assert() directly (Markus explanation)
- patch #2: use abort() without error_report() (Markus 'no lipstick')
- patch #3: replaced exit() by assert() (Markus)
- patch #4: no change, added R-b

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  hw/block/fdc: Replace error_setg(&error_abort) by assert()
  hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report()
    + exit()
  device_tree: Replace error_setg(&error_fatal) by error_report() +
    exit()

 device_tree.c       | 23 +++++++++++++----------
 hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++-------------------
 hw/block/fdc.c      |  9 +--------
 3 files changed, 40 insertions(+), 37 deletions(-)

-- 
2.18.0.rc2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v3 1/3] hw/block/fdc: Replace error_setg(&error_abort) by assert()
  2018-06-21 11:40 [Qemu-devel] [PATCH v3 0/3] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
@ 2018-06-21 11:40 ` Philippe Mathieu-Daudé
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 3/3] device_tree: " Philippe Mathieu-Daudé
  2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-21 11:40 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-block, John Snow,
	Kevin Wolf, Max Reitz, Eric Blake

Use assert() instead of error_setg(&error_abort),
as suggested by the "qapi/error.h" documentation:

    Please don't error_setg(&error_fatal, ...), use error_report() and
    exit(), because that's more obvious.
    Likewise, don't error_setg(&error_abort, ...), use assert().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cd29e27d8f..7c1c57f57f 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -396,16 +396,9 @@ static int pick_geometry(FDrive *drv)
                            nb_sectors,
                            FloppyDriveType_str(parse->drive));
         }
+        assert(type_match != -1 && "misconfigured fd_format");
         match = type_match;
     }
-
-    /* No match of any kind found -- fd_format is misconfigured, abort. */
-    if (match == -1) {
-        error_setg(&error_abort, "No candidate geometries present in table "
-                   " for floppy drive type '%s'",
-                   FloppyDriveType_str(drv->drive));
-    }
-
     parse = &(fd_formats[match]);
 
  out:
-- 
2.18.0.rc2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
  2018-06-21 11:40 [Qemu-devel] [PATCH v3 0/3] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 1/3] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
@ 2018-06-21 11:40 ` Philippe Mathieu-Daudé
  2018-06-21 13:02   ` Auger Eric
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 3/3] device_tree: " Philippe Mathieu-Daudé
  2 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-21 11:40 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-arm, Peter Maydell,
	Eric Blake

Use error_report() + exit() instead of error_setg(&error_fatal),
as suggested by the "qapi/error.h" documentation:

   Please don't error_setg(&error_fatal, ...), use error_report() and
   exit(), because that's more obvious.

This fixes CID 1352173:
    "Passing null pointer dt_name to qemu_fdt_node_path, which dereferences it."

And this also fixes:

    hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
        if (node_path[1]) {
            ^~~~~~~~~~~~

Fixes: Coverity CID 1352173 (Dereference after null check)
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 277ed872e7..63b74a353e 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -92,7 +92,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
         r = qemu_fdt_getprop(host_fdt, node_path,
                              props[i].name,
                              &prop_len,
-                             props[i].optional ? &err : &error_fatal);
+                             &err);
         if (r) {
             qemu_fdt_setprop(guest_fdt, nodename,
                              props[i].name, r, prop_len);
@@ -103,6 +103,10 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
             } else {
                 error_free(err);
             }
+            if (!props[i].optional) {
+                /* mandatory property not found: bail out */
+                exit(1);
+            }
         }
     }
 }
@@ -138,9 +142,9 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
 
     node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
     if (node_offset <= 0) {
-        error_setg(&error_fatal,
-                   "not able to locate clock handle %d in host device tree",
-                   host_phandle);
+        error_report("not able to locate clock handle %d in host device tree",
+                     host_phandle);
+        exit(1);
     }
     node_path = g_malloc(path_len);
     while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
@@ -149,16 +153,16 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
         node_path = g_realloc(node_path, path_len);
     }
     if (ret < 0) {
-        error_setg(&error_fatal,
-                   "not able to retrieve node path for clock handle %d",
-                   host_phandle);
+        error_report("not able to retrieve node path for clock handle %d",
+                     host_phandle);
+        exit(1);
     }
 
     r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
                          &error_fatal);
     if (strcmp(r, "fixed-clock")) {
-        error_setg(&error_fatal,
-                   "clock handle %d is not a fixed clock", host_phandle);
+        error_report("clock handle %d is not a fixed clock", host_phandle);
+        exit(1);
     }
 
     nodename = strrchr(node_path, '/');
@@ -301,34 +305,37 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
 
     dt_name = sysfs_to_dt_name(vbasedev->name);
     if (!dt_name) {
-        error_setg(&error_fatal, "%s incorrect sysfs device name %s",
-                    __func__, vbasedev->name);
+        error_report("%s incorrect sysfs device name %s",
+                     __func__, vbasedev->name);
+        exit(1);
     }
     node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
                                    &error_fatal);
     if (!node_path || !node_path[0]) {
-        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
-                   __func__, dt_name, vdev->compat);
+        error_report("%s unable to retrieve node path for %s/%s",
+                     __func__, dt_name, vdev->compat);
+        exit(1);
     }
 
     if (node_path[1]) {
-        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
-                   __func__, dt_name, vdev->compat);
+        error_report("%s more than one node matching %s/%s!",
+                     __func__, dt_name, vdev->compat);
+        exit(1);
     }
 
     g_free(dt_name);
 
     if (vbasedev->num_regions != 5) {
-        error_setg(&error_fatal, "%s Does the host dt node combine XGBE/PHY?",
-                   __func__);
+        error_report("%s Does the host dt node combine XGBE/PHY?", __func__);
+        exit(1);
     }
 
     /* generate nodes for DMA_CLK and PTP_CLK */
     r = qemu_fdt_getprop(host_fdt, node_path[0], "clocks",
                          &prop_len, &error_fatal);
     if (prop_len != 8) {
-        error_setg(&error_fatal, "%s clocks property should contain 2 handles",
-                   __func__);
+        error_report("%s clocks property should contain 2 handles", __func__);
+        exit(1);
     }
     host_clock_phandles = (uint32_t *)r;
     guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
-- 
2.18.0.rc2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v3 3/3] device_tree: Replace error_setg(&error_fatal) by error_report() + exit()
  2018-06-21 11:40 [Qemu-devel] [PATCH v3 0/3] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 1/3] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
@ 2018-06-21 11:40 ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-21 11:40 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Philippe Mathieu-Daudé, qemu-devel, Eric Blake, David Gibson,
	Peter Crosthwaite, Alexander Graf

Use error_report() + exit() instead of error_setg(&error_fatal),
as suggested by the "qapi/error.h" documentation:

   Please don't error_setg(&error_fatal, ...), use error_report() and
   exit(), because that's more obvious.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 device_tree.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 52c3358a55..3553819257 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -140,15 +140,16 @@ static void read_fstree(void *fdt, const char *dirname)
     const char *parent_node;
 
     if (strstr(dirname, root_dir) != dirname) {
-        error_setg(&error_fatal, "%s: %s must be searched within %s",
-                   __func__, dirname, root_dir);
+        error_report("%s: %s must be searched within %s",
+                     __func__, dirname, root_dir);
+        exit(1);
     }
     parent_node = &dirname[strlen(SYSFS_DT_BASEDIR)];
 
     d = opendir(dirname);
     if (!d) {
-        error_setg(&error_fatal, "%s cannot open %s", __func__, dirname);
-        return;
+        error_report("%s cannot open %s", __func__, dirname);
+        exit(1);
     }
 
     while ((de = readdir(d)) != NULL) {
@@ -162,7 +163,8 @@ static void read_fstree(void *fdt, const char *dirname)
         tmpnam = g_strdup_printf("%s/%s", dirname, de->d_name);
 
         if (lstat(tmpnam, &st) < 0) {
-            error_setg(&error_fatal, "%s cannot lstat %s", __func__, tmpnam);
+            error_report("%s cannot lstat %s", __func__, tmpnam);
+            exit(1);
         }
 
         if (S_ISREG(st.st_mode)) {
@@ -170,8 +172,9 @@ static void read_fstree(void *fdt, const char *dirname)
             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);
+                error_report("%s not able to extract info from %s",
+                             __func__, tmpnam);
+                exit(1);
             }
 
             if (strlen(parent_node) > 0) {
@@ -206,9 +209,9 @@ void *load_device_tree_from_sysfs(void)
     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__);
+        error_report("%s host device tree extracted into memory is invalid",
+                     __func__);
+        exit(1);
     }
     return host_fdt;
 }
-- 
2.18.0.rc2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
  2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
@ 2018-06-21 13:02   ` Auger Eric
  2018-06-25 16:11     ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
  0 siblings, 1 reply; 6+ messages in thread
From: Auger Eric @ 2018-06-21 13:02 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Markus Armbruster
  Cc: Peter Maydell, qemu-arm, qemu-devel

Hi Philippe,
On 06/21/2018 01:40 PM, Philippe Mathieu-Daudé wrote:
> Use error_report() + exit() instead of error_setg(&error_fatal),
> as suggested by the "qapi/error.h" documentation:
> 
>    Please don't error_setg(&error_fatal, ...), use error_report() and
>    exit(), because that's more obvious.
> 
> This fixes CID 1352173:
>     "Passing null pointer dt_name to qemu_fdt_node_path, which dereferences it."
> 
> And this also fixes:
> 
>     hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>         if (node_path[1]) {
>             ^~~~~~~~~~~~
> 
> Fixes: Coverity CID 1352173 (Dereference after null check)
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 277ed872e7..63b74a353e 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -92,7 +92,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
>          r = qemu_fdt_getprop(host_fdt, node_path,
>                               props[i].name,
>                               &prop_len,
> -                             props[i].optional ? &err : &error_fatal);
> +                             &err);
>          if (r) {
>              qemu_fdt_setprop(guest_fdt, nodename,
>                               props[i].name, r, prop_len);
> @@ -103,6 +103,10 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
>              } else {
>                  error_free(err);
>              }
> +            if (!props[i].optional) {
> +                /* mandatory property not found: bail out */
> +                exit(1);
reading that code again, I am not sure this does the same thing as
before. In case case the property is tagged not optional and is not
found in the host dt, qemu_fdt_getprop will fill err with error_setg.
However prop_len == -FDT_ERR_NOTFOUND so we are going to call error_free
and no trace will be output to the user. Before the
error_setg(&error_fatal) printed the cause of error error and exited. Do
I miss something?

Thanks

Eric
> +            }
>          }
>      }
>  }
> @@ -138,9 +142,9 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>  
>      node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>      if (node_offset <= 0) {
> -        error_setg(&error_fatal,
> -                   "not able to locate clock handle %d in host device tree",
> -                   host_phandle);
> +        error_report("not able to locate clock handle %d in host device tree",
> +                     host_phandle);
> +        exit(1);
>      }
>      node_path = g_malloc(path_len);
>      while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
> @@ -149,16 +153,16 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>          node_path = g_realloc(node_path, path_len);
>      }
>      if (ret < 0) {
> -        error_setg(&error_fatal,
> -                   "not able to retrieve node path for clock handle %d",
> -                   host_phandle);
> +        error_report("not able to retrieve node path for clock handle %d",
> +                     host_phandle);
> +        exit(1);
>      }
>  
>      r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
>                           &error_fatal);
>      if (strcmp(r, "fixed-clock")) {
> -        error_setg(&error_fatal,
> -                   "clock handle %d is not a fixed clock", host_phandle);
> +        error_report("clock handle %d is not a fixed clock", host_phandle);
> +        exit(1);
>      }
>  
>      nodename = strrchr(node_path, '/');
> @@ -301,34 +305,37 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>  
>      dt_name = sysfs_to_dt_name(vbasedev->name);
>      if (!dt_name) {
> -        error_setg(&error_fatal, "%s incorrect sysfs device name %s",
> -                    __func__, vbasedev->name);
> +        error_report("%s incorrect sysfs device name %s",
> +                     __func__, vbasedev->name);
> +        exit(1);
>      }
>      node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
>                                     &error_fatal);
>      if (!node_path || !node_path[0]) {
> -        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
> -                   __func__, dt_name, vdev->compat);
> +        error_report("%s unable to retrieve node path for %s/%s",
> +                     __func__, dt_name, vdev->compat);
> +        exit(1);
>      }
>  
>      if (node_path[1]) {
> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
> -                   __func__, dt_name, vdev->compat);
> +        error_report("%s more than one node matching %s/%s!",
> +                     __func__, dt_name, vdev->compat);
> +        exit(1);
>      }
>  
>      g_free(dt_name);
>  
>      if (vbasedev->num_regions != 5) {
> -        error_setg(&error_fatal, "%s Does the host dt node combine XGBE/PHY?",
> -                   __func__);
> +        error_report("%s Does the host dt node combine XGBE/PHY?", __func__);
> +        exit(1);
>      }
>  
>      /* generate nodes for DMA_CLK and PTP_CLK */
>      r = qemu_fdt_getprop(host_fdt, node_path[0], "clocks",
>                           &prop_len, &error_fatal);
>      if (prop_len != 8) {
> -        error_setg(&error_fatal, "%s clocks property should contain 2 handles",
> -                   __func__);
> +        error_report("%s clocks property should contain 2 handles", __func__);
> +        exit(1);
>      }
>      host_clock_phandles = (uint32_t *)r;
>      guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [Qemu-arm] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit()
  2018-06-21 13:02   ` Auger Eric
@ 2018-06-25 16:11     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-06-25 16:11 UTC (permalink / raw)
  To: Auger Eric, Markus Armbruster; +Cc: Peter Maydell, qemu-arm, qemu-devel

Hi Eric,

On 06/21/2018 10:02 AM, Auger Eric wrote:
> Hi Philippe,
> On 06/21/2018 01:40 PM, Philippe Mathieu-Daudé wrote:
>> Use error_report() + exit() instead of error_setg(&error_fatal),
>> as suggested by the "qapi/error.h" documentation:
>>
>>    Please don't error_setg(&error_fatal, ...), use error_report() and
>>    exit(), because that's more obvious.
>>
>> This fixes CID 1352173:
>>     "Passing null pointer dt_name to qemu_fdt_node_path, which dereferences it."
>>
>> And this also fixes:
>>
>>     hw/arm/sysbus-fdt.c:322:9: warning: Array access (from variable 'node_path') results in a null pointer dereference
>>         if (node_path[1]) {
>>             ^~~~~~~~~~~~
>>
>> Fixes: Coverity CID 1352173 (Dereference after null check)
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/arm/sysbus-fdt.c | 45 ++++++++++++++++++++++++++-------------------
>>  1 file changed, 26 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
>> index 277ed872e7..63b74a353e 100644
>> --- a/hw/arm/sysbus-fdt.c
>> +++ b/hw/arm/sysbus-fdt.c
>> @@ -92,7 +92,7 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
>>          r = qemu_fdt_getprop(host_fdt, node_path,
>>                               props[i].name,
>>                               &prop_len,
>> -                             props[i].optional ? &err : &error_fatal);
>> +                             &err);
>>          if (r) {
>>              qemu_fdt_setprop(guest_fdt, nodename,
>>                               props[i].name, r, prop_len);
>> @@ -103,6 +103,10 @@ static void copy_properties_from_host(HostProperty *props, int nb_props,
>>              } else {
>>                  error_free(err);
>>              }
>> +            if (!props[i].optional) {
>> +                /* mandatory property not found: bail out */
>> +                exit(1);
> reading that code again, I am not sure this does the same thing as
> before. In case case the property is tagged not optional and is not
> found in the host dt, qemu_fdt_getprop will fill err with error_setg.
> However prop_len == -FDT_ERR_NOTFOUND so we are going to call error_free
> and no trace will be output to the user. Before the
> error_setg(&error_fatal) printed the cause of error error and exited. Do
> I miss something?

You are correct something is wrong here, I'll respin.

Thanks for your review!

> 
> Thanks
> 
> Eric
>> +            }
>>          }
>>      }
>>  }
>> @@ -138,9 +142,9 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>>  
>>      node_offset = fdt_node_offset_by_phandle(host_fdt, host_phandle);
>>      if (node_offset <= 0) {
>> -        error_setg(&error_fatal,
>> -                   "not able to locate clock handle %d in host device tree",
>> -                   host_phandle);
>> +        error_report("not able to locate clock handle %d in host device tree",
>> +                     host_phandle);
>> +        exit(1);
>>      }
>>      node_path = g_malloc(path_len);
>>      while ((ret = fdt_get_path(host_fdt, node_offset, node_path, path_len))
>> @@ -149,16 +153,16 @@ static void fdt_build_clock_node(void *host_fdt, void *guest_fdt,
>>          node_path = g_realloc(node_path, path_len);
>>      }
>>      if (ret < 0) {
>> -        error_setg(&error_fatal,
>> -                   "not able to retrieve node path for clock handle %d",
>> -                   host_phandle);
>> +        error_report("not able to retrieve node path for clock handle %d",
>> +                     host_phandle);
>> +        exit(1);
>>      }
>>  
>>      r = qemu_fdt_getprop(host_fdt, node_path, "compatible", &prop_len,
>>                           &error_fatal);
>>      if (strcmp(r, "fixed-clock")) {
>> -        error_setg(&error_fatal,
>> -                   "clock handle %d is not a fixed clock", host_phandle);
>> +        error_report("clock handle %d is not a fixed clock", host_phandle);
>> +        exit(1);
>>      }
>>  
>>      nodename = strrchr(node_path, '/');
>> @@ -301,34 +305,37 @@ static int add_amd_xgbe_fdt_node(SysBusDevice *sbdev, void *opaque)
>>  
>>      dt_name = sysfs_to_dt_name(vbasedev->name);
>>      if (!dt_name) {
>> -        error_setg(&error_fatal, "%s incorrect sysfs device name %s",
>> -                    __func__, vbasedev->name);
>> +        error_report("%s incorrect sysfs device name %s",
>> +                     __func__, vbasedev->name);
>> +        exit(1);
>>      }
>>      node_path = qemu_fdt_node_path(host_fdt, dt_name, vdev->compat,
>>                                     &error_fatal);
>>      if (!node_path || !node_path[0]) {
>> -        error_setg(&error_fatal, "%s unable to retrieve node path for %s/%s",
>> -                   __func__, dt_name, vdev->compat);
>> +        error_report("%s unable to retrieve node path for %s/%s",
>> +                     __func__, dt_name, vdev->compat);
>> +        exit(1);
>>      }
>>  
>>      if (node_path[1]) {
>> -        error_setg(&error_fatal, "%s more than one node matching %s/%s!",
>> -                   __func__, dt_name, vdev->compat);
>> +        error_report("%s more than one node matching %s/%s!",
>> +                     __func__, dt_name, vdev->compat);
>> +        exit(1);
>>      }
>>  
>>      g_free(dt_name);
>>  
>>      if (vbasedev->num_regions != 5) {
>> -        error_setg(&error_fatal, "%s Does the host dt node combine XGBE/PHY?",
>> -                   __func__);
>> +        error_report("%s Does the host dt node combine XGBE/PHY?", __func__);
>> +        exit(1);
>>      }
>>  
>>      /* generate nodes for DMA_CLK and PTP_CLK */
>>      r = qemu_fdt_getprop(host_fdt, node_path[0], "clocks",
>>                           &prop_len, &error_fatal);
>>      if (prop_len != 8) {
>> -        error_setg(&error_fatal, "%s clocks property should contain 2 handles",
>> -                   __func__);
>> +        error_report("%s clocks property should contain 2 handles", __func__);
>> +        exit(1);
>>      }
>>      host_clock_phandles = (uint32_t *)r;
>>      guest_clock_phandles[0] = qemu_fdt_alloc_phandle(guest_fdt);
>>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-06-25 16:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-21 11:40 [Qemu-devel] [PATCH v3 0/3] qapi/error: converts error_setg(&error_fatal) to error_report() + exit() Philippe Mathieu-Daudé
2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 1/3] hw/block/fdc: Replace error_setg(&error_abort) by assert() Philippe Mathieu-Daudé
2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 2/3] hw/arm/sysbus-fdt: Replace error_setg(&error_fatal) by error_report() + exit() Philippe Mathieu-Daudé
2018-06-21 13:02   ` Auger Eric
2018-06-25 16:11     ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-06-21 11:40 ` [Qemu-devel] [PATCH v3 3/3] device_tree: " Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).