| Message ID | faeb030e6a1044f0fd88208edfdb1c5fafe5def9.1567171655.git.mprivozn@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] qapi/qmp-dispatch: Fix error class for reporting disabled commands | expand |
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>
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?
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
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);
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.
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.
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)) {
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