| Message ID | 1359646945-2876-3-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/31/13 16:54, Peter Maydell wrote: > On 31 January 2013 15:42, Laszlo Ersek <lersek@redhat.com> wrote: >> When searching for a bus by name, lookup failure means different things >> for qbus_find_bus() and qbus_find_recursive(); distinguish them by error >> codes as well. >> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> include/qapi/qmp/qerror.h | 3 +++ >> hw/qdev-monitor.c | 2 +- >> 2 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h >> index 6c0a18d..940ac21 100644 >> --- a/include/qapi/qmp/qerror.h >> +++ b/include/qapi/qmp/qerror.h >> @@ -69,6 +69,9 @@ void assert_no_error(Error *err); >> #define QERR_BUS_NOT_FOUND \ >> ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found" >> >> +#define QERR_BUS_WITH_SLOTS_NOT_FOUND \ >> + ERROR_CLASS_GENERIC_ERROR, "No bus called '%s' with free slots was found" >> + > > I thought we weren't adding any new QERR errors any more? How do you intend to inform the user about new error situations? If error codes are a fixed set, then we need either a facility for freely extending text in the error object, or for stacking separate messages in the error object. Thanks Laszlo
On 31 January 2013 16:05, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/31/13 16:54, Peter Maydell wrote: >> I thought we weren't adding any new QERR errors any more? > > How do you intend to inform the user about new error situations? If > error codes are a fixed set, then we need either a facility for freely > extending text in the error object, or for stacking separate messages in > the error object. The idea IIRC is that we just report everything with a free text message and a generic error class (apart from the legacy existing error classes). -- PMM
On 01/31/13 17:13, Peter Maydell wrote: > On 31 January 2013 16:05, Laszlo Ersek <lersek@redhat.com> wrote: >> On 01/31/13 16:54, Peter Maydell wrote: >>> I thought we weren't adding any new QERR errors any more? >> >> How do you intend to inform the user about new error situations? If >> error codes are a fixed set, then we need either a facility for freely >> extending text in the error object, or for stacking separate messages in >> the error object. > > The idea IIRC is that we just report everything with a free > text message and a generic error class (apart from the legacy > existing error classes). That is the polar opposite of what I'm trying to do; namely, accumulating all info & error messages up to do_device_add(), and branching hmp from qmp only above it. The internals should become fully silent. Of course the calltree rooted in do_device_add() prints a lot of informative text (not errors) as well "if !qmp": qbus_list_bus(), qbus_list_dev(), qdev_device_help() etc. I don't yet have details in mind for those, but people have convinced me that the grand idea would be extracting those info printouts as well (as separate top-level operations), extracting / reusing the path resolution code from the actual do_device_add() code. As first step I'm attacking the errors. I'm glad we found this conflict wrt. error propagation this early, as any further steps depend on it. Thanks! Laszlo
On 31 January 2013 16:36, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/31/13 17:13, Peter Maydell wrote: >> On 31 January 2013 16:05, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 01/31/13 16:54, Peter Maydell wrote: >>>> I thought we weren't adding any new QERR errors any more? >>> >>> How do you intend to inform the user about new error situations? If >>> error codes are a fixed set, then we need either a facility for freely >>> extending text in the error object, or for stacking separate messages in >>> the error object. >> >> The idea IIRC is that we just report everything with a free >> text message and a generic error class (apart from the legacy >> existing error classes). > > That is the polar opposite of what I'm trying to do; namely, > accumulating all info & error messages up to do_device_add(), and > branching hmp from qmp only above it. The internals should become fully > silent. You probably need to pass in an Error** then, so the leaf can provide useful information at the point of error and then the top level code can deal with it appropriately whether hmp or qmp. For Error** error_setg() is the right way to specify arbitrary error text I think. -- PMM
On 01/31/13 17:55, Peter Maydell wrote: > You probably need to pass in an Error** then, so the leaf can > provide useful information at the point of error and then the > top level code can deal with it appropriately whether hmp or qmp. That's what I'm doing, yes. > For Error** error_setg() is the right way to specify arbitrary > error text I think. The following call chain illustrates the problem. Suppose monitor_cur_is_qmp() returns false. do_device_add qdev_device_add qbus_find qbus_find_recursive qerror_report(ERROR_CLASS_GENERIC_ERROR) qerror_print error_vprintf qerror_report(QERR_BUS_NOT_FOUND) qerror_print error_vprintf Originally, when qbus_find_recursive() returns NULL to qbus_find(), QERR_BUS_NOT_FOUND is reported. This was not specific enough for Fred's case, so he extended the error for one particular case inside qbus_find_recursive(). In that case ERROR_CLASS_GENERIC_ERROR is generated *in addition*. If monitor_cur_is_qmp() is currently false, both messages are printed, either to the monitor or stderr. (qbus_find() itself provides no details for its caller, only that something went wrong (returns NULL); and my main goal is to change that.) If monitor_cur_is_qmp() returns true: do_device_add qdev_device_add qbus_find qbus_find_recursive qerror_report(ERROR_CLASS_GENERIC_ERROR) monitor_set_error() /* save it for reporting */ qerror_report(QERR_BUS_NOT_FOUND) monitor_set_error() QDECREF then the generic error code overrides the more specific error code and is therefore not useful to the qmp caller; there's no chance for the qmp client to act differently based on the generic error code. The goals are that (a) qbus_find() not care about monitor_cur_is_qmp() at all, (b) qbus_find() be silent, (c1) both pieces of error information (ERROR_CLASS_GENERIC_ERROR / QERR_BUS_NOT_FOUND) reach the QMP caller, (c2) (if that's not possible) the most specific error description reach the caller. (Currently the more specific human readable message is cross-connected to the less specific machine readable error code.) The problem is propagating more than one error (or adding them up somehow). In Java etc. one would throw a derived exception in the innermost frame (so that it would be specific but subject for catching by a more generic handler -- c2), or throw a new exception in the outer frame but link the inner frame's exception object to it (c1). Based on how qerror_report() works when monitor_cur_is_qmp() returns true (= swallow all errors after the first), I think I actually know how to propagate that, but I believe that approach will hurt at least one of hmp / qmp: If I prefer ERROR_CLASS_GENERIC_ERROR with the specific text, then the qmp user will suffer (which is what happens now). If I prefer the more specific QERR_BUS_NOT_FOUND with the *less* specific text, then the human user will suffer. Thanks, Laszlo
On 31 January 2013 18:19, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/31/13 17:55, Peter Maydell wrote: > >> You probably need to pass in an Error** then, so the leaf can >> provide useful information at the point of error and then the >> top level code can deal with it appropriately whether hmp or qmp. > > That's what I'm doing, yes. > >> For Error** error_setg() is the right way to specify arbitrary >> error text I think. > > The following call chain illustrates the problem. Suppose > monitor_cur_is_qmp() returns false. > > do_device_add > qdev_device_add > qbus_find > qbus_find_recursive > qerror_report(ERROR_CLASS_GENERIC_ERROR) > qerror_print > error_vprintf > qerror_report(QERR_BUS_NOT_FOUND) > qerror_print > error_vprintf > > Originally, when qbus_find_recursive() returns NULL to qbus_find(), > QERR_BUS_NOT_FOUND is reported. > > This was not specific enough for Fred's case, so he extended the error > for one particular case inside qbus_find_recursive(). In that case > ERROR_CLASS_GENERIC_ERROR is generated *in addition*. OK, that's a problem. We should only be reporting one error: "we failed because you asked for this bus and it's full" should override the default "we failed to find this bus". We can fix that by having the recursion stop as soon as we get an error. > The goals are that > (a) qbus_find() not care about monitor_cur_is_qmp() at all, > (b) qbus_find() be silent, > (c1) both pieces of error information (ERROR_CLASS_GENERIC_ERROR / > QERR_BUS_NOT_FOUND) reach the QMP caller, I think the QMP caller should also only get one error. > If I prefer ERROR_CLASS_GENERIC_ERROR with the specific text, then the > qmp user will suffer (which is what happens now). If I prefer the more > specific QERR_BUS_NOT_FOUND with the *less* specific text, then the > human user will suffer. Why does the qmp user need to get QERR_BUS_NOT_FOUND? (it would be an incorrect error anyway in the case where we have the GENERIC_ERROR text, because we have in fact found the bus, we just couldn't use it.) -- PMM
On 01/31/13 19:24, Peter Maydell wrote: > We should only be reporting one error: > "we failed because you asked for this bus and it's full" should > override the default "we failed to find this bus". We can fix > that by having the recursion stop as soon as we get an error. > I think the QMP caller should also only get one error. > Why does the qmp user need to get QERR_BUS_NOT_FOUND? > (it would be an incorrect error anyway in the case where > we have the GENERIC_ERROR text, because we have in fact found > the bus, we just couldn't use it.) That's a good clear goal which I can stick to -- let the first / innermost error (the one with the most specific human readable text usually) prevail, no matter the client type. Thanks! Laszlo
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 6c0a18d..940ac21 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -69,6 +69,9 @@ void assert_no_error(Error *err); #define QERR_BUS_NOT_FOUND \ ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found" +#define QERR_BUS_WITH_SLOTS_NOT_FOUND \ + ERROR_CLASS_GENERIC_ERROR, "No bus called '%s' with free slots was found" + #define QERR_COMMAND_DISABLED \ ERROR_CLASS_GENERIC_ERROR, "The command %s has been disabled for this instance" diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c index 34f5014..f18fe50 100644 --- a/hw/qdev-monitor.c +++ b/hw/qdev-monitor.c @@ -331,7 +331,7 @@ static BusState *qbus_find(const char *path) } bus = qbus_find_recursive(sysbus_get_default(), elem, NULL); if (!bus) { - qerror_report(QERR_BUS_NOT_FOUND, elem); + qerror_report(QERR_BUS_WITH_SLOTS_NOT_FOUND, elem); return NULL; } pos = len;
When searching for a bus by name, lookup failure means different things for qbus_find_bus() and qbus_find_recursive(); distinguish them by error codes as well. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- include/qapi/qmp/qerror.h | 3 +++ hw/qdev-monitor.c | 2 +- 2 files changed, 4 insertions(+), 1 deletions(-)