| Message ID | 20210216133837.2347190-1-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qga: return a more explicit error on why a command is disabled | expand |
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
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.
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
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).
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); }