[libvirt] [PATCH v3 0/8] Introduce aliases for virt-admin's srv-* commands
the original version: https://www.redhat.com/archives/libvir-list/2016-September/msg00312.html v2: https://www.redhat.com/archives/libvir-list/2016-September/msg00380.html Since this series (compared to v2) does a tiny bit of refactor (tweaking might be a better word in this case) as well, I was also looking into refactoring the parser in vsh.c....again...And again I gave up on doing that (this time a lot faster though) because that would mean to rewrite a lot of code mainly to enable usage of our virString* methods. I also found an ANSI C GNU-styled command line parser library [1] which might even cope with our command and option aliases (I didn't look into that much though, so take it with a grain of salt). However, though looking good, it would most likely hit the wall once it encountered our "multi-command" feature in the interactive shell mode :( [1] http://argtable.sourceforge.net/ since v1: - tweaked the virsh-self-test so that it also checks the aliased commands instead of skipping them (since there was a good reason for that before the changes this series introduces) - patches (now) 7-8 remained untouched since v2: - Michal was indeed right about having a proper check for missing aliases which would eventually result in a reasonable internal error - so the check was put into vshCmddefCheckInternals, but that function should not be called from within vshCmddefOptParse because it's not just about checking the .opts but the overall command structure and at the time of parsing the arguments we should already know whether the command structure is valid or not - since vshCmddefOptParse has been split once again, merge it with vshCmddefOptFill and drop the latter Erik Skultety (8): vsh: Fix NULL dereference leading to SIGSEGV if a command is missing '.info' vsh: vshCmddefHelp: Drop the unnecessary 'else' branch vsh: vshCmddefHelp: Retrieve command info after we know the command is non-NULL vsh: Extract vshCmddefCheckInternals from vshCmddefOptParse vsh: discard vshCmddefOptFill and move its body to vshCmddefOptParse virt-admin: Tweak command parsing logic so that aliases point to new commands virt-admin: Add some command aliases to provide syntax sugar over ugly commands virt-admin: Replace the (now) aliases with new command names in the man page tools/virsh-nodedev.c | 6 +- tools/virsh.pod | 2 - tools/virt-admin.c | 24 +++++ tools/virt-admin.pod | 30 +++--- tools/vsh.c | 280 ++++++++++++++++++++++++++------------------------ tools/vsh.h | 4 +- 6 files changed, 191 insertions(+), 155 deletions(-) -- 2.5.5
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index 4ee472c..3772d92 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -695,7 +695,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) } fputc('\n', stdout); - if (desc[0]) { + if (desc && *desc) { /* Print the description only if it's not empty. */ fputs(_("\n DESCRIPTION\n"), stdout); fprintf(stdout, " %s\n", _(desc)); -- 2.5.5
On Fri, Sep 16, 2016 at 12:50:38PM +0200, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.
diff --git a/tools/vsh.c b/tools/vsh.c index 4ee472c..3772d92 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -695,7 +695,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) } fputc('\n', stdout);
- if (desc[0]) { + if (desc && *desc) {
Or maybe report an error here if the description is missing? Jan
/* Print the description only if it's not empty. */ fputs(_("\n DESCRIPTION\n"), stdout); fprintf(stdout, " %s\n", _(desc)); -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 16/09/16 14:19, Ján Tomko wrote:
On Fri, Sep 16, 2016 at 12:50:38PM +0200, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.
Well, if I call vshCmddefCheckInternals from the self-test as suggested by 4/8, then we could also go your suggested way here as well. I fixed the rest and will push it in a while. I'll send an updated version of this later. Thanks, Erik
diff --git a/tools/vsh.c b/tools/vsh.c index 4ee472c..3772d92 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -695,7 +695,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) } fputc('\n', stdout);
- if (desc[0]) { + if (desc && *desc) {
Or maybe report an error here if the description is missing?
Jan
/* Print the description only if it's not empty. */ fputs(_("\n DESCRIPTION\n"), stdout); fprintf(stdout, " %s\n", _(desc)); -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 16/09/16 14:19, Ján Tomko wrote:
On Fri, Sep 16, 2016 at 12:50:38PM +0200, Erik Skultety wrote:
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.
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
diff --git a/tools/vsh.c b/tools/vsh.c index 4ee472c..3772d92 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -695,7 +695,7 @@ vshCmddefHelp(vshControl *ctl, const char *cmdname) } fputc('\n', stdout);
- if (desc[0]) { + if (desc && *desc) {
Or maybe report an error here if the description is missing?
Jan
/* Print the description only if it's not empty. */ fputs(_("\n DESCRIPTION\n"), stdout); fprintf(stdout, " %s\n", _(desc)); -- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
The intention is to move vshCmddefCheckInternals out of vshCmddefOptParse to our test suite. First step to do that is to enforce checking for an existing help string (that also means it's non-empty) in a command because a command without a help is not much of a use. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/vsh.c b/tools/vsh.c index 4ee472c..f3b3fca 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -328,6 +328,11 @@ static int vshCmddefCheckInternals(const vshCmdDef *cmd) { size_t i; + const char *help = NULL; + + /* Each command has to provide a non-empty help string. */ + if (!(help = vshCmddefGetInfo(cmd, "help")) || !*help) + return -1; if (!cmd->opts) return 0; -- 2.5.5
On Mon, Sep 19, 2016 at 03:00:26PM +0200, Erik Skultety wrote:
The intention is to move vshCmddefCheckInternals out of vshCmddefOptParse to our test suite. First step to do that is to enforce checking for an existing help string (that also means it's non-empty) in a command because a command without a help is not much of a use.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK Jan
On 20/09/16 14:43, Ján Tomko wrote:
On Mon, Sep 19, 2016 at 03:00:26PM +0200, Erik Skultety wrote:
The intention is to move vshCmddefCheckInternals out of vshCmddefOptParse to our test suite. First step to do that is to enforce checking for an existing help string (that also means it's non-empty) in a command because a command without a help is not much of a use.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK
Jan
Thanks, as I said in on of the latter patches, I adjusted the patches according to your notes and pushed the series. 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@redhat.com> --- 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
On Fri, Sep 16, 2016 at 12:50:39PM +0200, Erik Skultety wrote:
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@redhat.com> --- 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"));
This introduces a NULL defererence if the command does not exist. ACK with that fixed. Jan
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@redhat.com> --- 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
On Fri, Sep 16, 2016 at 12:50:40PM +0200, Erik Skultety wrote:
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@redhat.com> --- 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");
This can be squashed into the previous patch.
- 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")));
And this change (along with the 'help' variable removal) can stand on its own before the previous patch. Jan
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@redhat.com> --- 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:41PM +0200, Erik Skultety wrote:
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@redhat.com> --- tools/vsh.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
ACK While this change makes sense on its own, this function is only used for a run-time check of our data. Maybe the only place to call it should be the self test command? Jan
On 16/09/16 13:58, Ján Tomko wrote:
On Fri, Sep 16, 2016 at 12:50:41PM +0200, Erik Skultety wrote:
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@redhat.com> --- tools/vsh.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
ACK
While this change makes sense on its own, this function is only used for a run-time check of our data. Maybe the only place to call it should be the self test command?
Jan
I think we could do this, since not paying attention to the results of our test suite would be developer's ignorance, so I'll update the patch. Erik
Recent changes extracted the command internals validation routine from vshCmddefOptParse method which now just calls vshCmddefOptFill. Therefore, make vshCmddefOptFill the new vshCmddefOptParse and drop the unnecessary name. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 0626e4f..a66e2f9 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -373,10 +373,15 @@ vshCmddefCheckInternals(const vshCmdDef *cmd) return 0; } -/* Keeps track of options that are required or need and argument */ +/* 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. + */ static int -vshCmddefOptFill(const vshCmdDef *cmd, uint64_t *opts_need_arg, - uint64_t *opts_required) +vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, + uint64_t *opts_required) { size_t i; bool optional = false; @@ -415,16 +420,6 @@ vshCmddefOptFill(const vshCmdDef *cmd, uint64_t *opts_need_arg, optional = true; } } - return 0; -} - -/* Validate that the options associated with cmd can be parsed. */ -static int -vshCmddefOptParse(const vshCmdDef *cmd, uint64_t *opts_need_arg, - uint64_t *opts_required) -{ - if (vshCmddefOptFill(cmd, opts_need_arg, opts_required) < 0) - return -1; return 0; } @@ -2719,8 +2714,8 @@ vshReadlineParse(const char *text, int state) goto error; cmd_exists = true; - if (vshCmddefOptFill(cmd, &const_opts_need_arg, - &const_opts_required) < 0) + if (vshCmddefOptParse(cmd, &const_opts_need_arg, + &const_opts_required) < 0) goto error; opts_need_arg = const_opts_need_arg; opts_seen = const_opts_seen; -- 2.5.5
On Fri, Sep 16, 2016 at 12:50:42PM +0200, Erik Skultety wrote:
Recent changes extracted the command internals validation routine from vshCmddefOptParse method which now just calls vshCmddefOptFill. Therefore, make vshCmddefOptFill the new vshCmddefOptParse and drop the unnecessary name.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)
ACK Jan
Change the logic in a way, so that VSH_CMD_FLAG_ALIAS behaves similarly to how VSH_OT_ALIAS for command options, i.e. there is no need for code duplication for the alias and the aliased command structures. Along with that change, switch any existing VSH_CMD_FLAG_ALIAS occurrences to this new format. Also, since this patch introduces a new command structure element, adjust the virsh-self-test test to make sure we won't ever miss to specify the '.alias' member for an aliased command because doing that would lead to an internal error. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 6 ++---- tools/virsh.pod | 2 -- tools/vsh.c | 15 +++++++++++++-- tools/vsh.h | 4 +++- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 321f15c..9446664 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -986,10 +986,8 @@ const vshCmdDef nodedevCmds[] = { .flags = 0 }, {.name = "nodedev-dettach", - .handler = cmdNodeDeviceDetach, - .opts = opts_node_device_detach, - .info = info_node_device_detach, - .flags = VSH_CMD_FLAG_ALIAS + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "nodedev-detach" }, {.name = "nodedev-dumpxml", .handler = cmdNodeDeviceDumpXML, diff --git a/tools/virsh.pod b/tools/virsh.pod index 3da7879..49abda9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2936,8 +2936,6 @@ make that device unusable by the rest of the physical host until a reboot. Detach I<nodedev> from the host, so that it can safely be used by guests via <hostdev> passthrough. This is reversed with B<nodedev-reattach>, and is done automatically for managed devices. -For compatibility purposes, this command can also be spelled -B<nodedev-dettach>. Different backend drivers expect the device to be bound to different dummy devices. For example, QEMU's "kvm" backend driver (the default) diff --git a/tools/vsh.c b/tools/vsh.c index a66e2f9..88a6f37 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -329,6 +329,9 @@ vshCmddefCheckInternals(const vshCmdDef *cmd) { size_t i; + if (cmd->flags & VSH_CMD_FLAG_ALIAS && !cmd->alias) + return -1; + if (!cmd->opts) return 0; @@ -1408,6 +1411,13 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) vshError(ctl, _("unknown command: '%s'"), tkdata); goto syntaxError; /* ... or ignore this command only? */ } + + /* aliases need to be resolved to the actual commands */ + if (cmd->flags & VSH_CMD_FLAG_ALIAS) { + VIR_FREE(tkdata); + tkdata = vshStrdup(ctl, cmd->alias); + cmd = vshCmddefSearch(tkdata); + } if (vshCmddefCheckInternals(cmd) < 0) { vshError(ctl, _("internal error: wrong command structure: " "'%s'"), tkdata); @@ -3359,10 +3369,11 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) for (grp = cmdGroups; grp->name; grp++) { for (def = grp->commands; def->name; def++) { + const vshCmdDef *c = def; if (def->flags & VSH_CMD_FLAG_ALIAS) - continue; + c = vshCmddefSearch(c->alias); - if (!vshCmddefHelp(ctl, def->name)) + if (!vshCmddefHelp(ctl, c->name)) return false; } } diff --git a/tools/vsh.h b/tools/vsh.h index 7f08191..8f5d1a6 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -179,6 +179,7 @@ struct _vshCmdDef { const vshCmdOptDef *opts; /* definition of command options */ const vshCmdInfo *info; /* details about command */ unsigned int flags; /* bitwise OR of VSH_CMD_FLAG */ + const char *alias; /* name of the aliased command */ }; /* @@ -445,7 +446,8 @@ bool cmdSelfTest(vshControl *ctl, const vshCmd *cmd); .handler = cmdSelfTest, \ .opts = NULL, \ .info = info_selftest, \ - .flags = VSH_CMD_FLAG_NOCONNECT | VSH_CMD_FLAG_ALIAS \ + .flags = VSH_CMD_FLAG_NOCONNECT | VSH_CMD_FLAG_ALIAS, \ + .alias = "self-test" \ } -- 2.5.5
On Fri, Sep 16, 2016 at 12:50:43PM +0200, Erik Skultety wrote:
Change the logic in a way, so that VSH_CMD_FLAG_ALIAS behaves similarly to how VSH_OT_ALIAS for command options, i.e. there is no need for code duplication for the alias and the aliased command structures. Along with that change, switch any existing VSH_CMD_FLAG_ALIAS occurrences to this new format. Also, since this patch introduces a new command structure element, adjust the virsh-self-test test to make sure we won't ever miss to specify the '.alias' member for an aliased command because doing that would lead to an internal error.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh-nodedev.c | 6 ++---- tools/virsh.pod | 2 -- tools/vsh.c | 15 +++++++++++++-- tools/vsh.h | 4 +++- 4 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 321f15c..9446664 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -986,10 +986,8 @@ const vshCmdDef nodedevCmds[] = { .flags = 0 }, {.name = "nodedev-dettach", - .handler = cmdNodeDeviceDetach, - .opts = opts_node_device_detach, - .info = info_node_device_detach, - .flags = VSH_CMD_FLAG_ALIAS + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "nodedev-detach" }, {.name = "nodedev-dumpxml", .handler = cmdNodeDeviceDumpXML,
diff --git a/tools/virsh.pod b/tools/virsh.pod index 3da7879..49abda9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2936,8 +2936,6 @@ make that device unusable by the rest of the physical host until a reboot. Detach I<nodedev> from the host, so that it can safely be used by guests via <hostdev> passthrough. This is reversed with B<nodedev-reattach>, and is done automatically for managed devices. -For compatibility purposes, this command can also be spelled -B<nodedev-dettach>.
Different backend drivers expect the device to be bound to different dummy devices. For example, QEMU's "kvm" backend driver (the default)
This man page change is unrelated to the logic change and would fit better in a separate commit.
diff --git a/tools/vsh.c b/tools/vsh.c index a66e2f9..88a6f37 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -329,6 +329,9 @@ vshCmddefCheckInternals(const vshCmdDef *cmd) { size_t i;
+ if (cmd->flags & VSH_CMD_FLAG_ALIAS && !cmd->alias) + return -1; + if (!cmd->opts) return 0;
@@ -1408,6 +1411,13 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) vshError(ctl, _("unknown command: '%s'"), tkdata); goto syntaxError; /* ... or ignore this command only? */ } + + /* aliases need to be resolved to the actual commands */ + if (cmd->flags & VSH_CMD_FLAG_ALIAS) { + VIR_FREE(tkdata); + tkdata = vshStrdup(ctl, cmd->alias); + cmd = vshCmddefSearch(tkdata); + }
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
Make use of the new recently introduced alias handling for virt-admin srv-* commands. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 36c92f5..b749acb 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1266,18 +1266,30 @@ static const vshCmdDef vshAdmCmds[] = { static const vshCmdDef monitoringCmds[] = { {.name = "srv-list", + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "server-list" + }, + {.name = "server-list", .handler = cmdSrvList, .opts = NULL, .info = info_srv_list, .flags = 0 }, {.name = "srv-threadpool-info", + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "server-threadpool-info" + }, + {.name = "server-threadpool-info", .handler = cmdSrvThreadpoolInfo, .opts = opts_srv_threadpool_info, .info = info_srv_threadpool_info, .flags = 0 }, {.name = "srv-clients-list", + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "client-list" + }, + {.name = "client-list", .handler = cmdSrvClientsList, .opts = opts_srv_clients_list, .info = info_srv_clients_list, @@ -1290,6 +1302,10 @@ static const vshCmdDef monitoringCmds[] = { .flags = 0 }, {.name = "srv-clients-info", + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "server-clients-info" + }, + {.name = "server-clients-info", .handler = cmdSrvClientsInfo, .opts = opts_srv_clients_info, .info = info_srv_clients_info, @@ -1300,6 +1316,10 @@ static const vshCmdDef monitoringCmds[] = { static const vshCmdDef managementCmds[] = { {.name = "srv-threadpool-set", + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "server-threadpool-set" + }, + {.name = "server-threadpool-set", .handler = cmdSrvThreadpoolSet, .opts = opts_srv_threadpool_set, .info = info_srv_threadpool_set, @@ -1312,6 +1332,10 @@ static const vshCmdDef managementCmds[] = { .flags = 0 }, {.name = "srv-clients-set", + .flags = VSH_CMD_FLAG_ALIAS, + .alias = "server-clients-set" + }, + {.name = "server-clients-set", .handler = cmdSrvClientsSet, .opts = opts_srv_clients_set, .info = info_srv_clients_set, -- 2.5.5
On Fri, Sep 16, 2016 at 12:50:44PM +0200, Erik Skultety wrote:
Make use of the new recently introduced alias handling for virt-admin srv-* commands.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
ACK Jan
Since the old command names are being shadowed by the new ones in the code as well as in the help messages, update the man page accordingly. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.pod | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tools/virt-admin.pod b/tools/virt-admin.pod index 020eb51..443730e 100644 --- a/tools/virt-admin.pod +++ b/tools/virt-admin.pod @@ -150,7 +150,7 @@ change its internal configuration. =over 4 -=item B<srv-list> +=item B<server-list> Lists all manageable servers contained within the daemon the client is currently connected to. @@ -164,7 +164,7 @@ The I<server> is specified by its name. =over 4 -=item B<srv-threadpool-info> I<server> +=item B<server-threadpool-info> I<server> Retrieve server's threadpool attributes. These attributes include: @@ -207,11 +207,11 @@ control and libvirt guarantees that such a task cannot hang, thus will always finish. An example of such a task this would be destroying a domain: $ virsh destroy <domain>. -=item B<srv-threadpool-set> I<server> [I<--min-workers> B<count>] +=item B<server-threadpool-set> I<server> [I<--min-workers> B<count>] [I<--max-workers> B<count>] [I<--priority-workers> B<count>] Change threadpool attributes on a server. Only a fraction of all attributes as -described in I<srv-threadpool-info> is supported for the setter. +described in I<server-threadpool-info> is supported for the setter. =over 4 @@ -232,14 +232,7 @@ The current number of active priority workers in a threadpool. =back -=item B<srv-clients-list> I<server> - -Print a table showing the list of clients connected to <server>, also providing -information about transport type used on client's connection (supported -transports include B<unix>, B<tcp>, and B<tls>), as well as providing -information about client's connection time (system local time is used). - -=item B<srv-clients-info> I<server> +=item B<server-clients-info> I<server> Get information about the current setting of limits regarding connections of new clients. This information comprises of the limits to the maximum number of @@ -249,13 +242,13 @@ runtime values, more specifically, the current number of clients connected to I<server> and the current number of clients waiting for authentication. B<Example> - # virt-admin srv-clients-info libvirtd + # virt-admin server-clients-info libvirtd nclients_max : 120 nclients : 3 nclients_unauth_max : 20 nclients_unauth : 0 -=item B<srv-clients-set> I<server> [I<--max-clients> B<count>] +=item B<server-clients-set> I<server> [I<--max-clients> B<count>] [I<--max-unauth-clients> B<count>] Set new client-related limits on I<server>. @@ -284,10 +277,17 @@ I<--max-clients>. The following commands provide management and monitoring of clients connected to one of daemon's available servers. Clients are specified by their numeric ID which is obtained by listing all clients connected to a specified server -(see command B<srv-clients-list>). +(see command B<client-list>). =over 4 +=item B<client-list> I<server> + +Print a table showing the list of clients connected to <server>, also providing +information about transport type used on client's connection (supported +transports include B<unix>, B<tcp>, and B<tls>), as well as providing +information about client's connection time (system local time is used). + =item B<client-info> I<server> I<client> Retrieve identity information about I<client> from I<server>. The attributes -- 2.5.5
On Fri, Sep 16, 2016 at 12:50:45PM +0200, Erik Skultety wrote:
Since the old command names are being shadowed by the new ones in the code as well as in the help messages, update the man page accordingly.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virt-admin.pod | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
ACK, I would squash this together with the previous patch. Jan
participants (2)
-
Erik Skultety -
Ján Tomko