Re: [Qemu-devel] [RFC 1/2] qbus_find_recursive(): don't abort search for named bus on full bus node

[Laszlo Ersek]

On 01/31/13 16:52, Peter Maydell wrote:
> On 31 January 2013 15:42, Laszlo Ersek <address@hidden> wrote:
>> The bus we're looking for could be in the sub-tree rooted at the node
>> being checked; don't skip looping over the children.
>>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  hw/qdev-monitor.c |   10 +--------
>>  1 files changed, 1 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index 4e2a92b..34f5014 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -295,15 +295,7 @@ static BusState *qbus_find_recursive(BusState *bus, 
>> const char *name,
>>          match = 0;
>>      }
>>      if ((bus_class->max_dev != 0) && (bus_class->max_dev <= 
>> bus->max_index)) {
>> -        if (name != NULL) {
>> -            /* bus was explicitly specified: return an error. */
>> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
>> -                          bus->name);
>> -            return NULL;
>> -        } else {
>> -            /* bus was not specified: try to find another one. */
>> -            match = 0;
>> -        }
>> +        match = 0;
>>      }
> 
> This looks like the wrong fix to this problem -- if the user passed
> us a specific name to search for and we found it and it was full, then
> we definitely want to stop here.

You only skip the children, but not the siblings. When you return NULL
here, the sibling loop one stack frame higher up continues anyway.

This function deserves more intrusive changes, but that's usually
invitation for more bikeshedding, hence I wanted to avoid it. (For
example, before commit 1395af6f there was no reason to set "match" to
zero in two independent "if"s, then check "match" in a third "if".) I
think that a rewrite from scratch would be justified (and frowned upon).

Thanks
Laszlo