[PATCH 0/8] tools: vsh: Fix validation of command parameter lists
Recent patch: https://www.redhat.com/archives/libvir-list/2020-November/msg00555.html passes build check but then virsh complains when started that the command list is wrong. Fix it by moving the check to the appropriate place. You can use the above patch as reproducer. Peter Krempa (8): tools: vsh: Unexport vshCmddefHelp tools: vshCmddefCheckInternals: Add parameter name to error message tools: vshCmddefCheckInternals: Port mandatory options check from vshCmddefOptParse tools: cmdSelfTest: Drop misleading comment tools: vshCmddefOptParse: Remove 'optional' command validation tools: vshCmddefHelp: Don't call vshCmddefOptParse tools: vshCmddefOptParse: Remove return value tools: virsh: Reset error when keepalive registration fails tools/virsh.c | 1 + tools/vsh.c | 108 +++++++++++++++++++++----------------------------- tools/vsh.h | 1 - 3 files changed, 47 insertions(+), 63 deletions(-) -- 2.28.0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 2 +- tools/vsh.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index ca92bcd78c..e5ecc38f73 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -600,7 +600,7 @@ vshCmdGrpHelp(vshControl *ctl, const vshCmdGrp *grp) return true; } -bool +static bool vshCmddefHelp(vshControl *ctl, const vshCmdDef *def) { const char *desc = NULL; diff --git a/tools/vsh.h b/tools/vsh.h index 5583fef9cc..0c5584891d 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -248,7 +248,6 @@ void vshCloseLogFile(vshControl *ctl); const char *vshCmddefGetInfo(const vshCmdDef *cmd, const char *info); const vshCmdDef *vshCmddefSearch(const char *cmdname); -bool vshCmddefHelp(vshControl *ctl, const vshCmdDef *def); const vshCmdGrp *vshCmdGrpSearch(const char *grpname); bool vshCmdGrpHelp(vshControl *ctl, const vshCmdGrp *grp); -- 2.28.0
If a parameter definition is invalid we can include the name of the parameter for simpler debugging. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index e5ecc38f73..d1e795bbc1 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -307,8 +307,8 @@ vshCmddefCheckInternals(vshControl *ctl, case VSH_OT_STRING: case VSH_OT_BOOL: if (opt->flags & VSH_OFLAG_REQ) { - vshError(ctl, _("command '%s' misused VSH_OFLAG_REQ"), - cmd->name); + vshError(ctl, _("parameter '%s' of command '%s' misused VSH_OFLAG_REQ"), + opt->name, cmd->name); return -1; /* neither bool nor string options can be mandatory */ } break; @@ -319,8 +319,8 @@ vshCmddefCheckInternals(vshControl *ctl, char *p; if (opt->flags || !opt->help) { - vshError(ctl, _("command '%s' has incorrect alias option"), - cmd->name); + vshError(ctl, _("parameter '%s' of command '%s' has incorrect alias option"), + opt->name, cmd->name); return -1; /* alias options are tracked by the original name */ } if ((p = strchr(name, '='))) @@ -334,30 +334,30 @@ vshCmddefCheckInternals(vshControl *ctl, VIR_FREE(name); /* If alias comes with value, replacement must not be bool */ if (cmd->opts[j].type == VSH_OT_BOOL) { - vshError(ctl, _("command '%s' has mismatched alias type"), - cmd->name); + vshError(ctl, _("alias '%s' of command '%s' has mismatched alias type"), + opt->name, cmd->name); return -1; } } if (!cmd->opts[j].name) { - vshError(ctl, _("command '%s' has missing alias option"), - cmd->name); + vshError(ctl, _("alias '%s' of command '%s' has missing alias option"), + opt->name, cmd->name); return -1; /* alias option must map to a later option name */ } } break; case VSH_OT_ARGV: if (cmd->opts[i + 1].name) { - vshError(ctl, _("command '%s' does not list argv option last"), - cmd->name); + vshError(ctl, _("parameter '%s' of command '%s' must be listed last"), + opt->name, cmd->name); return -1; /* argv option must be listed last */ } break; case VSH_OT_DATA: if (!(opt->flags & VSH_OFLAG_REQ)) { - vshError(ctl, _("command '%s' has non-required VSH_OT_DATA"), - cmd->name); + vshError(ctl, _("parameter '%s' of command '%s' must use VSH_OFLAG_REQ flag"), + opt->name, cmd->name); return -1; /* OT_DATA should always be required. */ } break; -- 2.28.0
'vshCmddefCheckInternals' is the go-to place for all checks related to the definition of parameters for commands, but the check that all mandatory parameters must be ordered before optional parameters was still only in vshCmddefOptParse. Adding a non-compliant option would not be caught by our test suite as 'virsh self-test' doesn't call vshCmddefOptParse. Re-implement the check in vshCmddefCheckInternals. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tools/vsh.c b/tools/vsh.c index d1e795bbc1..28c7533a25 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -276,6 +276,7 @@ vshCmddefCheckInternals(vshControl *ctl, { size_t i; const char *help = NULL; + bool seenOptionalOption = false; /* in order to perform the validation resolve the alias first */ if (cmd->flags & VSH_CMD_FLAG_ALIAS) { @@ -311,6 +312,8 @@ vshCmddefCheckInternals(vshControl *ctl, opt->name, cmd->name); return -1; /* neither bool nor string options can be mandatory */ } + + seenOptionalOption = true; break; case VSH_OT_ALIAS: { @@ -360,9 +363,25 @@ vshCmddefCheckInternals(vshControl *ctl, opt->name, cmd->name); return -1; /* OT_DATA should always be required. */ } + + if (seenOptionalOption) { + vshError(ctl, _("parameter '%s' of command '%s' must be listed before optional parameters"), + opt->name, cmd->name); + return -1; /* mandatory options must be listed first */ + } break; case VSH_OT_INT: + if (opt->flags & VSH_OFLAG_REQ) { + if (seenOptionalOption) { + vshError(ctl, _("parameter '%s' of command '%s' must be listed before optional parameters"), + opt->name, cmd->name); + return -1; /* mandatory options must be listed first */ + } + } else { + seenOptionalOption = true; + } + break; } } -- 2.28.0
We no longer print help for every command to validate the args. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 28c7533a25..cf4ddc1c2c 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3324,9 +3324,6 @@ const vshCmdInfo info_selftest[] = { {.name = NULL} }; -/* Prints help for every command. - * That runs vshCmddefOptParse which validates - * the per-command options structure. */ bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) -- 2.28.0
Since vshCmddefCheckInternals now has this check we no longer need it in vshCmddefOptParse. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index cf4ddc1c2c..f92759f219 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -399,7 +399,6 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, uint64_t *opts_required) { size_t i; - bool optional = false; *opts_need_arg = 0; *opts_required = 0; @@ -410,30 +409,17 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, for (i = 0; cmd->opts[i].name; i++) { const vshCmdOptDef *opt = &cmd->opts[i]; - if (opt->type == VSH_OT_BOOL) { - optional = true; + if (opt->type == VSH_OT_BOOL) continue; - } - - if (opt->flags & VSH_OFLAG_REQ_OPT) { - if (opt->flags & VSH_OFLAG_REQ) - *opts_required |= 1ULL << i; - else - optional = true; - continue; - } if (opt->type == VSH_OT_ALIAS) continue; /* skip the alias option */ - *opts_need_arg |= 1ULL << i; - if (opt->flags & VSH_OFLAG_REQ) { - if (optional && opt->type != VSH_OT_ARGV) - return -1; /* mandatory options must be listed first */ + if (!(opt->flags & VSH_OFLAG_REQ_OPT)) + *opts_need_arg |= 1ULL << i; + + if (opt->flags & VSH_OFLAG_REQ) *opts_required |= 1ULL << i; - } else { - optional = true; - } } return 0; -- 2.28.0
The help formatter called vshCmddefOptParse just for validation purposes. Since vshCmddefOptParse no longer validates the command itself and we don't need the bitmaps returned by it we can drop the call entirely. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index f92759f219..2de940277e 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -606,20 +606,12 @@ vshCmdGrpHelp(vshControl *ctl, const vshCmdGrp *grp) } static bool -vshCmddefHelp(vshControl *ctl, const vshCmdDef *def) +vshCmddefHelp(const vshCmdDef *def) { const char *desc = NULL; char buf[256]; - uint64_t opts_need_arg; - uint64_t opts_required; bool shortopt = false; /* true if 'arg' works instead of '--opt arg' */ - if (vshCmddefOptParse(def, &opts_need_arg, &opts_required)) { - vshError(ctl, _("internal error: bad options in command: '%s'"), - def->name); - return false; - } - fputs(_(" NAME\n"), stdout); fprintf(stdout, " %s - %s\n", def->name, _(vshCmddefGetInfo(def, "help"))); @@ -3113,7 +3105,7 @@ cmdHelp(vshControl *ctl, const vshCmd *cmd) if ((def = vshCmddefSearch(name))) { if (def->flags & VSH_CMD_FLAG_ALIAS) def = vshCmddefSearch(def->alias); - return vshCmddefHelp(ctl, def); + return vshCmddefHelp(def); } else if ((grp = vshCmdGrpSearch(name))) { return vshCmdGrpHelp(ctl, grp); } else { -- 2.28.0
The function can't fail so there's no need to return a value or check it in the callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 2de940277e..badd37c08e 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -389,13 +389,11 @@ vshCmddefCheckInternals(vshControl *ctl, } /* Parse the options associated with @cmd, i.e. test whether options are - * required or need an argument. - * - * Returns -1 on error or 0 on success, filling the caller-provided bitmaps - * which keep track of required options and options needing an argument. + * required or need an argument and fill the appropriate caller-provided bitmaps */ -static int -vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, +static void +vshCmddefOptParse(const vshCmdDef *cmd, + uint64_t *opts_need_arg, uint64_t *opts_required) { size_t i; @@ -404,7 +402,7 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, *opts_required = 0; if (!cmd->opts) - return 0; + return; for (i = 0; cmd->opts[i].name; i++) { const vshCmdOptDef *opt = &cmd->opts[i]; @@ -421,8 +419,6 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, if (opt->flags & VSH_OFLAG_REQ) *opts_required |= 1ULL << i; } - - return 0; } static vshCmdOptDef helpopt = { @@ -1388,14 +1384,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser, vshCmd **partial) tkdata = g_strdup(cmd->alias); cmd = vshCmddefSearch(tkdata); } - if (vshCmddefOptParse(cmd, &opts_need_arg, - &opts_required) < 0) { - if (!partial) - vshError(ctl, - _("internal error: bad options in command: '%s'"), - tkdata); - goto syntaxError; - } + + vshCmddefOptParse(cmd, &opts_need_arg, &opts_required); VIR_FREE(tkdata); } else if (data_only) { goto get_data; -- 2.28.0
We try to enable keepalive oportunistically. If it's not supported by the connection driver and it was not explicitly requested we keep the error object set and can report it in some cases accidentally: --- stdout --- TEST: /home/pipo/libvirt/tests/virsh-self-test ! 1 FAILED --- stderr --- error: parameter 'target' of command 'attach-disk' must be listed before optional parameters error: this function is not supported by the connection driver: virConnectSetKeepAlive ------- Clear the stored libvirt error. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh.c b/tools/virsh.c index 954778b626..a6bfbbbb87 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -176,6 +176,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) } vshDebug(ctl, VSH_ERR_INFO, "%s", _("Failed to setup keepalive on connection\n")); + vshResetLibvirtError(); } cleanup: -- 2.28.0
On a Thursday in 2020, Peter Krempa wrote:
We try to enable keepalive oportunistically. If it's not supported by
*opportunistically
the connection driver and it was not explicitly requested we keep the error object set and can report it in some cases accidentally:
I was going to complain about the trailing ':' when I realized 'git am' dropped everything below. Consider indenting it or using a different separator. Jano
--- stdout --- TEST: /home/pipo/libvirt/tests/virsh-self-test ! 1 FAILED --- stderr --- error: parameter 'target' of command 'attach-disk' must be listed before optional parameters error: this function is not supported by the connection driver: virConnectSetKeepAlive -------
Clear the stored libvirt error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh.c | 1 + 1 file changed, 1 insertion(+)
On Thu, Nov 12, 2020 at 16:17:22 +0100, Ján Tomko wrote:
On a Thursday in 2020, Peter Krempa wrote:
We try to enable keepalive oportunistically. If it's not supported by
*opportunistically
the connection driver and it was not explicitly requested we keep the error object set and can report it in some cases accidentally:
I was going to complain about the trailing ':' when I realized 'git am' dropped everything below. Consider indenting it or using a different separator.
oops, I've copypasted from test output and it didn't occur to me.
Jano
--- stdout --- TEST: /home/pipo/libvirt/tests/virsh-self-test ! 1 FAILED --- stderr --- error: parameter 'target' of command 'attach-disk' must be listed before optional parameters error: this function is not supported by the connection driver: virConnectSetKeepAlive -------
Clear the stored libvirt error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh.c | 1 + 1 file changed, 1 insertion(+)
On a Thursday in 2020, Peter Krempa wrote:
Recent patch:
https://www.redhat.com/archives/libvir-list/2020-November/msg00555.html
passes build check but then virsh complains when started that the command list is wrong.
Fix it by moving the check to the appropriate place.
You can use the above patch as reproducer.
Peter Krempa (8): tools: vsh: Unexport vshCmddefHelp tools: vshCmddefCheckInternals: Add parameter name to error message tools: vshCmddefCheckInternals: Port mandatory options check from vshCmddefOptParse tools: cmdSelfTest: Drop misleading comment tools: vshCmddefOptParse: Remove 'optional' command validation tools: vshCmddefHelp: Don't call vshCmddefOptParse tools: vshCmddefOptParse: Remove return value tools: virsh: Reset error when keepalive registration fails
tools/virsh.c | 1 + tools/vsh.c | 108 +++++++++++++++++++++----------------------------- tools/vsh.h | 1 - 3 files changed, 47 insertions(+), 63 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa