[libvirt] [PATCH 0/6] virDomainFlagsUndefine cleanups
Pointed out by Peter in response to: https://www.redhat.com/archives/libvir-list/2019-July/msg00277.html Patches 1-5 fix a pre-existing problem in several drivers, then provides a way to ensure we don't encounter it again. Patch 6 is an example of what I plan to do to all the drivers that have virDomainUndefineFlags but not snapshot support. Eric Blake (6): bhyve: Add various vir*Flags API esx: Add various vir*Flags API phyp: Add various vir*Flags API test: Add various vir*Flags API libvirt: Ensure modern APIs are implemented bhyve: Ignore no-op flags during virDomainUndefine src/bhyve/bhyve_driver.c | 41 +++++++++++++++++++++++++++++++++------ src/esx/esx_driver.c | 13 ++++++++++++- src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++---- src/phyp/phyp_driver.c | 24 +++++++++++++++++++++-- src/test/test_driver.c | 34 ++++++++++++++++++++++++++------ 5 files changed, 135 insertions(+), 19 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/bhyve/bhyve_driver.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 061888ab0b..ffda7853b8 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -577,17 +577,18 @@ bhyveDomainDefineXML(virConnectPtr conn, const char *xml) } static int -bhyveDomainUndefine(virDomainPtr domain) +bhyveDomainUndefineFlags(virDomainPtr domain, unsigned int flags) { bhyveConnPtr privconn = domain->conn->privateData; virObjectEventPtr event = NULL; virDomainObjPtr vm; int ret = -1; + virCheckFlags(0, -1); if (!(vm = bhyveDomObjFromDomain(domain))) goto cleanup; - if (virDomainUndefineEnsureACL(domain->conn, vm->def) < 0) + if (virDomainUndefineFlagsEnsureACL(domain->conn, vm->def) < 0) goto cleanup; if (!vm->persistent) { @@ -618,6 +619,12 @@ bhyveDomainUndefine(virDomainPtr domain) return ret; } +static int +bhyveDomainUndefine(virDomainPtr domain) +{ + return bhyveDomainUndefineFlags(domain, 0); +} + static int bhyveConnectListDomains(virConnectPtr conn, int *ids, int maxids) { @@ -971,17 +978,19 @@ bhyveDomainCreateXML(virConnectPtr conn, } static int -bhyveDomainDestroy(virDomainPtr dom) +bhyveDomainDestroyFlags(virDomainPtr dom, unsigned int flags) { bhyveConnPtr privconn = dom->conn->privateData; virDomainObjPtr vm; virObjectEventPtr event = NULL; int ret = -1; + virCheckFlags(0, -1); + if (!(vm = bhyveDomObjFromDomain(dom))) goto cleanup; - if (virDomainDestroyEnsureACL(dom->conn, vm->def) < 0) + if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -1002,15 +1011,23 @@ bhyveDomainDestroy(virDomainPtr dom) } static int -bhyveDomainShutdown(virDomainPtr dom) +bhyveDomainDestroy(virDomainPtr dom) +{ + return bhyveDomainDestroyFlags(dom, 0); +} + +static int +bhyveDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { virDomainObjPtr vm; int ret = -1; + virCheckFlags(0, -1); + if (!(vm = bhyveDomObjFromDomain(dom))) goto cleanup; - if (virDomainShutdownEnsureACL(dom->conn, vm->def) < 0) + if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -1023,6 +1040,12 @@ bhyveDomainShutdown(virDomainPtr dom) return ret; } +static int +bhyveDomainShutdown(virDomainPtr dom) +{ + return bhyveDomainShutdownFlags(dom, 0); +} + static int bhyveDomainOpenConsole(virDomainPtr dom, const char *dev_name ATTRIBUTE_UNUSED, @@ -1658,13 +1681,16 @@ static virHypervisorDriver bhyveHypervisorDriver = { .domainCreateWithFlags = bhyveDomainCreateWithFlags, /* 1.2.3 */ .domainCreateXML = bhyveDomainCreateXML, /* 1.2.4 */ .domainDestroy = bhyveDomainDestroy, /* 1.2.2 */ + .domainDestroyFlags = bhyveDomainDestroyFlags, /* 5.6.0 */ .domainShutdown = bhyveDomainShutdown, /* 1.3.3 */ + .domainShutdownFlags = bhyveDomainShutdownFlags, /* 5.6.0 */ .domainLookupByUUID = bhyveDomainLookupByUUID, /* 1.2.2 */ .domainLookupByName = bhyveDomainLookupByName, /* 1.2.2 */ .domainLookupByID = bhyveDomainLookupByID, /* 1.2.3 */ .domainDefineXML = bhyveDomainDefineXML, /* 1.2.2 */ .domainDefineXMLFlags = bhyveDomainDefineXMLFlags, /* 1.2.12 */ .domainUndefine = bhyveDomainUndefine, /* 1.2.2 */ + .domainUndefineFlags = bhyveDomainUndefineFlags, /* 5.6.0 */ .domainGetOSType = bhyveDomainGetOSType, /* 1.2.21 */ .domainGetXMLDesc = bhyveDomainGetXMLDesc, /* 1.2.2 */ .domainIsActive = bhyveDomainIsActive, /* 1.2.2 */ -- 2.20.1
On Mon, Jul 08, 2019 at 22:36:58 -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/bhyve/bhyve_driver.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-)
ACK
On 7/8/19 10:36 PM, 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/bhyve/bhyve_driver.c | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 061888ab0b..ffda7853b8 100644
- if (virDomainUndefineEnsureACL(domain->conn, vm->def) < 0) + if (virDomainUndefineFlagsEnsureACL(domain->conn, vm->def) < 0)
Good,
- if (virDomainDestroyEnsureACL(dom->conn, vm->def) < 0) + if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
Good,
- if (virDomainShutdownEnsureACL(dom->conn, vm->def) < 0) + if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0)
Broke the build on BSD. 'make syntax-check' was enough to catch on Linux that I had to rename the functions, but was insufficient to point out which functions require 2 vs. 3 arguments for security checks based on flags. I'm pushing the obvious build-breaker fix: commit 846fe076ca929bf305fe06380d639433cd2c5b2f Author: Eric Blake <eblake@redhat.com> Date: Tue Jul 9 10:36:31 2019 -0500 bhyve: Fix build Continuous integration caught that although 'make syntax-check' was sufficient to let me be aware that I had to change bhyve to use s/virDomainShutdownEnsureACL/virDomainShutdownFlagsEnsureACL/, it was not sufficient to note which ACL functions require 2 vs. 3 arguments for flag validation. Fixes: eded8aad Signed-off-by: Eric Blake <eblake@redhat.com> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index ffda7853b8..4ce9ef0b95 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1027,7 +1027,7 @@ bhyveDomainShutdownFlags(virDomainPtr dom, unsigned int flags) if (!(vm = bhyveDomObjFromDomain(dom))) goto cleanup; - if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def) < 0) + if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) -- 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/esx/esx_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index deb800a6b7..47d95abd6d 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -1984,7 +1984,9 @@ esxDomainSetMaxMemory(virDomainPtr domain, unsigned long memory) static int -esxDomainSetMemory(virDomainPtr domain, unsigned long memory) +esxDomainSetMemoryFlags(virDomainPtr domain, + unsigned long memory, + unsigned int flags) { int result = -1; esxPrivate *priv = domain->conn->privateData; @@ -1994,6 +1996,8 @@ esxDomainSetMemory(virDomainPtr domain, unsigned long memory) esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; + virCheckFlags(0, -1); + if (esxVI_EnsureSession(priv->primary) < 0) return -1; @@ -2037,6 +2041,12 @@ esxDomainSetMemory(virDomainPtr domain, unsigned long memory) } +static int +esxDomainSetMemory(virDomainPtr domain, unsigned long memory) +{ + return esxDomainSetMemoryFlags(domain, memory, 0); +} + /* * libvirt exposed virtual CPU usage in absolute time, ESX doesn't provide this @@ -5122,6 +5132,7 @@ static virHypervisorDriver esxHypervisorDriver = { .domainGetMaxMemory = esxDomainGetMaxMemory, /* 0.7.0 */ .domainSetMaxMemory = esxDomainSetMaxMemory, /* 0.7.0 */ .domainSetMemory = esxDomainSetMemory, /* 0.7.0 */ + .domainSetMemoryFlags = esxDomainSetMemoryFlags, /* 5.6.0 */ .domainSetMemoryParameters = esxDomainSetMemoryParameters, /* 0.8.6 */ .domainGetMemoryParameters = esxDomainGetMemoryParameters, /* 0.8.6 */ .domainGetInfo = esxDomainGetInfo, /* 0.7.0 */ -- 2.20.1
On Mon, Jul 08, 2019 at 22:36:59 -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/esx/esx_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
ACK
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/phyp/phyp_driver.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index e54799dbb4..a4b440a24f 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1657,7 +1657,9 @@ phypGetVIOSFreeSCSIAdapter(virConnectPtr conn) static int -phypDomainAttachDevice(virDomainPtr domain, const char *xml) +phypDomainAttachDeviceFlags(virDomainPtr domain, + const char *xml, + unsigned int flags) { int result = -1; virConnectPtr conn = domain->conn; @@ -1677,6 +1679,8 @@ phypDomainAttachDevice(virDomainPtr domain, const char *xml) virBuffer buf = VIR_BUFFER_INITIALIZER; char *domain_name = NULL; + virCheckFlags(0, -1); + if (!(def = virDomainDefNew())) goto cleanup; @@ -1808,6 +1812,12 @@ phypDomainAttachDevice(virDomainPtr domain, const char *xml) return result; } +static int +phypDomainAttachDevice(virDomainPtr domain, const char *xml) +{ + return phypDomainAttachDeviceFlags(domain, xml, 0); +} + static char * phypStorageVolGetKey(virConnectPtr conn, const char *name) { @@ -3330,7 +3340,7 @@ phypDomainReboot(virDomainPtr dom, unsigned int flags) } static int -phypDomainShutdown(virDomainPtr dom) +phypDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { int result = -1; virConnectPtr conn = dom->conn; @@ -3342,6 +3352,8 @@ phypDomainShutdown(virDomainPtr dom) char *ret = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(0, -1); + virBufferAddLit(&buf, "chsysstate"); if (system_type == HMC) virBufferAsprintf(&buf, " -m %s", managed_system); @@ -3359,6 +3371,12 @@ phypDomainShutdown(virDomainPtr dom) return result; } +static int +phypDomainShutdown(virDomainPtr dom) +{ + return phypDomainShutdownFlags(dom, 0); +} + static int phypDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) { @@ -3675,6 +3693,7 @@ static virHypervisorDriver phypHypervisorDriver = { .domainLookupByName = phypDomainLookupByName, /* 0.7.0 */ .domainResume = phypDomainResume, /* 0.7.0 */ .domainShutdown = phypDomainShutdown, /* 0.7.0 */ + .domainShutdownFlags = phypDomainShutdownFlags, /* 5.6.0 */ .domainReboot = phypDomainReboot, /* 0.9.1 */ .domainDestroy = phypDomainDestroy, /* 0.7.3 */ .domainDestroyFlags = phypDomainDestroyFlags, /* 0.9.4 */ @@ -3688,6 +3707,7 @@ static virHypervisorDriver phypHypervisorDriver = { .connectListDefinedDomains = phypConnectListDefinedDomains, /* 0.7.0 */ .connectNumOfDefinedDomains = phypConnectNumOfDefinedDomains, /* 0.7.0 */ .domainAttachDevice = phypDomainAttachDevice, /* 0.8.2 */ + .domainAttachDeviceFlags = phypDomainAttachDeviceFlags, /* 5.6.0 */ .connectIsEncrypted = phypConnectIsEncrypted, /* 0.7.3 */ .connectIsSecure = phypConnectIsSecure, /* 0.7.3 */ .domainIsUpdated = phypDomainIsUpdated, /* 0.8.6 */ -- 2.20.1
On Mon, Jul 08, 2019 at 22:37:00 -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/phyp/phyp_driver.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
ACK
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. Note that virDomainBlockStats does not trivially forward to virDomainBlockStatsFlags, so that one is omitted. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/test/test_driver.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7dd448bb20..49d7030d21 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2469,12 +2469,15 @@ static int testDomainSetMaxMemory(virDomainPtr domain, return 0; } -static int testDomainSetMemory(virDomainPtr domain, - unsigned long memory) +static int testDomainSetMemoryFlags(virDomainPtr domain, + unsigned long memory, + unsigned int flags) { virDomainObjPtr privdom; int ret = -1; + virCheckFlags(0, -1); + if (!(privdom = testDomObjFromDomain(domain))) return -1; @@ -2491,6 +2494,12 @@ static int testDomainSetMemory(virDomainPtr domain, return ret; } +static int testDomainSetMemory(virDomainPtr domain, + unsigned long memory) +{ + return testDomainSetMemoryFlags(domain, memory, 0); +} + static int testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) { @@ -2679,16 +2688,19 @@ static int testDomainGetVcpus(virDomainPtr domain, return ret; } -static int testDomainPinVcpu(virDomainPtr domain, - unsigned int vcpu, - unsigned char *cpumap, - int maplen) +static int testDomainPinVcpuFlags(virDomainPtr domain, + unsigned int vcpu, + unsigned char *cpumap, + int maplen, + unsigned int flags) { virDomainVcpuDefPtr vcpuinfo; virDomainObjPtr privdom; virDomainDefPtr def; int ret = -1; + virCheckFlags(0, -1); + if (!(privdom = testDomObjFromDomain(domain))) return -1; @@ -2720,6 +2732,14 @@ static int testDomainPinVcpu(virDomainPtr domain, return ret; } +static int testDomainPinVcpu(virDomainPtr domain, + unsigned int vcpu, + unsigned char *cpumap, + int maplen) +{ + return testDomainPinVcpuFlags(domain, vcpu, cpumap, maplen, 0); +} + static int testDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, @@ -7595,6 +7615,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetMaxMemory = testDomainGetMaxMemory, /* 0.1.4 */ .domainSetMaxMemory = testDomainSetMaxMemory, /* 0.1.1 */ .domainSetMemory = testDomainSetMemory, /* 0.1.4 */ + .domainSetMemoryFlags = testDomainSetMemoryFlags, /* 5.6.0 */ .domainGetHostname = testDomainGetHostname, /* 5.5.0 */ .domainGetInfo = testDomainGetInfo, /* 0.1.1 */ .domainGetState = testDomainGetState, /* 0.9.2 */ @@ -7611,6 +7632,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainSetVcpusFlags = testDomainSetVcpusFlags, /* 0.8.5 */ .domainGetVcpusFlags = testDomainGetVcpusFlags, /* 0.8.5 */ .domainPinVcpu = testDomainPinVcpu, /* 0.7.3 */ + .domainPinVcpuFlags = testDomainPinVcpuFlags, /* 5.6.0 */ .domainGetVcpus = testDomainGetVcpus, /* 0.7.3 */ .domainGetVcpuPinInfo = testDomainGetVcpuPinInfo, /* 1.2.18 */ .domainGetMaxVcpus = testDomainGetMaxVcpus, /* 0.7.3 */ -- 2.20.1
On Mon, Jul 08, 2019 at 22:37:01 -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.
Note that virDomainBlockStats does not trivially forward to virDomainBlockStatsFlags, so that one is omitted.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/test/test_driver.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
This may conflict with Ilias' test driver work. ACK
On 7/8/19 10:37 PM, 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.
Note that virDomainBlockStats does not trivially forward to virDomainBlockStatsFlags, so that one is omitted.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/test/test_driver.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7dd448bb20..49d7030d21 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2469,12 +2469,15 @@ static int testDomainSetMaxMemory(virDomainPtr domain, return 0; }
-static int testDomainSetMemory(virDomainPtr domain, - unsigned long memory) +static int testDomainSetMemoryFlags(virDomainPtr domain, + unsigned long memory, + unsigned int flags) { virDomainObjPtr privdom; int ret = -1;
+ virCheckFlags(0, -1); +
As discussed in v2, this should probably accept VIR_DOMAIN_AFFECT_LIVE, and maybe even VIR_DOMAIN_AFFECT_MAXIMUM_MEMORY...
+static int testDomainSetMemory(virDomainPtr domain, + unsigned long memory) +{ + return testDomainSetMemoryFlags(domain, memory, 0);
where this should pass VIR_DOMAIN_AFFECT_LIVE instead of 0, and we may want to implement testDomainSetMaxMemory(),...
+static int testDomainPinVcpu(virDomainPtr domain, + unsigned int vcpu, + unsigned char *cpumap, + int maplen) +{ + return testDomainPinVcpuFlags(domain, vcpu, cpumap, maplen, 0);
and another interface that should probably pass VIR_DOMAIN_AFFECT_LIVE. Looks like I'll be doing a v3. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 failing the driver at initialization time if a modern API is forgotten when an older API is present. For now, the list includes any interface with a Flags counterpart, except virDomainBlockStatsFlags which is a bit more complex than virDomainBlockStats. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 7e665b6cba..a12a72a31b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -567,19 +567,53 @@ int virRegisterConnectDriver(virConnectDriverPtr driver, bool setSharedDrivers) { - VIR_DEBUG("driver=%p name=%s", driver, - driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)"); + const char *driver_name; + + driver_name = driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)"; + VIR_DEBUG("driver=%p name=%s", driver, driver_name); virCheckNonNullArgReturn(driver, -1); if (virConnectDriverTabCount >= MAX_DRIVERS) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Too many drivers, cannot register %s"), - driver->hypervisorDriver->name); + driver_name); return -1; } + /* Check for drivers failing to provide a modern counterpart to an + * older API */ +#define REQUIRE_API(old, new) \ + do { \ + if (driver->hypervisorDriver->old && \ + !driver->hypervisorDriver->new) { \ + fprintf(stderr, " ***FIXME!: driver %s is broken on %s\n", \ + driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)", #new); \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("driver %s is missing %s interface"), \ + driver_name, #new); \ + return -1; \ + } \ + } while (0) + REQUIRE_API(domainShutdown, domainShutdownFlags); + REQUIRE_API(domainDestroy, domainDestroyFlags); + REQUIRE_API(domainSetMemory, domainSetMemoryFlags); + REQUIRE_API(domainSave, domainSaveFlags); + REQUIRE_API(domainRestore, domainRestoreFlags); + REQUIRE_API(domainSetVcpus, domainSetVcpusFlags); + REQUIRE_API(domainGetVcpus, domainGetVcpusFlags); + REQUIRE_API(domainPinVcpu, domainPinVcpuFlags); + REQUIRE_API(domainCreate, domainCreateWithFlags); + REQUIRE_API(domainDefineXML, domainDefineXMLFlags); + REQUIRE_API(domainUndefine, domainUndefineFlags); + REQUIRE_API(domainAttachDevice, domainAttachDeviceFlags); + REQUIRE_API(domainDetachDevice, domainDetachDeviceFlags); + REQUIRE_API(domainGetSchedulerParameters, domainGetSchedulerParametersFlags); + REQUIRE_API(domainSetSchedulerParameters, domainSetSchedulerParametersFlags); + REQUIRE_API(nodeDeviceDettach, nodeDeviceDetachFlags); +#undef REQUIRE_API + VIR_DEBUG("registering %s as driver %d", - driver->hypervisorDriver->name, virConnectDriverTabCount); + driver_name, virConnectDriverTabCount); if (setSharedDrivers) { if (driver->interfaceDriver == NULL) -- 2.20.1
On Mon, Jul 08, 2019 at 22:37:02 -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 failing the driver at initialization time if a modern API is forgotten when an older API is present. For now, the list includes any interface with a Flags counterpart, except virDomainBlockStatsFlags which is a bit more complex than virDomainBlockStats.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 7e665b6cba..a12a72a31b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -567,19 +567,53 @@ int virRegisterConnectDriver(virConnectDriverPtr driver, bool setSharedDrivers) { - VIR_DEBUG("driver=%p name=%s", driver, - driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)"); + const char *driver_name; + + driver_name = driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)"; + VIR_DEBUG("driver=%p name=%s", driver, driver_name);
virCheckNonNullArgReturn(driver, -1); if (virConnectDriverTabCount >= MAX_DRIVERS) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Too many drivers, cannot register %s"), - driver->hypervisorDriver->name); + driver_name); return -1; }
+ /* Check for drivers failing to provide a modern counterpart to an + * older API */ +#define REQUIRE_API(old, new) \ + do { \ + if (driver->hypervisorDriver->old && \ + !driver->hypervisorDriver->new) { \ + fprintf(stderr, " ***FIXME!: driver %s is broken on %s\n", \ + driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)", #new); \
fprintf in a library function is really wrong. Also I don't think this really requires a runtime check as the APIs aren't really going to just disappear.
On Tue, Jul 09, 2019 at 11:02:03AM +0200, Peter Krempa wrote:
On Mon, Jul 08, 2019 at 22:37:02 -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 failing the driver at initialization time if a modern API is forgotten when an older API is present. For now, the list includes any interface with a Flags counterpart, except virDomainBlockStatsFlags which is a bit more complex than virDomainBlockStats.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index 7e665b6cba..a12a72a31b 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -567,19 +567,53 @@ int virRegisterConnectDriver(virConnectDriverPtr driver, bool setSharedDrivers) { - VIR_DEBUG("driver=%p name=%s", driver, - driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)"); + const char *driver_name; + + driver_name = driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)"; + VIR_DEBUG("driver=%p name=%s", driver, driver_name);
virCheckNonNullArgReturn(driver, -1); if (virConnectDriverTabCount >= MAX_DRIVERS) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Too many drivers, cannot register %s"), - driver->hypervisorDriver->name); + driver_name); return -1; }
+ /* Check for drivers failing to provide a modern counterpart to an + * older API */ +#define REQUIRE_API(old, new) \ + do { \ + if (driver->hypervisorDriver->old && \ + !driver->hypervisorDriver->new) { \ + fprintf(stderr, " ***FIXME!: driver %s is broken on %s\n", \ + driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)", #new); \
fprintf in a library function is really wrong.
Also I don't think this really requires a runtime check as the APIs aren't really going to just disappear.
Yeah, I think we can easily do this validation via a make check rule using static analysis. We could hack either check-driverimpls.pl or check-drivername.pl to mandate that both the old + new method are always provided as a pair, or just create a new dedicated check script for that. Failing during "make check" is preferable to printf at runtime. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 7/9/19 4:36 AM, Daniel P. Berrangé wrote:
On Tue, Jul 09, 2019 at 11:02:03AM +0200, Peter Krempa wrote:
On Mon, Jul 08, 2019 at 22:37:02 -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 failing the driver at initialization time if a modern API is forgotten when an older API is present. For now, the list includes any interface with a Flags counterpart, except virDomainBlockStatsFlags which is a bit more complex than virDomainBlockStats.
+#define REQUIRE_API(old, new) \ + do { \ + if (driver->hypervisorDriver->old && \ + !driver->hypervisorDriver->new) { \ + fprintf(stderr, " ***FIXME!: driver %s is broken on %s\n", \ + driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)", #new); \
fprintf in a library function is really wrong.
Whoops, leftovers while writing the other patches, that I meant to remove. That said,
Also I don't think this really requires a runtime check as the APIs aren't really going to just disappear.
Yeah, I think we can easily do this validation via a make check rule using static analysis.
We could hack either check-driverimpls.pl or check-drivername.pl to mandate that both the old + new method are always provided as a pair, or just create a new dedicated check script for that.
this is indeed a much nicer idea. I'll do v2 along those lines. Other ideas: we have few enough hypervisor drivers that the work of duplicating an API in each driver is still doable - but if we really wanted to worry about scaling, a better approach might be to rewrite src/libvirt*.c to do automatic fallback any time we have paired APIs. After all, each API already has central code that checks if the hypervisor has a callback and supplies a sane error if not, as well as doing global argument sanity checking; it would be easy enough to expand the central code for each paired API to do automatic fallback to the other half of the pair before completely failing. This can be done in both directions: if a driver provides only the new and not the old interface, we can synthesize the old Foo(args) by calling the new FooFlags(args, 0). Or if only the new interface is lacking, the new one can do virCheckFlags(0, RET) and then call into the old. It would let us remove quite a bit of code from the various drivers, AND it has the benefit that we don't need a syntax check, but it may make it harder for the web page listing which interfaces were supported in which release for a given driver. Is that worth pursuing instead? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
We can ignore flags rather than rejecting them as unknown since a correct implementation of those flags is a no-op given that bhyve lacks managed save or snapshots. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/bhyve/bhyve_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index ffda7853b8..c2378c536f 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -584,7 +584,10 @@ bhyveDomainUndefineFlags(virDomainPtr domain, unsigned int flags) virDomainObjPtr vm; int ret = -1; - virCheckFlags(0, -1); + /* We have no managed save or snapshots, so we can ignore those flags */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, NULL); + if (!(vm = bhyveDomObjFromDomain(domain))) goto cleanup; -- 2.20.1
On 7/8/19 10:37 PM, Eric Blake wrote:
We can ignore flags rather than rejecting them as unknown since a correct implementation of those flags is a no-op given that bhyve lacks managed save or snapshots.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/bhyve/bhyve_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index ffda7853b8..c2378c536f 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -584,7 +584,10 @@ bhyveDomainUndefineFlags(virDomainPtr domain, unsigned int flags) virDomainObjPtr vm; int ret = -1;
- virCheckFlags(0, -1); + /* We have no managed save or snapshots, so we can ignore those flags */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, NULL);
Returning NULL is wrong; this should be -1. What's more, this disagrees with esx, which does: /* No managed save, so we explicitly reject * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for * ESX, so we can trivially ignore that flag. */ virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); so I'll use that construct in v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Peter Krempa