Message ID | 1359740299-27968-6-git-send-email-lersek@redhat.com |
---|---|
State | New |
Headers | show |
On Fri, 1 Feb 2013 18:38:17 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > Use an Error to communicate the "stop the search" message. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > hw/qdev-monitor.c | 24 ++++++++++++++++-------- > 1 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c > index 284dafa..83540fc 100644 > --- a/hw/qdev-monitor.c > +++ b/hw/qdev-monitor.c > @@ -288,7 +288,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem) > } > > static BusState *qbus_find_recursive(BusState *bus, const char *name, > - const char *type) > + const char *type, Error **errp) > { > BusChild *kid; > > @@ -303,8 +303,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name, > > /* bus is full -- fatal error for search by name */ > if (name != NULL) { > - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full", > - bus->name); > + error_setg(errp, "Bus '%s' is full", bus->name); > return NULL; > } > } > @@ -316,8 +315,8 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name, > QLIST_FOREACH(child, &dev->child_bus, sibling) { > BusState *ret; > > - ret = qbus_find_recursive(child, name, type); > - if (ret) { > + ret = qbus_find_recursive(child, name, type, errp); > + if (ret || error_is_set(errp)) { Does this mean ret is returned unconditionally? We should have only one way to check for error. > return ret; > } > } > @@ -337,13 +336,20 @@ static BusState *qbus_find(const char *path) > bus = sysbus_get_default(); > pos = 0; > } else { > + Error *err = NULL; > + > if (sscanf(path, "%127[^/]%n", elem, &len) != 1) { > assert(!path[0]); > elem[0] = len = 0; > } > - bus = qbus_find_recursive(sysbus_get_default(), elem, NULL); > + bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err); > if (!bus) { > - qerror_report(QERR_BUS_NOT_FOUND, elem); > + if (error_is_set(&err)) { > + qerror_report_err(err); > + error_free(err); > + } else { > + qerror_report(QERR_BUS_NOT_FOUND, elem); > + } I'll take it that this last qerror_report() call is an error generated by qbus_find(). That is, qbus_find_recursive() _can_ return bus=NULL on success. > return NULL; > } > pos = len; > @@ -458,8 +464,10 @@ DeviceState *qdev_device_add(QemuOpts *opts) > return NULL; > } > } else { > - bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type); > + bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type, > + &local_err); > if (!bus) { > + assert(!error_is_set(&local_err)); > qerror_report(QERR_NO_BUS_FOR_DEVICE, > k->bus_type, driver); > return NULL;
On 02/04/13 18:46, Luiz Capitulino wrote: > On Fri, 1 Feb 2013 18:38:17 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> Use an Error to communicate the "stop the search" message. >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> hw/qdev-monitor.c | 24 ++++++++++++++++-------- >> 1 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c >> index 284dafa..83540fc 100644 >> --- a/hw/qdev-monitor.c >> +++ b/hw/qdev-monitor.c >> @@ -288,7 +288,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem) >> } >> >> static BusState *qbus_find_recursive(BusState *bus, const char *name, >> - const char *type) >> + const char *type, Error **errp) >> { >> BusChild *kid; >> >> @@ -303,8 +303,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name, >> >> /* bus is full -- fatal error for search by name */ >> if (name != NULL) { >> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full", >> - bus->name); >> + error_setg(errp, "Bus '%s' is full", bus->name); >> return NULL; >> } >> } >> @@ -316,8 +315,8 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name, >> QLIST_FOREACH(child, &dev->child_bus, sibling) { >> BusState *ret; >> >> - ret = qbus_find_recursive(child, name, type); >> - if (ret) { >> + ret = qbus_find_recursive(child, name, type, errp); >> + if (ret || error_is_set(errp)) { > > Does this mean ret is returned unconditionally? We should have only > one way to check for error. qbus_find_recursive() - is recursive, - returns a bus if the function found an appropriate node (= bus), - returns NULL without setting an error if the examined sub-tree didn't contain an appropriate node (= bus), ie. traversal should continue with siblings of the sub-tree, - returns NULL and sets an Error if traversal should be aborted (we can see for sure a good bus will never be found). > >> return ret; >> } >> } >> @@ -337,13 +336,20 @@ static BusState *qbus_find(const char *path) >> bus = sysbus_get_default(); >> pos = 0; >> } else { >> + Error *err = NULL; >> + >> if (sscanf(path, "%127[^/]%n", elem, &len) != 1) { >> assert(!path[0]); >> elem[0] = len = 0; >> } >> - bus = qbus_find_recursive(sysbus_get_default(), elem, NULL); >> + bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err); >> if (!bus) { >> - qerror_report(QERR_BUS_NOT_FOUND, elem); >> + if (error_is_set(&err)) { >> + qerror_report_err(err); >> + error_free(err); >> + } else { >> + qerror_report(QERR_BUS_NOT_FOUND, elem); >> + } > > I'll take it that this last qerror_report() call is an error generated > by qbus_find(). That is, qbus_find_recursive() _can_ return bus=NULL > on success. The interface contract of qbus_find_recursive() is a mess. Refer to what I wrote above, and consider what it means for the top level qbus_find_recursive() call: NULL without Error and NULL *with* Error are both failures. In the former case (NULL without Error), we traversed the entire tree and didn't find a good bus, but we kept looking until the very end. In the latter case (NULL with Error), we found the bus with the requested name (which is unique), but its properties make it unusable (it's full, no more devices can be added). In this case we abort the search immediately. Now since it's a recursive function, qbus_find_recursive() must distinguish (a) "found", (b) "keep going", (c) "abort". I could have introduced a separate flag for "abort", either as an in-out parameter or by turning the retval into a structure, but adding errp already covered the "new flag" approach. Semantically it's clean I think; "abort" corresponds to "Error determined during traversal". Here in qbus_find() we forward any error that was encountered during traversal. If we simply ran out of busses / nodes to check without a traversal error, we wrap that fact into another error. > >> return NULL; >> } >> pos = len; >> @@ -458,8 +464,10 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> return NULL; >> } >> } else { >> - bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type); >> + bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type, >> + &local_err); >> if (!bus) { >> + assert(!error_is_set(&local_err)); >> qerror_report(QERR_NO_BUS_FOR_DEVICE, >> k->bus_type, driver); >> return NULL; > Laszlo
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index 284dafa..83540fc 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -288,7 +288,7 @@ static DeviceState *qbus_find_dev(BusState *bus, char *elem) } static BusState *qbus_find_recursive(BusState *bus, const char *name, - const char *type) + const char *type, Error **errp) { BusChild *kid; @@ -303,8 +303,7 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name, /* bus is full -- fatal error for search by name */ if (name != NULL) { - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full", - bus->name); + error_setg(errp, "Bus '%s' is full", bus->name); return NULL; } } @@ -316,8 +315,8 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name, QLIST_FOREACH(child, &dev->child_bus, sibling) { BusState *ret; - ret = qbus_find_recursive(child, name, type); - if (ret) { + ret = qbus_find_recursive(child, name, type, errp); + if (ret || error_is_set(errp)) { return ret; } } @@ -337,13 +336,20 @@ static BusState *qbus_find(const char *path) bus = sysbus_get_default(); pos = 0; } else { + Error *err = NULL; + if (sscanf(path, "%127[^/]%n", elem, &len) != 1) { assert(!path[0]); elem[0] = len = 0; } - bus = qbus_find_recursive(sysbus_get_default(), elem, NULL); + bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err); if (!bus) { - qerror_report(QERR_BUS_NOT_FOUND, elem); + if (error_is_set(&err)) { + qerror_report_err(err); + error_free(err); + } else { + qerror_report(QERR_BUS_NOT_FOUND, elem); + } return NULL; } pos = len; @@ -458,8 +464,10 @@ DeviceState *qdev_device_add(QemuOpts *opts) return NULL; } } else { - bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type); + bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type, + &local_err); if (!bus) { + assert(!error_is_set(&local_err)); qerror_report(QERR_NO_BUS_FOR_DEVICE, k->bus_type, driver); return NULL;
Use an Error to communicate the "stop the search" message. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- hw/qdev-monitor.c | 24 ++++++++++++++++-------- 1 files changed, 16 insertions(+), 8 deletions(-)