Patchwork [RFC,1/2] qbus_find_recursive(): don't abort search for named bus on full bus node

login
register
mail settings
Submitter Laszlo Ersek
Date Jan. 31, 2013, 3:42 p.m.
Message ID <1359646945-2876-2-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/217202/
State New
Headers show

Comments

Laszlo Ersek - Jan. 31, 2013, 3:42 p.m.
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 <lersek@redhat.com>
---
 hw/qdev-monitor.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)
Peter Maydell - Jan. 31, 2013, 3:52 p.m.
On 31 January 2013 15:42, Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
> ---
>  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
fred.konrad@greensocs.com - Jan. 31, 2013, 3:58 p.m.
On 31/01/2013 16:52, Peter Maydell wrote:
> On 31 January 2013 15:42, Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
>> ---
>>   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)) {
         ...

Fred
Peter Maydell - Jan. 31, 2013, 4:03 p.m.
On 31 January 2013 15:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 31/01/2013 16:52, Peter Maydell wrote:
>>
>> On 31 January 2013 15:42, Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
>>> ---
>>>   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
Laszlo Ersek - Jan. 31, 2013, 4:05 p.m.
On 01/31/13 16:52, Peter Maydell wrote:
> On 31 January 2013 15:42, Laszlo Ersek <lersek@redhat.com> 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 <lersek@redhat.com>
>> ---
>>  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
Peter Maydell - Jan. 31, 2013, 4:09 p.m.
On 31 January 2013 16:05, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/31/13 16:52, Peter Maydell wrote:
>> 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.

Then that's a bug in the caller -- it should actually stop on
error, not plough ahead. [that is, we need to distinguish
"not found" from "found and it won't work" from "found".]

> 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).

I think refactorings that make the code make more sense are
entirely reasonable.

-- PMM
Laszlo Ersek - Jan. 31, 2013, 4:26 p.m.
On 01/31/13 17:09, Peter Maydell wrote:
> On 31 January 2013 16:05, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 01/31/13 16:52, Peter Maydell wrote:
>>> 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.
> 
> Then that's a bug in the caller

The caller is the same function; it's recursive.

> -- it should actually stop on
> error, not plough ahead. [that is, we need to distinguish
> "not found" from "found and it won't work" from "found".]

Agreed.

Is uniqueness of bus names enforced somewhere?

Thanks!
Laszlo

Patch

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;
     }
     if (match) {
         return bus;