Re: [PATCH v2] Remove VIR_FREE in favor of g_autofree in some functions in libvrit-domain.c
Please advise on what actions (if any) should be taken so that the changes in the patch is merged into main. Cheers, Mostafa Mahmoud. From: m kamal Sent: Tuesday, March 12, 2024 5:58 PM To: Peter Krempa Cc: devel@lists.libvirt.org Subject: Re: [PATCH v2] Remove VIR_FREE in favor of g_autofree in some functions in libvrit-domain.c Thanks a lot for the review, Peter. The first name I used was by mistake, it's the name of my Github profile, Github desktop probably auto-configured it as the global user name for git on my machine. It's in Arabic so it could confuse many non-Unicode-aware programs or protocols. The other name "m kamal" is just a contraction I wrote when I was subscribing to the mailing list from the web UI, I didn't realize it's important. I will change my configured name in the .gitpublish config to be the longer name "Mostafa Mahmoud", that's the first 2 names of my legal name, it appears that most names here are also 2 names. If there is also a way to change my forum name "m kamal" please let me know. Sorry for any inconvenience. One last question: After the patch got your approval, is there something on my end to do so that it gets merged? Will it automatically gets picked up by the Gitlab CI pipeline somehow? Cheers, Mostafa Mahmoud. From: Peter Krempa Sent: Tuesday, March 12, 2024 9:57 AM To: Mostafa Cc: devel@lists.libvirt.org Subject: Re: [PATCH v2] Remove VIR_FREE in favor of g_autofree in some functions in libvrit-domain.c On Tue, Mar 12, 2024 at 00:11:28 +0200, Mostafa wrote: The patch looks good now, but you've used multiple names in various fields over the two postings and the preference in terms of both authorship and sign-off is to use a full real name Used previously: > From: مصطفي محمود كمال الدين <48567303+most...@users.noreply.github.com> > From: m kamal The one in this patch: > Signed-off-by: Mostafa I don't want to assume anything about foreign names, but the last one looks more like a nickname or given name only. > --- > src/libvirt-domain.c | 32 > 1 file changed, 8 insertions(+), 24 deletions(-) For the code: Reviewed-by: Peter Krempa ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH] virthreadpool: create threads from the newly expanded workers
when the thread pool is dynamically expanded, threads should not be created from the existing workers; they should be created from the newly expanded workers Signed-off-by: Wei Gong --- src/util/virthreadpool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index d45fa92061..6140b7a0a3 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -199,7 +199,7 @@ virThreadPoolExpand(virThreadPool *pool, size_t gain, bool priority) else name = g_strdup(pool->jobName); -if (virThreadCreateFull(&(*workers)[i], +if (virThreadCreateFull(&(*workers)[*curWorkers - gain + i], false, virThreadPoolWorker, name, -- 2.32.1 (Apple Git-133) ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 24/28] virsh: Annodate 'unwanted_positional' arguments
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
2
3
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
---
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
[PATCH 26/28] vsh: Make positional parsing of arguments opt-in
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
---
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 =
[PATCH 28/28] vshCmdOptDef: Remove unused 'flags' member
Drop the last enum member VSH_OFLAG_NONE and remove the 'flags' variable
from vshCmdOptDef.
Signed-off-by: Peter Krempa
---
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
@@ -,17 +,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
[PATCH 27/28] vsh: Replace 'VSH_OFLAG_EMPTY_OK' bitwise flag with a separate struct member
Replace the last bitwise flag with a separate member.
Signed-off-by: Peter Krempa
---
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, , 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
[PATCH 23/28] virsh: Annotate "unwanted_positional" arguments for 'pool-(define|create)-as' commands
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 25/28] virt-admin: Annodate 'unwanted_positional' arguments
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
2
3
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 22/28] virsh: volume: Mark optional 'pool' argument as 'positional'
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 21/28] virsh: Annotate some optional arguments as positional
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 20/28] virsh-backup: Fix argument annotations of 'backup-begin' command
Mark the 'backupxml' as positional optional and the 'checkpointxml' as
'unwanted_positional' to preserve the positioanl parsing quirk.
Signed-off-by: Peter Krempa
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 19/28] virsh: Make '(snapshot|checkpoint)-create' 'xmlfile' argument positional
The argument is optional thus couldn't be marked as positional until now,
despite being parsed positionally.
Signed-off-by: Peter Krempa
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 18/28] virsh: snapshot: Make 'snapshotname' argument positional
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 16/28] vsh: Allow one optional positional argument
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 15/28] virsh-checkpoint: Make 'checkpointname' positional and required
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
---
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, ) < 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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 17/28] vsh: Make the only argument of 'connect', 'cd', and 'help' commands positional
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 12/28] virsh: Require option flags for 'blkdeviotune' arguments
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
---
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,
[PATCH 14/28] virsh: Require option flags for all optional arguments of 'attach-disk'
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
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 13/28] virsh: Fix "positional" argument annotations for 'migrate' command
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
---
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
[PATCH 11/28] vsh: Fix option formatting for 'VHS_OT_ARGV' options
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
---
tools/vsh.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/vsh.c b/tools/vsh.c
index 58d39e..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] }..."),
opt->name);
+fprintf(stdout, _(" --%1$s ..."), opt->name);
} else {
-fprintf(stdout, _(" [[--%1$s] ]..."),
opt->name);
+fprintf(stdout, _(" [--%1$s ]..."), 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] ..."),
opt->name);
} else {
-optstr = g_strdup_printf(_("[--%1$s] "),
opt->name);
+optstr = g_strdup_printf(_("--%1$s ..."),
opt->name);
}
break;
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 10/28] virsh: Annotate rest of _ARGV arguments as positional
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
---
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 +1,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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 09/28] virsh: Annotate '--diskspec' _ARGV options as unwanted positional
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 05/28] virsh: Inline VIRSH_COMMON_OPT_FILE_FULL macro
The macro is used in one place only and the command definition will be
altered. Inline it.
Signed-off-by: Peter Krempa
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 08/28] vsh: Introduce annotation for vsh options which are unexpectedly parsed positionally
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
---
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..58d39e 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(, 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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 07/28] vsh: Introduce tool to find unwanted positional arguments to 'self-test'
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 [--name ] [--description ]
[--print-xml] [--no-metadata] [--halt] [--disk-only]
[--reuse-external] [--quiesce] [--atomic] [--live] [--validate]
[--memspec ] [[--diskspec] ]...
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
2
3
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
---
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(, 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(, ", ");
+virBufferTrim(, ", ");
if (missingCompleters && virBufferUse() > 0)
vshPrintExtra(ctl, "%s: %s\n", cmd->name,
virBufferCurrentContent());
+if (virBufferUse()) {
+vshPrintExtra(ctl, "%s: %s\n", cmd->name,
virBufferCurrentContent());
+}
+
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",
));
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
[PATCH 03/28] vshCmddefCheckInternals: Improve some checks
- 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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 06/28] virsh: Inline VIRSH_COMMON_OPT_NETWORK_OT_STRING macro
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 01/28] vshCmddefHelp: Drop empty line at the end
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 04/28] virsh: Inline VIRSH_COMMON_OPT_DOMAIN_OT_STRING macro
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[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 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH 02/28] virsh: cmdDomdisplayReload: Require option name for --type
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
---
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
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [libvirt PATCH 00/12] Add vmx-* features from qemu
On Thu, Mar 14, 2024 at 16:51:31 -0400, Sergio Durigan Junior wrote: > On Wednesday, March 13 2024, I wrote: > > > On Tuesday, March 12 2024, Jiri Denemark wrote: > > > >> On Wed, Jan 24, 2024 at 22:19:56 -0500, Sergio Durigan Junior wrote: > >>> On Monday, January 22 2024, I wrote: > >>> > >>> > On Wednesday, November 22 2023, Jiri Denemark wrote: > >>> > > >>> >> On Thu, Nov 09, 2023 at 15:30:46 +0100, Tim Wiederhake wrote: > >>> >>> For rationale, see patch 3 (cpu_map: No longer ignore vmx- features in > >>> >>> sync_qemu_features_i386.py). > >>> >>> > >>> >>> Adding features in bunches (one patch per msr index), as this series > >>> >>> is adding ~100 features. > >>> >>> > >>> >>> This also adds cpu features "gds-no" and "amx-complex" and brings > >>> >>> libvirt in sync with qemu commit ad6ef0a42e. > >>> >>> > >>> >>> Tim Wiederhake (12): > >>> >>> cpu_map: Add missing feature "gds-no" > >>> >>> cpu_map: Add missing feature "amx-complex" > >>> >>> cpu_map: No longer ignore vmx- features in > >>> >>> sync_qemu_features_i386.py > >>> >>> cpu_map: Add missing vmx features from MSR 0x480 > >>> >>> cpu_map: Add missing vmx features from MSR 0x485 > >>> >>> cpu_map: Add missing vmx features from MSR 0x48B > >>> >>> cpu_map: Add missing vmx features from MSR 0x48C > >>> >>> cpu_map: Add missing vmx features from MSR 0x48D > >>> >>> cpu_map: Add missing vmx features from MSR 0x48E > >>> >>> cpu_map: Add missing vmx features from MSR 0x48F > >>> >>> cpu_map: Add missing vmx features from MSR 0x490 > >>> >>> cpu_map: Add missing vmx features from MSR 0x491 > >>> >> > >>> >> I haven't really checked the values of each feature you're adding, but > >>> >> all changes except for those in patch 3 are generated so I expect they > >>> >> are correct :-) > >>> > > >>> > Hi there, > >>> > > >>> > Apologies if this is a PEBCAK, but I'm noticing a strange behaviour when > >>> > testing live migrations from libvirt 9.6 to 9.10+ (including 10.0). > >>> > "virsh migrate --live ..." says: > >>> > > >>> > error: operation failed: guest CPU doesn't match specification: > >>> > extra features: > >>> > vmx-ins-outs,vmx-true-ctls,vmx-store-lma,vmx-activity-hlt,vmx-vmwrite-vmexit-fields,vmx-apicv-xapic,vmx-ept,vmx-desc-exit,vmx-rdtscp-exit,vmx-apicv-x2apic,vmx-vpid,vmx-wbinvd-exit,vmx-unrestricted-guest,vmx-apicv-register,vmx-apicv-vid,vmx-rdrand-exit,vmx-invpcid-exit,vmx-vmfunc,vmx-shadow-vmcs,vmx-invvpid,vmx-invvpid-single-addr,vmx-invvpid-all-context,vmx-ept-execonly,vmx-page-walk-4,vmx-ept-2mb,vmx-ept-1gb,vmx-invept,vmx-eptad,vmx-invept-single-context,vmx-invept-all-context,vmx-intr-exit,vmx-nmi-exit,vmx-vnmi,vmx-preemption-timer,vmx-posted-intr,vmx-vintr-pending,vmx-tsc-offset,vmx-hlt-exit,vmx-invlpg-exit,vmx-mwait-exit,vmx-rdpmc-exit,vmx-rdtsc-exit,vmx-cr3-load-noexit,vmx-cr3-store-noexit,vmx-cr8-load-exit,vmx-cr8-store-exit,vmx-flexpriority,vmx-vnmi-pending,vmx-movdr-exit,vmx-io-exit,vmx-io-bitmap,vmx-mtf,vmx-msr-bitmap,vmx-monitor-exit,vmx-pause-exit,vmx-secondary-ctls,vmx-exit-nosave-debugctl,vmx-exit-load-perf-global-ctrl,vmx-exit-ack-intr,vmx-exit-save-pat,vmx-exit-load-pat,vmx-exit-save-efer,vmx-exit-load-efer,vmx-exit-save-preemption-timer,vmx-entry-noload-debugctl,vmx-entry-ia32e-mode,vmx-entry-load-perf-global-ctrl,vmx-entry-load-pat,vmx-entry-load-efer,vmx-eptp-switching > >>> > > >>> > Judging by the missing features it's complaining about, it seems to be > >>> > something related to what was discussed in this thread. > >>> > > >>> > I could provide more details about the setup, although it's pretty > >>> > straightforward (create a VM under libvirt < 9.10 and try to live > >>> > migrate it to a system running libvirt >= 9.10). > >>> > >>> Hi again, > >>> > >>> Any news on this one? If it is indeed a regression, it seems like an > >>> important one. While looking through the open issues on gitlab I > >>> stumbled upon this one: > >>> > >>> https://gitlab.com/libvirt/libvirt/-/issues/568 > >>> > >>> ... which seems to describe the same problem. > >> > >> Yes, it's the same issue. I've just sent patches that should fix this > >> regression for review: > >> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ROAHSG43CLIZPPJV4ZL76VQYUAFXOXXE/ > >> > >> Sorry for the delay. > > > > Thank you for the fix, Jiri. > > > > I'll test the series locally and check if the issue is fixed on my end. > > I tested the patchset and there seems to be a few vmx-* still missing > from the definition: > > error: operation failed: guest CPU doesn't match specification: missing > features: > vmx-apicv-xapic,vmx-apicv-register,vmx-apicv-vid,vmx-vmfunc,vmx-posted-intr,vmx-eptp-switching This is impossible when migrating from < 9.10 to current master. It means the listed features where enabled on the source, but the destination cannot enable them. In which case, you definitely don't want to proceed with migration as the guest could crash when it tries to use the feature that is
