Re: [Qemu-devel] [PATCH 6/7] qbus_find(): report errors via Error

[Laszlo Ersek]

On 02/04/13 18:51, Luiz Capitulino wrote:
> On Fri,  1 Feb 2013 18:38:18 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>>
>> Signed-off-by: Laszlo Ersek <address@hidden>
> 
> 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)) {
>