Patchwork [RFC,2/2] qbus_find_recursive(): the "free slots" constraint needs a dedicated error

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

Comments

Laszlo Ersek - Jan. 31, 2013, 3:42 p.m.
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(-)
Laszlo Ersek - Jan. 31, 2013, 4:05 p.m.
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
Peter Maydell - Jan. 31, 2013, 4:13 p.m.
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
Laszlo Ersek - Jan. 31, 2013, 4:36 p.m.
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
Peter Maydell - Jan. 31, 2013, 4:55 p.m.
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
Laszlo Ersek - Jan. 31, 2013, 6:19 p.m.
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
Peter Maydell - Jan. 31, 2013, 6:24 p.m.
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
Laszlo Ersek - Feb. 1, 2013, 9:42 a.m.
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

Patch

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;