2:42 p.m.
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
2:42 p.m.
New subject: [PATCH 1/8] tools: vsh: Unexport vshCmddefHelp
Signed-off-by: Peter Krempa <pkrempa(a)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
2:42 p.m.
New subject: [PATCH 2/8] tools: vshCmddefCheckInternals: Add parameter name to error message
If a parameter definition is invalid we can include the name of the
parameter for simpler debugging.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:43 p.m.
New subject: [PATCH 3/8] tools: vshCmddefCheckInternals: Port mandatory options check from vshCmddefOptParse
'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(a)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
2:43 p.m.
New subject: [PATCH 4/8] tools: cmdSelfTest: Drop misleading comment
We no longer print help for every command to validate the args.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:43 p.m.
New subject: [PATCH 5/8] tools: vshCmddefOptParse: Remove 'optional' command validation
Since vshCmddefCheckInternals now has this check we no longer need it in
vshCmddefOptParse.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:43 p.m.
New subject: [PATCH 6/8] tools: vshCmddefHelp: Don't call vshCmddefOptParse
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(a)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
2:43 p.m.
New subject: [PATCH 7/8] tools: vshCmddefOptParse: Remove return value
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(a)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
2:43 p.m.
New subject: [PATCH 8/8] tools: virsh: Reset error when keepalive registration fails
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(a)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
4:17 p.m.
New subject: [PATCH 8/8] tools: virsh: Reset error when keepalive registration fails
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(a)redhat.com>
---
tools/virsh.c | 1 +
1 file changed, 1 insertion(+)
5:09 p.m.
New subject: [PATCH 8/8] tools: virsh: Reset error when keepalive registration fails
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(a)redhat.com>
> ---
> tools/virsh.c | 1 +
> 1 file changed, 1 insertion(+)
4:18 p.m.
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(a)redhat.com>
Jano
1407
days inactive
1407
days old
11 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa