On Fri, Sep 16, 2016 at 12:50:38PM +0200, Erik Skultety wrote: Is there a use-case for a command without info? I think all the commands should have a description and we should enforce that in the test suite. Or maybe report an error here if the description is missing? Jan

On 16/09/16 14:19, Ján Tomko wrote: Well, if you look at domhostname command, it does provide an empty string for description, since there already is a help string that says everything important. So I think description does not need to be enforced in the test suite. But we sure should enforce checking for help. Command without help is not much of a help, is it... Erik

If the initial check is true the function immediately returns so there's no need to enclose the code following the check within an 'else' block. Also, by removing the 'else' block, the declarations need to be moved to beginning of the function block to conform with our guidelines. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tools/vsh.c | 222 ++++++++++++++++++++++++++++++------------------------------ 1 file changed, 111 insertions(+), 111 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 3772d92..4c63bd3 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -624,131 +624,131 @@ bool vshCmddefHelp(vshControl *ctl, const char *cmdname) { const vshCmdDef *def = vshCmddefSearch(cmdname); + /* Don't translate desc if it is "". */ + const char *desc = vshCmddefGetInfo(def, "desc"); + const char *help = _(vshCmddefGetInfo(def, "help")); + char buf[256]; + uint64_t opts_need_arg; + uint64_t opts_required; + bool shortopt = false; /* true if 'arg' works instead of '--opt arg' */ if (!def) { vshError(ctl, _("command '%s' doesn't exist"), cmdname); return false; - } else { - /* Don't translate desc if it is "". */ - const char *desc = vshCmddefGetInfo(def, "desc"); - const char *help = _(vshCmddefGetInfo(def, "help")); - 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, help); - - fputs(_("\n SYNOPSIS\n"), stdout); - fprintf(stdout, " %s", def->name); - if (def->opts) { - const vshCmdOptDef *opt; - for (opt = def->opts; opt->name; opt++) { - const char *fmt = "%s"; - switch (opt->type) { - case VSH_OT_BOOL: - fmt = "[--%s]"; - break; - case VSH_OT_INT: - /* xgettext:c-format */ - fmt = ((opt->flags & VSH_OFLAG_REQ) ? "<%s>" - : _("[--%s <number>]")); - if (!(opt->flags & VSH_OFLAG_REQ_OPT)) - shortopt = true; - break; - case VSH_OT_STRING: - /* xgettext:c-format */ - fmt = _("[--%s <string>]"); - if (!(opt->flags & VSH_OFLAG_REQ_OPT)) - shortopt = true; - break; - case VSH_OT_DATA: - fmt = ((opt->flags & VSH_OFLAG_REQ) ? "<%s>" : "[<%s>]"); - if (!(opt->flags & VSH_OFLAG_REQ_OPT)) - shortopt = true; - break; - case VSH_OT_ARGV: - /* xgettext:c-format */ - if (shortopt) { - fmt = (opt->flags & VSH_OFLAG_REQ) - ? _("{[--%s] <string>}...") - : _("[[--%s] <string>]..."); - } else { - fmt = (opt->flags & VSH_OFLAG_REQ) ? _("<%s>...") - : _("[<%s>]..."); - } - break; - case VSH_OT_ALIAS: - /* aliases are intentionally undocumented */ - continue; + 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, help); + + fputs(_("\n SYNOPSIS\n"), stdout); + fprintf(stdout, " %s", def->name); + if (def->opts) { + const vshCmdOptDef *opt; + for (opt = def->opts; opt->name; opt++) { + const char *fmt = "%s"; + switch (opt->type) { + case VSH_OT_BOOL: + fmt = "[--%s]"; + break; + case VSH_OT_INT: + /* xgettext:c-format */ + fmt = ((opt->flags & VSH_OFLAG_REQ) ? "<%s>" + : _("[--%s <number>]")); + if (!(opt->flags & VSH_OFLAG_REQ_OPT)) + shortopt = true; + break; + case VSH_OT_STRING: + /* xgettext:c-format */ + fmt = _("[--%s <string>]"); + if (!(opt->flags & VSH_OFLAG_REQ_OPT)) + shortopt = true; + break; + case VSH_OT_DATA: + fmt = ((opt->flags & VSH_OFLAG_REQ) ? "<%s>" : "[<%s>]"); + if (!(opt->flags & VSH_OFLAG_REQ_OPT)) + shortopt = true; + break; + case VSH_OT_ARGV: + /* xgettext:c-format */ + if (shortopt) { + fmt = (opt->flags & VSH_OFLAG_REQ) + ? _("{[--%s] <string>}...") + : _("[[--%s] <string>]..."); + } else { + fmt = (opt->flags & VSH_OFLAG_REQ) ? _("<%s>...") + : _("[<%s>]..."); } - fputc(' ', stdout); - fprintf(stdout, fmt, opt->name); + break; + case VSH_OT_ALIAS: + /* aliases are intentionally undocumented */ + continue; } + fputc(' ', stdout); + fprintf(stdout, fmt, opt->name); } - fputc('\n', stdout); + } + fputc('\n', stdout); - if (desc && *desc) { - /* Print the description only if it's not empty. */ - fputs(_("\n DESCRIPTION\n"), stdout); - fprintf(stdout, " %s\n", _(desc)); - } + if (desc && *desc) { + /* Print the description only if it's not empty. */ + fputs(_("\n DESCRIPTION\n"), stdout); + fprintf(stdout, " %s\n", _(desc)); + } - if (def->opts && def->opts->name) { - const vshCmdOptDef *opt; - fputs(_("\n OPTIONS\n"), stdout); - for (opt = def->opts; opt->name; opt++) { - switch (opt->type) { - case VSH_OT_BOOL: - snprintf(buf, sizeof(buf), "--%s", opt->name); - break; - case VSH_OT_INT: - snprintf(buf, sizeof(buf), - (opt->flags & VSH_OFLAG_REQ) ? _("[--%s] <number>") - : _("--%s <number>"), opt->name); - break; - case VSH_OT_STRING: - /* OT_STRING should never be VSH_OFLAG_REQ */ - if (opt->flags & VSH_OFLAG_REQ) { - vshError(ctl, - _("internal error: bad options in command: '%s'"), - def->name); - return false; - } - snprintf(buf, sizeof(buf), _("--%s <string>"), opt->name); - break; - case VSH_OT_DATA: - /* OT_DATA should always be VSH_OFLAG_REQ */ - if (!(opt->flags & VSH_OFLAG_REQ)) { - vshError(ctl, - _("internal error: bad options in command: '%s'"), - def->name); - return false; - } - snprintf(buf, sizeof(buf), _("[--%s] <string>"), - opt->name); - break; - case VSH_OT_ARGV: - snprintf(buf, sizeof(buf), - shortopt ? _("[--%s] <string>") : _("<%s>"), - opt->name); - break; - case VSH_OT_ALIAS: - continue; + if (def->opts && def->opts->name) { + const vshCmdOptDef *opt; + fputs(_("\n OPTIONS\n"), stdout); + for (opt = def->opts; opt->name; opt++) { + switch (opt->type) { + case VSH_OT_BOOL: + snprintf(buf, sizeof(buf), "--%s", opt->name); + break; + case VSH_OT_INT: + snprintf(buf, sizeof(buf), + (opt->flags & VSH_OFLAG_REQ) ? _("[--%s] <number>") + : _("--%s <number>"), opt->name); + break; + case VSH_OT_STRING: + /* OT_STRING should never be VSH_OFLAG_REQ */ + if (opt->flags & VSH_OFLAG_REQ) { + vshError(ctl, + _("internal error: bad options in command: '%s'"), + def->name); + return false; } - - fprintf(stdout, " %-15s %s\n", buf, _(opt->help)); + snprintf(buf, sizeof(buf), _("--%s <string>"), opt->name); + break; + case VSH_OT_DATA: + /* OT_DATA should always be VSH_OFLAG_REQ */ + if (!(opt->flags & VSH_OFLAG_REQ)) { + vshError(ctl, + _("internal error: bad options in command: '%s'"), + def->name); + return false; + } + snprintf(buf, sizeof(buf), _("[--%s] <string>"), + opt->name); + break; + case VSH_OT_ARGV: + snprintf(buf, sizeof(buf), + shortopt ? _("[--%s] <string>") : _("<%s>"), + opt->name); + break; + case VSH_OT_ALIAS: + continue; } + + fprintf(stdout, " %-15s %s\n", buf, _(opt->help)); } - fputc('\n', stdout); } + fputc('\n', stdout); + return true; } -- 2.5.5

Recent changes discarded an unnecessary 'else' block from vshCmddefHelp. However, that also moved some var declarations as well as definitions to the beginning of the function block and now we're trying to retrieve command's .info element before actually checking whether we got a valid command or not. So this patch fixes it accordingly. Also it removes variable 'help', since there isn't any functional need for it. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tools/vsh.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 4c63bd3..4d16299 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -624,9 +624,7 @@ bool vshCmddefHelp(vshControl *ctl, const char *cmdname) { const vshCmdDef *def = vshCmddefSearch(cmdname); - /* Don't translate desc if it is "". */ - const char *desc = vshCmddefGetInfo(def, "desc"); - const char *help = _(vshCmddefGetInfo(def, "help")); + const char *desc = NULL; char buf[256]; uint64_t opts_need_arg; uint64_t opts_required; @@ -644,7 +642,8 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) } fputs(_(" NAME\n"), stdout); - fprintf(stdout, " %s - %s\n", def->name, help); + fprintf(stdout, " %s - %s\n", def->name, + _(vshCmddefGetInfo(def, "help"))); fputs(_("\n SYNOPSIS\n"), stdout); fprintf(stdout, " %s", def->name); @@ -695,6 +694,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) } fputc('\n', stdout); + desc = vshCmddefGetInfo(def, "desc"); if (desc && *desc) { /* Print the description only if it's not empty. */ fputs(_("\n DESCRIPTION\n"), stdout); -- 2.5.5

Originally introduced by commit 2432521e which correctly split vshCmddefOptParse into command's options validation and options parsing. However, command's 'internals' are not tied solely to .options, rather it should be about the overall structure, therefore the validation should be extracted from vshCmddefOptParse and performed before any attempt to parse the command's options. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- tools/vsh.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 4d16299..0626e4f 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -423,9 +423,6 @@ static int vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, uint64_t *opts_required) { - if (vshCmddefCheckInternals(cmd) < 0) - return -1; - if (vshCmddefOptFill(cmd, opts_need_arg, opts_required) < 0) return -1; @@ -635,6 +632,12 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) return false; } + if (vshCmddefCheckInternals(def) < 0) { + vshError(ctl, _("internal error: wrong command structure: '%s'"), + def->name); + return false; + } + if (vshCmddefOptParse(def, &opts_need_arg, &opts_required)) { vshError(ctl, _("internal error: bad options in command: '%s'"), def->name); @@ -1410,6 +1413,11 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) vshError(ctl, _("unknown command: '%s'"), tkdata); goto syntaxError; /* ... or ignore this command only? */ } + if (vshCmddefCheckInternals(cmd) < 0) { + vshError(ctl, _("internal error: wrong command structure: " + "'%s'"), tkdata); + goto syntaxError; + } if (vshCmddefOptParse(cmd, &opts_need_arg, &opts_required) < 0) { vshError(ctl, @@ -3344,8 +3352,8 @@ const vshCmdInfo info_selftest[] = { }; /* Prints help for every command. - * That runs vshCmddefOptParse which validates - * the per-command options structure. */ + * That runs vshCmddefCheckInternals which validates + * the overall command structure. */ bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { -- 2.5.5

On Fri, Sep 16, 2016 at 12:50:43PM +0200, Erik Skultety wrote: This man page change is unrelated to the logic change and would fit better in a separate commit. There is a call to vshCmddefSearch in vshReadlineParse which also seems to need to resolve the alias. ACK with that fixed. Without it, the behavior is a bit unexpected: virsh # nodedev-det nodedev-detach nodedev-dettach But the options are only offered for nodedev-detach, not the alias. Jan