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

[Markus Armbruster <armbru@redhat.com>]

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.