Re: [Qemu-devel] traversal termination in qbus_find_recursive() due to "max_dev"

[Laszlo Ersek]

On 01/31/13 14:49, KONRAD Frédéric wrote:
> On 31/01/2013 14:23, Laszlo Ersek wrote:
>> Hi,
>>
>> I'm working on propagating errors out to do_device_add(). I've come
>> accross qbus_find_recursive().
>>
>> Re commit 1395af6f ("qdev: add a maximum device allowed field for the
>> bus."), could someone please explain:
>>
>> (1) why the early termination due to max_index vs. max_dev merits an
>> error message -- both non-recursive call-sites print an error of their
>> own anyway,
>>
>> (2) why the
>>
>>    max_dev != 0 &&
>>    max_dev <= max_index &&
>>    name != NULL
>>
>> condition justifies traversal termination -- in general, a bus called
>> "foo", albeit full of devices, could allow some of those devices to be a
>> child bus. Let's call one such (direct) child bus "bar".
>>
>> If we're looking for "bar", or for one of its descendants, the current
>> logic seems to stop the search early, at "foo".
>>
>> I think "bus_class->max_dev" should be checked only when we're actually
>> trying to add the device to the bus. (In qdev_set_parent_bus() -->
>> bus_add_child().)
>>
>> - bus_add_child() should check the limit and set an Error,
>> - qdev_set_parent_bus_nofail() would abort() if there's an Error,
>> - qdev_set_parent_bus() would change signature and propagate error,
>> - all callers would be converted to either the new _nofail() or the old
>> function with the new signature.
>>
>> I can try to write the patch if there's consensus about what should
>> happen.
>>
>> Thoughts?
>>
>> Thanks!
>> Laszlo
> Hi,
> 
> I don't think I understand what is a traversal termination.

Early traversal termination is that you stop the recursive traversal of
the bus tree when you've discovered (traversed) only part of the tree.
The bus we're looking for could be in that part.

> But, this was added to specify a maximum amount of device on a bus.
> (for virtio-bus which can have only one device connected)

Yep, I suspected that much from the original series posting and "git
grep -E '\<max_dev\>'".

> 1 - When you explicitly set the bus in command line with bus=...and it
> is full
>       it returns the error:
>       qemu-system-i386: --device virtio-net,bus=virtio-pci-bus.0: Bus
> 'virtio-pci-bus.0' not found
>       That's why I added the error.

Indeed, this is a consequence of the fact that the max_dev checking
happens in a less-than-ideal place. qbus_find_recursive() promises to
look up a bus (starting at the specified root bus) that will have the
optionally requested name and the optionally requested type. If the
search fails, it's considered lookup error; which is why callers of
qbus_find_recursive() report QERR_BUS_NOT_FOUND and
QERR_NO_BUS_FOR_DEVICE.


> 2 - I understand what you mean, but does your idea allows us to connect
> a device to the first non-full bus?

I guess this refers to the name==NULL case (when we only set match=0). I
indeed haven't considered it until now.

We must distinguish between

(a) looking up a bus based on (root_bus, optional_name, optional_type),
and if such a bus is found, attempting to add a child device to it, and

(b) looking up a bus based on (root_bus, optional_name, optional_type,
free_child_slots).

If (b) is needed, that affects the error message. It must change from

    Bus 'virtio-pci-bus.0' not found

to

    No bus called 'virtio-pci-bus.0' with free slots was found

QERR_NO_BUS_FOR_DEVICE is unaffected: the corresponding call site passes
name=NULL to qbus_find_recursive(), hence the extra "bus is full" branch
never runs when called from there anyway.

QERR_BUS_NOT_FOUND is set in two places. Once after
qbus_find_recursive(), which carries the above (modified) meaning, and
at another time after qbus_find_bus(), which seems to be a flat search.

If the logic between these two is indeed different (ie. the first needs
to check for free slots, and the second *must not*), then we have to
split off a new QERR_BUS_WITH_SLOTS_NOT_FOUND from QERR_BUS_NOT_FOUND.

I'm posting a small series in this thread for illustration (I intend to
carry these in the bigger series I'm working on).

Thanks
Laszlo

Laszlo Ersek (2):
  qbus_find_recursive(): don't abort search for named bus on full bus
    node
  qbus_find_recursive(): the "free slots" constraint needs a dedicated
    error

 include/qapi/qmp/qerror.h |    3 +++
 hw/qdev-monitor.c         |   12 ++----------
 2 files changed, 5 insertions(+), 10 deletions(-)