In local testing, I accidentally introduced a self-test failure,
and spent way too much time debugging it. Make sure the testsuite
log includes some hint as to why command option validation failed.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
tools/vsh.c | 56 +++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 42 insertions(+), 14 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 2fd1564d15..1d30019c2c 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -331,21 +331,26 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name)
/* Check if the internal command definitions are correct */
static int
-vshCmddefCheckInternals(const vshCmdDef *cmd)
+vshCmddefCheckInternals(vshControl *ctl,
+ const vshCmdDef *cmd)
{
size_t i;
const char *help = NULL;
/* in order to perform the validation resolve the alias first */
if (cmd->flags & VSH_CMD_FLAG_ALIAS) {
- if (!cmd->alias)
+ if (!cmd->alias) {
+ vshError(ctl, _("command '%s' has inconsistent alias"), cmd->name);
return -1;
+ }
cmd = vshCmddefSearch(cmd->alias);
}
/* Each command has to provide a non-empty help string. */
- if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help)
+ if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help) {
+ vshError(ctl, _("command '%s' lacks help"), cmd->name);
return -1;
+ }
if (!cmd->opts)
return 0;
@@ -353,14 +358,19 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
for (i = 0; cmd->opts[i].name; i++) {
const vshCmdOptDef *opt = &cmd->opts[i];
- if (i > 63)
+ if (i > 63) {
+ vshError(ctl, _("command '%s' has too many options"), cmd->name);
return -1; /* too many options */
+ }
switch (opt->type) {
case VSH_OT_STRING:
case VSH_OT_BOOL:
- if (opt->flags & VSH_OFLAG_REQ)
- return -1; /* nor bool nor string options can't be mandatory */
+ if (opt->flags & VSH_OFLAG_REQ) {
+ vshError(ctl, _("command '%s' misused VSH_OFLAG_REQ"),
+ cmd->name);
+ return -1; /* neither bool nor string options can be mandatory */
+ }
break;
case VSH_OT_ALIAS: {
@@ -368,11 +378,17 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
char *name = (char *)opt->help; /* cast away const */
char *p;
- if (opt->flags || !opt->help)
+ if (opt->flags || !opt->help) {
+ vshError(ctl, _("command '%s' has incorrect alias option"),
+ cmd->name);
return -1; /* alias options are tracked by the original name */
+ }
if ((p = strchr(name, '=')) &&
- VIR_STRNDUP(name, name, p - name) < 0)
+ VIR_STRNDUP(name, name, p - name) < 0) {
+ vshError(ctl, _("allocation failure while checking command '%s'"),
+ cmd->name);
return -1;
+ }
for (j = i + 1; cmd->opts[j].name; j++) {
if (STREQ(name, cmd->opts[j].name) &&
cmd->opts[j].type != VSH_OT_ALIAS)
@@ -381,21 +397,33 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
if (name != opt->help) {
VIR_FREE(name);
/* If alias comes with value, replacement must not be bool */
- if (cmd->opts[j].type == VSH_OT_BOOL)
+ if (cmd->opts[j].type == VSH_OT_BOOL) {
+ vshError(ctl, _("command '%s' has mismatched alias type"),
+ cmd->name);
return -1;
+ }
}
- if (!cmd->opts[j].name)
+ if (!cmd->opts[j].name) {
+ vshError(ctl, _("command '%s' has missing alias option"),
+ cmd->name);
return -1; /* alias option must map to a later option name */
+ }
}
break;
case VSH_OT_ARGV:
- if (cmd->opts[i + 1].name)
+ if (cmd->opts[i + 1].name) {
+ vshError(ctl, _("command '%s' has option after argv"),
+ cmd->name);
return -1; /* argv option must be listed last */
+ }
break;
case VSH_OT_DATA:
- if (!(opt->flags & VSH_OFLAG_REQ))
+ if (!(opt->flags & VSH_OFLAG_REQ)) {
+ vshError(ctl, _("command '%s' has non-required VSH_OT_DATA"),
+ cmd->name);
return -1; /* OT_DATA should always be required. */
+ }
break;
case VSH_OT_INT:
@@ -3405,7 +3433,7 @@ const vshCmdInfo info_selftest[] = {
* That runs vshCmddefOptParse which validates
* the per-command options structure. */
bool
-cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,
+cmdSelfTest(vshControl *ctl,
const vshCmd *cmd ATTRIBUTE_UNUSED)
{
const vshCmdGrp *grp;
@@ -3413,7 +3441,7 @@ cmdSelfTest(vshControl *ctl ATTRIBUTE_UNUSED,
for (grp = cmdGroups; grp->name; grp++) {
for (def = grp->commands; def->name; def++) {
- if (vshCmddefCheckInternals(def) < 0)
+ if (vshCmddefCheckInternals(ctl, def) < 0)
return false;
}
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 11, 2019 at 21:18:44 -0500, Eric Blake wrote: > In local testing, I accidentally introduced a self-test failure, > and spent way too much time debugging it. Make sure the testsuite > log includes some hint as to why command option validation failed. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tools/vsh.c | 56 +++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 42 insertions(+), 14 deletions(-) ACK -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 11, 2019 at 09:18:44PM -0500, Eric Blake wrote:
> In local testing, I accidentally introduced a self-test failure,
> and spent way too much time debugging it. Make sure the testsuite
> log includes some hint as to why command option validation failed.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> tools/vsh.c | 56 +++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 2fd1564d15..1d30019c2c 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -331,21 +331,26 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name)
>
> /* Check if the internal command definitions are correct */
> static int
> -vshCmddefCheckInternals(const vshCmdDef *cmd)
> +vshCmddefCheckInternals(vshControl *ctl,
> + const vshCmdDef *cmd)
> {
> size_t i;
> const char *help = NULL;
>
> /* in order to perform the validation resolve the alias first */
> if (cmd->flags & VSH_CMD_FLAG_ALIAS) {
> - if (!cmd->alias)
> + if (!cmd->alias) {
> + vshError(ctl, _("command '%s' has inconsistent alias"), cmd->name);
> return -1;
> + }
> cmd = vshCmddefSearch(cmd->alias);
> }
>
> /* Each command has to provide a non-empty help string. */
> - if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help)
> + if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help) {
> + vshError(ctl, _("command '%s' lacks help"), cmd->name);
> return -1;
> + }
>
> if (!cmd->opts)
> return 0;
> @@ -353,14 +358,19 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
> for (i = 0; cmd->opts[i].name; i++) {
> const vshCmdOptDef *opt = &cmd->opts[i];
>
> - if (i > 63)
> + if (i > 63) {
> + vshError(ctl, _("command '%s' has too many options"), cmd->name);
> return -1; /* too many options */
> + }
>
> switch (opt->type) {
> case VSH_OT_STRING:
> case VSH_OT_BOOL:
> - if (opt->flags & VSH_OFLAG_REQ)
> - return -1; /* nor bool nor string options can't be mandatory */
> + if (opt->flags & VSH_OFLAG_REQ) {
> + vshError(ctl, _("command '%s' misused VSH_OFLAG_REQ"),
> + cmd->name);
> + return -1; /* neither bool nor string options can be mandatory */
> + }
> break;
>
> case VSH_OT_ALIAS: {
> @@ -368,11 +378,17 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
> char *name = (char *)opt->help; /* cast away const */
> char *p;
>
> - if (opt->flags || !opt->help)
> + if (opt->flags || !opt->help) {
> + vshError(ctl, _("command '%s' has incorrect alias option"),
> + cmd->name);
> return -1; /* alias options are tracked by the original name */
> + }
> if ((p = strchr(name, '=')) &&
> - VIR_STRNDUP(name, name, p - name) < 0)
> + VIR_STRNDUP(name, name, p - name) < 0) {
> + vshError(ctl, _("allocation failure while checking command '%s'"),
> + cmd->name);
I think ^this one can be dropped, if there was an allocation failure, we have
much bigger problems and it's likely it will fail again which vshError will
transitively try to do if you look at vshOutputLogFile for example.
> return -1;
> + }
> for (j = i + 1; cmd->opts[j].name; j++) {
> if (STREQ(name, cmd->opts[j].name) &&
> cmd->opts[j].type != VSH_OT_ALIAS)
> @@ -381,21 +397,33 @@ vshCmddefCheckInternals(const vshCmdDef *cmd)
> if (name != opt->help) {
> VIR_FREE(name);
> /* If alias comes with value, replacement must not be bool */
> - if (cmd->opts[j].type == VSH_OT_BOOL)
> + if (cmd->opts[j].type == VSH_OT_BOOL) {
> + vshError(ctl, _("command '%s' has mismatched alias type"),
> + cmd->name);
> return -1;
> + }
> }
> - if (!cmd->opts[j].name)
> + if (!cmd->opts[j].name) {
> + vshError(ctl, _("command '%s' has missing alias option"),
> + cmd->name);
> return -1; /* alias option must map to a later option name */
> + }
> }
> break;
> case VSH_OT_ARGV:
> - if (cmd->opts[i + 1].name)
> + if (cmd->opts[i + 1].name) {
> + vshError(ctl, _("command '%s' has option after argv"),
> + cmd->name);
The commentary below actually gives me better idea about the error than the
error itself, can we use that text instead?
Reviewed-by: Erik Skultety <eskultet@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/12/19 2:50 AM, Erik Skultety wrote:
> On Mon, Mar 11, 2019 at 09:18:44PM -0500, Eric Blake wrote:
>> In local testing, I accidentally introduced a self-test failure,
>> and spent way too much time debugging it. Make sure the testsuite
>> log includes some hint as to why command option validation failed.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> - if (opt->flags || !opt->help)
>> + if (opt->flags || !opt->help) {
>> + vshError(ctl, _("command '%s' has incorrect alias option"),
>> + cmd->name);
>> return -1; /* alias options are tracked by the original name */
>> + }
>> if ((p = strchr(name, '=')) &&
>> - VIR_STRNDUP(name, name, p - name) < 0)
>> + VIR_STRNDUP(name, name, p - name) < 0) {
>> + vshError(ctl, _("allocation failure while checking command '%s'"),
>> + cmd->name);
>
> I think ^this one can be dropped, if there was an allocation failure, we have
> much bigger problems and it's likely it will fail again which vshError will
> transitively try to do if you look at vshOutputLogFile for example.
Agreed. We can't use assert() in libvirt.so, but virsh has other places
where it fits, so I'll switch this one to assert.
>> case VSH_OT_ARGV:
>> - if (cmd->opts[i + 1].name)
>> + if (cmd->opts[i + 1].name) {
>> + vshError(ctl, _("command '%s' has option after argv"),
>> + cmd->name);
>
> The commentary below actually gives me better idea about the error than the
> error itself, can we use that text instead?
Switched to 'does not list argv option last'
>
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>
Thanks; adjusted and pushed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Mar 12, 2019 at 06:40:42 -0500, Eric Blake wrote:
> On 3/12/19 2:50 AM, Erik Skultety wrote:
> > On Mon, Mar 11, 2019 at 09:18:44PM -0500, Eric Blake wrote:
> >> In local testing, I accidentally introduced a self-test failure,
> >> and spent way too much time debugging it. Make sure the testsuite
> >> log includes some hint as to why command option validation failed.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> >> - if (opt->flags || !opt->help)
> >> + if (opt->flags || !opt->help) {
> >> + vshError(ctl, _("command '%s' has incorrect alias option"),
> >> + cmd->name);
> >> return -1; /* alias options are tracked by the original name */
> >> + }
> >> if ((p = strchr(name, '=')) &&
> >> - VIR_STRNDUP(name, name, p - name) < 0)
> >> + VIR_STRNDUP(name, name, p - name) < 0) {
> >> + vshError(ctl, _("allocation failure while checking command '%s'"),
> >> + cmd->name);
> >
> > I think ^this one can be dropped, if there was an allocation failure, we have
> > much bigger problems and it's likely it will fail again which vshError will
> > transitively try to do if you look at vshOutputLogFile for example.
>
> Agreed. We can't use assert() in libvirt.so, but virsh has other places
> where it fits, so I'll switch this one to assert.
assert() is for debug-builds only, isn't it? Did you mean abort() ?
Still the construction proposed in the patch looks much better than
either of those.
Also for local output the printing is (partially) done into a file
descriptor, so you'll at least get the string 'error' printed.
Note also that virsh uses exit(EXIT_FAILURE) on allocation failure, so
neither abort nor assert should be used.
I think the appropriate solution is vshErrorOOM though, which by the way
is called also if the allocation in vshError fails.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.