[Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Michal Privoznik]

If a command is disabled an error is reported. But due to usage
of error_setg() the class of the error is GenericError which does
not help callers in distinguishing this case from a case where a
qmp command fails regularly due to other reasons. Use
CommandNotFound error class which is much closer to the actual
root cause.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

This is a v2 of:

https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg06327.html

diff to v1:
- Don't introduce new error class (CommandDisabled)
- Use CommandNotFound error class

 qapi/qmp-dispatch.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 3037d353a4..bc264b3c9b 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -104,8 +104,9 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
         return NULL;
     }
     if (!cmd->enabled) {
-        error_setg(errp, "The command %s has been disabled for this instance",
-                   command);
+        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                  "The command %s has been disabled for this instance",
+                  command);
         return NULL;
     }
     if (oob && !(cmd->options & QCO_ALLOW_OOB)) {
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 851 bytes --]

On 8/30/19 8:29 AM, Michal Privoznik wrote:
> If a command is disabled an error is reported. But due to usage
> of error_setg() the class of the error is GenericError which does
> not help callers in distinguishing this case from a case where a
> qmp command fails regularly due to other reasons. Use
> CommandNotFound error class which is much closer to the actual
> root cause.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> This is a v2 of:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2019-08/msg06327.html
> 
> diff to v1:
> - Don't introduce new error class (CommandDisabled)
> - Use CommandNotFound error class
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Markus Armbruster]

Michal Privoznik <mprivozn@redhat.com> writes:

> If a command is disabled an error is reported. But due to usage
> of error_setg() the class of the error is GenericError which does
> not help callers in distinguishing this case from a case where a
> qmp command fails regularly due to other reasons. Use
> CommandNotFound error class which is much closer to the actual
> root cause.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---

I'd like to tweak the commit message a bit:

  qmp-dispatch: Use CommandNotFound error for disabled commands

  If a command is disabled an error is reported.  But due to usage of
  error_setg() the class of the error is GenericError which does not
  help callers in distinguishing this case from a case where a qmp
  command fails regularly due to other reasons.

  We used to use class CommandDisabled until the great error
  simplification (commit de253f1491 for QMP and commit 93b91c59db for
  qemu-ga, both v1.2.0).

  Use CommandNotFound error class, which is close enough.

Objections?

Re: [Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Michal Privoznik]

On 9/13/19 2:52 PM, Markus Armbruster wrote:
> Michal Privoznik <mprivozn@redhat.com> writes:
> 
>> If a command is disabled an error is reported. But due to usage
>> of error_setg() the class of the error is GenericError which does
>> not help callers in distinguishing this case from a case where a
>> qmp command fails regularly due to other reasons. Use
>> CommandNotFound error class which is much closer to the actual
>> root cause.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
> 
> I'd like to tweak the commit message a bit:
> 
>    qmp-dispatch: Use CommandNotFound error for disabled commands
> 
>    If a command is disabled an error is reported.  But due to usage of
>    error_setg() the class of the error is GenericError which does not
>    help callers in distinguishing this case from a case where a qmp
>    command fails regularly due to other reasons.
> 
>    We used to use class CommandDisabled until the great error
>    simplification (commit de253f1491 for QMP and commit 93b91c59db for
>    qemu-ga, both v1.2.0).
> 
>    Use CommandNotFound error class, which is close enough.
> 
> Objections?
> 

None, thanks for taking care of this.

Michal

Re: [Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Markus Armbruster]

Michal Privoznik <mprivozn@redhat.com> writes:

> On 9/13/19 2:52 PM, Markus Armbruster wrote:
>> Michal Privoznik <mprivozn@redhat.com> writes:
>>
>>> If a command is disabled an error is reported. But due to usage
>>> of error_setg() the class of the error is GenericError which does
>>> not help callers in distinguishing this case from a case where a
>>> qmp command fails regularly due to other reasons. Use
>>> CommandNotFound error class which is much closer to the actual
>>> root cause.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>
>> I'd like to tweak the commit message a bit:
>>
>>    qmp-dispatch: Use CommandNotFound error for disabled commands
>>
>>    If a command is disabled an error is reported.  But due to usage of
>>    error_setg() the class of the error is GenericError which does not
>>    help callers in distinguishing this case from a case where a qmp
>>    command fails regularly due to other reasons.
>>
>>    We used to use class CommandDisabled until the great error
>>    simplification (commit de253f1491 for QMP and commit 93b91c59db for
>>    qemu-ga, both v1.2.0).
>>
>>    Use CommandNotFound error class, which is close enough.
>>
>> Objections?
>>
>
> None, thanks for taking care of this.

Need to squash in:

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 891aa3d322..1ca49bbced 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -668,7 +668,7 @@ static void test_qga_blacklist(gconstpointer data)
     error = qdict_get_qdict(ret, "error");
     class = qdict_get_try_str(error, "class");
     desc = qdict_get_try_str(error, "desc");
-    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpstr(class, ==, "CommandNotFound");
     g_assert_nonnull(g_strstr_len(desc, -1, "has been disabled"));
     qobject_unref(ret);
 
@@ -677,7 +677,7 @@ static void test_qga_blacklist(gconstpointer data)
     error = qdict_get_qdict(ret, "error");
     class = qdict_get_try_str(error, "class");
     desc = qdict_get_try_str(error, "desc");
-    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpstr(class, ==, "CommandNotFound");
     g_assert_nonnull(g_strstr_len(desc, -1, "has been disabled"));
     qobject_unref(ret);
 

Re: [Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Michal Privoznik <mprivozn@redhat.com> writes:
>
>> On 9/13/19 2:52 PM, Markus Armbruster wrote:
>>> Michal Privoznik <mprivozn@redhat.com> writes:
>>>
>>>> If a command is disabled an error is reported. But due to usage
>>>> of error_setg() the class of the error is GenericError which does
>>>> not help callers in distinguishing this case from a case where a
>>>> qmp command fails regularly due to other reasons. Use
>>>> CommandNotFound error class which is much closer to the actual
>>>> root cause.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>
>>> I'd like to tweak the commit message a bit:
>>>
>>>    qmp-dispatch: Use CommandNotFound error for disabled commands
>>>
>>>    If a command is disabled an error is reported.  But due to usage of
>>>    error_setg() the class of the error is GenericError which does not
>>>    help callers in distinguishing this case from a case where a qmp
>>>    command fails regularly due to other reasons.
>>>
>>>    We used to use class CommandDisabled until the great error
>>>    simplification (commit de253f1491 for QMP and commit 93b91c59db for
>>>    qemu-ga, both v1.2.0).
>>>
>>>    Use CommandNotFound error class, which is close enough.
>>>
>>> Objections?
>>>
>>
>> None, thanks for taking care of this.
>
> Need to squash in:
>
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 891aa3d322..1ca49bbced 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -668,7 +668,7 @@ static void test_qga_blacklist(gconstpointer data)
>      error = qdict_get_qdict(ret, "error");
>      class = qdict_get_try_str(error, "class");
>      desc = qdict_get_try_str(error, "desc");
> -    g_assert_cmpstr(class, ==, "GenericError");
> +    g_assert_cmpstr(class, ==, "CommandNotFound");
>      g_assert_nonnull(g_strstr_len(desc, -1, "has been disabled"));
>      qobject_unref(ret);
>  
> @@ -677,7 +677,7 @@ static void test_qga_blacklist(gconstpointer data)
>      error = qdict_get_qdict(ret, "error");
>      class = qdict_get_try_str(error, "class");
>      desc = qdict_get_try_str(error, "desc");
> -    g_assert_cmpstr(class, ==, "GenericError");
> +    g_assert_cmpstr(class, ==, "CommandNotFound");
>      g_assert_nonnull(g_strstr_len(desc, -1, "has been disabled"));
>      qobject_unref(ret);
>  

I tried to include the amended patch in today's pull request, but
observed "make check" hangs with it.

Re: [Qemu-devel] [PATCH v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> I tried to include the amended patch in today's pull request, but
> observed "make check" hangs with it.

False alarm: I just got a hang on master.  I intend to include this
patch in my next pull request.  Sorry for the delay.