[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 1/2] qbus_find_recursive(): don't abort search for
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [RFC 1/2] qbus_find_recursive(): don't abort search for named bus on full bus node |
Date: |
Thu, 31 Jan 2013 16:03:21 +0000 |
On 31 January 2013 15:58, KONRAD Frédéric <address@hidden> wrote:
> On 31/01/2013 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. On the other hand, if the user passed
>> us a specific name and this bus isn't that named bus then we shouldn't
>> be checking the max_index at all.
>>
>> So I think the right fix is that the condition should be
>> if (match && (bus_class->max_dev != 0)
>> && (bus_class->max_dev <= bus->max_index)) {
>>
>> and the rest of the code in this function stays as is.
>>
>> -- PMM
>
> The final result looks the same no?
>
> If you look the qdev-monitor.c code just above the patch, you'll find the
> same thing:
>
> if (name && (strcmp(bus->name, name) != 0)) {
> match = 0;
> }
> if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
>
> match = 0;
> }
> if ((bus_class->max_dev != 0) && (bus_class->max_dev <= bus->max_index))
> {
> ...
You appear to be assuming this is an if..else if...else if
ladder -- it is not. Even if one of the first two if()s sets
match == 0, if we find the bus is at its max index we will
incorrectly go into the qerror_report case if name is not NULL.
(The other fix for this would be to change it to "if .. else if
.. else if", of course.)
-- PMM
Re: [Qemu-devel] traversal termination in qbus_find_recursive() due to "max_dev", Laszlo Ersek, 2013/01/31
[Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Laszlo Ersek, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Peter Maydell, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Laszlo Ersek, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Peter Maydell, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Laszlo Ersek, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Peter Maydell, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Laszlo Ersek, 2013/01/31
- Re: [Qemu-devel] [RFC 2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error, Peter Maydell, 2013/01/31