* [PATCH] qga: return a more explicit error on why a command is disabled
@ 2021-02-16 13:38 marcandre.lureau
2021-02-16 13:46 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: marcandre.lureau @ 2021-02-16 13:38 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, pkrempa, armbru, Marc-André Lureau
From: Marc-André Lureau <marcandre.lureau@redhat.com>
qmp_disable_command() now takes an optional error string to return a
more explicit error message.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1928806
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
include/qapi/qmp/dispatch.h | 3 ++-
qapi/qmp-dispatch.c | 2 +-
qapi/qmp-registry.c | 9 +++++----
qga/main.c | 4 ++--
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1486cac3ef..d50420cdd8 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -36,6 +36,7 @@ typedef struct QmpCommand
QmpCommandOptions options;
QTAILQ_ENTRY(QmpCommand) node;
bool enabled;
+ const char *err_msg;
} QmpCommand;
typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
@@ -44,7 +45,7 @@ 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, const char *err_msg);
void qmp_enable_command(QmpCommandList *cmds, const char *name);
bool qmp_command_is_enabled(const QmpCommand *cmd);
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 0a2b20a4e4..ce4bf56b2c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -157,7 +157,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
}
if (!cmd->enabled) {
error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
- "The command %s has been disabled for this instance",
+ cmd->err_msg ?: "The command %s has been disabled for this instance",
command);
goto out;
}
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 58c65b5052..ddc49b8f2d 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -43,26 +43,27 @@ const QmpCommand *qmp_find_command(const QmpCommandList *cmds, const char *name)
}
static void qmp_toggle_command(QmpCommandList *cmds, const char *name,
- bool enabled)
+ bool enabled, const char *err_msg)
{
QmpCommand *cmd;
QTAILQ_FOREACH(cmd, cmds, node) {
if (strcmp(cmd->name, name) == 0) {
cmd->enabled = enabled;
+ cmd->err_msg = err_msg;
return;
}
}
}
-void qmp_disable_command(QmpCommandList *cmds, const char *name)
+void qmp_disable_command(QmpCommandList *cmds, const char *name, const char *err_msg)
{
- qmp_toggle_command(cmds, name, false);
+ qmp_toggle_command(cmds, name, false, err_msg);
}
void qmp_enable_command(QmpCommandList *cmds, const char *name)
{
- qmp_toggle_command(cmds, name, true);
+ qmp_toggle_command(cmds, name, true, NULL);
}
bool qmp_command_is_enabled(const QmpCommand *cmd)
diff --git a/qga/main.c b/qga/main.c
index e7f8f3b161..c59b610691 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, "The command was disabled after fsfreeze.");
}
}
@@ -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, NULL);
l = g_list_next(l);
} while (l);
}
--
2.29.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: return a more explicit error on why a command is disabled
2021-02-16 13:38 [PATCH] qga: return a more explicit error on why a command is disabled marcandre.lureau
@ 2021-02-16 13:46 ` no-reply
2021-02-16 14:30 ` Peter Krempa
2021-02-16 15:14 ` Eric Blake
2 siblings, 0 replies; 5+ messages in thread
From: no-reply @ 2021-02-16 13:46 UTC (permalink / raw)
To: marcandre.lureau
Cc: michael.roth, pkrempa, qemu-devel, marcandre.lureau, armbru
Patchew URL: https://patchew.org/QEMU/20210216133837.2347190-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: 20210216133837.2347190-1-marcandre.lureau@redhat.com
Subject: [PATCH] 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
Switched to a new branch 'test'
a762179 qga: return a more explicit error on why a command is disabled
=== OUTPUT BEGIN ===
WARNING: line over 80 characters
#39: FILE: include/qapi/qmp/dispatch.h:48:
+void qmp_disable_command(QmpCommandList *cmds, const char *name, const char *err_msg);
WARNING: line over 80 characters
#52: FILE: qapi/qmp-dispatch.c:160:
+ cmd->err_msg ?: "The command %s has been disabled for this instance",
WARNING: line over 80 characters
#79: FILE: qapi/qmp-registry.c:59:
+void qmp_disable_command(QmpCommandList *cmds, const char *name, const char *err_msg)
ERROR: line over 90 characters
#101: FILE: qga/main.c:378:
+ qmp_disable_command(&ga_commands, name, "The command was disabled after fsfreeze.");
total: 1 errors, 3 warnings, 70 lines checked
Commit a7621793c6d6 (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/20210216133837.2347190-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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: return a more explicit error on why a command is disabled
2021-02-16 13:38 [PATCH] qga: return a more explicit error on why a command is disabled marcandre.lureau
2021-02-16 13:46 ` no-reply
@ 2021-02-16 14:30 ` Peter Krempa
2021-02-16 14:58 ` Marc-André Lureau
2021-02-16 15:14 ` Eric Blake
2 siblings, 1 reply; 5+ messages in thread
From: Peter Krempa @ 2021-02-16 14:30 UTC (permalink / raw)
To: marcandre.lureau; +Cc: michael.roth, qemu-devel, armbru
On Tue, Feb 16, 2021 at 17:38:37 +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qmp_disable_command() now takes an optional error string to return a
> more explicit error message.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1928806
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> include/qapi/qmp/dispatch.h | 3 ++-
> qapi/qmp-dispatch.c | 2 +-
> qapi/qmp-registry.c | 9 +++++----
> qga/main.c | 4 ++--
> 4 files changed, 10 insertions(+), 8 deletions(-)
[...]
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 0a2b20a4e4..ce4bf56b2c 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -157,7 +157,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> }
> if (!cmd->enabled) {
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> - "The command %s has been disabled for this instance",
> + cmd->err_msg ?: "The command %s has been disabled for this instance",
Passing in the formatting string from a variable looks shady and it's
hard to ensure that callers pass in the appropriate format modifier ...
> command);
> goto out;
> }
[...]
> diff --git a/qga/main.c b/qga/main.c
> index e7f8f3b161..c59b610691 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, "The command was disabled after fsfreeze.");
... such as here where '%s' is missing. Luckily that is not a problem
compared to when there would be more than one format modifier.
But the error message looks definitely better.
> }
> }
My suggestion would be to store a boolean flag that the command was
disabled due to an ongoing fsfreeze and then choose the appropriate
error message directly at the point where it's reported.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: return a more explicit error on why a command is disabled
2021-02-16 14:30 ` Peter Krempa
@ 2021-02-16 14:58 ` Marc-André Lureau
0 siblings, 0 replies; 5+ messages in thread
From: Marc-André Lureau @ 2021-02-16 14:58 UTC (permalink / raw)
To: Peter Krempa; +Cc: Michael Roth, QEMU, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]
On Tue, Feb 16, 2021 at 6:33 PM Peter Krempa <pkrempa@redhat.com> wrote:
> On Tue, Feb 16, 2021 at 17:38:37 +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > qmp_disable_command() now takes an optional error string to return a
> > more explicit error message.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1928806
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > include/qapi/qmp/dispatch.h | 3 ++-
> > qapi/qmp-dispatch.c | 2 +-
> > qapi/qmp-registry.c | 9 +++++----
> > qga/main.c | 4 ++--
> > 4 files changed, 10 insertions(+), 8 deletions(-)
>
> [...]
>
> > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> > index 0a2b20a4e4..ce4bf56b2c 100644
> > --- a/qapi/qmp-dispatch.c
> > +++ b/qapi/qmp-dispatch.c
> > @@ -157,7 +157,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds,
> QObject *request,
> > }
> > if (!cmd->enabled) {
> > error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> > - "The command %s has been disabled for this instance",
> > + cmd->err_msg ?: "The command %s has been disabled for
> this instance",
>
> Passing in the formatting string from a variable looks shady and it's
> hard to ensure that callers pass in the appropriate format modifier ...
>
> > command);
> > goto out;
> > }
>
> [...]
>
> > diff --git a/qga/main.c b/qga/main.c
> > index e7f8f3b161..c59b610691 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, "The command was
> disabled after fsfreeze.");
>
> ... such as here where '%s' is missing. Luckily that is not a problem
> compared to when there would be more than one format modifier.
>
>
Sure, although it's an internal API, I agree it's error prone.
But the error message looks definitely better.
>
> > }
> > }
>
> My suggestion would be to store a boolean flag that the command was
> disabled due to an ongoing fsfreeze and then choose the appropriate
> error message directly at the point where it's reported.
>
Let's make it an enum for the reason.
thanks
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3989 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qga: return a more explicit error on why a command is disabled
2021-02-16 13:38 [PATCH] qga: return a more explicit error on why a command is disabled marcandre.lureau
2021-02-16 13:46 ` no-reply
2021-02-16 14:30 ` Peter Krempa
@ 2021-02-16 15:14 ` Eric Blake
2 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2021-02-16 15:14 UTC (permalink / raw)
To: marcandre.lureau, qemu-devel; +Cc: michael.roth, pkrempa, armbru
On 2/16/21 7:38 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> qmp_disable_command() now takes an optional error string to return a
> more explicit error message.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1928806
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> +++ b/qapi/qmp-dispatch.c
> @@ -157,7 +157,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request,
> }
> if (!cmd->enabled) {
> error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> - "The command %s has been disabled for this instance",
> + cmd->err_msg ?: "The command %s has been disabled for this instance",
No trailing dot (good),...
> +++ 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, "The command was disabled after fsfreeze.");
...while this introduces one (not good).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-16 15:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-16 13:38 [PATCH] qga: return a more explicit error on why a command is disabled marcandre.lureau
2021-02-16 13:46 ` no-reply
2021-02-16 14:30 ` Peter Krempa
2021-02-16 14:58 ` Marc-André Lureau
2021-02-16 15:14 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.