| Message ID | 1359646945-2876-2-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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;
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(-)