[Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Eric Blake]

I noticed that introspection was not documenting either
qmp_capabilities nor the ErrorClass enum.  I think this is worth
fixing for 2.5 when introspection is brand new, so that if we later
extend the ErrorClass enum or add future capability negotiation (and
in particular if such additions get backported in downstream builds),
a client will be able to use introspection to learn whether the new
features are supported, regardless of the qemu version.

Note that this also adds qmp_capabilities to 'query-commands'.

Yes, this is borderline, and you may decide that it doesn't deserve
to be called a bug and should wait for 2.6.

Eric Blake (3):
  qapi: Add type.is_empty() helper
  qapi: Fix command with named empty argument type
  qapi: Expose ErrorClass through introspection

Marc-André Lureau (1):
  monitor: use qapi for qmp_capabilities command

 docs/qmp-spec.txt                       | 16 ++++++++++++++++
 monitor.c                               |  8 ++++++--
 qapi-schema.json                        | 32 ++++++++++++++++++++++++++++++++
 qmp-commands.hx                         |  4 ++--
 scripts/qapi-commands.py                |  6 +++---
 scripts/qapi-event.py                   |  6 +++---
 scripts/qapi-types.py                   |  2 +-
 scripts/qapi.py                         |  3 +++
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 tests/test-qmp-commands.c               |  5 +++++
 11 files changed, 75 insertions(+), 11 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 1/4] qapi: Add type.is_empty() helper

[Eric Blake]

And use it in qapi-types and qapi-event.  Down the road, we may
want to lift our artificial restriction of no variants at the
top level of an event, at which point, inlining our check for
whether members is empty will no longer be sufficient.  More
immediately, the new .is_empty() helper will help fix a bug in
qapi-visit.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-event.py | 6 +++---
 scripts/qapi-types.py | 2 +-
 scripts/qapi.py       | 3 +++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 720486f..51128f4 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -39,7 +39,7 @@ def gen_event_send(name, arg_type):
 ''',
                 proto=gen_event_send_proto(name, arg_type))

-    if arg_type and arg_type.members:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
     QmpOutputVisitor *qov;
     Visitor *v;
@@ -58,7 +58,7 @@ def gen_event_send(name, arg_type):
 ''',
                  name=name)

-    if arg_type and arg_type.members:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
     qov = qmp_output_visitor_new();
     g_assert(qov);
@@ -90,7 +90,7 @@ def gen_event_send(name, arg_type):
 ''',
                  c_enum=c_enum_const(event_enum_name, name))

-    if arg_type and arg_type.members:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
 out:
     qmp_output_visitor_cleanup(qov);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b37900f..2d843a5 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -83,7 +83,7 @@ struct %(c_name)s {
     # potential issues with attempting to malloc space for zero-length
     # structs in C, and also incompatibility with C++ (where an empty
     # struct is size 1).
-    if not (base and base.members) and not members:
+    if (not base or base.is_empty()) and not members:
         ret += mcgen('''
     char qapi_dummy_field_for_empty_struct;
 ''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 7c50cc4..744dc23 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -996,6 +996,9 @@ class QAPISchemaObjectType(QAPISchemaType):
         # See QAPISchema._make_implicit_object_type()
         return self.name[0] == ':'

+    def is_empty(self):
+        return not self.members and not self.variants
+
     def c_name(self):
         assert not self.is_implicit()
         return QAPISchemaType.c_name(self)
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 2/4] qapi: Fix command with named empty argument type

[Eric Blake]

The generator special-cased
 { 'command':'foo', 'data': {} }
to avoid emitting a visitor variable, but failed to see that
 { 'struct':'NamedEmptyType, 'data': {} }
 { 'command':'foo', 'data':'NamedEmptyType' }
needs the same treatment.  Without a fix to the generator, the
change to qapi-schema-test.json fails to compile with:

tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable]
     Visitor *v;
              ^

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py                | 6 +++---
 tests/qapi-schema/qapi-schema-test.json | 2 ++
 tests/qapi-schema/qapi-schema-test.out  | 2 ++
 tests/test-qmp-commands.c               | 5 +++++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 561e47a..38cbffc 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type):
 ''',
                      c_type=ret_type.c_type())

-    if arg_type:
+    if arg_type and not arg_type.is_empty():
         ret += mcgen('''
     QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
     QapiDeallocVisitor *qdv;
@@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type):
 def gen_marshal_input_visit(arg_type, dealloc=False):
     ret = ''

-    if not arg_type:
+    if not arg_type or arg_type.is_empty():
         return ret

     if dealloc:
@@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type):

     # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
     # for each arg_type member, and by gen_call() for ret_type
-    if (arg_type and arg_type.members) or ret_type:
+    if (arg_type and not arg_type.is_empty()) or ret_type:
         ret += mcgen('''

 out:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 44638da..2aa53fe 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -18,6 +18,8 @@
 { 'struct': 'Empty1', 'data': { } }
 { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }

+{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
+
 # for testing override of default naming heuristic
 { 'enum': 'QEnumTwo',
   'prefix': 'QENUM_TWO',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index e20a823..26cb094 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -201,6 +201,8 @@ command guest-sync :obj-guest-sync-arg -> any
    gen=True success_response=True
 command user_def_cmd None -> None
    gen=True success_response=True
+command user_def_cmd0 Empty2 -> Empty2
+   gen=True success_response=True
 command user_def_cmd1 :obj-user_def_cmd1-arg -> None
    gen=True success_response=True
 command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 888fb5f..93bd7de 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -12,6 +12,11 @@ void qmp_user_def_cmd(Error **errp)
 {
 }

+Empty2 *qmp_user_def_cmd0(Error **errp)
+{
+    return g_new0(Empty2, 1);
+}
+
 void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
 {
 }
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 3/4] monitor: use qapi for qmp_capabilities command

[Eric Blake]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This was initially done to add qmp_capabilities documentation to the
schema. Then I figured it would also help to get rid of the "middle
mode" monitor dispatch code.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <1443189844-20341-5-git-send-email-marcandre.lureau@redhat.com>
[Add empty 'Capabilities' to make it obvious that we don't have any yet.]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 monitor.c        |  4 ++--
 qapi-schema.json | 28 ++++++++++++++++++++++++++++
 qmp-commands.hx  |  2 +-
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3295840..608c70f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -599,7 +599,7 @@ static void monitor_qapi_event_init(void)
     qmp_event_set_func_emit(monitor_qapi_event_queue);
 }

-static void qmp_capabilities(QDict *params, QObject **ret_data, Error **errp)
+void qmp_qmp_capabilities(Error **errp)
 {
     cur_mon->qmp.in_command_mode = true;
 }
@@ -3575,7 +3575,7 @@ static int monitor_can_read(void *opaque)
 static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
                              Error **errp)
 {
-    bool is_cap = cmd->mhandler.cmd_new == qmp_capabilities;
+    bool is_cap = cmd->mhandler.cmd_new == qmp_marshal_qmp_capabilities;

     if (is_cap && mon->qmp.in_command_mode) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
diff --git a/qapi-schema.json b/qapi-schema.json
index f99d413..ce2dd45 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -21,6 +21,34 @@
 { 'include': 'qapi/introspect.json' }

 ##
+# @Capabilities
+#
+# The set of recognized QMP capabilities; currently empty.
+#
+# Since 2.5
+##
+{ 'struct': 'Capabilities', 'data': {} }
+
+##
+# @qmp_capabilities:
+#
+# Enable QMP capabilities.
+#
+# Arguments: None.
+#
+# Example:
+#
+# -> { "execute": "qmp_capabilities" }
+# <- { "return": {} }
+#
+# Notes: This command must be issued before issuing any other command.
+#
+# Since 0.14
+#
+##
+{ 'command': 'qmp_capabilities', 'data': 'Capabilities' }
+
+##
 # @LostTickPolicy:
 #
 # Policy for handling lost ticks in timer devices.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index cde7505..672fcfc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2090,7 +2090,7 @@ EQMP
         .args_type  = "",
         .params     = "",
         .help       = "enable QMP capabilities",
-        .mhandler.cmd_new = qmp_capabilities,
+        .mhandler.cmd_new = qmp_marshal_qmp_capabilities,
     },

 SQMP
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 4/4] qapi: Expose ErrorClass through introspection

[Eric Blake]

Introspection is already able to describe all possible commands and
their valid argument types.  But consider what happens if a future
qemu extends the ErrorClass enum.  It is feasible that a client
will want to know whether the server is new enough to send the new
error class as a result of a particular failure scenario, or even
desire to refuse to talk to a server too old to know about the
error class.  But since no existing commands reference the
ErrorClass type, introspection was silent on the set of recognized
error classes.

Solve this by adding a new optional parameter to qmp_capabilities.
Because it is optional, no existing client is broken, but now it
is referenced in introspection.  Meanwhile, a client can supply a
list of error class strings that it expects the server to know
about, and the qapi parser will accept the command only if all
the client's strings were recognized.

Note that we are using this only as a one-way verification, and
not a two-way negotiation method (that is, a client omitting an
error class string does NOT require the server to avoid sending
that error class, and the server's output is unchanged except for
whether it rejects the attempt with an error).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/qmp-spec.txt | 16 ++++++++++++++++
 monitor.c         |  6 +++++-
 qapi-schema.json  | 10 +++++++---
 qmp-commands.hx   |  2 +-
 4 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/docs/qmp-spec.txt b/docs/qmp-spec.txt
index 4fb10a5..594f969 100644
--- a/docs/qmp-spec.txt
+++ b/docs/qmp-spec.txt
@@ -247,6 +247,22 @@ Clients should use the qmp_capabilities command to enable capabilities
 advertised in the Server's greeting (section '2.2 Server Greeting') they
 support.

+With a new enough server, the client can pass an argument 'errors'
+with a list of error class strings that the server must support; if
+the server does not recognize one of the error classes, the command
+will fail and the Server remains in negotiation mode.
+
+C: { "execute": "qmp_capabilities",
+     "arguments": { "errors": [ "None" ] } }
+S: { "error": { "class": "GenericError",
+     "desc": "Invalid parameter 'None'" } }
+C: { "execute": "qmp_capabilities",
+     "arguments": { "errors": [ "GenericError" ] } }
+S: { "return": {} }
+
+However, the Server is still free to return error classes not
+mentioned by the Client.
+
 When the qmp_capabilities command is issued, and if it does not return an
 error, the Server enters in Command mode where capabilities changes take
 effect, all commands (except qmp_capabilities) are allowed and asynchronous
diff --git a/monitor.c b/monitor.c
index 608c70f..3c2cf1e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -599,8 +599,12 @@ static void monitor_qapi_event_init(void)
     qmp_event_set_func_emit(monitor_qapi_event_queue);
 }

-void qmp_qmp_capabilities(Error **errp)
+void qmp_qmp_capabilities(bool has_errors, ErrorClassList *errors,
+                          Error **errp)
 {
+    /* If we get here, the qapi front end has already validated that
+     * any errors supplied by the user are recognized, so we have
+     * nothing further to do */
     cur_mon->qmp.in_command_mode = true;
 }

diff --git a/qapi-schema.json b/qapi-schema.json
index ce2dd45..1545f9c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -23,18 +23,22 @@
 ##
 # @Capabilities
 #
-# The set of recognized QMP capabilities; currently empty.
+# Used to determine the set of recognized QMP capabilities.
+#
+# @errors: #optional A list of ErrorClass strings that the server
+#          must recognize (however, the server is free to return
+#          errors that were not in the list supplied by the client).
 #
 # Since 2.5
 ##
-{ 'struct': 'Capabilities', 'data': {} }
+{ 'struct': 'Capabilities', 'data': { '*errors': ['ErrorClass'] } }

 ##
 # @qmp_capabilities:
 #
 # Enable QMP capabilities.
 #
-# Arguments: None.
+# Arguments: See documentation of Capabilities.
 #
 # Example:
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 672fcfc..1d02031 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2087,7 +2087,7 @@ Example:
 EQMP
     {
         .name       = "qmp_capabilities",
-        .args_type  = "",
+        .args_type  = "errors:q?",
         .params     = "",
         .help       = "enable QMP capabilities",
         .mhandler.cmd_new = qmp_marshal_qmp_capabilities,
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> I noticed that introspection was not documenting either
> qmp_capabilities nor the ErrorClass enum.  I think this is worth
> fixing for 2.5 when introspection is brand new, so that if we later
> extend the ErrorClass enum or add future capability negotiation (and
> in particular if such additions get backported in downstream builds),
> a client will be able to use introspection to learn whether the new
> features are supported, regardless of the qemu version.
>
> Note that this also adds qmp_capabilities to 'query-commands'.
>
> Yes, this is borderline, and you may decide that it doesn't deserve
> to be called a bug and should wait for 2.6.

Two entirely separate issues: introspecting qmp_capabilities and
introspecting error classes.  This reply is about the former.  I'll
discuss the latter in a separate message.

Introspecting qmp_capabilities with query-qmp-schema is kind of
academic, because by the time you can query-qmp-schema, you're already
done using qmp_capabilities.  That's why docs/qmp-spec.txt nails down
qmp_capabilities, unlike other commands.  Unfortunately, it's a bit
screwy.  Let's have a closer look.

Right after connect, the server sends a greeting that includes a list of
available capabilities (docs/qmp-spec.txt section 2.2 Server Greeting),
currently none.

The only valid command then is qmp_capabilities (section 4. Capabilities
Negotiation).  The command is meant to let clients enable capabilities
from the greeting, but we've neglected to specify how.  Fortunately, QMP
rejects all arguments, so we can still fix that by adding optional
arguments, either a list of capabilities to enable, or a (boolean)
argument for each capability.

My point is: unlike other commands, qmp_capabilities is baked into the
protocol.  Introspection is useful to examine *variable* aspects.  With
the hole in the protocol specification plugged, the only variable aspect
of qmp_capabilities are the capabilities, and the practical way to
introspect those is the server greeting.

Qapifying qmp_capabilities can make sense regardless.  But why does it
need to be rushed into 2.5 then?

Re: [Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3360 bytes --]

On 11/12/2015 03:48 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> I noticed that introspection was not documenting either
>> qmp_capabilities nor the ErrorClass enum.  I think this is worth
>> fixing for 2.5 when introspection is brand new, so that if we later
>> extend the ErrorClass enum or add future capability negotiation (and
>> in particular if such additions get backported in downstream builds),
>> a client will be able to use introspection to learn whether the new
>> features are supported, regardless of the qemu version.
>>
>> Note that this also adds qmp_capabilities to 'query-commands'.
>>
>> Yes, this is borderline, and you may decide that it doesn't deserve
>> to be called a bug and should wait for 2.6.

Or in other words, I posted this series precisely for this conversation.
 The mere fact that you have questions means that it is NOT appropriate
for 2.5 this late in the game.  But to answer you...

> 
> Two entirely separate issues: introspecting qmp_capabilities and
> introspecting error classes.  This reply is about the former.  I'll
> discuss the latter in a separate message.
> 
> Introspecting qmp_capabilities with query-qmp-schema is kind of
> academic, because by the time you can query-qmp-schema, you're already
> done using qmp_capabilities.  That's why docs/qmp-spec.txt nails down
> qmp_capabilities, unlike other commands.  Unfortunately, it's a bit
> screwy.  Let's have a closer look.
> 
> Right after connect, the server sends a greeting that includes a list of
> available capabilities (docs/qmp-spec.txt section 2.2 Server Greeting),
> currently none.
> 
> The only valid command then is qmp_capabilities (section 4. Capabilities
> Negotiation).  The command is meant to let clients enable capabilities
> from the greeting, but we've neglected to specify how.  Fortunately, QMP
> rejects all arguments, so we can still fix that by adding optional
> arguments, either a list of capabilities to enable, or a (boolean)
> argument for each capability.
> 
> My point is: unlike other commands, qmp_capabilities is baked into the
> protocol.  Introspection is useful to examine *variable* aspects.  With
> the hole in the protocol specification plugged, the only variable aspect
> of qmp_capabilities are the capabilities, and the practical way to
> introspect those is the server greeting.

Good point - if we ever add a capability, a client that knows how to
request the capability can easily learn whether the server will honor
the request by reading the server's advertisements.

> 
> Qapifying qmp_capabilities can make sense regardless.  But why does it
> need to be rushed into 2.5 then?

No need for the rush after all.  You are correct that qapi-fying
qmp_capabilities will NOT help the client learn anything it couldn't
have already learned from the server greeting.

And if we don't rush qmp_capabilities into 2.5 (patch 3), then we also
don't need to rush in a fix for explicitly empty types (patch 2) or a
way to detect empty types (patch 1).

As for patch 4 - we could still expose ErrorClass, via some means other
than qmp_capabilities, if desired; but that's a discussion for your
other mail.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> I noticed that introspection was not documenting either
> qmp_capabilities nor the ErrorClass enum.  I think this is worth
> fixing for 2.5 when introspection is brand new, so that if we later
> extend the ErrorClass enum or add future capability negotiation (and
> in particular if such additions get backported in downstream builds),
> a client will be able to use introspection to learn whether the new
> features are supported, regardless of the qemu version.
>
> Note that this also adds qmp_capabilities to 'query-commands'.
>
> Yes, this is borderline, and you may decide that it doesn't deserve
> to be called a bug and should wait for 2.6.

Before I discuss the error class proposal in more detail, a preliminary
remark: error classes are a leftover from the days of "rich" error
objects, and any new use of an error class other than
ERROR_CLASS_GENERIC_ERROR is immediately suspect.  I'm not saying that
we won't add such uses anymore, just that there's a significant bar to
overcome, which we haven't for quite some time now.

I think I could be persuaded that a client might be able to use
knowledge on what error classes a specific command can produce.  Of
course, presence of an error class doesn't tell what actual error
conditions map to it, i.e. the client still needs to make assumptions on
the meaning of error classes.  Humans make those, too, but humans can
read the contract in the comments.

The value of a global list of error classes seems even more dubious,
though.  Existence of an error class by itself guarantees nothing.  How
would a client use the information?  Assume that existence of a class
implies a certain command uses it in a certain way?  That's an even
bigger jump than above.

I guess using the presence or absence of an error class as a witness for
a certain feature or behavior could work.  Seems practical when the
written contract guarantees the connection between the two (de jure
connection), or the commit that introduces the feature or behavior also
adds or removes the error class (de facto connecton).  This applies both
to a global list of error classes and to per-command lists.

Example 1: MigrationExpected

    Before commit 1e99814 "qmp: handle stop/cont in INMIGRATE state",
    cont could fail with error MigrationExpected.  Libvirt dealt with it
    by trying again.

    Commit 1e99814 made cont just work, and dropped the error class.
    The error class was never used for anything else.

    Exposing a global list of error classes like your patch does would
    permit detecting the presence of this commit.  However, detecting it
    is pointless: to deal with its absence, you have to loop on
    MigrationExpected anyway, and that code works just fine with and
    without the commit.

Example 2: Unwanted DeviceNotFound dropped

    During the 2.3 development cycle, a few unwanted uses of
    DeviceNotFound crept in.  Commits 5b347c5, f3cf80e, 6ec46ad backed
    them out before the release.

    For the sake of argument, ignore the fact that these unwanted
    DeviceNotFound never made it to a release, and if they had, we
    would've left them in, because taking them out would've been an ABI
    break.

    A client could use a per-command error class list to detect them,
    but not a global error class list.  But what could it do with the
    information?  If DeviceNotFound is there, the client can handle it
    specially, if not, it can't, and I can't see how knowing it would
    make a difference.

Example 3 (hypothetical): New error class to support a client's need

    Say we discover that a client wants to handle a certain error
    specially, and we decide to make that possible by changing its error
    class from GenericError to something specific to that error.
    Hypothetical, because changing an error's error class is an ABI
    break, and we normally don't do that.

    The client could then refrain from using the command in certain ways
    unless it uses the specific error class for this error.

    Detecting that by finding the error class in the global list of
    error classes works only if the error class is new, and only works
    as long as it doesn't get used for other things that then get
    backported without the original use.

    Detecting it by finding the error class in the command's list of
    error classes would be less brittle.

Example 4: Use per-command error list to catch unwanted error class

    If we declare a command's errors, we can detect undeclared errors at
    run time.  This should help catching unwanted ones early (see
    example 2).

    Having to declare error classes may facilitate proper review of new
    uses of funky error classes.

None of these examples is a particularly convincing use case.  Can you
think of better ones?

Finally, what happens if error class introspection misses 2.5 and makes
a later version?

If we add a global error class list like this patch does, a client has
to assume that anything that doesn't has one has the usual error
classes: GenericError, CommandNotFound, DeviceEncrypted,
DeviceNotActive, DeviceNotFound, KVMMissingCap[*].  Whether "doesn't has
one" is "doesn't has one in query-qmp-schema" or "doesn't even have
query-qmp-schema" doesn't matter.

If we add per-command error class lists, it's the same, only the
assumptions become a bit more involved.

Of course, the fewer discernible versions of introspection we have, the
better.  If we can figure out what we need in time to get it into the
very first version, great!  But it's awfully late for 2.5, and I'm not
at all sure we know what we need.  Perhaps we can find out quickly, but
let's not rush the job.


[*] Ancient versions may also have MigrationExpected (see above), but
backporting introspection that far, but not backporting the fix for the
migration stop/cont race would be insane.

Re: [Qemu-devel] [PATCH for-2.5 0/4] Expose ErrorClass through introspection

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

On 11/12/2015 03:48 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> I noticed that introspection was not documenting either
>> qmp_capabilities nor the ErrorClass enum.  I think this is worth
>> fixing for 2.5 when introspection is brand new, so that if we later
>> extend the ErrorClass enum or add future capability negotiation (and
>> in particular if such additions get backported in downstream builds),
>> a client will be able to use introspection to learn whether the new
>> features are supported, regardless of the qemu version.
>>
>> Note that this also adds qmp_capabilities to 'query-commands'.
>>
>> Yes, this is borderline, and you may decide that it doesn't deserve
>> to be called a bug and should wait for 2.6.
> 
> Before I discuss the error class proposal in more detail, a preliminary
> remark: error classes are a leftover from the days of "rich" error
> objects, and any new use of an error class other than
> ERROR_CLASS_GENERIC_ERROR is immediately suspect.  I'm not saying that
> we won't add such uses anymore, just that there's a significant bar to
> overcome, which we haven't for quite some time now.
> 
> I think I could be persuaded that a client might be able to use
> knowledge on what error classes a specific command can produce.  Of
> course, presence of an error class doesn't tell what actual error
> conditions map to it, i.e. the client still needs to make assumptions on
> the meaning of error classes.  Humans make those, too, but humans can
> read the contract in the comments.

Indeed - knowing the global set of possible errors is NOT the same as
knowing the set of command-specific errors; and the latter is more
likely to be useful.  But exposing the latter would require per-command
documentation in the .json files, and is certainly not doable in time
for 2.5.

> 
> The value of a global list of error classes seems even more dubious,
> though.  Existence of an error class by itself guarantees nothing.  How
> would a client use the information?  Assume that existence of a class
> implies a certain command uses it in a certain way?  That's an even
> bigger jump than above.
> 
> I guess using the presence or absence of an error class as a witness for
> a certain feature or behavior could work.  Seems practical when the
> written contract guarantees the connection between the two (de jure
> connection), or the commit that introduces the feature or behavior also
> adds or removes the error class (de facto connecton).  This applies both
> to a global list of error classes and to per-command lists.
> 

[snip good examples]

> 
> None of these examples is a particularly convincing use case.  Can you
> think of better ones?

No.

> 
> Finally, what happens if error class introspection misses 2.5 and makes
> a later version?
> 

It would not be the first time that libvirt has been coded along the
lines of "if the information is available, trust it; if not, go by a
hard-coded list that matched the upstream state prior to the information
being made available".  It's a bit more work on clients, but not
insurmountable.

> If we add a global error class list like this patch does, a client has
> to assume that anything that doesn't has one has the usual error
> classes: GenericError, CommandNotFound, DeviceEncrypted,
> DeviceNotActive, DeviceNotFound, KVMMissingCap[*].  Whether "doesn't has
> one" is "doesn't has one in query-qmp-schema" or "doesn't even have
> query-qmp-schema" doesn't matter.

Correct - a client can easily hard-code the correct information for all
older qemu (at worse, it will miss an error case that has been
backported - but in all likelihood, if backporting an error was that
important, downstream could also backport the way to introspect for it).

> 
> If we add per-command error class lists, it's the same, only the
> assumptions become a bit more involved.
> 
> Of course, the fewer discernible versions of introspection we have, the
> better.  If we can figure out what we need in time to get it into the
> very first version, great!  But it's awfully late for 2.5, and I'm not
> at all sure we know what we need.  Perhaps we can find out quickly, but
> let's not rush the job.
> 
> 
> [*] Ancient versions may also have MigrationExpected (see above), but
> backporting introspection that far, but not backporting the fix for the
> migration stop/cont race would be insane.

Therefore, I agree with you that there is no rush to get this into 2.5.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]