include/qapi/qmp/dispatch.h | 12 ++++++++++-- monitor/qmp-cmds-control.c | 2 +- qapi/qmp-dispatch.c | 10 +++++++++- qapi/qmp-registry.c | 16 +++++++++------- qga/main.c | 4 ++-- 5 files changed, 31 insertions(+), 13 deletions(-)
From: Marc-André Lureau <marcandre.lureau@redhat.com>
qmp_disable_command() now takes an enum for the reason, to be able
to give more explicit error messages.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1928806
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
v2:
- replace string with an enum for disabling reason
- remove trailing dot
include/qapi/qmp/dispatch.h | 12 ++++++++++--
monitor/qmp-cmds-control.c | 2 +-
qapi/qmp-dispatch.c | 10 +++++++++-
qapi/qmp-registry.c | 16 +++++++++-------
qga/main.c | 4 ++--
5 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1486cac3ef..fda9ffad73 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -28,6 +28,13 @@ typedef enum QmpCommandOptions
QCO_COROUTINE = (1U << 3),
} QmpCommandOptions;
+typedef enum QmpDisabled
+{
+ QMP_DISABLED_NONE,
+ QMP_DISABLED_GENERIC,
+ QMP_DISABLED_FROZEN,
+} QmpDisabled;
+
typedef struct QmpCommand
{
const char *name;
@@ -35,7 +42,7 @@ typedef struct QmpCommand
QmpCommandFunc *fn;
QmpCommandOptions options;
QTAILQ_ENTRY(QmpCommand) node;
- bool enabled;
+ QmpDisabled disabled;
} QmpCommand;
typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
@@ -44,7 +51,8 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
QmpCommandFunc *fn, QmpCommandOptions options);
const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
const char *name);
-void qmp_disable_command(QmpCommandList *cmds, const char *name);
+void qmp_disable_command(QmpCommandList *cmds, const char *name,
+ QmpDisabled disabled);
void qmp_enable_command(QmpCommandList *cmds, const char *name);
bool qmp_command_is_enabled(const QmpCommand *cmd);
diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
index 509ae870bd..94a8e133b6 100644
--- a/monitor/qmp-cmds-control.c
+++ b/monitor/qmp-cmds-control.c
@@ -107,7 +107,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque)
CommandInfo *info;
CommandInfoList **list = opaque;
- if (!cmd->enabled) {
+ if (!qmp_command_is_enabled(cmd)) {
return;
}
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 0a2b20a4e4..b65f670152 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -155,11 +155,19 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
"The command %s has not been found", command);
goto out;
}
- if (!cmd->enabled) {
+ switch (cmd->disabled) {
+ case QMP_DISABLED_FROZEN:
+ error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
+ "The command %s has been disabled after fsfreeze",
+ command);
+ goto out;
+ case QMP_DISABLED_GENERIC:
error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
"The command %s has been disabled for this instance",
command);
goto out;
+ case QMP_DISABLED_NONE:
+ break;
}
if (oob && !(cmd->options & QCO_ALLOW_OOB)) {
error_setg(&err, "The command %s does not support OOB",
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 58c65b5052..e39e3b449c 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -25,7 +25,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
cmd->name = name;
cmd->fn = fn;
- cmd->enabled = true;
+ cmd->disabled = QMP_DISABLED_NONE;
cmd->options = options;
QTAILQ_INSERT_TAIL(cmds, cmd, node);
}
@@ -43,31 +43,33 @@ const QmpCommand *qmp_find_command(const QmpCommandList *cmds, const char *name)
}
static void qmp_toggle_command(QmpCommandList *cmds, const char *name,
- bool enabled)
+ QmpDisabled disabled)
{
QmpCommand *cmd;
QTAILQ_FOREACH(cmd, cmds, node) {
if (strcmp(cmd->name, name) == 0) {
- cmd->enabled = enabled;
+ cmd->disabled = disabled;
return;
}
}
}
-void qmp_disable_command(QmpCommandList *cmds, const char *name)
+void qmp_disable_command(QmpCommandList *cmds, const char *name,
+ QmpDisabled disabled)
{
- qmp_toggle_command(cmds, name, false);
+ assert(disabled != QMP_DISABLED_NONE);
+ qmp_toggle_command(cmds, name, disabled);
}
void qmp_enable_command(QmpCommandList *cmds, const char *name)
{
- qmp_toggle_command(cmds, name, true);
+ qmp_toggle_command(cmds, name, QMP_DISABLED_NONE);
}
bool qmp_command_is_enabled(const QmpCommand *cmd)
{
- return cmd->enabled;
+ return cmd->disabled != QMP_DISABLED_NONE;
}
const char *qmp_command_name(const QmpCommand *cmd)
diff --git a/qga/main.c b/qga/main.c
index e7f8f3b161..0dbf0cacd2 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -375,7 +375,7 @@ static void ga_disable_non_whitelisted(const QmpCommand *cmd, void *opaque)
}
if (!whitelisted) {
g_debug("disabling command: %s", name);
- qmp_disable_command(&ga_commands, name);
+ qmp_disable_command(&ga_commands, name, QMP_DISABLED_FROZEN);
}
}
@@ -1329,7 +1329,7 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation)
s->blacklist = config->blacklist;
do {
g_debug("disabling command: %s", (char *)l->data);
- qmp_disable_command(&ga_commands, l->data);
+ qmp_disable_command(&ga_commands, l->data, QMP_DISABLED_GENERIC);
l = g_list_next(l);
} while (l);
}
--
2.29.0
Patchew URL: https://patchew.org/QEMU/20210217070944.2371327-1-marcandre.lureau@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210217070944.2371327-1-marcandre.lureau@redhat.com
Subject: [PATCH v2] qga: return a more explicit error on why a command is disabled
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/20210211225246.17315-1-danielhb413@gmail.com -> patchew/20210211225246.17315-1-danielhb413@gmail.com
- [tag update] patchew/20210216181316.794276-1-alxndr@bu.edu -> patchew/20210216181316.794276-1-alxndr@bu.edu
- [tag update] patchew/20210216224543.16142-1-rebecca@nuviainc.com -> patchew/20210216224543.16142-1-rebecca@nuviainc.com
* [new tag] patchew/20210217070944.2371327-1-marcandre.lureau@redhat.com -> patchew/20210217070944.2371327-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
56eb0c9 qga: return a more explicit error on why a command is disabled
=== OUTPUT BEGIN ===
ERROR: open brace '{' following enum go on the same line
#32: FILE: include/qapi/qmp/dispatch.h:32:
+typedef enum QmpDisabled
+{
total: 1 errors, 0 warnings, 121 lines checked
Commit 56eb0c90cf28 (qga: return a more explicit error on why a command is disabled) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210217070944.2371327-1-marcandre.lureau@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qmp_disable_command() now takes an enum for the reason, to be able
> to give more explicit error messages.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1928806
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>
> v2:
> - replace string with an enum for disabling reason
> - remove trailing dot
>
> include/qapi/qmp/dispatch.h | 12 ++++++++++--
> monitor/qmp-cmds-control.c | 2 +-
> qapi/qmp-dispatch.c | 10 +++++++++-
> qapi/qmp-registry.c | 16 +++++++++-------
> qga/main.c | 4 ++--
> 5 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1486cac3ef..fda9ffad73 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -28,6 +28,13 @@ typedef enum QmpCommandOptions
> QCO_COROUTINE = (1U << 3),
> } QmpCommandOptions;
>
> +typedef enum QmpDisabled
> +{
> + QMP_DISABLED_NONE,
> + QMP_DISABLED_GENERIC,
> + QMP_DISABLED_FROZEN,
> +} QmpDisabled;
> +
I strongly dislike baking QGA-specific things into the generic
dispatcher. I believe it's easy enough to avoid; see below.
> typedef struct QmpCommand
> {
> const char *name;
> @@ -35,7 +42,7 @@ typedef struct QmpCommand
> QmpCommandFunc *fn;
> QmpCommandOptions options;
> QTAILQ_ENTRY(QmpCommand) node;
> - bool enabled;
> + QmpDisabled disabled;
> } QmpCommand;
>
> typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
> @@ -44,7 +51,8 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
> QmpCommandFunc *fn, QmpCommandOptions options);
> const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
> const char *name);
> -void qmp_disable_command(QmpCommandList *cmds, const char *name);
> +void qmp_disable_command(QmpCommandList *cmds, const char *name,
> + QmpDisabled disabled);
> void qmp_enable_command(QmpCommandList *cmds, const char *name);
>
> bool qmp_command_is_enabled(const QmpCommand *cmd);
> diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
> index 509ae870bd..94a8e133b6 100644
> --- a/monitor/qmp-cmds-control.c
> +++ b/monitor/qmp-cmds-control.c
> @@ -107,7 +107,7 @@ static void query_commands_cb(const QmpCommand *cmd, void *opaque)
> CommandInfo *info;
> CommandInfoList **list = opaque;
>
> - if (!cmd->enabled) {
> + if (!qmp_command_is_enabled(cmd)) {
> return;
> }
>
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 0a2b20a4e4..b65f670152 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -155,11 +155,19 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> "The command %s has not been found", command);
> goto out;
> }
> - if (!cmd->enabled) {
> + switch (cmd->disabled) {
> + case QMP_DISABLED_FROZEN:
> + error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> + "The command %s has been disabled after fsfreeze",
> + command);
> + goto out;
> + case QMP_DISABLED_GENERIC:
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> "The command %s has been disabled for this instance",
> command);
> goto out;
> + case QMP_DISABLED_NONE:
> + break;
> }
> if (oob && !(cmd->options & QCO_ALLOW_OOB)) {
v1 put an optional error message template into struct QmpCommand, and
set the error with
error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
cmd->err_msg ?: "The command %s has been disabled for this instance",
command);
Peter Krempa pointed out that this defeats the compiler's format string
checking.
I feel the proper way to avoid this is to keep an optional string in
QmpCommand that explains the disablement, and use it like this:
if (!cmd->enabled) {
error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
"Command %s has been disabled%s%s",
command,
cmd->disable_reason ? ": ", "",
cmd->disable_reason ?: "");
goto out;
}
If we make the string mandatory, we can ditch cmd->enabled, and say
if (cmd->disabled) {
error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
"Command %s has been disabled: %s",
command, cmd->disabled);
goto out;
}
[...]
Hi
On Thu, Feb 18, 2021 at 8:40 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > qmp_disable_command() now takes an enum for the reason, to be able
> > to give more explicit error messages.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1928806
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >
> > v2:
> > - replace string with an enum for disabling reason
> > - remove trailing dot
> >
> > include/qapi/qmp/dispatch.h | 12 ++++++++++--
> > monitor/qmp-cmds-control.c | 2 +-
> > qapi/qmp-dispatch.c | 10 +++++++++-
> > qapi/qmp-registry.c | 16 +++++++++-------
> > qga/main.c | 4 ++--
> > 5 files changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> > index 1486cac3ef..fda9ffad73 100644
> > --- a/include/qapi/qmp/dispatch.h
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -28,6 +28,13 @@ typedef enum QmpCommandOptions
> > QCO_COROUTINE = (1U << 3),
> > } QmpCommandOptions;
> >
> > +typedef enum QmpDisabled
> > +{
> > + QMP_DISABLED_NONE,
> > + QMP_DISABLED_GENERIC,
> > + QMP_DISABLED_FROZEN,
> > +} QmpDisabled;
> > +
>
> I strongly dislike baking QGA-specific things into the generic
> dispatcher. I believe it's easy enough to avoid; see below.
>
> > typedef struct QmpCommand
> > {
> > const char *name;
> > @@ -35,7 +42,7 @@ typedef struct QmpCommand
> > QmpCommandFunc *fn;
> > QmpCommandOptions options;
> > QTAILQ_ENTRY(QmpCommand) node;
> > - bool enabled;
> > + QmpDisabled disabled;
> > } QmpCommand;
> >
> > typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
> > @@ -44,7 +51,8 @@ void qmp_register_command(QmpCommandList *cmds, const
> char *name,
> > QmpCommandFunc *fn, QmpCommandOptions
> options);
> > const QmpCommand *qmp_find_command(const QmpCommandList *cmds,
> > const char *name);
> > -void qmp_disable_command(QmpCommandList *cmds, const char *name);
> > +void qmp_disable_command(QmpCommandList *cmds, const char *name,
> > + QmpDisabled disabled);
> > void qmp_enable_command(QmpCommandList *cmds, const char *name);
> >
> > bool qmp_command_is_enabled(const QmpCommand *cmd);
> > diff --git a/monitor/qmp-cmds-control.c b/monitor/qmp-cmds-control.c
> > index 509ae870bd..94a8e133b6 100644
> > --- a/monitor/qmp-cmds-control.c
> > +++ b/monitor/qmp-cmds-control.c
> > @@ -107,7 +107,7 @@ static void query_commands_cb(const QmpCommand *cmd,
> void *opaque)
> > CommandInfo *info;
> > CommandInfoList **list = opaque;
> >
> > - if (!cmd->enabled) {
> > + if (!qmp_command_is_enabled(cmd)) {
> > return;
> > }
> >
> > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> > index 0a2b20a4e4..b65f670152 100644
> > --- a/qapi/qmp-dispatch.c
> > +++ b/qapi/qmp-dispatch.c
> > @@ -155,11 +155,19 @@ QDict *qmp_dispatch(const QmpCommandList *cmds,
> QObject *request,
> > "The command %s has not been found", command);
> > goto out;
> > }
> > - if (!cmd->enabled) {
> > + switch (cmd->disabled) {
> > + case QMP_DISABLED_FROZEN:
> > + error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> > + "The command %s has been disabled after fsfreeze",
> > + command);
> > + goto out;
> > + case QMP_DISABLED_GENERIC:
> > error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> > "The command %s has been disabled for this instance",
> > command);
> > goto out;
> > + case QMP_DISABLED_NONE:
> > + break;
> > }
> > if (oob && !(cmd->options & QCO_ALLOW_OOB)) {
>
> v1 put an optional error message template into struct QmpCommand, and
> set the error with
>
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> cmd->err_msg ?: "The command %s has been disabled for
> this instance",
> command);
>
> Peter Krempa pointed out that this defeats the compiler's format string
> checking.
>
> I feel the proper way to avoid this is to keep an optional string in
> QmpCommand that explains the disablement, and use it like this:
>
> if (!cmd->enabled) {
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> "Command %s has been disabled%s%s",
> command,
> cmd->disable_reason ? ": ", "",
> cmd->disable_reason ?: "");
> goto out;
> }
>
>
Works for me, I'll send a v3.
thanks
If we make the string mandatory, we can ditch cmd->enabled, and say
>
> if (cmd->disabled) {
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> "Command %s has been disabled: %s",
> command, cmd->disabled);
> goto out;
> }
>
> [...]
>
>
© 2016 - 2025 Red Hat, Inc.