[PATCH 00/28] vsh: Fix handling of commands and help - part 2 (unwanted positional parsing of optional arguments)
Part 2 was supposed to be the refactor of the parser, but it turns out that our annotations of arguments and quirks of the parser itself resulted with basically every optional argument with value being also considered for positional parisng. This series explains where this happens and annotates all cases. Peter Krempa (28): vshCmddefHelp: Drop empty line at the end virsh: cmdDomdisplayReload: Require option name for --type vshCmddefCheckInternals: Improve some checks virsh: Inline VIRSH_COMMON_OPT_DOMAIN_OT_STRING macro virsh: Inline VIRSH_COMMON_OPT_FILE_FULL macro virsh: Inline VIRSH_COMMON_OPT_NETWORK_OT_STRING macro vsh: Introduce tool to find unwanted positional arguments to 'self-test' vsh: Introduce annotation for vsh options which are unexpectedly parsed positionally virsh: Annotate '--diskspec' _ARGV options as unwanted positional virsh: Annotate rest of _ARGV arguments as positional vsh: Fix option formatting for 'VHS_OT_ARGV' options virsh: Require option flags for 'blkdeviotune' arguments virsh: Fix "positional" argument annotations for 'migrate' command virsh: Require option flags for all optional arguments of 'attach-disk' virsh-checkpoint: Make 'checkpointname' positional and required vsh: Allow one optional positional argument vsh: Make the only argument of 'connect', 'cd', and 'help' commands positional virsh: snapshot: Make 'snapshotname' argument positional virsh: Make '(snapshot|checkpoint)-create' 'xmlfile' argument positional virsh-backup: Fix argument annotations of 'backup-begin' command virsh: Annotate some optional arguments as positional virsh: volume: Mark optional 'pool' argument as 'positional' virsh: Annotate "unwanted_positional" arguments for 'pool-(define|create)-as' commands virsh: Annodate 'unwanted_positional' arguments virt-admin: Annodate 'unwanted_positional' arguments vsh: Make positional parsing of arguments opt-in vsh: Replace 'VSH_OFLAG_EMPTY_OK' bitwise flag with a separate struct member vshCmdOptDef: Remove unused 'flags' member tools/virsh-backup.c | 3 +- tools/virsh-checkpoint.c | 26 ++++--- tools/virsh-domain-event.c | 10 ++- tools/virsh-domain-monitor.c | 11 +-- tools/virsh-domain.c | 134 ++++++++++++++++++++++++++++------- tools/virsh-host.c | 33 +++++++-- tools/virsh-interface.c | 2 +- tools/virsh-network.c | 28 ++++---- tools/virsh-nodedev.c | 6 +- tools/virsh-nwfilter.c | 2 - tools/virsh-pool.c | 31 ++++++-- tools/virsh-secret.c | 6 +- tools/virsh-snapshot.c | 13 +++- tools/virsh-volume.c | 12 +++- tools/virsh.c | 3 +- tools/virsh.h | 20 +----- tools/virt-admin.c | 15 ++-- tools/vsh.c | 64 +++++++++++++---- tools/vsh.h | 18 +++-- 19 files changed, 316 insertions(+), 121 deletions(-) -- 2.44.0
All virsh commands in non-quiet mode append another separator line thus having two is unnecessary and in quiet mode it still has a trailing blank line. Remove it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/vsh.c b/tools/vsh.c index f96071060a..e6ea3a398a 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -725,7 +725,6 @@ vshCmddefHelp(const vshCmdDef *def) fprintf(stdout, " %-15s %s\n", optstr, _(opt->help)); } } - fputc('\n', stdout); return true; } -- 2.44.0
On a Friday in 2024, Peter Krempa wrote:
All virsh commands in non-quiet mode append another separator line thus having two is unnecessary and in quiet mode it still has a trailing blank line. Remove it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
As this command was introduced in this release add the flag requiring to pass optionname. This is needed to actually disallow positional parsing of the value despite documenting that the flag name is required. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 35809a866b..cd37828660 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13290,6 +13290,7 @@ static const vshCmdOptDef opts_domdisplay_reload[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "type", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("graphics display type") }, {.name = NULL} -- 2.44.0
On a Friday in 2024, Peter Krempa wrote:
As this command was introduced in this release add the flag requiring to pass optionname.
This is needed to actually disallow positional parsing of the value despite documenting that the flag name is required.
The flag name is documented as optional in the man page.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Mar 20, 2024 at 18:03:16 +0100, Ján Tomko wrote:
On a Friday in 2024, Peter Krempa wrote:
As this command was introduced in this release add the flag requiring to pass optionname.
This is needed to actually disallow positional parsing of the value despite documenting that the flag name is required.
The flag name is documented as optional in the man page.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
I've fixed the manpage and pushed this one as it's modifying definition for an unreleased command. The rest will wait for the release.
- move the check that completer_flags are 0 if no completer is set into a common place and remove duplication - add check that _BOOL arguments are not positional - add missing checks to _ALIAS Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index e6ea3a398a..a4235905dc 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -332,6 +332,13 @@ vshCmddefCheckInternals(vshControl *ctl, return -1; } + /* Mandate no completer flags if no completer is specified */ + if (opt->completer_flags != 0 && !opt->completer) { + vshError(ctl, "completer_flags of argument '%s' of command '%s' must be 0 if no completer is used", + opt->name, cmd->name); + return -1; + } + switch (opt->type) { case VSH_OT_NONE: vshError(ctl, "invalid type 'NONE' of option '%s' of command '%s'", @@ -339,12 +346,18 @@ vshCmddefCheckInternals(vshControl *ctl, return -1; case VSH_OT_BOOL: - if (opt->completer || opt->completer_flags) { + if (opt->completer) { vshError(ctl, "bool parameter '%s' of command '%s' has completer set", opt->name, cmd->name); return -1; } + if (opt->positional) { + vshError(ctl, "boolean parameter '%s' of command '%s' must not be positional", + opt->name, cmd->name); + return -1; + } + if (opt->required) { vshError(ctl, "parameter '%s' of command '%s' misused 'required' flag", opt->name, cmd->name); @@ -358,7 +371,11 @@ vshCmddefCheckInternals(vshControl *ctl, g_autofree char *name = NULL; char *p; - if (opt->flags || !opt->help) { + if (opt->required || + opt->positional || + opt->completer || + opt->flags || + !opt->help) { vshError(ctl, "parameter '%s' of command '%s' has incorrect alias option", opt->name, cmd->name); return -1; -- 2.44.0
Upcoming patches will need to tweak some of the properties of the command. Since the macro is used in just two places expand it inline. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 7 +++++-- tools/virsh-domain.c | 14 +++++++++++--- tools/virsh.h | 13 ------------- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 2ff7f7b81c..73b00f3bb2 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -865,8 +865,11 @@ static const vshCmdInfo info_event = { }; static const vshCmdOptDef opts_event[] = { - VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), - 0, 0), + {.name = "domain", + .type = VSH_OT_STRING, + .help = N_("filter by domain name, id or uuid"), + .completer = virshDomainNameCompleter, + }, {.name = "event", .type = VSH_OT_STRING, .completer = virshDomainEventNameCompleter, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index cd37828660..0bf4d524b0 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9826,8 +9826,11 @@ static const vshCmdInfo info_qemu_monitor_event = { }; static const vshCmdOptDef opts_qemu_monitor_event[] = { - VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("filter by domain name, id or uuid"), - 0, 0), + {.name = "domain", + .type = VSH_OT_STRING, + .help = N_("filter by domain name, id or uuid"), + .completer = virshDomainNameCompleter, + }, {.name = "event", .type = VSH_OT_STRING, .help = N_("filter by event name") @@ -10322,7 +10325,12 @@ static const vshCmdOptDef opts_domxmltonative[] = { .required = true, .help = N_("target config data type format") }, - VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(VSH_OFLAG_REQ_OPT, 0), + {.name = "domain", + .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("domain name, id or uuid"), + .completer = virshDomainNameCompleter, + }, {.name = "xml", .type = VSH_OT_STRING, .completer = virshCompletePathLocalExisting, diff --git a/tools/virsh.h b/tools/virsh.h index 1114c1572b..ff0cf80911 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -109,19 +109,6 @@ .help = _helpstr \ } -#define VIRSH_COMMON_OPT_DOMAIN_OT_STRING(_helpstr, oflags, cflags) \ - {.name = "domain", \ - .type = VSH_OT_STRING, \ - .flags = oflags, \ - .help = _helpstr, \ - .completer = virshDomainNameCompleter, \ - .completer_flags = cflags, \ - } - -#define VIRSH_COMMON_OPT_DOMAIN_OT_STRING_FULL(oflags, cflags) \ - VIRSH_COMMON_OPT_DOMAIN_OT_STRING(N_("domain name, id or uuid"), \ - oflags, cflags) - typedef struct _virshControl virshControl; typedef struct _virshCtrlData virshCtrlData; -- 2.44.0
The macro is used in one place only and the command definition will be altered. Inline it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-host.c | 7 +++++-- tools/virsh.h | 7 ++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index ffb993de79..712db39d35 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -1635,8 +1635,11 @@ static const vshCmdInfo info_hypervisor_cpu_baseline = { }; static const vshCmdOptDef opts_hypervisor_cpu_baseline[] = { - VIRSH_COMMON_OPT_FILE_FULL(N_("file containing XML CPU descriptions"), - false), + {.name = "file", + .type = VSH_OT_STRING, + .completer = virshCompletePathLocalExisting, + .help = N_("file containing XML CPU descriptions"), + }, {.name = "virttype", .type = VSH_OT_STRING, .completer = virshDomainVirtTypeCompleter, diff --git a/tools/virsh.h b/tools/virsh.h index ff0cf80911..90ffc4bf18 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -98,13 +98,10 @@ /* Use this only for files which are existing and used locally by virsh */ #define VIRSH_COMMON_OPT_FILE(_helpstr) \ - VIRSH_COMMON_OPT_FILE_FULL(_helpstr, true) - -#define VIRSH_COMMON_OPT_FILE_FULL(_helpstr, required_) \ {.name = "file", \ .type = VSH_OT_STRING, \ - .required = required_, \ - .positional = required_, \ + .required = true, \ + .positional = true, \ .completer = virshCompletePathLocalExisting, \ .help = _helpstr \ } -- 2.44.0
The macro is used in just one place and the definition of the option is going to be modified. Inline the macro. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-network.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 597e3d4530..240d10eea7 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -43,17 +43,6 @@ #define VIRSH_COMMON_OPT_NETWORK_FULL(cflags) \ VIRSH_COMMON_OPT_NETWORK(N_("network name or uuid"), cflags) -#define VIRSH_COMMON_OPT_NETWORK_OT_STRING(_helpstr, cflags) \ - {.name = "network", \ - .type = VSH_OT_STRING, \ - .help = _helpstr, \ - .completer = virshNetworkNameCompleter, \ - .completer_flags = cflags, \ - } - -#define VIRSH_COMMON_OPT_NETWORK_OT_STRING_FULL(cflags) \ - VIRSH_COMMON_OPT_NETWORK_OT_STRING(N_("network name or uuid"), cflags) - #define VIRSH_COMMON_OPT_NETWORK_PORT(cflags) \ {.name = "port", \ .type = VSH_OT_STRING, \ @@ -1587,7 +1576,11 @@ static const vshCmdInfo info_network_event = { }; static const vshCmdOptDef opts_network_event[] = { - VIRSH_COMMON_OPT_NETWORK_OT_STRING(N_("filter by network name or uuid"), 0), + {.name = "network", + .type = VSH_OT_STRING, + .help = N_("filter by network name or uuid"), + .completer = virshNetworkNameCompleter, + }, {.name = "event", .type = VSH_OT_STRING, .completer = virshNetworkEventNameCompleter, -- 2.44.0
While the virsh option definitions specify (either explicitly after recent refactors, or implicitly before) whether an argument is positional or not, the actual parser is way more lax and actually and allows also arguments which were considered/documented as non-positional to be filled positionally unless VSH_OFLAG_REQ_OPT is used in the flags. This creates situations such as 'snapshot-create-as' which has the following docs: SYNOPSIS snapshot-create-as <domain> [--name <string>] [--description <string>] [--print-xml] [--no-metadata] [--halt] [--disk-only] [--reuse-external] [--quiesce] [--atomic] [--live] [--validate] [--memspec <string>] [[--diskspec] <string>]... Thus showing as if '--name' and '--description' required the option, but in fact the following happens when only positionals are passed: $ virsh snapshot-create-as --print-xml 1 2 3 4 5 <domainsnapshot> <name>2</name> <description>3</description> <disks> <disk name='4'/> <disk name='5'/> </disks> </domainsnapshot> In the above example e.g. '--memspec' is not populated. This disconnect makes it impossible to refactor the parser itself and allows users to write buggy interactions with virsh. In order to address this we'll be annotating every single of these unwanted positional options as such so that this doesn't happen in the future, while still preserving the quirk in the parser. This patch introduces a tool which outputs list of options which are not marked as positional but are lacking the VSH_OFLAG_REQ_OPT flag. This tool will be removed once all the offenders found by it will be addressed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index a4235905dc..96d6a5c238 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -244,11 +244,13 @@ static int disconnected; /* we may have been disconnected */ static int vshCmddefCheckInternals(vshControl *ctl, const vshCmdDef *cmd, - bool missingCompleters) + bool missingCompleters, + int brokenPositionals) { size_t i; bool seenOptionalOption = false; g_auto(virBuffer) complbuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) posbuf = VIR_BUFFER_INITIALIZER; /* in order to perform the validation resolve the alias first */ if (cmd->alias) { @@ -325,6 +327,28 @@ vshCmddefCheckInternals(vshControl *ctl, } } + if (brokenPositionals >= 0) { + switch (opt->type) { + case VSH_OT_INT: + case VSH_OT_STRING: + case VSH_OT_ARGV: + if (brokenPositionals == 0 || + brokenPositionals == opt->type) { + if (!(opt->flags & VSH_OFLAG_REQ_OPT) && !opt->positional) + virBufferStrcat(&posbuf, opt->name, ", ", NULL); + } + break; + + case VSH_OT_BOOL: + /* only name is completed */ + /* no point in completing numbers */ + case VSH_OT_ALIAS: + /* alias is handled in the referenced command */ + case VSH_OT_NONE: + break; + } + } + /* require that positional non-argv options are required */ if (opt->positional && !opt->required && opt->type != VSH_OT_ARGV) { vshError(ctl, "positional argument '%s' of command '%s' must be required", @@ -427,10 +451,15 @@ vshCmddefCheckInternals(vshControl *ctl, } virBufferTrim(&complbuf, ", "); + virBufferTrim(&posbuf, ", "); if (missingCompleters && virBufferUse(&complbuf) > 0) vshPrintExtra(ctl, "%s: %s\n", cmd->name, virBufferCurrentContent(&complbuf)); + if (virBufferUse(&posbuf)) { + vshPrintExtra(ctl, "%s: %s\n", cmd->name, virBufferCurrentContent(&posbuf)); + } + return 0; } @@ -3336,6 +3365,11 @@ const vshCmdOptDef opts_selftest[] = { .type = VSH_OT_BOOL, .help = N_("output help for each command") }, + {.name = "broken-positionals", + .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("debug positional args") + }, {.name = NULL} }; const vshCmdInfo info_selftest = { @@ -3350,6 +3384,9 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd) const vshCmdDef *def; bool completers = vshCommandOptBool(cmd, "completers-missing"); bool dumphelp = vshCommandOptBool(cmd, "dump-help"); + int brokenPositionals = -1; + + ignore_value(vshCommandOptInt(ctl, cmd, "broken-positionals", &brokenPositionals)); for (grp = cmdGroups; grp->name; grp++) { for (def = grp->commands; def->name; def++) { @@ -3357,7 +3394,7 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd) if (dumphelp && !def->alias) vshCmddefHelp(def); - if (vshCmddefCheckInternals(ctl, def, completers) < 0) + if (vshCmddefCheckInternals(ctl, def, completers, brokenPositionals) < 0) return false; } } -- 2.44.0
Based on the rationale in previous commit, all commands which were parsed as positional but not documented as such will be annotated with this flag. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 12 ++++++++++-- tools/vsh.h | 7 +++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 96d6a5c238..eeee58d39e 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -334,7 +334,8 @@ vshCmddefCheckInternals(vshControl *ctl, case VSH_OT_ARGV: if (brokenPositionals == 0 || brokenPositionals == opt->type) { - if (!(opt->flags & VSH_OFLAG_REQ_OPT) && !opt->positional) + if (!(opt->flags & VSH_OFLAG_REQ_OPT) && + !(opt->positional || opt->unwanted_positional)) virBufferStrcat(&posbuf, opt->name, ", ", NULL); } break; @@ -363,6 +364,12 @@ vshCmddefCheckInternals(vshControl *ctl, return -1; } + if (opt->unwanted_positional && opt->positional) { + vshError(ctl, "unwanted_positional flag of argument '%s' of command '%s' must not be used together with positional", + opt->name, cmd->name); + return -1; + } + switch (opt->type) { case VSH_OT_NONE: vshError(ctl, "invalid type 'NONE' of option '%s' of command '%s'", @@ -376,7 +383,7 @@ vshCmddefCheckInternals(vshControl *ctl, return -1; } - if (opt->positional) { + if (opt->positional || opt->unwanted_positional) { vshError(ctl, "boolean parameter '%s' of command '%s' must not be positional", opt->name, cmd->name); return -1; @@ -397,6 +404,7 @@ vshCmddefCheckInternals(vshControl *ctl, if (opt->required || opt->positional || + opt->unwanted_positional || opt->completer || opt->flags || !opt->help) { diff --git a/tools/vsh.h b/tools/vsh.h index cd833a55fa..fdcc89b12a 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -133,6 +133,13 @@ struct _vshCmdOptDef { vshCmdOptType type; /* option type */ bool required; /* option is required */ bool positional; /* option is a positional option (not requiring '--optionname') */ + + /* Historically the command parser in virsh allowed many optional arguments + * which were documented as non-positional to be filled positionally. To + * preserve this functionality those need to be annotated with the + * 'unwanted_positional' flag. New options must not use this flag */ + bool unwanted_positional; + unsigned int flags; /* flags */ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ -- 2.44.0
Our documentation in most places explicitly mentions --diskspec and it was never meant to be positional, although we can't change the parser any more. Annotate them as 'unwanted_positional'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-checkpoint.c | 1 + tools/virsh-snapshot.c | 1 + 2 files changed, 2 insertions(+) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 869c071700..65061cbf3d 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -206,6 +206,7 @@ static const vshCmdOptDef opts_checkpoint_create_as[] = { }, {.name = "diskspec", .type = VSH_OT_ARGV, + .unwanted_positional = true, .help = N_("disk attributes: disk[,checkpoint=type][,bitmap=name]") }, {.name = NULL} diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 899bae7e9a..638333f4d6 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -361,6 +361,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { }, {.name = "diskspec", .type = VSH_OT_ARGV, + .unwanted_positional = true, .help = N_("disk attributes: disk[,snapshot=type][,driver=type][,stype=type][,file=name]") }, {.name = NULL} -- 2.44.0
In most cases it's the usual/recommended way to use those commands: $ virsh qemu-monitor-command VMNAME cmd args args args Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 8 ++++++++ tools/virsh-network.c | 1 + 2 files changed, 9 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0bf4d524b0..801b01bb42 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -5000,6 +5000,7 @@ static const vshCmdOptDef opts_schedinfo[] = { VIRSH_COMMON_OPT_LIVE(N_("get/set value from running domain")), {.name = "set", .type = VSH_OT_ARGV, + .positional = true, .flags = VSH_OFLAG_NONE, .help = N_("parameter=value") }, @@ -8188,6 +8189,7 @@ static const vshCmdOptDef opts_desc[] = { }, {.name = "new-desc", .type = VSH_OT_ARGV, + .positional = true, .help = N_("message") }, {.name = NULL} @@ -8493,6 +8495,7 @@ static const vshCmdOptDef opts_send_key[] = { {.name = "keycode", .type = VSH_OT_ARGV, .required = true, + .positional = true, .completer = virshKeycodeNameCompleter, .help = N_("the key code") }, @@ -9600,6 +9603,7 @@ static const vshCmdOptDef opts_qemu_monitor_command[] = { }, {.name = "cmd", .type = VSH_OT_ARGV, + .positional = true, .required = true, .help = N_("command") }, @@ -9996,6 +10000,7 @@ static const vshCmdOptDef opts_qemu_agent_command[] = { }, {.name = "cmd", .type = VSH_OT_ARGV, + .positional = true, .required = true, .help = N_("command") }, @@ -10092,6 +10097,7 @@ static const vshCmdOptDef opts_lxc_enter_namespace[] = { }, {.name = "cmd", .type = VSH_OT_ARGV, + .positional = true, .required = true, .help = N_("command to run") }, @@ -12795,6 +12801,7 @@ static const vshCmdOptDef opts_domfsfreeze[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "mountpoint", .type = VSH_OT_ARGV, + .positional = true, .completer = virshDomainFSMountpointsCompleter, .help = N_("mountpoint path to be frozen") }, @@ -12835,6 +12842,7 @@ static const vshCmdOptDef opts_domfsthaw[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "mountpoint", .type = VSH_OT_ARGV, + .positional = true, .completer = virshDomainFSMountpointsCompleter, .help = N_("mountpoint path to be thawed") }, diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 240d10eea7..59ca842181 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -324,6 +324,7 @@ static const vshCmdOptDef opts_network_desc[] = { }, {.name = "new-desc", .type = VSH_OT_ARGV, + .positional = true, .help = N_("message") }, {.name = NULL} -- 2.44.0
While previous fixes kept the help output unchanged as base for the refactors it turns out that the formatting of help for argv options is wrong. Specifically in SYNOPSIS the non-positional _ARGV would have the option name in square brackets (which in other cases means that given thing is optional) despite being required. Similarly in the DESCRIPTION section positional versions would not show the optional argument name and also didn't use the three dots to signal that it can be used multiple times. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index eeee58d39e..cbddc5ed92 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -714,9 +714,9 @@ vshCmddefHelp(const vshCmdDef *def) } } else { if (opt->required) { - fprintf(stdout, _(" {[--%1$s] <string>}..."), opt->name); + fprintf(stdout, _(" --%1$s <string>..."), opt->name); } else { - fprintf(stdout, _(" [[--%1$s] <string>]..."), opt->name); + fprintf(stdout, _(" [--%1$s <string>]..."), opt->name); } } break; @@ -765,9 +765,9 @@ vshCmddefHelp(const vshCmdDef *def) case VSH_OT_ARGV: if (opt->positional) { - optstr = g_strdup_printf("<%s>", opt->name); + optstr = g_strdup_printf(_("[--%1$s] <string>..."), opt->name); } else { - optstr = g_strdup_printf(_("[--%1$s] <string>"), opt->name); + optstr = g_strdup_printf(_("--%1$s <string>..."), opt->name); } break; -- 2.44.0
Make all of the tunable parameter flags require the option name (don't parse them positionally). While techically this would be a breaking change if anyone were to specify the tunable values positionally this is not the case as the first two tunables are not compatible with each other: $ virsh blkdeviotune cd vda 4 5 error: Unable to change block I/O throttle error: invalid argument: total and read/write of bytes_sec cannot be set at the same time The above is produced by all implementations of the API (qemu and test drivers). It is true that the first tunable can be specified positionally (--total-bytes-sec) but it is misleading and shoud not be allowed either. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 801b01bb42..c8b0896eb6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1204,6 +1204,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-bytes-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("total throughput limit, as scaled integer (default bytes)") }, {.name = "read_bytes_sec", @@ -1212,6 +1213,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-bytes-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("read throughput limit, as scaled integer (default bytes)") }, {.name = "write_bytes_sec", @@ -1220,6 +1222,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-bytes-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("write throughput limit, as scaled integer (default bytes)") }, {.name = "total_iops_sec", @@ -1228,6 +1231,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-iops-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("total I/O operations limit per second") }, {.name = "read_iops_sec", @@ -1236,6 +1240,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-iops-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("read I/O operations limit per second") }, {.name = "write_iops_sec", @@ -1244,6 +1249,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-iops-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("write I/O operations limit per second") }, {.name = "total_bytes_sec_max", @@ -1252,6 +1258,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-bytes-sec-max", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("total max, as scaled integer (default bytes)") }, {.name = "read_bytes_sec_max", @@ -1260,6 +1267,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-bytes-sec-max", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("read max, as scaled integer (default bytes)") }, {.name = "write_bytes_sec_max", @@ -1268,6 +1276,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-bytes-sec-max", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("write max, as scaled integer (default bytes)") }, {.name = "total_iops_sec_max", @@ -1276,6 +1285,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-iops-sec-max", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("total I/O operations max") }, {.name = "read_iops_sec_max", @@ -1284,6 +1294,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-iops-sec-max", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("read I/O operations max") }, {.name = "write_iops_sec_max", @@ -1292,6 +1303,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-iops-sec-max", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("write I/O operations max") }, {.name = "size_iops_sec", @@ -1300,6 +1312,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "size-iops-sec", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("I/O size in bytes") }, {.name = "group_name", @@ -1308,6 +1321,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "group-name", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("group name to share I/O quota between multiple drives") }, @@ -1317,6 +1331,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-bytes-sec-max-length", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow total max bytes") }, {.name = "read_bytes_sec_max_length", @@ -1325,6 +1340,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-bytes-sec-max-length", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow read max bytes") }, {.name = "write_bytes_sec_max_length", @@ -1333,6 +1349,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-bytes-sec-max-length", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow write max bytes") }, {.name = "total_iops_sec_max_length", @@ -1341,6 +1358,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-iops-sec-max-length", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow total I/O operations max") }, {.name = "read_iops_sec_max_length", @@ -1349,6 +1367,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-iops-sec-max-length", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow read I/O operations max") }, {.name = "write_iops_sec_max_length", @@ -1357,6 +1376,7 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-iops-sec-max-length", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow write I/O operations max") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, -- 2.44.0
Annotate arguments which can be unintentionally parsed positionally. (See previous commits for explanation.) Annotate '--migrateuri', '--graphicsuri', '--listen-address', '-dname', '--timeout', '--xml', '--migrate-disks' and '--disks port' as 'unwanted_positional'. These were declared in chronological order per git history. All others are annotated with VSH_OFLAG_REQ_OPT which makes the parser require the '--optionname'. This is due to the fact that '--disks-uri' was introduced later and put in front of others declared earlier breaking the order they would be accepted, thus changing the behaviour between versions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c8b0896eb6..a558afe273 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10653,26 +10653,31 @@ static const vshCmdOptDef opts_migrate[] = { }, {.name = "migrateuri", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("migration URI, usually can be omitted") }, {.name = "graphicsuri", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("graphics URI to be used for seamless graphics migration") }, {.name = "listen-address", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("listen address that destination should bind to for incoming migration") }, {.name = "dname", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("rename to new name during migration (if supported)") }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("run action specified by --timeout-* option (suspend by " "default) if live migration exceeds timeout (in seconds)") }, @@ -10686,54 +10691,66 @@ static const vshCmdOptDef opts_migrate[] = { }, {.name = "xml", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("filename containing updated XML for the target") }, {.name = "migrate-disks", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainMigrateDisksCompleter, .help = N_("comma separated list of disks to be migrated") }, {.name = "disks-port", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("port to use by target server for incoming disks migration") }, {.name = "disks-uri", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("URI to use for disks migration (overrides --disks-port)") }, {.name = "comp-methods", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshDomainMigrateCompMethodsCompleter, .help = N_("comma separated list of compression methods to be used") }, {.name = "comp-mt-level", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("compress level for multithread compression") }, {.name = "comp-mt-threads", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("number of compression threads for multithread compression") }, {.name = "comp-mt-dthreads", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("number of decompression threads for multithread compression") }, {.name = "comp-xbzrle-cache", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("page cache size for xbzrle compression") }, {.name = "auto-converge-initial", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("initial CPU throttling rate for auto-convergence") }, {.name = "auto-converge-increment", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("CPU throttling rate increment for auto-convergence") }, {.name = "persistent-xml", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompletePathLocalExisting, .help = N_("filename containing updated persistent XML for the target") }, @@ -10743,31 +10760,38 @@ static const vshCmdOptDef opts_migrate[] = { }, {.name = "postcopy-bandwidth", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("post-copy migration bandwidth limit in MiB/s") }, {.name = "parallel", .type = VSH_OT_BOOL, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("enable parallel migration") }, {.name = "parallel-connections", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("number of connections for parallel migration") }, {.name = "bandwidth", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("migration bandwidth limit in MiB/s") }, {.name = "tls-destination", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("override the destination host name used for TLS verification") }, {.name = "comp-zlib-level", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("compress level for zlib compression") }, {.name = "comp-zstd-level", .type = VSH_OT_INT, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("compress level for zstd compression") }, {.name = NULL} -- 2.44.0
Annotate arguments which can be unintentionally parsed positionally. (See previous commits for explanation.) Currently virsh accepts the arguments such as: $ virsh attach-disk --print-xml 1 2 3 4 5 6 7 8 9 10 <disk type='file' device='10'> <driver name='5' type='6' iothread='7' cache='8' io='9'/> <source file='2'/> <target dev='3' bus='4'/> </disk> While making virsh require the flags is technically a breaking change, there were multiple instances where arguments were added to the argument list thus changing the order the positional arguments would be interpreted as. Examples are commits: 7e157858b4b, bc5a8090afa, ca21d75d25. As of such there are multiple breaks of compatibiluity for the positional arguments. As of such, require the option flag for all optional arguments with value for 'virsh attach-disk'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a558afe273..d7b86ded7d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -434,31 +434,38 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "targetbus", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("target bus of disk device") }, {.name = "driver", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("driver of disk device") }, {.name = "subdriver", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("subdriver of disk device") }, {.name = "iothread", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshDomainIOThreadIdCompleter, .help = N_("IOThread to be used by supported device") }, {.name = "cache", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("cache mode of disk device") }, {.name = "io", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("io policy of disk device") }, {.name = "type", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("target device type") }, {.name = "shareable", @@ -467,24 +474,29 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "mode", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("mode of device reading and writing") }, {.name = "sourcetype", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("type of source (block|file|network)") }, {.name = "serial", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("serial of disk device") }, {.name = "wwn", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("wwn of disk device") }, {.name = "alias", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("custom alias name of disk device") }, @@ -494,6 +506,7 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "address", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("address of disk device") }, @@ -507,19 +520,23 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "source-protocol", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("protocol used by disk device source") }, {.name = "source-host-name", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("host name for source of disk device") }, {.name = "source-host-transport", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("host transport for source of disk device") }, {.name = "source-host-socket", .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, .help = N_("host socket for source of disk device") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, -- 2.44.0
On a Friday in 2024, Peter Krempa wrote:
Annotate arguments which can be unintentionally parsed positionally. (See previous commits for explanation.)
Currently virsh accepts the arguments such as:
$ virsh attach-disk --print-xml 1 2 3 4 5 6 7 8 9 10 <disk type='file' device='10'> <driver name='5' type='6' iothread='7' cache='8' io='9'/> <source file='2'/> <target dev='3' bus='4'/> </disk>
While making virsh require the flags is technically a breaking change, there were multiple instances where arguments were added to the argument list thus changing the order the positional arguments would be interpreted as. Examples are commits: 7e157858b4b, bc5a8090afa, ca21d75d25. As of such there are multiple breaks of compatibiluity for
compatibility
the positional arguments.
As of such, require the option flag for all optional arguments with value for 'virsh attach-disk'.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The argument was being parsed positionally due to the command parser quirk as we didn't opt out of it. Since the code in virshLookupCheckpoint requires that the checkpointname is present we can mark all the options as positional and required and remove the redundant check from virshLookupCheckpoint. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-checkpoint.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 65061cbf3d..9aeb8a5e7e 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -281,16 +281,8 @@ virshLookupCheckpoint(vshControl *ctl, if (vshCommandOptStringReq(ctl, cmd, arg, &chkname) < 0) return -1; - if (chkname) { - *chk = virDomainCheckpointLookupByName(dom, chkname, 0); - } else { - vshError(ctl, _("--%1$s is required"), arg); + if (!(*chk = virDomainCheckpointLookupByName(dom, chkname, 0))) return -1; - } - if (!*chk) { - vshReportError(ctl); - return -1; - } *name = virDomainCheckpointGetName(*chk); return 0; @@ -309,6 +301,8 @@ static const vshCmdOptDef opts_checkpoint_edit[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT), {.name = "checkpointname", .type = VSH_OT_STRING, + .positional = true, + .required = true, .help = N_("checkpoint name"), .completer = virshCheckpointNameCompleter, }, @@ -420,6 +414,8 @@ static const vshCmdOptDef opts_checkpoint_info[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT), {.name = "checkpointname", .type = VSH_OT_STRING, + .positional = true, + .required = true, .help = N_("checkpoint name"), .completer = virshCheckpointNameCompleter, }, @@ -810,6 +806,8 @@ static const vshCmdOptDef opts_checkpoint_dumpxml[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT), {.name = "checkpointname", .type = VSH_OT_STRING, + .positional = true, + .required = true, .help = N_("checkpoint name"), .completer = virshCheckpointNameCompleter, }, @@ -886,6 +884,8 @@ static const vshCmdOptDef opts_checkpoint_parent[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_CHECKPOINT), {.name = "checkpointname", .type = VSH_OT_STRING, + .positional = true, + .required = true, .help = N_("find parent of checkpoint name"), .completer = virshCheckpointNameCompleter, }, @@ -935,6 +935,8 @@ static const vshCmdOptDef opts_checkpoint_delete[] = { VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "checkpointname", .type = VSH_OT_STRING, + .positional = true, + .required = true, .help = N_("checkpoint name"), .completer = virshCheckpointNameCompleter, }, -- 2.44.0
We already allow a optional positional _ARGV argument but there's no reason why any other argument type could not be allowed this way. Add checks that there's just one such argument and it's placed after required positional arguments. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index cbddc5ed92..80057654c8 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -249,6 +249,7 @@ vshCmddefCheckInternals(vshControl *ctl, { size_t i; bool seenOptionalOption = false; + const char *seenOptionalPositionalOption = NULL; g_auto(virBuffer) complbuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) posbuf = VIR_BUFFER_INITIALIZER; @@ -350,10 +351,21 @@ vshCmddefCheckInternals(vshControl *ctl, } } - /* require that positional non-argv options are required */ - if (opt->positional && !opt->required && opt->type != VSH_OT_ARGV) { - vshError(ctl, "positional argument '%s' of command '%s' must be required", - opt->name, cmd->name); + /* allow at most one optional positional option */ + if (opt->positional && !opt->required) { + if (seenOptionalPositionalOption) { + vshError(ctl, "multiple optional positional arguments (%s, %s) of command '%s' are not allowed", + seenOptionalPositionalOption, opt->name, cmd->name); + return -1; + } + + seenOptionalPositionalOption = opt->name; + } + + /* all optional positional arguments must be defined after the required ones */ + if (seenOptionalPositionalOption && opt->positional && opt->required) { + vshError(ctl, "required positional argument '%s' declared after an optional positional argument '%s' of command '%s'", + opt->name, seenOptionalPositionalOption, cmd->name); return -1; } -- 2.44.0
The intended use of those commands is to use the argument directly without the flag. Since the argument is optional in all cases we couldn't declare them as positional until now. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh.c | 1 + tools/virt-admin.c | 1 + tools/vsh.c | 2 ++ 3 files changed, 4 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index 5b38881066..890c96e552 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -245,6 +245,7 @@ virshReconnect(vshControl *ctl, const char *name, bool readonly, bool force) static const vshCmdOptDef opts_connect[] = { {.name = "name", .type = VSH_OT_STRING, + .positional = true, .flags = VSH_OFLAG_EMPTY_OK, .completer = virshCompleteEmpty, .help = N_("hypervisor connection URI") diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 37bc6fe4f0..3e3361fbf1 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -254,6 +254,7 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) static const vshCmdOptDef opts_connect[] = { {.name = "name", .type = VSH_OT_STRING, + .positional = true, .flags = VSH_OFLAG_EMPTY_OK, .help = N_("daemon's admin server connection URI") }, diff --git a/tools/vsh.c b/tools/vsh.c index 80057654c8..2c90ca44f7 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3163,6 +3163,7 @@ vshCompleteHelpCommand(vshControl *ctl G_GNUC_UNUSED, const vshCmdOptDef opts_help[] = { {.name = "command", .type = VSH_OT_STRING, + .positional = true, .completer = vshCompleteHelpCommand, .help = N_("Prints global help, command specific help, or help for a group of related commands") }, @@ -3220,6 +3221,7 @@ cmdHelp(vshControl *ctl, const vshCmd *cmd) const vshCmdOptDef opts_cd[] = { {.name = "dir", .type = VSH_OT_STRING, + .positional = true, .help = N_("directory to switch to (default: home or else root)") }, {.name = NULL} -- 2.44.0
The 'snapshotname' argument is optional as by default "current" snapshot is considered. Regardless of that we should treat it as positional as it's the common usage. This is now possible as we can have one optional positional argument. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-snapshot.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 638333f4d6..6c8194d67f 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -486,6 +486,7 @@ static const vshCmdOptDef opts_snapshot_edit[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT), {.name = "snapshotname", .type = VSH_OT_STRING, + .positional = true, .help = N_("snapshot name"), .completer = virshSnapshotNameCompleter, }, @@ -599,6 +600,7 @@ static const vshCmdOptDef opts_snapshot_current[] = { }, {.name = "snapshotname", .type = VSH_OT_STRING, + .positional = true, .help = N_("name of existing snapshot to make current"), .completer = virshSnapshotNameCompleter, }, @@ -798,6 +800,7 @@ static const vshCmdOptDef opts_snapshot_info[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT), {.name = "snapshotname", .type = VSH_OT_STRING, + .positional = true, .help = N_("snapshot name"), .completer = virshSnapshotNameCompleter, }, @@ -1635,6 +1638,7 @@ static const vshCmdOptDef opts_snapshot_parent[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT), {.name = "snapshotname", .type = VSH_OT_STRING, + .positional = true, .help = N_("find parent of snapshot name"), .completer = virshSnapshotNameCompleter, }, @@ -1682,6 +1686,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT), {.name = "snapshotname", .type = VSH_OT_STRING, + .positional = true, .help = N_("snapshot name"), .completer = virshSnapshotNameCompleter, }, @@ -1763,6 +1768,7 @@ static const vshCmdOptDef opts_snapshot_delete[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT), {.name = "snapshotname", .type = VSH_OT_STRING, + .positional = true, .help = N_("snapshot name"), .completer = virshSnapshotNameCompleter, }, -- 2.44.0
The argument is optional thus couldn't be marked as positional until now, despite being parsed positionally. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-checkpoint.c | 1 + tools/virsh-snapshot.c | 1 + 2 files changed, 2 insertions(+) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 9aeb8a5e7e..7151e2b182 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -81,6 +81,7 @@ static const vshCmdOptDef opts_checkpoint_create[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "xmlfile", .type = VSH_OT_STRING, + .positional = true, .completer = virshCompletePathLocalExisting, .help = N_("domain checkpoint XML") }, diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 6c8194d67f..b47733d05b 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -109,6 +109,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "xmlfile", .type = VSH_OT_STRING, + .positional = true, .completer = virshCompletePathLocalExisting, .help = N_("domain snapshot XML") }, -- 2.44.0
Mark the 'backupxml' as positional optional and the 'checkpointxml' as 'unwanted_positional' to preserve the positioanl parsing quirk. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-backup.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/virsh-backup.c b/tools/virsh-backup.c index 7a7834d7ff..52325b5ca0 100644 --- a/tools/virsh-backup.c +++ b/tools/virsh-backup.c @@ -34,11 +34,13 @@ static const vshCmdOptDef opts_backup_begin[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "backupxml", .type = VSH_OT_STRING, + .positional = true, .completer = virshCompletePathLocalExisting, .help = N_("domain backup XML"), }, {.name = "checkpointxml", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("domain checkpoint XML"), }, -- 2.44.0
On a Friday in 2024, Peter Krempa wrote:
Mark the 'backupxml' as positional optional and the 'checkpointxml' as 'unwanted_positional' to preserve the positioanl parsing quirk.
*positional
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-backup.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Make certain optional arguments truly positional in cases when it makes semantic sense. Previously it wasn't possible to have optional positional arguments, but the parser filled them regardless, thus this preserves functionality. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-monitor.c | 3 +++ tools/virsh-domain.c | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 6c2499fb9f..71a5086c00 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -385,6 +385,7 @@ static const vshCmdOptDef opts_domblkinfo[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "device", .type = VSH_OT_STRING, + .positional = true, .completer = virshDomainDiskTargetCompleter, .help = N_("block device") }, @@ -865,6 +866,7 @@ static const vshCmdOptDef opts_domblkstat[] = { {.name = "device", .type = VSH_OT_STRING, .flags = VSH_OFLAG_EMPTY_OK, + .positional = true, .completer = virshDomainDiskTargetCompleter, .help = N_("block device") }, @@ -2219,6 +2221,7 @@ static const vshCmdOptDef opts_domifaddr[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "interface", .type = VSH_OT_STRING, + .positional = true, .flags = VSH_OFLAG_NONE, .completer = virshDomainInterfaceCompleter, .help = N_("network interface name")}, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d7b86ded7d..9a41e32e3d 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2939,6 +2939,7 @@ static const vshCmdOptDef opts_blockresize[] = { }, {.name = "size", .type = VSH_OT_INT, + .positional = true, .help = N_("New size of the block device, as scaled integer (default KiB)") }, {.name = "capacity", @@ -5805,6 +5806,7 @@ static const vshCmdOptDef opts_shutdown[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "mode", .type = VSH_OT_STRING, + .positional = true, .completer = virshDomainShutdownModeCompleter, .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt") }, @@ -5880,6 +5882,7 @@ static const vshCmdOptDef opts_reboot[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "mode", .type = VSH_OT_STRING, + .positional = true, .completer = virshDomainShutdownModeCompleter, .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt") }, @@ -11531,6 +11534,7 @@ static const vshCmdOptDef opts_domdisplay[] = { }, {.name = "type", .type = VSH_OT_STRING, + .positional = true, .help = N_("select particular graphical display " "(e.g. \"vnc\", \"spice\", \"rdp\", \"dbus\")") }, @@ -12675,6 +12679,7 @@ static const vshCmdOptDef opts_change_media[] = { }, {.name = "source", .type = VSH_OT_STRING, + .positional = true, .help = N_("source of the media") }, {.name = "eject", @@ -13199,6 +13204,7 @@ static const vshCmdOptDef opts_set_user_sshkeys[] = { }, {.name = "file", .type = VSH_OT_STRING, + .positional = true, .completer = virshCompletePathLocalExisting, .help = N_("optional file to read keys from"), }, -- 2.44.0
Annotate arguments which can be unintentionally parsed positionally. (See previous commits for explanation.) The pool name is optional but in all cases it can be promoted to an optional positional argument so that it can be properly aligned with the expectations of the parser. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-volume.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 8794126f21..7e6c6d5ef5 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -49,6 +49,7 @@ #define VIRSH_COMMON_OPT_POOL_OPTIONAL \ {.name = "pool", \ .type = VSH_OT_STRING, \ + .positional = true, \ .help = N_("pool name or uuid"), \ .completer = virshStoragePoolNameCompleter, \ .completer_flags = VIR_CONNECT_LIST_STORAGE_POOLS_ACTIVE, \ -- 2.44.0
Annotate arguments which can be unintentionally parsed positionally. (See previous commits for explanation.) All of these options were added in order thus we must declare all of them as 'unwanted_positional'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-pool.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 67cc1b94cf..66f8516017 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -76,80 +76,99 @@ }, \ {.name = "source-host", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .completer = virshCompleteEmpty, \ .help = N_("source-host for underlying storage") \ }, \ {.name = "source-path", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("source path for underlying storage") \ }, \ {.name = "source-dev", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("source device for underlying storage") \ }, \ {.name = "source-name", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("source name for underlying storage") \ }, \ {.name = "target", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("target for underlying storage") \ }, \ {.name = "source-format", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("format for underlying storage") \ }, \ {.name = "auth-type", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("auth type to be used for underlying storage") \ }, \ {.name = "auth-username", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .completer = virshCompleteEmpty, \ .help = N_("auth username to be used for underlying storage") \ }, \ {.name = "secret-usage", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("auth secret usage to be used for underlying storage") \ }, \ {.name = "secret-uuid", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("auth secret UUID to be used for underlying storage") \ }, \ {.name = "adapter-name", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter name to be used for underlying storage") \ }, \ {.name = "adapter-wwnn", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter wwnn to be used for underlying storage") \ }, \ {.name = "adapter-wwpn", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter wwpn to be used for underlying storage") \ }, \ {.name = "adapter-parent", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter parent scsi_hostN to be used for underlying vHBA storage") \ }, \ {.name = "adapter-parent-wwnn", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter parent scsi_hostN wwnn to be used for underlying vHBA storage") \ }, \ {.name = "adapter-parent-wwpn", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter parent scsi_hostN wwpn to be used for underlying vHBA storage") \ }, \ {.name = "adapter-parent-fabric-wwn", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("adapter parent scsi_hostN fabric_wwn to be used for underlying vHBA storage") \ }, \ {.name = "source-protocol-ver", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .help = N_("nfsvers value for NFS pool mount option") \ }, \ {.name = "source-initiator", \ .type = VSH_OT_STRING, \ + .unwanted_positional = true, \ .completer = virshCompleteEmpty, \ .help = N_("initiator iqn for underlying storage") \ } -- 2.44.0
Historically the command parser in virsh parses/fills even optional arguments with values as if they were positional unless opted out using VSH_OFLAG_REQ_OPT. This creates unexpected situations when commands can break in this unwanted semantics: $ virsh snapshot-create-as --print-xml 1 2 3 <domainsnapshot> <name>2</name> <description>3</description> </domainsnapshot> To prevent any further addition annotate the rest of the arguments with the 'unwanted_positional' flag, so that the parser can keep parsing them as such but any further optional argument will not have this behaviour. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-checkpoint.c | 3 ++ tools/virsh-domain-event.c | 3 ++ tools/virsh-domain-monitor.c | 2 + tools/virsh-domain.c | 75 ++++++++++++++++++++++++++++++++++++ tools/virsh-host.c | 24 ++++++++++++ tools/virsh-interface.c | 1 + tools/virsh-network.c | 7 ++++ tools/virsh-nodedev.c | 5 +++ tools/virsh-pool.c | 8 ++++ tools/virsh-secret.c | 4 ++ tools/virsh-snapshot.c | 3 ++ tools/virsh-volume.c | 10 +++++ 12 files changed, 145 insertions(+) diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 7151e2b182..48e3a586e4 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -189,11 +189,13 @@ static const vshCmdOptDef opts_checkpoint_create_as[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "name", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("name of checkpoint") }, {.name = "description", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("description of checkpoint") }, @@ -630,6 +632,7 @@ static const vshCmdOptDef opts_checkpoint_list[] = { }, {.name = "from", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("limit list to children of given checkpoint"), .completer = virshCheckpointNameCompleter, }, diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 73b00f3bb2..8bf57ade7a 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -867,11 +867,13 @@ static const vshCmdInfo info_event = { static const vshCmdOptDef opts_event[] = { {.name = "domain", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("filter by domain name, id or uuid"), .completer = virshDomainNameCompleter, }, {.name = "event", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainEventNameCompleter, .help = N_("which event type to wait for") }, @@ -885,6 +887,7 @@ static const vshCmdOptDef opts_event[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("timeout seconds") }, {.name = "list", diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 71a5086c00..d88cf64235 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1349,6 +1349,7 @@ static const vshCmdOptDef opts_domtime[] = { }, {.name = "time", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("time to set") }, {.name = NULL} @@ -2231,6 +2232,7 @@ static const vshCmdOptDef opts_domifaddr[] = { .help = N_("always display names and MACs of interfaces")}, {.name = "source", .type = VSH_OT_STRING, + .unwanted_positional = true, .flags = VSH_OFLAG_NONE, .completer = virshDomainInterfaceAddrSourceCompleter, .help = N_("address source: 'lease', 'agent', or 'arp'")}, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9a41e32e3d..600388dceb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -844,34 +844,41 @@ static const vshCmdOptDef opts_attach_interface[] = { }, {.name = "target", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("target network name") }, {.name = "mac", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("MAC address") }, {.name = "script", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("script used to bridge network interface") }, {.name = "model", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("model type") }, {.name = "alias", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("custom alias name of interface device") }, {.name = "inbound", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("control domain's incoming traffics") }, {.name = "outbound", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("control domain's outgoing traffics") }, @@ -889,6 +896,7 @@ static const vshCmdOptDef opts_attach_interface[] = { }, {.name = "source-mode", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainInterfaceSourceModeCompleter, .help = N_("mode attribute of <source/> element") }, @@ -1550,30 +1558,36 @@ static const vshCmdOptDef opts_blkiotune[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "weight", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("IO Weight") }, {.name = "device-weights", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("per-device IO Weights, in the form of /path/to/device,weight,...") }, {.name = "device-read-iops-sec", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("per-device read I/O limit per second, in the form of /path/to/device,read_iops_sec,...") }, {.name = "device-write-iops-sec", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("per-device write I/O limit per second, in the form of /path/to/device,write_iops_sec,...") }, {.name = "device-read-bytes-sec", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("per-device bytes read per second, in the form of /path/to/device,read_bytes_sec,...") }, {.name = "device-write-bytes-sec", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("per-device bytes wrote per second, in the form of /path/to/device,write_bytes_sec,...") }, @@ -2031,10 +2045,12 @@ static const vshCmdOptDef opts_blockcommit[] = { }, {.name = "bandwidth", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("bandwidth limit in MiB/s") }, {.name = "base", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainBlockjobBaseTopCompleter, .help = N_("path of base file to commit into (default bottom of chain)") }, @@ -2044,6 +2060,7 @@ static const vshCmdOptDef opts_blockcommit[] = { }, {.name = "top", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainBlockjobBaseTopCompleter, .help = N_("path of top file to commit from (default top of chain)") }, @@ -2066,6 +2083,7 @@ static const vshCmdOptDef opts_blockcommit[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("implies --wait, abort if copy exceeds timeout (in seconds)") }, {.name = "pivot", @@ -2251,10 +2269,12 @@ static const vshCmdOptDef opts_blockcopy[] = { }, {.name = "dest", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("path of the copy to create") }, {.name = "bandwidth", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("bandwidth limit in MiB/s") }, {.name = "shallow", @@ -2283,6 +2303,7 @@ static const vshCmdOptDef opts_blockcopy[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("implies --wait, abort if copy exceeds timeout (in seconds)") }, {.name = "pivot", @@ -2299,21 +2320,25 @@ static const vshCmdOptDef opts_blockcopy[] = { }, {.name = "xml", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("filename containing XML description of the copy destination") }, {.name = "format", .type = VSH_OT_STRING, + .unwanted_positional = true, .flags = VSH_OFLAG_NONE, .completer = virshDomainStorageFileFormatCompleter, .help = N_("format of the destination file") }, {.name = "granularity", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("power-of-two granularity to use during the copy") }, {.name = "buf-size", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("maximum amount of in-flight data during the copy") }, {.name = "bytes", @@ -2608,6 +2633,7 @@ static const vshCmdOptDef opts_blockjob[] = { }, {.name = "bandwidth", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("set the bandwidth limit in MiB/s") }, {.name = NULL} @@ -2796,10 +2822,12 @@ static const vshCmdOptDef opts_blockpull[] = { }, {.name = "bandwidth", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("bandwidth limit in MiB/s") }, {.name = "base", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainBlockjobBaseTopCompleter, .help = N_("path of backing file in chain for a partial pull") }, @@ -2813,6 +2841,7 @@ static const vshCmdOptDef opts_blockpull[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("with --wait, abort if pull exceeds timeout (in seconds)") }, {.name = "async", @@ -3000,6 +3029,7 @@ static const vshCmdOptDef opts_console[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "devname", /* sc_prohibit_devname */ .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainConsoleCompleter, .help = N_("character device name") }, @@ -3246,11 +3276,13 @@ static const vshCmdOptDef opts_domiftune[] = { }, {.name = "inbound", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("control domain's incoming traffics") }, {.name = "outbound", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("control domain's outgoing traffics") }, @@ -3568,6 +3600,7 @@ static const vshCmdOptDef opts_undefine[] = { }, {.name = "storage", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainUndefineStorageDisksCompleter, .help = N_("remove associated storage volumes (comma separated list of " "targets or source paths) (see domblklist)") @@ -4013,6 +4046,7 @@ static const vshCmdOptDef opts_start[] = { }, {.name = "pass-fds", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("pass file descriptors N,M,... to the guest") }, @@ -4159,6 +4193,7 @@ static const vshCmdOptDef opts_save[] = { }, {.name = "xml", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("filename containing updated XML for the target") }, @@ -5250,6 +5285,7 @@ static const vshCmdOptDef opts_restore[] = { }, {.name = "xml", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("filename containing updated XML for the target") }, @@ -5352,6 +5388,7 @@ static const vshCmdOptDef opts_dump[] = { {.name = "format", .type = VSH_OT_STRING, .flags = VSH_OFLAG_NONE, + .unwanted_positional = true, .completer = virshDomainCoreDumpFormatCompleter, .help = N_("specify the format of memory-only dump") }, @@ -5495,11 +5532,13 @@ static const vshCmdOptDef opts_screenshot[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "file", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("where to store the screenshot") }, {.name = "screen", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("ID of a screen to take screenshot of") }, {.name = NULL} @@ -6834,11 +6873,13 @@ static const vshCmdOptDef opts_vcpupin[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "vcpu", .type = VSH_OT_INT, + .unwanted_positional = true, .completer = virshDomainVcpuCompleter, .help = N_("vcpu number") }, {.name = "cpulist", .type = VSH_OT_STRING, + .unwanted_positional = true, .flags = VSH_OFLAG_EMPTY_OK, .completer = virshDomainCpulistCompleter, .help = N_("host cpu number(s) to set, or omit option to query") @@ -7047,6 +7088,7 @@ static const vshCmdOptDef opts_emulatorpin[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "cpulist", .type = VSH_OT_STRING, + .unwanted_positional = true, .flags = VSH_OFLAG_EMPTY_OK, .completer = virshDomainCpulistCompleter, .help = N_("host cpu number(s) to set, or omit option to query") @@ -7231,6 +7273,7 @@ static const vshCmdOptDef opts_guestvcpus[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "cpulist", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainVcpulistViaAgentCompleter, .help = N_("list of cpus to enable or disable") }, @@ -7653,22 +7696,27 @@ static const vshCmdOptDef opts_iothreadset[] = { }, {.name = "poll-max-ns", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("set the maximum IOThread polling time in ns") }, {.name = "poll-grow", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("set the value to increase the IOThread polling time") }, {.name = "poll-shrink", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("set the value for reduction of the IOThread polling time") }, {.name = "thread-pool-min", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("lower boundary for worker thread pool") }, {.name = "thread-pool-max", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("upper boundary for worker thread pool") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, @@ -7824,10 +7872,12 @@ static const vshCmdOptDef opts_cpu_stats[] = { }, {.name = "start", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Show statistics from this CPU") }, {.name = "count", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Number of shown CPUs at most") }, {.name = NULL} @@ -8010,6 +8060,7 @@ static const vshCmdOptDef opts_create[] = { }, {.name = "pass-fds", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("pass file descriptors N,M,... to the guest") }, @@ -8368,10 +8419,12 @@ static const vshCmdOptDef opts_metadata[] = { }, {.name = "key", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("key to be used as a namespace identifier"), }, {.name = "set", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("new metadata to set"), }, @@ -8856,15 +8909,18 @@ static const vshCmdOptDef opts_update_memory_device[] = { }, {.name = "alias", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainDeviceAliasCompleter, .help = N_("memory device alias") }, {.name = "node", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("memory device target node") }, {.name = "requested-size", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("new value of <requested/> size, as scaled integer (default KiB)") }, {.name = NULL} @@ -8998,18 +9054,22 @@ static const vshCmdOptDef opts_memtune[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "hard-limit", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Max memory, as scaled integer (default KiB)") }, {.name = "soft-limit", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Memory during contention, as scaled integer (default KiB)") }, {.name = "swap-hard-limit", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Max memory plus swap, as scaled integer (default KiB)") }, {.name = "min-guarantee", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Min guaranteed memory, as scaled integer (default KiB)") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, @@ -9165,11 +9225,13 @@ static const vshCmdOptDef opts_perf[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "enable", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainPerfEnableCompleter, .help = N_("perf events which will be enabled") }, {.name = "disable", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainPerfDisableCompleter, .help = N_("perf events which will be disabled") }, @@ -9289,12 +9351,14 @@ static const vshCmdOptDef opts_numatune[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "mode", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainNumatuneModeCompleter, .help = N_("NUMA mode, one of strict, preferred and interleave " "or a number from the virDomainNumatuneMemMode enum") }, {.name = "nodeset", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("NUMA node selections to set") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, @@ -9470,6 +9534,7 @@ static const vshCmdOptDef opts_domsetlaunchsecstate[] = { }, {.name = "set-address", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("physical address within the guest domain's memory to set the secret"), }, {.name = NULL} @@ -9872,11 +9937,13 @@ static const vshCmdInfo info_qemu_monitor_event = { static const vshCmdOptDef opts_qemu_monitor_event[] = { {.name = "domain", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("filter by domain name, id or uuid"), .completer = virshDomainNameCompleter, }, {.name = "event", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("filter by event name") }, {.name = "pretty", @@ -9889,6 +9956,7 @@ static const vshCmdOptDef opts_qemu_monitor_event[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("timeout seconds") }, {.name = "regex", @@ -10379,6 +10447,7 @@ static const vshCmdOptDef opts_domxmltonative[] = { }, {.name = "xml", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("xml data file to export from") }, @@ -11871,6 +11940,7 @@ static const vshCmdOptDef opts_domhostname[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "source", .type = VSH_OT_STRING, + .unwanted_positional = true, .flags = VSH_OFLAG_NONE, .completer = virshDomainHostnameSourceCompleter, .help = N_("address source: 'lease' or 'agent'")}, @@ -12144,6 +12214,7 @@ static const vshCmdOptDef opts_detach_interface[] = { }, {.name = "mac", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainInterfaceCompleter, .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, .help = N_("MAC address") @@ -12823,11 +12894,13 @@ static const vshCmdOptDef opts_domfstrim[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "minimum", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Just a hint to ignore contiguous " "free ranges smaller than this (Bytes)") }, {.name = "mountpoint", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainFSMountpointsCompleter, .help = N_("which mount point to trim") }, @@ -13295,11 +13368,13 @@ static const vshCmdOptDef opts_domdirtyrate_calc[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "seconds", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("calculate memory dirty rate within specified seconds, " "the supported value range from 1 to 60, default to 1.") }, {.name = "mode", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainDirtyRateCalcModeCompleter, .help = N_("dirty page rate calculation mode, either of these 3 options " "'page-sampling, dirty-bitmap, dirty-ring' can be specified.") diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 712db39d35..52e0dc55de 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -86,20 +86,24 @@ static const vshCmdInfo info_domcapabilities = { static const vshCmdOptDef opts_domcapabilities[] = { {.name = "virttype", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainVirtTypeCompleter, .help = N_("virtualization type (/domain/@type)"), }, {.name = "emulatorbin", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("path to emulator binary (/domain/devices/emulator)"), }, {.name = "arch", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshArchCompleter, .help = N_("domain architecture (/domain/os/type/@arch)"), }, {.name = "machine", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("machine type (/domain/os/type/@machine)"), }, {.name = "xpath", @@ -156,6 +160,7 @@ static const vshCmdInfo info_freecell = { static const vshCmdOptDef opts_freecell[] = { {.name = "cellno", .type = VSH_OT_INT, + .unwanted_positional = true, .completer = virshCellnoCompleter, .help = N_("NUMA cell number") }, @@ -264,11 +269,13 @@ static const vshCmdInfo info_freepages = { static const vshCmdOptDef opts_freepages[] = { {.name = "cellno", .type = VSH_OT_INT, + .unwanted_positional = true, .completer = virshCellnoCompleter, .help = N_("NUMA cell number") }, {.name = "pagesize", .type = VSH_OT_INT, + .unwanted_positional = true, .completer = virshAllocpagesPagesizeCompleter, .help = N_("page size (in kibibytes)") }, @@ -481,6 +488,7 @@ static const vshCmdOptDef opts_allocpages[] = { }, {.name = "cellno", .type = VSH_OT_INT, + .unwanted_positional = true, .completer = virshCellnoCompleter, .help = N_("NUMA cell number") }, @@ -582,6 +590,7 @@ static const vshCmdInfo info_maxvcpus = { static const vshCmdOptDef opts_maxvcpus[] = { {.name = "type", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainVirtTypeCompleter, .help = N_("domain type") }, @@ -712,6 +721,7 @@ static const vshCmdInfo info_nodecpustats = { static const vshCmdOptDef opts_node_cpustats[] = { {.name = "cpu", .type = VSH_OT_INT, + .unwanted_positional = true, .completer = virshNodeCpuCompleter, .help = N_("prints specified cpu statistics only.") }, @@ -850,6 +860,7 @@ static const vshCmdInfo info_nodememstats = { static const vshCmdOptDef opts_node_memstats[] = { {.name = "cell", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("prints specified cell statistics only.") }, {.name = NULL} @@ -1421,16 +1432,19 @@ static const vshCmdInfo info_node_memory_tune = { static const vshCmdOptDef opts_node_memory_tune[] = { {.name = "shm-pages-to-scan", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("number of pages to scan before the shared memory service " "goes to sleep") }, {.name = "shm-sleep-millisecs", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("number of millisecs the shared memory service should " "sleep before next scan") }, {.name = "shm-merge-across-nodes", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Specifies if pages from different numa nodes can be merged") }, {.name = NULL} @@ -1535,20 +1549,24 @@ static const vshCmdOptDef opts_hypervisor_cpu_compare[] = { VIRSH_COMMON_OPT_FILE(N_("file containing an XML CPU description")), {.name = "virttype", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainVirtTypeCompleter, .help = N_("virtualization type (/domain/@type)"), }, {.name = "emulator", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("path to emulator binary (/domain/devices/emulator)"), }, {.name = "arch", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshArchCompleter, .help = N_("CPU architecture (/domain/os/type/@arch)"), }, {.name = "machine", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("machine type (/domain/os/type/@machine)"), }, {.name = "error", @@ -1637,25 +1655,30 @@ static const vshCmdInfo info_hypervisor_cpu_baseline = { static const vshCmdOptDef opts_hypervisor_cpu_baseline[] = { {.name = "file", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("file containing XML CPU descriptions"), }, {.name = "virttype", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshDomainVirtTypeCompleter, .help = N_("virtualization type (/domain/@type)"), }, {.name = "emulator", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("path to emulator binary (/domain/devices/emulator)"), }, {.name = "arch", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshArchCompleter, .help = N_("CPU architecture (/domain/os/type/@arch)"), }, {.name = "machine", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("machine type (/domain/os/type/@machine)"), }, {.name = "features", @@ -1668,6 +1691,7 @@ static const vshCmdOptDef opts_hypervisor_cpu_baseline[] = { }, {.name = "model", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCPUModelCompleter, .help = N_("Shortcut for calling the command with a single CPU model " "and no additional features") diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 1b1344c17c..a08bb822ed 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -736,6 +736,7 @@ static const vshCmdOptDef opts_interface_bridge[] = { }, {.name = "delay", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("number of seconds to squelch traffic on newly connected ports") }, {.name = "no-start", diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 59ca842181..5bc91a361f 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -506,11 +506,13 @@ static const vshCmdOptDef opts_network_metadata[] = { .help = N_("use an editor to change the metadata") }, {.name = "key", + .unwanted_positional = true, .type = VSH_OT_STRING, .help = N_("key to be used as a namespace identifier"), }, {.name = "set", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("new metadata to set"), }, @@ -1240,6 +1242,7 @@ static const vshCmdOptDef opts_network_update[] = { }, {.name = "parent-index", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("which parent object to search through") }, VIRSH_COMMON_OPT_CONFIG(N_("affect next network startup")), @@ -1579,11 +1582,13 @@ static const vshCmdInfo info_network_event = { static const vshCmdOptDef opts_network_event[] = { {.name = "network", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("filter by network name or uuid"), .completer = virshNetworkNameCompleter, }, {.name = "event", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshNetworkEventNameCompleter, .help = N_("which event type to wait for") }, @@ -1593,6 +1598,7 @@ static const vshCmdOptDef opts_network_event[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("timeout seconds") }, {.name = "list", @@ -1694,6 +1700,7 @@ static const vshCmdOptDef opts_network_dhcp_leases[] = { VIRSH_COMMON_OPT_NETWORK_FULL(VIR_CONNECT_LIST_NETWORKS_ACTIVE), {.name = "mac", .type = VSH_OT_STRING, + .unwanted_positional = true, .flags = VSH_OFLAG_NONE, .help = N_("MAC address"), .completer = virshNetworkDhcpMacCompleter, diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 90cfc187e5..fafebf9972 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -365,6 +365,7 @@ static const vshCmdOptDef opts_node_list_devices[] = { }, {.name = "cap", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshNodeDeviceCapabilityNameCompleter, .help = N_("capability names, separated by comma") }, @@ -643,6 +644,7 @@ static const vshCmdOptDef opts_node_device_detach[] = { }, {.name = "driver", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshNodeDevicePCIBackendCompleter, .help = N_("pci device assignment backend driver (e.g. 'vfio' or 'xen')") }, @@ -884,11 +886,13 @@ static const vshCmdInfo info_node_device_event = { static const vshCmdOptDef opts_node_device_event[] = { {.name = "device", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("filter by node device name"), .completer = virshNodeDeviceNameCompleter, }, {.name = "event", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshNodeDeviceEventNameCompleter, .help = N_("which event type to wait for") }, @@ -898,6 +902,7 @@ static const vshCmdOptDef opts_node_device_event[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("timeout seconds") }, {.name = "list", diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 66f8516017..c93204dd30 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -1055,6 +1055,7 @@ static const vshCmdOptDef opts_pool_list[] = { {.name = "type", .type = VSH_OT_STRING, .completer = virshPoolTypeCompleter, + .unwanted_positional = true, .completer_flags = VIRSH_POOL_TYPE_COMPLETER_COMMA, .help = N_("only list pool of specified type(s) (if supported)") }, @@ -1382,16 +1383,19 @@ static const vshCmdOptDef opts_find_storage_pool_sources_as[] = { }, {.name = "host", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("optional host to query") }, {.name = "port", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("optional port to query") }, {.name = "initiator", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("optional initiator IQN to use for query") }, @@ -1466,6 +1470,7 @@ static const vshCmdOptDef opts_find_storage_pool_sources[] = { }, {.name = "srcSpec", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompletePathLocalExisting, .help = N_("optional file of source xml to query for pools") }, @@ -1901,11 +1906,13 @@ static const vshCmdInfo info_pool_event = { static const vshCmdOptDef opts_pool_event[] = { {.name = "pool", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshStoragePoolNameCompleter, .help = N_("filter by storage pool name or uuid") }, {.name = "event", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshPoolEventNameCompleter, .help = N_("which event type to wait for") }, @@ -1915,6 +1922,7 @@ static const vshCmdOptDef opts_pool_event[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("timeout seconds") }, {.name = "list", diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 7e10afa765..edf3c47362 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -201,6 +201,7 @@ static const vshCmdOptDef opts_secret_set_value[] = { }, {.name = "base64", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("base64-encoded secret value") }, @@ -714,11 +715,13 @@ static const vshCmdInfo info_secret_event = { static const vshCmdOptDef opts_secret_event[] = { {.name = "secret", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("filter by secret name or uuid"), .completer = virshSecretUUIDCompleter, }, {.name = "event", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshSecretEventNameCompleter, .help = N_("which event type to wait for") }, @@ -728,6 +731,7 @@ static const vshCmdOptDef opts_secret_event[] = { }, {.name = "timeout", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("timeout seconds") }, {.name = "list", diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index b47733d05b..2fcf6e3d5e 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -314,11 +314,13 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "name", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("name of snapshot") }, {.name = "description", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("description of snapshot") }, @@ -1379,6 +1381,7 @@ static const vshCmdOptDef opts_snapshot_list[] = { }, {.name = "from", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshSnapshotNameCompleter, .help = N_("limit list to children of given snapshot") }, diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 7e6c6d5ef5..4e5673e0ee 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -190,19 +190,23 @@ static const vshCmdOptDef opts_vol_create_as[] = { }, {.name = "allocation", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshCompleteEmpty, .help = N_("initial allocation size, as scaled integer (default bytes)") }, {.name = "format", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("file format type raw,bochs,qcow,qcow2,qed,vmdk") }, {.name = "backing-vol", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("the backing volume if taking a snapshot") }, {.name = "backing-vol-format", .type = VSH_OT_STRING, + .unwanted_positional = true, .help = N_("format of backing volume if taking a snapshot") }, {.name = "prealloc-metadata", @@ -432,6 +436,7 @@ static const vshCmdOptDef opts_vol_create_from[] = { VIRSH_COMMON_OPT_VOL_FULL, {.name = "inputpool", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshStoragePoolNameCompleter, .help = N_("pool name or uuid of the input volume's pool") }, @@ -613,10 +618,12 @@ static const vshCmdOptDef opts_vol_upload[] = { VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "offset", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("volume offset to upload to") }, {.name = "length", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("amount of data to upload") }, {.name = "sparse", @@ -721,10 +728,12 @@ static const vshCmdOptDef opts_vol_download[] = { VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "offset", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("volume offset to download from") }, {.name = "length", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("amount of data to download") }, {.name = "sparse", @@ -875,6 +884,7 @@ static const vshCmdOptDef opts_vol_wipe[] = { VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "algorithm", .type = VSH_OT_STRING, + .unwanted_positional = true, .completer = virshStorageVolWipeAlgorithmCompleter, .help = N_("perform selected wiping algorithm") }, -- 2.44.0
Historically the command parser in virsh parses/fills even optional arguments with values as if they were positional unless opted out using VSH_OFLAG_REQ_OPT. This creates unexpected situations when commands can break in this unwanted semantics: $ virsh snapshot-create-as --print-xml 1 2 3 <domainsnapshot> <name>2</name> <description>3</description> </domainsnapshot> To prevent any further addition annotate the rest of the arguments with the 'unwanted_positional' flag, so that the parser can keep parsing them as such but any further optional argument will not have this behaviour. Certain arguments where it makes sense are annotated as 'positional' too in this patch. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-admin.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 3e3361fbf1..514f434c94 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -425,14 +425,17 @@ static const vshCmdOptDef opts_srv_threadpool_set[] = { }, {.name = "min-workers", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Change bottom limit to number of workers."), }, {.name = "max-workers", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Change upper limit to number of workers."), }, {.name = "priority-workers", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Change the current number of priority workers"), }, {.name = NULL} @@ -812,11 +815,13 @@ static const vshCmdOptDef opts_srv_clients_set[] = { }, {.name = "max-clients", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Change the upper limit to overall number of clients " "connected to the server."), }, {.name = "max-unauth-clients", .type = VSH_OT_INT, + .unwanted_positional = true, .help = N_("Change the upper limit to number of clients waiting for " "authentication to be connected to the server"), }, @@ -954,6 +959,7 @@ static const vshCmdInfo info_daemon_log_filters = { static const vshCmdOptDef opts_daemon_log_filters[] = { {.name = "filters", .type = VSH_OT_STRING, + .positional = true, .help = N_("redefine the existing set of logging filters"), .flags = VSH_OFLAG_EMPTY_OK }, @@ -1037,6 +1043,7 @@ static const vshCmdInfo info_daemon_timeout = { static const vshCmdOptDef opts_daemon_log_outputs[] = { {.name = "outputs", .type = VSH_OT_STRING, + .positional = true, .help = N_("redefine the existing set of logging outputs"), .flags = VSH_OFLAG_EMPTY_OK }, -- 2.44.0
Switch the command parser from using the VSH_OFLAG_REQ_OPT flag opting out from positional parsing of arguments to a combination of the 'positional' flags for truly positional arguments and 'unwanted_positional' preserving semantics for the existing arguments where the parser did it due to bad design. This patch retires VSH_OFLAG_REQ_OPT along with the infrastructure that was needed to refactor all uses properly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-backup.c | 1 - tools/virsh-checkpoint.c | 1 - tools/virsh-domain-monitor.c | 1 - tools/virsh-domain.c | 69 ------------------------------------ tools/virsh-host.c | 2 -- tools/virsh-interface.c | 1 - tools/virsh-network.c | 2 -- tools/virsh-nodedev.c | 1 - tools/virsh-nwfilter.c | 2 -- tools/virsh-pool.c | 1 - tools/virsh-secret.c | 2 -- tools/virsh-snapshot.c | 2 -- tools/virsh-volume.c | 1 - tools/virt-admin.c | 1 - tools/vsh.c | 44 ++--------------------- tools/vsh.h | 1 - 16 files changed, 3 insertions(+), 129 deletions(-) diff --git a/tools/virsh-backup.c b/tools/virsh-backup.c index 52325b5ca0..9a1e89760f 100644 --- a/tools/virsh-backup.c +++ b/tools/virsh-backup.c @@ -109,7 +109,6 @@ static const vshCmdOptDef opts_backup_dumpxml[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index 48e3a586e4..e425041ca7 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -829,7 +829,6 @@ static const vshCmdOptDef opts_checkpoint_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index d88cf64235..5531d3b737 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -277,7 +277,6 @@ static const vshCmdOptDef opts_dommemstat[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "period", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("period in seconds to set collection") }, VIRSH_COMMON_OPT_CONFIG(N_("affect next boot")), diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 600388dceb..91a9dfd96a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -434,38 +434,31 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "targetbus", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("target bus of disk device") }, {.name = "driver", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("driver of disk device") }, {.name = "subdriver", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("subdriver of disk device") }, {.name = "iothread", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshDomainIOThreadIdCompleter, .help = N_("IOThread to be used by supported device") }, {.name = "cache", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("cache mode of disk device") }, {.name = "io", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("io policy of disk device") }, {.name = "type", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("target device type") }, {.name = "shareable", @@ -474,29 +467,24 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "mode", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("mode of device reading and writing") }, {.name = "sourcetype", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("type of source (block|file|network)") }, {.name = "serial", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("serial of disk device") }, {.name = "wwn", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("wwn of disk device") }, {.name = "alias", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("custom alias name of disk device") }, @@ -506,7 +494,6 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "address", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("address of disk device") }, @@ -520,23 +507,19 @@ static const vshCmdOptDef opts_attach_disk[] = { }, {.name = "source-protocol", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("protocol used by disk device source") }, {.name = "source-host-name", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("host name for source of disk device") }, {.name = "source-host-transport", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("host transport for source of disk device") }, {.name = "source-host-socket", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("host socket for source of disk device") }, VIRSH_COMMON_OPT_DOMAIN_PERSISTENT, @@ -1229,7 +1212,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-bytes-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("total throughput limit, as scaled integer (default bytes)") }, {.name = "read_bytes_sec", @@ -1238,7 +1220,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-bytes-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("read throughput limit, as scaled integer (default bytes)") }, {.name = "write_bytes_sec", @@ -1247,7 +1228,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-bytes-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("write throughput limit, as scaled integer (default bytes)") }, {.name = "total_iops_sec", @@ -1256,7 +1236,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-iops-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("total I/O operations limit per second") }, {.name = "read_iops_sec", @@ -1265,7 +1244,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-iops-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("read I/O operations limit per second") }, {.name = "write_iops_sec", @@ -1274,7 +1252,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-iops-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("write I/O operations limit per second") }, {.name = "total_bytes_sec_max", @@ -1283,7 +1260,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-bytes-sec-max", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("total max, as scaled integer (default bytes)") }, {.name = "read_bytes_sec_max", @@ -1292,7 +1268,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-bytes-sec-max", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("read max, as scaled integer (default bytes)") }, {.name = "write_bytes_sec_max", @@ -1301,7 +1276,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-bytes-sec-max", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("write max, as scaled integer (default bytes)") }, {.name = "total_iops_sec_max", @@ -1310,7 +1284,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-iops-sec-max", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("total I/O operations max") }, {.name = "read_iops_sec_max", @@ -1319,7 +1292,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-iops-sec-max", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("read I/O operations max") }, {.name = "write_iops_sec_max", @@ -1328,7 +1300,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-iops-sec-max", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("write I/O operations max") }, {.name = "size_iops_sec", @@ -1337,7 +1308,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "size-iops-sec", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("I/O size in bytes") }, {.name = "group_name", @@ -1346,7 +1316,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "group-name", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("group name to share I/O quota between multiple drives") }, @@ -1356,7 +1325,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-bytes-sec-max-length", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow total max bytes") }, {.name = "read_bytes_sec_max_length", @@ -1365,7 +1333,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-bytes-sec-max-length", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow read max bytes") }, {.name = "write_bytes_sec_max_length", @@ -1374,7 +1341,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-bytes-sec-max-length", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow write max bytes") }, {.name = "total_iops_sec_max_length", @@ -1383,7 +1349,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "total-iops-sec-max-length", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow total I/O operations max") }, {.name = "read_iops_sec_max_length", @@ -1392,7 +1357,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "read-iops-sec-max-length", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow read I/O operations max") }, {.name = "write_iops_sec_max_length", @@ -1401,7 +1365,6 @@ static const vshCmdOptDef opts_blkdeviotune[] = { }, {.name = "write-iops-sec-max-length", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds to allow write I/O operations max") }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, @@ -3506,7 +3469,6 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = { }, {.name = "duration", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("duration in seconds") }, {.name = NULL} @@ -4544,7 +4506,6 @@ static const vshCmdOptDef opts_save_image_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -4950,7 +4911,6 @@ static const vshCmdOptDef opts_managed_save_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -5060,12 +5020,10 @@ static const vshCmdOptDef opts_schedinfo[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "weight", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("weight for XEN_CREDIT") }, {.name = "cap", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("cap for XEN_CREDIT") }, VIRSH_COMMON_OPT_CURRENT(N_("get/set current scheduler info")), @@ -8576,13 +8534,11 @@ static const vshCmdOptDef opts_send_key[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "codeset", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCodesetNameCompleter, .help = N_("the codeset of keycodes, default:linux") }, {.name = "holdtime", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("the time (in milliseconds) how long the keys will be held") }, {.name = "keycode", @@ -9524,12 +9480,10 @@ static const vshCmdOptDef opts_domsetlaunchsecstate[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "secrethdr", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("path to file containing the secret header"), }, {.name = "secret", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("path to file containing the secret"), }, {.name = "set-address", @@ -9702,7 +9656,6 @@ static const vshCmdOptDef opts_qemu_monitor_command[] = { }, {.name = "pass-fds", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("pass file descriptors N,M,... along with the command") }, @@ -10091,7 +10044,6 @@ static const vshCmdOptDef opts_qemu_agent_command[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "timeout", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("timeout seconds. must be positive.") }, {.name = "async", @@ -10329,7 +10281,6 @@ static const vshCmdOptDef opts_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -10441,7 +10392,6 @@ static const vshCmdOptDef opts_domxmltonative[] = { }, {.name = "domain", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("domain name, id or uuid"), .completer = virshDomainNameCompleter, }, @@ -10797,49 +10747,40 @@ static const vshCmdOptDef opts_migrate[] = { }, {.name = "disks-uri", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("URI to use for disks migration (overrides --disks-port)") }, {.name = "comp-methods", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshDomainMigrateCompMethodsCompleter, .help = N_("comma separated list of compression methods to be used") }, {.name = "comp-mt-level", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("compress level for multithread compression") }, {.name = "comp-mt-threads", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("number of compression threads for multithread compression") }, {.name = "comp-mt-dthreads", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("number of decompression threads for multithread compression") }, {.name = "comp-xbzrle-cache", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("page cache size for xbzrle compression") }, {.name = "auto-converge-initial", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("initial CPU throttling rate for auto-convergence") }, {.name = "auto-converge-increment", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("CPU throttling rate increment for auto-convergence") }, {.name = "persistent-xml", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompletePathLocalExisting, .help = N_("filename containing updated persistent XML for the target") }, @@ -10849,38 +10790,31 @@ static const vshCmdOptDef opts_migrate[] = { }, {.name = "postcopy-bandwidth", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("post-copy migration bandwidth limit in MiB/s") }, {.name = "parallel", .type = VSH_OT_BOOL, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("enable parallel migration") }, {.name = "parallel-connections", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("number of connections for parallel migration") }, {.name = "bandwidth", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("migration bandwidth limit in MiB/s") }, {.name = "tls-destination", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("override the destination host name used for TLS verification") }, {.name = "comp-zlib-level", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("compress level for zlib compression") }, {.name = "comp-zstd-level", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("compress level for zstd compression") }, {.name = NULL} @@ -11439,7 +11373,6 @@ static const vshCmdOptDef opts_migrate_compcache[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "size", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("requested size of the cache (in bytes) used for compression") }, {.name = NULL} @@ -13100,7 +13033,6 @@ static const vshCmdOptDef opts_guest_agent_timeout[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "timeout", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("timeout seconds.") }, {.name = NULL} @@ -13448,7 +13380,6 @@ static const vshCmdOptDef opts_domdisplay_reload[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(0), {.name = "type", .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("graphics display type") }, {.name = NULL} diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 52e0dc55de..c338b5cd85 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -45,7 +45,6 @@ static const vshCmdInfo info_capabilities = { static const vshCmdOptDef opts_capabilities[] = { {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -108,7 +107,6 @@ static const vshCmdOptDef opts_domcapabilities[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index a08bb822ed..755c0d6455 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -453,7 +453,6 @@ static const vshCmdOptDef opts_interface_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5bc91a361f..065e59c3a1 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -639,7 +639,6 @@ static const vshCmdOptDef opts_network_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -1874,7 +1873,6 @@ static const vshCmdOptDef opts_network_port_dumpxml[] = { VIRSH_COMMON_OPT_NETWORK_PORT(0), {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index fafebf9972..b0563395f0 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -584,7 +584,6 @@ static const vshCmdOptDef opts_node_device_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index f54d2a0ae5..69473b4935 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -175,7 +175,6 @@ static const vshCmdOptDef opts_nwfilter_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -577,7 +576,6 @@ static const vshCmdOptDef opts_nwfilter_binding_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index c93204dd30..aff1201ef7 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -748,7 +748,6 @@ static const vshCmdOptDef opts_pool_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index edf3c47362..5653451862 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -131,7 +131,6 @@ static const vshCmdOptDef opts_secret_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, @@ -187,7 +186,6 @@ static const vshCmdOptDef opts_secret_set_value[] = { }, {.name = "file", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompletePathLocalExisting, .help = N_("read secret from file"), }, diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 2fcf6e3d5e..d64649a8da 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -359,7 +359,6 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { }, {.name = "memspec", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .help = N_("memory attributes: [file=]name[,snapshot=type]") }, {.name = "diskspec", @@ -1587,7 +1586,6 @@ static const vshCmdOptDef opts_snapshot_dumpxml[] = { }, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 4e5673e0ee..67a6f2eda0 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -1130,7 +1130,6 @@ static const vshCmdOptDef opts_vol_dumpxml[] = { VIRSH_COMMON_OPT_POOL_OPTIONAL, {.name = "xpath", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_REQ_OPT, .completer = virshCompleteEmpty, .help = N_("xpath expression to filter the XML document") }, diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 514f434c94..9a29a67999 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1010,7 +1010,6 @@ static const vshCmdOptDef opts_daemon_timeout[] = { .type = VSH_OT_INT, .required = true, .help = N_("number of seconds the daemon will run without any active connection"), - .flags = VSH_OFLAG_REQ_OPT }, {.name = NULL} }; diff --git a/tools/vsh.c b/tools/vsh.c index 2c90ca44f7..1dac869413 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -244,14 +244,12 @@ static int disconnected; /* we may have been disconnected */ static int vshCmddefCheckInternals(vshControl *ctl, const vshCmdDef *cmd, - bool missingCompleters, - int brokenPositionals) + bool missingCompleters) { size_t i; bool seenOptionalOption = false; const char *seenOptionalPositionalOption = NULL; g_auto(virBuffer) complbuf = VIR_BUFFER_INITIALIZER; - g_auto(virBuffer) posbuf = VIR_BUFFER_INITIALIZER; /* in order to perform the validation resolve the alias first */ if (cmd->alias) { @@ -328,29 +326,6 @@ vshCmddefCheckInternals(vshControl *ctl, } } - if (brokenPositionals >= 0) { - switch (opt->type) { - case VSH_OT_INT: - case VSH_OT_STRING: - case VSH_OT_ARGV: - if (brokenPositionals == 0 || - brokenPositionals == opt->type) { - if (!(opt->flags & VSH_OFLAG_REQ_OPT) && - !(opt->positional || opt->unwanted_positional)) - virBufferStrcat(&posbuf, opt->name, ", ", NULL); - } - break; - - case VSH_OT_BOOL: - /* only name is completed */ - /* no point in completing numbers */ - case VSH_OT_ALIAS: - /* alias is handled in the referenced command */ - case VSH_OT_NONE: - break; - } - } - /* allow at most one optional positional option */ if (opt->positional && !opt->required) { if (seenOptionalPositionalOption) { @@ -471,15 +446,10 @@ vshCmddefCheckInternals(vshControl *ctl, } virBufferTrim(&complbuf, ", "); - virBufferTrim(&posbuf, ", "); if (missingCompleters && virBufferUse(&complbuf) > 0) vshPrintExtra(ctl, "%s: %s\n", cmd->name, virBufferCurrentContent(&complbuf)); - if (virBufferUse(&posbuf)) { - vshPrintExtra(ctl, "%s: %s\n", cmd->name, virBufferCurrentContent(&posbuf)); - } - return 0; } @@ -508,7 +478,7 @@ vshCmddefOptParse(const vshCmdDef *cmd, if (opt->type == VSH_OT_ALIAS) continue; /* skip the alias option */ - if (!(opt->flags & VSH_OFLAG_REQ_OPT)) + if (opt->positional || opt->unwanted_positional) *opts_need_arg |= 1ULL << i; if (opt->required) @@ -3387,11 +3357,6 @@ const vshCmdOptDef opts_selftest[] = { .type = VSH_OT_BOOL, .help = N_("output help for each command") }, - {.name = "broken-positionals", - .type = VSH_OT_INT, - .flags = VSH_OFLAG_REQ_OPT, - .help = N_("debug positional args") - }, {.name = NULL} }; const vshCmdInfo info_selftest = { @@ -3406,9 +3371,6 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd) const vshCmdDef *def; bool completers = vshCommandOptBool(cmd, "completers-missing"); bool dumphelp = vshCommandOptBool(cmd, "dump-help"); - int brokenPositionals = -1; - - ignore_value(vshCommandOptInt(ctl, cmd, "broken-positionals", &brokenPositionals)); for (grp = cmdGroups; grp->name; grp++) { for (def = grp->commands; def->name; def++) { @@ -3416,7 +3378,7 @@ cmdSelfTest(vshControl *ctl, const vshCmd *cmd) if (dumphelp && !def->alias) vshCmddefHelp(def); - if (vshCmddefCheckInternals(ctl, def, completers, brokenPositionals) < 0) + if (vshCmddefCheckInternals(ctl, def, completers) < 0) return false; } } diff --git a/tools/vsh.h b/tools/vsh.h index fdcc89b12a..02c35488b9 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -99,7 +99,6 @@ typedef enum { enum { VSH_OFLAG_NONE = 0, /* without flags */ VSH_OFLAG_EMPTY_OK = (1 << 1), /* empty string option allowed */ - VSH_OFLAG_REQ_OPT = (1 << 2), /* --optionname required */ }; /* forward declarations */ -- 2.44.0
Replace the last bitwise flag with a separate member. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-monitor.c | 2 +- tools/virsh-domain.c | 6 +++--- tools/virsh.c | 2 +- tools/virt-admin.c | 6 +++--- tools/vsh.c | 6 +++--- tools/vsh.h | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 5531d3b737..eec97b7d59 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -864,8 +864,8 @@ static const vshCmdOptDef opts_domblkstat[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "device", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_EMPTY_OK, .positional = true, + .allowEmpty = true, .completer = virshDomainDiskTargetCompleter, .help = N_("block device") }, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 91a9dfd96a..1ba38629ac 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -422,7 +422,7 @@ static const vshCmdOptDef opts_attach_disk[] = { .type = VSH_OT_STRING, .positional = true, .required = true, - .flags = VSH_OFLAG_EMPTY_OK, + .allowEmpty = true, .help = N_("source of disk device or name of network disk") }, {.name = "target", @@ -6838,7 +6838,7 @@ static const vshCmdOptDef opts_vcpupin[] = { {.name = "cpulist", .type = VSH_OT_STRING, .unwanted_positional = true, - .flags = VSH_OFLAG_EMPTY_OK, + .allowEmpty = true, .completer = virshDomainCpulistCompleter, .help = N_("host cpu number(s) to set, or omit option to query") }, @@ -7047,7 +7047,7 @@ static const vshCmdOptDef opts_emulatorpin[] = { {.name = "cpulist", .type = VSH_OT_STRING, .unwanted_positional = true, - .flags = VSH_OFLAG_EMPTY_OK, + .allowEmpty = true, .completer = virshDomainCpulistCompleter, .help = N_("host cpu number(s) to set, or omit option to query") }, diff --git a/tools/virsh.c b/tools/virsh.c index 890c96e552..0a586fd639 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -246,7 +246,7 @@ static const vshCmdOptDef opts_connect[] = { {.name = "name", .type = VSH_OT_STRING, .positional = true, - .flags = VSH_OFLAG_EMPTY_OK, + .allowEmpty = true, .completer = virshCompleteEmpty, .help = N_("hypervisor connection URI") }, diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 9a29a67999..ce60077672 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -255,7 +255,7 @@ static const vshCmdOptDef opts_connect[] = { {.name = "name", .type = VSH_OT_STRING, .positional = true, - .flags = VSH_OFLAG_EMPTY_OK, + .allowEmpty = true, .help = N_("daemon's admin server connection URI") }, {.name = NULL} @@ -961,7 +961,7 @@ static const vshCmdOptDef opts_daemon_log_filters[] = { .type = VSH_OT_STRING, .positional = true, .help = N_("redefine the existing set of logging filters"), - .flags = VSH_OFLAG_EMPTY_OK + .allowEmpty = true }, {.name = NULL} }; @@ -1044,7 +1044,7 @@ static const vshCmdOptDef opts_daemon_log_outputs[] = { .type = VSH_OT_STRING, .positional = true, .help = N_("redefine the existing set of logging outputs"), - .flags = VSH_OFLAG_EMPTY_OK + .allowEmpty = true }, {.name = NULL} }; diff --git a/tools/vsh.c b/tools/vsh.c index 1dac869413..a12f0a635d 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1030,7 +1030,7 @@ vshCommandOptStringQuiet(vshControl *ctl G_GNUC_UNUSED, const vshCmd *cmd, if ((ret = vshCommandOpt(cmd, name, &arg, true)) <= 0) return ret; - if (!*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK)) + if (!arg->def->allowEmpty && *arg->data == '\0') return -1; *value = arg->data; return 1; @@ -1069,7 +1069,7 @@ vshCommandOptStringReq(vshControl *ctl, /* this should not be propagated here, just to be sure */ if (ret == -1) error = N_("Mandatory option not present"); - else if (arg && !*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK)) + else if (arg && *arg->data == '\0' && !arg->def->allowEmpty) error = N_("Option argument is empty"); if (error) { @@ -3394,7 +3394,7 @@ const vshCmdOptDef opts_complete[] = { {.name = "string", .type = VSH_OT_ARGV, .positional = true, - .flags = VSH_OFLAG_EMPTY_OK, + .allowEmpty = true, .help = N_("partial string to autocomplete") }, {.name = NULL} diff --git a/tools/vsh.h b/tools/vsh.h index 02c35488b9..1921645fca 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -98,7 +98,6 @@ typedef enum { */ enum { VSH_OFLAG_NONE = 0, /* without flags */ - VSH_OFLAG_EMPTY_OK = (1 << 1), /* empty string option allowed */ }; /* forward declarations */ @@ -140,6 +139,7 @@ struct _vshCmdOptDef { bool unwanted_positional; unsigned int flags; /* flags */ + bool allowEmpty; /* allow empty string */ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ vshCompleter completer; /* option completer */ -- 2.44.0
Drop the last enum member VSH_OFLAG_NONE and remove the 'flags' variable from vshCmdOptDef. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-monitor.c | 3 --- tools/virsh-domain.c | 4 ---- tools/virsh-network.c | 1 - tools/virsh-pool.c | 3 --- tools/vsh.c | 1 - tools/vsh.h | 8 -------- 6 files changed, 20 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index eec97b7d59..599ae71e7a 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -2222,17 +2222,14 @@ static const vshCmdOptDef opts_domifaddr[] = { {.name = "interface", .type = VSH_OT_STRING, .positional = true, - .flags = VSH_OFLAG_NONE, .completer = virshDomainInterfaceCompleter, .help = N_("network interface name")}, {.name = "full", .type = VSH_OT_BOOL, - .flags = VSH_OFLAG_NONE, .help = N_("always display names and MACs of interfaces")}, {.name = "source", .type = VSH_OT_STRING, .unwanted_positional = true, - .flags = VSH_OFLAG_NONE, .completer = virshDomainInterfaceAddrSourceCompleter, .help = N_("address source: 'lease', 'agent', or 'arp'")}, {.name = NULL} diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1ba38629ac..694958f990 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2290,7 +2290,6 @@ static const vshCmdOptDef opts_blockcopy[] = { {.name = "format", .type = VSH_OT_STRING, .unwanted_positional = true, - .flags = VSH_OFLAG_NONE, .completer = virshDomainStorageFileFormatCompleter, .help = N_("format of the destination file") }, @@ -5032,7 +5031,6 @@ static const vshCmdOptDef opts_schedinfo[] = { {.name = "set", .type = VSH_OT_ARGV, .positional = true, - .flags = VSH_OFLAG_NONE, .help = N_("parameter=value") }, {.name = NULL} @@ -5345,7 +5343,6 @@ static const vshCmdOptDef opts_dump[] = { }, {.name = "format", .type = VSH_OT_STRING, - .flags = VSH_OFLAG_NONE, .unwanted_positional = true, .completer = virshDomainCoreDumpFormatCompleter, .help = N_("specify the format of memory-only dump") @@ -11874,7 +11871,6 @@ static const vshCmdOptDef opts_domhostname[] = { {.name = "source", .type = VSH_OT_STRING, .unwanted_positional = true, - .flags = VSH_OFLAG_NONE, .completer = virshDomainHostnameSourceCompleter, .help = N_("address source: 'lease' or 'agent'")}, {.name = NULL} diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 065e59c3a1..e6552cbe57 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1700,7 +1700,6 @@ static const vshCmdOptDef opts_network_dhcp_leases[] = { {.name = "mac", .type = VSH_OT_STRING, .unwanted_positional = true, - .flags = VSH_OFLAG_NONE, .help = N_("MAC address"), .completer = virshNetworkDhcpMacCompleter, }, diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index aff1201ef7..24e7bb9bf5 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -37,21 +37,18 @@ #define VIRSH_COMMON_OPT_POOL_BUILD \ {.name = "build", \ .type = VSH_OT_BOOL, \ - .flags = 0, \ .help = N_("build the pool as normal") \ } #define VIRSH_COMMON_OPT_POOL_NO_OVERWRITE \ {.name = "no-overwrite", \ .type = VSH_OT_BOOL, \ - .flags = 0, \ .help = N_("do not overwrite any existing data") \ } #define VIRSH_COMMON_OPT_POOL_OVERWRITE \ {.name = "overwrite", \ .type = VSH_OT_BOOL, \ - .flags = 0, \ .help = N_("overwrite any existing data") \ } diff --git a/tools/vsh.c b/tools/vsh.c index a12f0a635d..7305160ed9 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -393,7 +393,6 @@ vshCmddefCheckInternals(vshControl *ctl, opt->positional || opt->unwanted_positional || opt->completer || - opt->flags || !opt->help) { vshError(ctl, "parameter '%s' of command '%s' has incorrect alias option", opt->name, cmd->name); diff --git a/tools/vsh.h b/tools/vsh.h index 1921645fca..f06d65407d 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -93,13 +93,6 @@ typedef enum { VSH_OT_ALIAS, /* alternate spelling for a later argument */ } vshCmdOptType; -/* - * Command Option Flags - */ -enum { - VSH_OFLAG_NONE = 0, /* without flags */ -}; - /* forward declarations */ typedef struct _vshClientHooks vshClientHooks; typedef struct _vshCmd vshCmd; @@ -138,7 +131,6 @@ struct _vshCmdOptDef { * 'unwanted_positional' flag. New options must not use this flag */ bool unwanted_positional; - unsigned int flags; /* flags */ bool allowEmpty; /* allow empty string */ const char *help; /* non-NULL help string; or for VSH_OT_ALIAS * the name of a later public option */ -- 2.44.0
On a Friday in 2024, Peter Krempa wrote:
Part 2 was supposed to be the refactor of the parser, but it turns out that our annotations of arguments and quirks of the parser itself resulted with basically every optional argument with value being also considered for positional parisng.
This series explains where this happens and annotates all cases.
Peter Krempa (28): vshCmddefHelp: Drop empty line at the end virsh: cmdDomdisplayReload: Require option name for --type vshCmddefCheckInternals: Improve some checks virsh: Inline VIRSH_COMMON_OPT_DOMAIN_OT_STRING macro virsh: Inline VIRSH_COMMON_OPT_FILE_FULL macro virsh: Inline VIRSH_COMMON_OPT_NETWORK_OT_STRING macro vsh: Introduce tool to find unwanted positional arguments to 'self-test' vsh: Introduce annotation for vsh options which are unexpectedly parsed positionally virsh: Annotate '--diskspec' _ARGV options as unwanted positional virsh: Annotate rest of _ARGV arguments as positional vsh: Fix option formatting for 'VHS_OT_ARGV' options virsh: Require option flags for 'blkdeviotune' arguments virsh: Fix "positional" argument annotations for 'migrate' command virsh: Require option flags for all optional arguments of 'attach-disk' virsh-checkpoint: Make 'checkpointname' positional and required vsh: Allow one optional positional argument vsh: Make the only argument of 'connect', 'cd', and 'help' commands positional virsh: snapshot: Make 'snapshotname' argument positional virsh: Make '(snapshot|checkpoint)-create' 'xmlfile' argument positional virsh-backup: Fix argument annotations of 'backup-begin' command virsh: Annotate some optional arguments as positional virsh: volume: Mark optional 'pool' argument as 'positional' virsh: Annotate "unwanted_positional" arguments for 'pool-(define|create)-as' commands virsh: Annodate 'unwanted_positional' arguments virt-admin: Annodate 'unwanted_positional' arguments vsh: Make positional parsing of arguments opt-in vsh: Replace 'VSH_OFLAG_EMPTY_OK' bitwise flag with a separate struct member vshCmdOptDef: Remove unused 'flags' member
tools/virsh-backup.c | 3 +- tools/virsh-checkpoint.c | 26 ++++--- tools/virsh-domain-event.c | 10 ++- tools/virsh-domain-monitor.c | 11 +-- tools/virsh-domain.c | 134 ++++++++++++++++++++++++++++------- tools/virsh-host.c | 33 +++++++-- tools/virsh-interface.c | 2 +- tools/virsh-network.c | 28 ++++---- tools/virsh-nodedev.c | 6 +- tools/virsh-nwfilter.c | 2 - tools/virsh-pool.c | 31 ++++++-- tools/virsh-secret.c | 6 +- tools/virsh-snapshot.c | 13 +++- tools/virsh-volume.c | 12 +++- tools/virsh.c | 3 +- tools/virsh.h | 20 +----- tools/virt-admin.c | 15 ++-- tools/vsh.c | 64 +++++++++++++---- tools/vsh.h | 18 +++-- 19 files changed, 316 insertions(+), 121 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa