Patchwork [6/7] qbus_find(): report errors via Error

login
register
mail settings
Submitter Laszlo Ersek
Date Feb. 1, 2013, 5:38 p.m.
Message ID <1359740299-27968-7-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/217558/
State New
Headers show

Comments

Laszlo Ersek - Feb. 1, 2013, 5:38 p.m.
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/qdev-monitor.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)
Luiz Capitulino - Feb. 4, 2013, 5:51 p.m.
On Fri,  1 Feb 2013 18:38:18 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Splitting this as I suggested in the other patch would make review
easier. I honestly got a bit lost while reviewing this one.

> ---
>  hw/qdev-monitor.c |   29 +++++++++++++++--------------
>  1 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 83540fc..0c01b04 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -324,7 +324,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>      return NULL;
>  }
>  
> -static BusState *qbus_find(const char *path)
> +static BusState *qbus_find(const char *path, Error **errp)
>  {
>      DeviceState *dev;
>      BusState *bus;
> @@ -336,20 +336,19 @@ static BusState *qbus_find(const char *path)
>          bus = sysbus_get_default();
>          pos = 0;
>      } else {
> -        Error *err = NULL;
> +        Error *find_err = NULL;
>  
>          if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
>              assert(!path[0]);
>              elem[0] = len = 0;
>          }
> -        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
> +        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &find_err);
>          if (!bus) {
> -            if (error_is_set(&err)) {
> -                qerror_report_err(err);
> -                error_free(err);
> -            } else {
> -                qerror_report(QERR_BUS_NOT_FOUND, elem);
> -            }
> +            Error *not_found;
> +
> +            error_set(&not_found, QERR_BUS_NOT_FOUND, elem);
> +            error_propagate(errp, find_err);
> +            error_propagate(errp, not_found);
>              return NULL;
>          }
>          pos = len;
> @@ -372,7 +371,7 @@ static BusState *qbus_find(const char *path)
>          pos += len;
>          dev = qbus_find_dev(bus, elem);
>          if (!dev) {
> -            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
> +            error_set(errp, QERR_DEVICE_NOT_FOUND, elem);
>              if (!monitor_cur_is_qmp()) {
>                  qbus_list_dev(bus);
>              }
> @@ -388,12 +387,12 @@ static BusState *qbus_find(const char *path)
>               * one child bus accept it nevertheless */
>              switch (dev->num_child_bus) {
>              case 0:
> -                qerror_report(QERR_DEVICE_NO_BUS, elem);
> +                error_set(errp, QERR_DEVICE_NO_BUS, elem);
>                  return NULL;
>              case 1:
>                  return QLIST_FIRST(&dev->child_bus);
>              default:
> -                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
> +                error_set(errp, QERR_DEVICE_MULTIPLE_BUSSES, elem);
>                  if (!monitor_cur_is_qmp()) {
>                      qbus_list_bus(dev);
>                  }
> @@ -409,7 +408,7 @@ static BusState *qbus_find(const char *path)
>          pos += len;
>          bus = qbus_find_bus(dev, elem);
>          if (!bus) {
> -            qerror_report(QERR_BUS_NOT_FOUND, elem);
> +            error_set(errp, QERR_BUS_NOT_FOUND, elem);
>              if (!monitor_cur_is_qmp()) {
>                  qbus_list_bus(dev);
>              }
> @@ -454,8 +453,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      /* find bus */
>      path = qemu_opt_get(opts, "bus");
>      if (path != NULL) {
> -        bus = qbus_find(path);
> +        bus = qbus_find(path, &local_err);
>          if (!bus) {
> +            qerror_report_err(local_err);
> +            error_free(local_err);
>              return NULL;
>          }
>          if (!object_dynamic_cast(OBJECT(bus), k->bus_type)) {
Laszlo Ersek - Feb. 4, 2013, 6:23 p.m.
On 02/04/13 18:51, Luiz Capitulino wrote:
> On Fri,  1 Feb 2013 18:38:18 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Splitting this as I suggested in the other patch would make review
> easier. I honestly got a bit lost while reviewing this one.

I think you cannot review this series without applying each patch in
lock-step with the review, and contrasting the next patch with the
just-updated tree. In that light it's especially regrettable that my
series didn't apply to master (it certainly did when I posted it).

Anyway the approach I took was:
- determine the set of functions to modify,
- work in a bottom-up fashion,
- when changing a function signature, change all callers.

Suppose there are three functions, X, Y, Z, with calls like X->Y, X->Z.

#1 First I'd fix Y (signature change) and adapt its call site in X --
acccept the error and print it. At this point X consumes a few errors,
and some other errors don't reach it at all.

#2 Then I'd fix Z (signature change) and adapt its call site in X --
accept the error and print it. At this point Y and Z are OK, and X has
several places that accept and consume (print/free) errors.

#3 In the next patch, X's signature is changed, all error consumption /
printing sites in X are changed to propagate / generate instead, and all
call sites or X are updated.

The only patch I could split is #3, but then X would in part consume, in
part propagate errors. Do you suggest I do that?

Thanks!
Laszlo

> 
>> ---
>>  hw/qdev-monitor.c |   29 +++++++++++++++--------------
>>  1 files changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index 83540fc..0c01b04 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -324,7 +324,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
>>      return NULL;
>>  }
>>  
>> -static BusState *qbus_find(const char *path)
>> +static BusState *qbus_find(const char *path, Error **errp)
>>  {
>>      DeviceState *dev;
>>      BusState *bus;
>> @@ -336,20 +336,19 @@ static BusState *qbus_find(const char *path)
>>          bus = sysbus_get_default();
>>          pos = 0;
>>      } else {
>> -        Error *err = NULL;
>> +        Error *find_err = NULL;
>>  
>>          if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
>>              assert(!path[0]);
>>              elem[0] = len = 0;
>>          }
>> -        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
>> +        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &find_err);
>>          if (!bus) {
>> -            if (error_is_set(&err)) {
>> -                qerror_report_err(err);
>> -                error_free(err);
>> -            } else {
>> -                qerror_report(QERR_BUS_NOT_FOUND, elem);
>> -            }
>> +            Error *not_found;
>> +
>> +            error_set(&not_found, QERR_BUS_NOT_FOUND, elem);
>> +            error_propagate(errp, find_err);
>> +            error_propagate(errp, not_found);
>>              return NULL;
>>          }
>>          pos = len;
>> @@ -372,7 +371,7 @@ static BusState *qbus_find(const char *path)
>>          pos += len;
>>          dev = qbus_find_dev(bus, elem);
>>          if (!dev) {
>> -            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
>> +            error_set(errp, QERR_DEVICE_NOT_FOUND, elem);
>>              if (!monitor_cur_is_qmp()) {
>>                  qbus_list_dev(bus);
>>              }
>> @@ -388,12 +387,12 @@ static BusState *qbus_find(const char *path)
>>               * one child bus accept it nevertheless */
>>              switch (dev->num_child_bus) {
>>              case 0:
>> -                qerror_report(QERR_DEVICE_NO_BUS, elem);
>> +                error_set(errp, QERR_DEVICE_NO_BUS, elem);
>>                  return NULL;
>>              case 1:
>>                  return QLIST_FIRST(&dev->child_bus);
>>              default:
>> -                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
>> +                error_set(errp, QERR_DEVICE_MULTIPLE_BUSSES, elem);
>>                  if (!monitor_cur_is_qmp()) {
>>                      qbus_list_bus(dev);
>>                  }
>> @@ -409,7 +408,7 @@ static BusState *qbus_find(const char *path)
>>          pos += len;
>>          bus = qbus_find_bus(dev, elem);
>>          if (!bus) {
>> -            qerror_report(QERR_BUS_NOT_FOUND, elem);
>> +            error_set(errp, QERR_BUS_NOT_FOUND, elem);
>>              if (!monitor_cur_is_qmp()) {
>>                  qbus_list_bus(dev);
>>              }
>> @@ -454,8 +453,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>      /* find bus */
>>      path = qemu_opt_get(opts, "bus");
>>      if (path != NULL) {
>> -        bus = qbus_find(path);
>> +        bus = qbus_find(path, &local_err);
>>          if (!bus) {
>> +            qerror_report_err(local_err);
>> +            error_free(local_err);
>>              return NULL;
>>          }
>>          if (!object_dynamic_cast(OBJECT(bus), k->bus_type)) {
>
Luiz Capitulino - Feb. 4, 2013, 7:38 p.m.
On Mon, 04 Feb 2013 19:23:15 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 02/04/13 18:51, Luiz Capitulino wrote:
> > On Fri,  1 Feb 2013 18:38:18 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> >>
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > 
> > Splitting this as I suggested in the other patch would make review
> > easier. I honestly got a bit lost while reviewing this one.
> 
> I think you cannot review this series without applying each patch in
> lock-step with the review, and contrasting the next patch with the
> just-updated tree. In that light it's especially regrettable that my
> series didn't apply to master (it certainly did when I posted it).

I'll review it in more detail when you respin.

> 
> Anyway the approach I took was:
> - determine the set of functions to modify,
> - work in a bottom-up fashion,
> - when changing a function signature, change all callers.
> 
> Suppose there are three functions, X, Y, Z, with calls like X->Y, X->Z.
> 
> #1 First I'd fix Y (signature change) and adapt its call site in X --
> acccept the error and print it. At this point X consumes a few errors,
> and some other errors don't reach it at all.

May work for simple cases (although has the minor problem of mixing
signature change with adding new error handling to X), but doesn't scale
when Y is called by more than one function and their error handling differ.

> 
> #2 Then I'd fix Z (signature change) and adapt its call site in X --
> accept the error and print it. At this point Y and Z are OK, and X has
> several places that accept and consume (print/free) errors.
> 
> #3 In the next patch, X's signature is changed, all error consumption /
> printing sites in X are changed to propagate / generate instead, and all
> call sites or X are updated.
> 
> The only patch I could split is #3, but then X would in part consume, in
> part propagate errors. Do you suggest I do that?

The suggestion I gave in another email is to separate signature changes
from error handling changes.

Taking your example above, I'd have the following patches:

 1. Change Y's signature to take an Error ** argument and change
    X to pass NULL for the new argument

 2. Change Z's signature to take an Error ** argument and change
    X to pass NULL for the new argument

 3. Change X to pass an non-NULL Error ** argument to Y and Z and
    handle error accordingly

 4. If Y is non-void and if the return value is success/failure, drop it

 5. If Y is non-void and if the return value is success/failure, drop it

Now, you could merge patches 1 and 2 if the functions are only used
by X. On the other hand, if Y and/or Z are called by other functions,
you'd have to add more patches after 3 to add error handling to them.

I know this is more work, but it makes it easier to review.

Patch

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 83540fc..0c01b04 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -324,7 +324,7 @@  static BusState *qbus_find_recursive(BusState *bus, const char *name,
     return NULL;
 }
 
-static BusState *qbus_find(const char *path)
+static BusState *qbus_find(const char *path, Error **errp)
 {
     DeviceState *dev;
     BusState *bus;
@@ -336,20 +336,19 @@  static BusState *qbus_find(const char *path)
         bus = sysbus_get_default();
         pos = 0;
     } else {
-        Error *err = NULL;
+        Error *find_err = NULL;
 
         if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
             assert(!path[0]);
             elem[0] = len = 0;
         }
-        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
+        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &find_err);
         if (!bus) {
-            if (error_is_set(&err)) {
-                qerror_report_err(err);
-                error_free(err);
-            } else {
-                qerror_report(QERR_BUS_NOT_FOUND, elem);
-            }
+            Error *not_found;
+
+            error_set(&not_found, QERR_BUS_NOT_FOUND, elem);
+            error_propagate(errp, find_err);
+            error_propagate(errp, not_found);
             return NULL;
         }
         pos = len;
@@ -372,7 +371,7 @@  static BusState *qbus_find(const char *path)
         pos += len;
         dev = qbus_find_dev(bus, elem);
         if (!dev) {
-            qerror_report(QERR_DEVICE_NOT_FOUND, elem);
+            error_set(errp, QERR_DEVICE_NOT_FOUND, elem);
             if (!monitor_cur_is_qmp()) {
                 qbus_list_dev(bus);
             }
@@ -388,12 +387,12 @@  static BusState *qbus_find(const char *path)
              * one child bus accept it nevertheless */
             switch (dev->num_child_bus) {
             case 0:
-                qerror_report(QERR_DEVICE_NO_BUS, elem);
+                error_set(errp, QERR_DEVICE_NO_BUS, elem);
                 return NULL;
             case 1:
                 return QLIST_FIRST(&dev->child_bus);
             default:
-                qerror_report(QERR_DEVICE_MULTIPLE_BUSSES, elem);
+                error_set(errp, QERR_DEVICE_MULTIPLE_BUSSES, elem);
                 if (!monitor_cur_is_qmp()) {
                     qbus_list_bus(dev);
                 }
@@ -409,7 +408,7 @@  static BusState *qbus_find(const char *path)
         pos += len;
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
-            qerror_report(QERR_BUS_NOT_FOUND, elem);
+            error_set(errp, QERR_BUS_NOT_FOUND, elem);
             if (!monitor_cur_is_qmp()) {
                 qbus_list_bus(dev);
             }
@@ -454,8 +453,10 @@  DeviceState *qdev_device_add(QemuOpts *opts)
     /* find bus */
     path = qemu_opt_get(opts, "bus");
     if (path != NULL) {
-        bus = qbus_find(path);
+        bus = qbus_find(path, &local_err);
         if (!bus) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             return NULL;
         }
         if (!object_dynamic_cast(OBJECT(bus), k->bus_type)) {