10:36 p.m.
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
10:36 p.m.
New subject: [libvirt] [PATCH 1/6] bhyve: Add various vir*Flags API
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(a)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
3:57 a.m.
New subject: [libvirt] [PATCH 1/6] bhyve: Add various vir*Flags API
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(a)redhat.com>
---
src/bhyve/bhyve_driver.c | 38 ++++++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)
ACK
10:43 a.m.
New subject: [libvirt] [PATCH 1/6] bhyve: Add various vir*Flags API
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(a)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(a)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(a)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
10:36 p.m.
New subject: [libvirt] [PATCH 2/6] esx: Add various vir*Flags API
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(a)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
3:57 a.m.
New subject: [libvirt] [PATCH 2/6] esx: Add various vir*Flags API
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(a)redhat.com>
---
src/esx/esx_driver.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
ACK
10:37 p.m.
New subject: [libvirt] [PATCH 3/6] phyp: Add various vir*Flags API
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(a)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
3:58 a.m.
New subject: [libvirt] [PATCH 3/6] phyp: Add various vir*Flags API
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(a)redhat.com>
---
src/phyp/phyp_driver.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
ACK
10:37 p.m.
New subject: [libvirt] [PATCH 4/6] test: Add various vir*Flags API
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(a)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
4 a.m.
New subject: [libvirt] [PATCH 4/6] test: Add various vir*Flags API
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(a)redhat.com>
---
src/test/test_driver.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
This may conflict with Ilias' test driver work.
ACK
12:03 p.m.
New subject: [libvirt] [PATCH 4/6] test: Add various vir*Flags API
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(a)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
10:37 p.m.
New subject: [libvirt] [PATCH 5/6] libvirt: Ensure modern APIs are implemented
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(a)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
4:02 a.m.
New subject: [libvirt] [PATCH 5/6] libvirt: Ensure modern APIs are implemented
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(a)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.
4:36 a.m.
New subject: [libvirt] [PATCH 5/6] libvirt: Ensure modern APIs are implemented
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(a)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 :|
8:29 a.m.
New subject: [libvirt] [PATCH 5/6] libvirt: Ensure modern APIs are implemented
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
10:37 p.m.
New subject: [libvirt] [PATCH 6/6] bhyve: Ignore no-op flags during virDomainUndefine
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(a)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
12:30 p.m.
New subject: [libvirt] [PATCH 6/6] bhyve: Ignore no-op flags during virDomainUndefine
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(a)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
1963
days inactive
1964
days old
16 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Peter Krempa