On 31 January 2013 15:58, KONRAD Frédéric <fred.kon...@greensocs.com> wrote: > On 31/01/2013 16:52, Peter Maydell wrote: >> >> On 31 January 2013 15:42, Laszlo Ersek <ler...@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 <ler...@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