Re: [Qemu-devel] [PATCH 5/7] qbus_find_recursive(): terminate search by name in case of fatal error

[Laszlo Ersek]

On 02/04/13 18:46, Luiz Capitulino wrote:
> On Fri,  1 Feb 2013 18:38:17 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> Use an Error to communicate the "stop the search" message.
>>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  hw/qdev-monitor.c |   24 ++++++++++++++++--------
>>  1 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index 284dafa..83540fc 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -288,7 +288,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char 
>> *elem)
>>  }
>>  
>>  static BusState *qbus_find_recursive(BusState *bus, const char *name,
>> -                                     const char *type)
>> +                                     const char *type, Error **errp)
>>  {
>>      BusChild *kid;
>>  
>> @@ -303,8 +303,7 @@ static BusState *qbus_find_recursive(BusState *bus, 
>> const char *name,
>>  
>>          /* bus is full -- fatal error for search by name */
>>          if (name != NULL) {
>> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
>> -                          bus->name);
>> +            error_setg(errp, "Bus '%s' is full", bus->name);
>>              return NULL;
>>          }
>>      }
>> @@ -316,8 +315,8 @@ static BusState *qbus_find_recursive(BusState *bus, 
>> const char *name,
>>          QLIST_FOREACH(child, &dev->child_bus, sibling) {
>>              BusState *ret;
>>  
>> -            ret = qbus_find_recursive(child, name, type);
>> -            if (ret) {
>> +            ret = qbus_find_recursive(child, name, type, errp);
>> +            if (ret || error_is_set(errp)) {
> 
> Does this mean ret is returned unconditionally? We should have only
> one way to check for error.

qbus_find_recursive()
- is recursive,
- returns a bus if the function found an appropriate node (= bus),
- returns NULL without setting an error if the examined sub-tree didn't
contain an appropriate node (= bus), ie. traversal should continue with
siblings of the sub-tree,
- returns NULL and sets an Error if traversal should be aborted (we can
see for sure a good bus will never be found).


> 
>>                  return ret;
>>              }
>>          }
>> @@ -337,13 +336,20 @@ static BusState *qbus_find(const char *path)
>>          bus = sysbus_get_default();
>>          pos = 0;
>>      } else {
>> +        Error *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);
>> +        bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
>>          if (!bus) {
>> -            qerror_report(QERR_BUS_NOT_FOUND, elem);
>> +            if (error_is_set(&err)) {
>> +                qerror_report_err(err);
>> +                error_free(err);
>> +            } else {
>> +                qerror_report(QERR_BUS_NOT_FOUND, elem);
>> +            }
> 
> I'll take it that this last qerror_report() call is an error generated
> by qbus_find(). That is, qbus_find_recursive() _can_ return bus=NULL
> on success.

The interface contract of qbus_find_recursive() is a mess. Refer to what
I wrote above, and consider what it means for the top level
qbus_find_recursive() call: NULL without Error and NULL *with* Error are
both failures.

In the former case (NULL without Error), we traversed the entire tree
and didn't find a good bus, but we kept looking until the very end.

In the latter case (NULL with Error), we found the bus with the
requested name (which is unique), but its properties make it unusable
(it's full, no more devices can be added). In this case we abort the
search immediately.

Now since it's a recursive function, qbus_find_recursive() must
distinguish (a) "found", (b) "keep going", (c) "abort". I could have
introduced a separate flag for "abort", either as an in-out parameter or
by turning the retval into a structure, but adding errp already covered
the "new flag" approach.

Semantically it's clean I think; "abort" corresponds to "Error
determined during traversal".

Here in qbus_find() we forward any error that was encountered during
traversal. If we simply ran out of busses / nodes to check without a
traversal error, we wrap that fact into another error.

> 
>>              return NULL;
>>          }
>>          pos = len;
>> @@ -458,8 +464,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>              return NULL;
>>          }
>>      } else {
>> -        bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type);
>> +        bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type,
>> +                                  &local_err);
>>          if (!bus) {
>> +            assert(!error_is_set(&local_err));
>>              qerror_report(QERR_NO_BUS_FOR_DEVICE,
>>                            k->bus_type, driver);
>>              return NULL;
> 

Laszlo