[libvirt] [PATCH v2 0/9] More consistent virDomainUndefine flag handling
Since v1: - use syntax-check rather than dynamic runtime check for API mismatch - fix more stragglers with mismatched API, found by the syntax-check - fix a bug in bhyve no-op flag handling - expand no-op flag handling to other affected drivers Eric Blake (9): vbox: Add various vir*Flags API xenapi: Add various vir*Flags API maint: Enhance check-driverimpls.pl to check for API pairing bhyve: Ignore no-op flag during virDomainUndefine libxl: Ignore no-op flag during virDomainUndefine lxc: Ignore no-op flag during virDomainUndefine openvz: Ignore no-op flag during virDomainUndefine vmware: Ignore no-op flag during virDomainUndefine xenapi: Ignore no-op flag during virDomainUndefine src/bhyve/bhyve_driver.c | 6 +++++- src/check-driverimpls.pl | 33 +++++++++++++++++++++++++++++++-- src/libxl/libxl_driver.c | 4 +++- src/lxc/lxc_driver.c | 5 ++++- src/openvz/openvz_driver.c | 5 ++++- src/vbox/vbox_common.c | 24 ++++++++++++++++++++++-- src/vmware/vmware_driver.c | 5 ++++- src/xenapi/xenapi_driver.c | 28 ++++++++++++++++++++++++---- 8 files changed, 97 insertions(+), 13 deletions(-) -- 2.20.1
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vbox/vbox_common.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 54e31bec9d..44c98cadf6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -553,7 +553,8 @@ static int vboxConnectClose(virConnectPtr conn) } static int -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, + const char *dxml, unsigned int flags) { vboxDriverPtr data = dom->conn->privateData; IConsole *console = NULL; @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) nsresult rc; int ret = -1; + virCheckFlags(0, -1); + virCheckNonNullArgReturn(dxml, -1); + if (!data->vboxObj) return ret; @@ -607,6 +611,12 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) return ret; } +static int +vboxDomainSave(virDomainPtr dom, const char *path) +{ + return vboxDomainSaveFlags(dom, path, NULL, 0); +} + static int vboxConnectGetVersion(virConnectPtr conn, unsigned long *version) { vboxDriverPtr data = conn->privateData; @@ -2717,7 +2727,8 @@ static char *vboxDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED) { return osType; } -static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) +static int vboxDomainSetMemoryFlags(virDomainPtr dom, unsigned long memory, + unsigned int flags) { vboxDriverPtr data = dom->conn->privateData; IMachine *machine = NULL; @@ -2727,6 +2738,8 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) nsresult rc; int ret = -1; + virCheckFlags(0, -1); + if (!data->vboxObj) return ret; @@ -2775,6 +2788,11 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) return ret; } +static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) +{ + return vboxDomainSetMemoryFlags(dom, memory, 0); +} + static int vboxDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { vboxDriverPtr data = dom->conn->privateData; @@ -7995,9 +8013,11 @@ static virHypervisorDriver vboxCommonDriver = { .domainDestroyFlags = vboxDomainDestroyFlags, /* 0.9.4 */ .domainGetOSType = vboxDomainGetOSType, /* 0.6.3 */ .domainSetMemory = vboxDomainSetMemory, /* 0.6.3 */ + .domainSetMemoryFlags = vboxDomainSetMemoryFlags, /* 5.6.0 */ .domainGetInfo = vboxDomainGetInfo, /* 0.6.3 */ .domainGetState = vboxDomainGetState, /* 0.9.2 */ .domainSave = vboxDomainSave, /* 0.6.3 */ + .domainSaveFlags = vboxDomainSaveFlags, /* 5.6.0 */ .domainSetVcpus = vboxDomainSetVcpus, /* 0.7.1 */ .domainSetVcpusFlags = vboxDomainSetVcpusFlags, /* 0.8.5 */ .domainGetVcpusFlags = vboxDomainGetVcpusFlags, /* 0.8.5 */ -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vbox/vbox_common.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 54e31bec9d..44c98cadf6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -553,7 +553,8 @@ static int vboxConnectClose(virConnectPtr conn) }
static int -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, + const char *dxml, unsigned int flags) { vboxDriverPtr data = dom->conn->privateData; IConsole *console = NULL; @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) nsresult rc; int ret = -1;
+ virCheckFlags(0, -1); + virCheckNonNullArgReturn(dxml, -1);
This reports: invalid argument: dxml in vboxDomainSave must not be NULL I'm not certain that the internal function name makes sense to external users.
+ if (!data->vboxObj) return ret;
@@ -607,6 +611,12 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) return ret; }
+static int +vboxDomainSave(virDomainPtr dom, const char *path) +{ + return vboxDomainSaveFlags(dom, path, NULL, 0);
So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly rejects it. What's the point? Ah I see. Was the above supposed to be virCheckNullArgGoto?
On 7/10/19 2:02 AM, Peter Krempa wrote:
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vbox/vbox_common.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 54e31bec9d..44c98cadf6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -553,7 +553,8 @@ static int vboxConnectClose(virConnectPtr conn) }
static int -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, + const char *dxml, unsigned int flags) { vboxDriverPtr data = dom->conn->privateData; IConsole *console = NULL; @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) nsresult rc; int ret = -1;
+ virCheckFlags(0, -1); + virCheckNonNullArgReturn(dxml, -1);
This reports: invalid argument: dxml in vboxDomainSave must not be NULL
I'm not certain that the internal function name makes sense to external users.
In which case I can hand-roll a more specific error instead of reusing the common macro. But I do see pre-existing uses of virCheckNonNullArgXXX in src/esx and src/vz prior to this patch, so it's not the first time we've used the common macros.
+ if (!data->vboxObj) return ret;
@@ -607,6 +611,12 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) return ret; }
+static int +vboxDomainSave(virDomainPtr dom, const char *path) +{ + return vboxDomainSaveFlags(dom, path, NULL, 0);
So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly rejects it. What's the point?
Ah I see. Was the above supposed to be virCheckNullArgGoto?
D'oh. Yes, I got it backwards. The function wants to succeed only if the user omitted the optional dxml argument. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 7/10/19 6:50 AM, Eric Blake wrote:
On 7/10/19 2:02 AM, Peter Krempa wrote:
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
-vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, + const char *dxml, unsigned int flags) { vboxDriverPtr data = dom->conn->privateData; IConsole *console = NULL; @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) nsresult rc; int ret = -1;
+ virCheckFlags(0, -1); + virCheckNonNullArgReturn(dxml, -1);
This reports: invalid argument: dxml in vboxDomainSave must not be NULL
Revisiting this thread: internal.h has: virCheckNullArgGoto (complains if argument is not NULL) virCheckNonNullArgReturn (complains if argument is NULL) virCheckNonNullArgGoto (complains if argument is NULL) but is missing virCheckNullArgReturn, which is the form I really meant to use here. I have no idea if that missing macro is intentional, but it would be easy enough to add.
I'm not certain that the internal function name makes sense to external users.
In which case I can hand-roll a more specific error instead of reusing the common macro. But I do see pre-existing uses of virCheckNonNullArgXXX in src/esx and src/vz prior to this patch, so it's not the first time we've used the common macros.
Directly calling virReportInvalidNonNullArg() would be the only way to hand-roll this properly, but no one outside of internal.h and src/util/virobject.c uses it. I'd prefer to sick with virCheckNullArgReturn().
+static int +vboxDomainSave(virDomainPtr dom, const char *path) +{ + return vboxDomainSaveFlags(dom, path, NULL, 0);
So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly rejects it. What's the point?
Ah I see. Was the above supposed to be virCheckNullArgGoto?
D'oh. Yes, I got it backwards. The function wants to succeed only if the user omitted the optional dxml argument.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vbox/vbox_common.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
[...]
@@ -2775,6 +2788,11 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) return ret; }
+static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) +{ + return vboxDomainSetMemoryFlags(dom, memory, 0);
The old API was operating on a live VM only so this shim should imply VIR_DOMAIN_AFFECT_LIVE.
+} + static int vboxDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { vboxDriverPtr data = dom->conn->privateData;
On 7/10/19 2:09 AM, Peter Krempa wrote:
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vbox/vbox_common.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
[...]
@@ -2775,6 +2788,11 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) return ret; }
+static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory) +{ + return vboxDomainSetMemoryFlags(dom, memory, 0);
The old API was operating on a live VM only so this shim should imply VIR_DOMAIN_AFFECT_LIVE.
Hmm. Commit 667ac11e used flag 0 for the test driver. But if I want to reduce the number of driver callbacks by instead teaching the remote driver when to call the legacy RPC, it's easier if ALL drivers shim-call SetMemoryFlags(VIR_DOMAIN_AFFECT_LIVE) rather than some calling with 0 and others calling with AFFECT_LIVE. The current list: v5.5.0:src/hyperv/hyperv_driver.c: return hypervDomainSetMemoryFlags(domain, memory, 0); v5.5.0:src/libxl/libxl_driver.c: return libxlDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_LIVE); v5.5.0:src/libxl/libxl_driver.c: return libxlDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_MAXIMUM); v5.5.0:src/lxc/lxc_driver.c: return lxcDomainSetMemoryFlags(dom, newmem, VIR_DOMAIN_AFFECT_LIVE); v5.5.0:src/lxc/lxc_driver.c: return lxcDomainSetMemoryFlags(dom, newmax, VIR_DOMAIN_MEM_MAXIMUM); v5.5.0:src/qemu/qemu_driver.c: return qemuDomainSetMemoryFlags(dom, newmem, VIR_DOMAIN_AFFECT_LIVE); v5.5.0:src/qemu/qemu_driver.c: return qemuDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_MAXIMUM); my series adds: test vbox xenapi So it looks like hyperv has a bug in being inconsistent from everyone else, if I follow your advice for the 3 new shims, although it currently lacks a SetMaxMemory API at all. You also appear to be right that making SetMaxMemory a shim everywhere for SetMemoryFlags(VIR_DOMAIN_MEM_MAXMIUM) is a sensible idea, at least for drivers that have a notion of setting max memory, but since not all drivers had SetMaxMemory, it's harder to argue that the presence of the new interface must require the legacy pair (especially if the new interface blindly requires a specific flags pattern). I'll have to look more closely at what each driver is doing. Our public docs currently state: * and will fail for transient domains. If neither flag is specified * (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain * modifies persistent setup, while an active domain is hypervisor-dependent * on whether just live or both live and persistent state is changed. which means that calling SetMemory() may be a synonym to either SetMemoryFlags(_CURRENT==0) or to SetMemoryFlags(_LIVE). But by tweaking hyperv, we could change it so that the spec is more tightly-worded as always being equivalent to SetMemoryFlags(_LIVE). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/xenapi/xenapi_driver.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 672117822f..026acef680 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1184,19 +1184,21 @@ xenapiDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) } /* - * xenapiDomainPinVcpu + * xenapiDomainPinVcpuFlags * * Dynamically change the real CPUs which can be allocated to a virtual CPU * Returns 0 on success or -1 in case of error */ static int -xenapiDomainPinVcpu(virDomainPtr dom, unsigned int vcpu ATTRIBUTE_UNUSED, - unsigned char *cpumap, int maplen) +xenapiDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu ATTRIBUTE_UNUSED, + unsigned char *cpumap, int maplen, + unsigned int flags) { char *value = NULL; xen_vm vm; xen_vm_set *vms; xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session; + virCheckFlags(0, -1); if (xen_vm_get_by_name_label(session, &vms, dom->name) && vms->size > 0) { if (vms->size != 1) { xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, @@ -1225,6 +1227,19 @@ xenapiDomainPinVcpu(virDomainPtr dom, unsigned int vcpu ATTRIBUTE_UNUSED, return -1; } +/* + * xenapiDomainPinVcpu + * + * Dynamically change the real CPUs which can be allocated to a virtual CPU + * Returns 0 on success or -1 in case of error + */ +static int +xenapiDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, + unsigned char *cpumap, int maplen) +{ + return xenapiDomainPinVcpuFlags(dom, vcpu, cpumap, maplen, 0); +} + /* * xenapiDomainGetVcpus * @@ -2029,6 +2044,7 @@ static virHypervisorDriver xenapiHypervisorDriver = { .domainSetVcpusFlags = xenapiDomainSetVcpusFlags, /* 0.8.5 */ .domainGetVcpusFlags = xenapiDomainGetVcpusFlags, /* 0.8.5 */ .domainPinVcpu = xenapiDomainPinVcpu, /* 0.8.0 */ + .domainPinVcpuFlags = xenapiDomainPinVcpuFlags, /* 5.6.0 */ .domainGetVcpus = xenapiDomainGetVcpus, /* 0.8.0 */ .domainGetMaxVcpus = xenapiDomainGetMaxVcpus, /* 0.8.0 */ .domainGetXMLDesc = xenapiDomainGetXMLDesc, /* 0.8.0 */ -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:31 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/xenapi/xenapi_driver.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
ACK
On Tue, Jul 09, 2019 at 12:46:31 -0500, Eric Blake wrote:
Even though we don't accept any flags, it is unfriendly to callers that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/xenapi/xenapi_driver.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
[...]
@@ -1225,6 +1227,19 @@ xenapiDomainPinVcpu(virDomainPtr dom, unsigned int vcpu ATTRIBUTE_UNUSED, return -1; }
+/* + * xenapiDomainPinVcpu + * + * Dynamically change the real CPUs which can be allocated to a virtual CPU + * Returns 0 on success or -1 in case of error + */ +static int +xenapiDomainPinVcpu(virDomainPtr dom, unsigned int vcpu, + unsigned char *cpumap, int maplen) +{ + return xenapiDomainPinVcpuFlags(dom, vcpu, cpumap, maplen, 0);
Actually in the qemu driver we pass in VIR_DOMAIN_AFFECT_LIVE as the flags here, because the old API worked only on the live definition.
As shown in recent patches, several drivers provided only an older counterpart of an API, making it harder to uniformly use the newer preferred API form. We can prevent future instances of this by enhancing 'make syntax-check' to flag any time a modern API is forgotten when an older API is present. It also flags if a modern API is provided without an old counterpart; but thankfully, that situation didn't flag, which gives us some room for future patches to confine the magic of API pairs to just src/libvirt*.c and the remote driver. Also, drop support for special-casing xenUnified, since 1dac5fbbbb0 dropped support for that naming scheme. --- src/check-driverimpls.pl | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index b175e710f1..34273ddbba 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -23,14 +23,41 @@ use warnings; my $intable = 0; my $table; my $mainprefix; +my %apis; + +# API pairs where a driver should provide both or neither alternative. +my %pairs = ( + 'domainShutdown' => 'domainShutdownFlags', + 'domainDestroy' => 'domainDestroyFlags', + 'domainSetMemory' => 'domainSetMemoryFlags', + 'domainSave' => 'domainSaveFlags', + 'domainRestore' => 'domainRestoreFlags', + 'domainSetVcpus' => 'domainSetVcpusFlags', + 'domainPinVcpu' => 'domainPinVcpuFlags', + 'domainCreate' => 'domainCreateWithFlags', + 'domainDefineXML' => 'domainDefineXMLFlags', + 'domainUndefine' => 'domainUndefineFlags', + 'domainAttachDevice' => 'domainAttachDeviceFlags', + 'domainDetachDevice' => 'domainDetachDeviceFlags', + 'domainGetSchedulerParameters' => 'domainGetSchedulerParametersFlags', + 'domainSetSchedulerParameters' => 'domainSetSchedulerParametersFlags', + 'nodeDeviceDettach' => 'nodeDeviceDetachFlags', +); my $status = 0; while (<>) { if ($intable) { if (/}/) { + while (my ($old, $new) = each %pairs) { + if (exists $apis{$old} != exists $apis{$new}) { + print "$ARGV:$. Inconsistent paired API '$old' vs. '$new'\n"; + $status = 1; + } + } $intable = 0; $table = undef; $mainprefix = undef; + %apis = (); } elsif (/\.(\w+)\s*=\s*(\w+),?/) { my $api = $1; my $impl = $2; @@ -39,9 +66,11 @@ while (<>) { next if $api eq "name"; next if $impl eq "NULL"; + $apis{$api} = 1; + my $suffix = $impl; my $prefix = $impl; - $prefix =~ s/^([a-z]+(?:Unified)?)(.*?)$/$1/; + $prefix =~ s/^([a-z]+)(.*?)$/$1/; if (defined $mainprefix) { if ($mainprefix ne $prefix) { @@ -53,7 +82,7 @@ while (<>) { } if ($api !~ /^$mainprefix/) { - $suffix =~ s/^[a-z]+(?:Unified)?//; + $suffix =~ s/^[a-z]+//; $suffix =~ s/^([A-Z]+)/lc $1/e; } -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:32 -0500, Eric Blake wrote:
As shown in recent patches, several drivers provided only an older counterpart of an API, making it harder to uniformly use the newer preferred API form. We can prevent future instances of this by enhancing 'make syntax-check' to flag any time a modern API is forgotten when an older API is present. It also flags if a modern API is provided without an old counterpart; but thankfully, that situation didn't flag, which gives us some room for future patches to confine the magic of API pairs to just src/libvirt*.c and the remote driver.
Also, drop support for special-casing xenUnified, since 1dac5fbbbb0 dropped support for that naming scheme.
Please put it in a separate patch.
--- src/check-driverimpls.pl | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl index b175e710f1..34273ddbba 100755 --- a/src/check-driverimpls.pl +++ b/src/check-driverimpls.pl @@ -23,14 +23,41 @@ use warnings; my $intable = 0; my $table; my $mainprefix; +my %apis; + +# API pairs where a driver should provide both or neither alternative. +my %pairs = ( + 'domainShutdown' => 'domainShutdownFlags', + 'domainDestroy' => 'domainDestroyFlags', + 'domainSetMemory' => 'domainSetMemoryFlags',
domainSetMaxMemory is in the same relationship here as it just implies VIR_DOMAIN_MEM_MAXIMUM passed to the *Flags API.
+ 'domainSave' => 'domainSaveFlags', + 'domainRestore' => 'domainRestoreFlags', + 'domainSetVcpus' => 'domainSetVcpusFlags', + 'domainPinVcpu' => 'domainPinVcpuFlags', + 'domainCreate' => 'domainCreateWithFlags', + 'domainDefineXML' => 'domainDefineXMLFlags', + 'domainUndefine' => 'domainUndefineFlags', + 'domainAttachDevice' => 'domainAttachDeviceFlags', + 'domainDetachDevice' => 'domainDetachDeviceFlags', + 'domainGetSchedulerParameters' => 'domainGetSchedulerParametersFlags', + 'domainSetSchedulerParameters' => 'domainSetSchedulerParametersFlags', + 'nodeDeviceDettach' => 'nodeDeviceDetachFlags', +);
my $status = 0; while (<>) { if ($intable) { if (/}/) { + while (my ($old, $new) = each %pairs) { + if (exists $apis{$old} != exists $apis{$new}) { + print "$ARGV:$. Inconsistent paired API '$old' vs. '$new'\n";
Without the context of the patch the message does not seem to explain what's happening to the unindoctrinated. I don't have a better suggestion though.
+ $status = 1; + } + } $intable = 0; $table = undef; $mainprefix = undef;
ACK if you split out the "Also fix this" stuff. Your call whether domainSetMaxMemory is worth covering or the error message needs changing.
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/bhyve/bhyve_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 4ce9ef0b95..84720e89e7 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -584,7 +584,11 @@ bhyveDomainUndefineFlags(virDomainPtr domain, unsigned int flags) virDomainObjPtr vm; int ret = -1; - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * bhyve, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); + if (!(vm = bhyveDomObjFromDomain(domain))) goto cleanup; -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:33 -0500, Eric Blake wrote:
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libxl/libxl_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 731700ded6..89959890c3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2921,7 +2921,9 @@ libxlDomainUndefineFlags(virDomainPtr dom, char *name = NULL; int ret = -1; - virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); + /* No snapshot support, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); if (!(vm = libxlDomObjFromDomain(dom))) goto cleanup; -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:34 -0500, Eric Blake wrote:
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/lxc/lxc_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 9db2a02dee..a1bb29d5ae 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -491,7 +491,10 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, int ret = -1; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * lxc, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:35 -0500, Eric Blake wrote:
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/openvz/openvz_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 39eeb2c12e..19c7db48ed 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1126,7 +1126,10 @@ openvzDomainUndefineFlags(virDomainPtr dom, int ret = -1; int status; - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * openvz, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); openvzDriverLock(driver); if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:36 -0500, Eric Blake wrote:
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/vmware/vmware_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 1bc8a06c39..934378808d 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -776,7 +776,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * vmware, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); vmwareDriverLock(driver); if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:37 -0500, Eric Blake wrote:
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/xenapi/xenapi_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 026acef680..2cc6dbd22d 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1791,7 +1791,11 @@ xenapiDomainUndefineFlags(virDomainPtr dom, unsigned int flags) struct xen_vm_set *vms; xen_vm vm; xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session; - virCheckFlags(0, -1); + + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * xenapi, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); if (xen_vm_get_by_name_label(session, &vms, dom->name) && vms->size > 0) { if (vms->size != 1) { -- 2.20.1
On Tue, Jul 09, 2019 at 12:46:38 -0500, Eric Blake wrote:
Copy what esx does in ignoring the SNAPSHOTS_METADATA flag as a no-op, and in line with the recent doc tweak in commit c049f022.
Signed-off-by: Eric Blake <eblake@redhat.com> ---
ACK
participants (2)
-
Eric Blake -
Peter Krempa