2:09 p.m.
Extending domaincapabilities with new XML schema is currently a bit of
a maintenance pain. Consider the case of adding a new enum for listing
<sound> models. I want to output this info for the qemu driver.
Internally in the domaincapabilities plumbing, whether a <device> is
supported= is tracked with boolean true/false. If I extend that
pattern for <sound> devices and fill in data for the qemu driver, the
other domcaps implementations will now automatically output a new XML
element:
<sound supported='no'>
Now, for bhyve I can 'git grep' confirm that it doesn't have any
<sound> support, but for xen/libxl it _is_ supported. So if I don't
fill in accurate support in the xen driver, I've just made their
domcaps report blatantly incorrect info.
Ideally I would make these <sound> changes and the other drivers output
would _not_ change. xen output would now be incomplete, but not
obviously wrong, which is easier on me the developer, and safer for the
API consumer.
This moves domcaps plumbing in that direction. It switches most
internal 'supported' fields to virTristateBool so we can track an
ABSENT state and map that to outputting no XML. Explicit supported='no'
values are filled in where needed to ensure existing driver XML doesn't
change. cpu and sev supported= values are left unconverted, but they
require semi-special handling anyways so aren't really affected by the
problem I laid out above.
Cole Robinson (10):
tests: domcaps: Add a default 'empty' test
tests: domcaps: Remove unused typedef
tests: domcaps: Remove 'full' test
conf: domcaps: Add single line formatting macro
conf: domcaps: use virTristateBool for 'supported'
qemu: domcaps: fill in explicit supported BOOL_NO
libxl: domcaps: fill in explicit supported BOOL_NO
bhyve: domcaps: fill in explicit supported BOOL_NO
schemas: domcaps: Make more elements optional
conf: domcaps: Don't output XML on tristate ABSENT
docs/schemas/domaincaps.rng | 20 +++++--
src/bhyve/bhyve_capabilities.c | 20 +++++--
src/conf/domain_capabilities.c | 26 ++++++----
src/conf/domain_capabilities.h | 20 +++----
src/libxl/libxl_capabilities.c | 18 ++++---
src/qemu/qemu_capabilities.c | 26 ++++++----
tests/domaincapsschemadata/empty.xml | 16 ++++++
tests/domaincapstest.c | 78 ++--------------------------
8 files changed, 103 insertions(+), 121 deletions(-)
create mode 100644 tests/domaincapsschemadata/empty.xml
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 01/10] tests: domcaps: Add a default 'empty' test
The 'empty' demonstrates XML generated when only bare minimum caps
data has been filled in. This will demonstrate changes that alter
the default XML output.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
tests/domaincapsschemadata/empty.xml | 25 +++++++++++++++++++++++++
tests/domaincapstest.c | 3 +++
2 files changed, 28 insertions(+)
create mode 100644 tests/domaincapsschemadata/empty.xml
diff --git a/tests/domaincapsschemadata/empty.xml b/tests/domaincapsschemadata/empty.xml
new file mode 100644
index 0000000000..2b2e97d3b3
--- /dev/null
+++ b/tests/domaincapsschemadata/empty.xml
@@ -0,0 +1,25 @@
+<domainCapabilities>
+ <path>/bin/emulatorbin</path>
+ <domain>kvm</domain>
+ <machine>my-machine-type</machine>
+ <arch>x86_64</arch>
+ <iothreads supported='no'/>
+ <os supported='no'/>
+ <cpu>
+ <mode name='host-passthrough' supported='no'/>
+ <mode name='host-model' supported='no'/>
+ <mode name='custom' supported='no'/>
+ </cpu>
+ <devices>
+ <disk supported='no'/>
+ <graphics supported='no'/>
+ <video supported='no'/>
+ <hostdev supported='no'/>
+ </devices>
+ <features>
+ <gic supported='no'/>
+ <vmcoreinfo supported='no'/>
+ <genid supported='no'/>
+ <sev supported='no'/>
+ </features>
+</domainCapabilities>
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index b9ab148fab..2333147252 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -432,6 +432,9 @@ mymain(void)
VIR_FREE(name); \
} while (0)
+ DO_TEST("empty", "/bin/emulatorbin",
"my-machine-type",
+ "x86_64", VIR_DOMAIN_VIRT_KVM, CAPS_NONE);
+
#if WITH_QEMU
DO_TEST_QEMU("1.7.0", "caps_1.7.0",
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 02/10] tests: domcaps: Remove unused typedef
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
tests/domaincapstest.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 2333147252..07ed8b750e 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -24,9 +24,6 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-typedef int (*virDomainCapsFill)(virDomainCapsPtr domCaps,
- void *opaque);
-
#define SET_ALL_BITS(x) \
memset(&(x.values), 0xff, sizeof(x.values))
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 03/10] tests: domcaps: Remove 'full' test
The 'full' test verifies the output of a virDomainCapsPtr built
by hand. It has the following problems:
The domcaps test suite nowadays has 3 hypervisor driver implementations
which should give us plenty of opportunity to get full domcaps coverage.
I don't think this test has much value. And it has the following issues:
- Requires manual intervention to test new domcaps XML, which is easy
to miss, for example gic bits aren't covered there.
- The SET_ALL_BITS trick it uses to fill in enums will output
values that are never reported by any driver implementation
(strings like 'default')
Let's remove it
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
tests/domaincapstest.c | 72 ------------------------------------------
1 file changed, 72 deletions(-)
diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
index 07ed8b750e..3afa84b42e 100644
--- a/tests/domaincapstest.c
+++ b/tests/domaincapstest.c
@@ -24,9 +24,6 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-#define SET_ALL_BITS(x) \
- memset(&(x.values), 0xff, sizeof(x.values))
-
static int ATTRIBUTE_SENTINEL
fillStringValues(virDomainCapsStringValuesPtr values, ...)
{
@@ -48,66 +45,6 @@ fillStringValues(virDomainCapsStringValuesPtr values, ...)
return ret;
}
-static int
-fillAllCaps(virDomainCapsPtr domCaps)
-{
- virDomainCapsOSPtr os = &domCaps->os;
- virDomainCapsLoaderPtr loader = &os->loader;
- virDomainCapsCPUPtr cpu = &domCaps->cpu;
- virDomainCapsDeviceDiskPtr disk = &domCaps->disk;
- virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics;
- virDomainCapsDeviceVideoPtr video = &domCaps->video;
- virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev;
- virCPUDef host = {
- .type = VIR_CPU_TYPE_HOST,
- .arch = VIR_ARCH_X86_64,
- .model = (char *) "host",
- .vendor = (char *) "CPU Vendorrr",
- };
-
- domCaps->maxvcpus = 255;
- os->supported = true;
-
- loader->supported = true;
- SET_ALL_BITS(loader->type);
- SET_ALL_BITS(loader->readonly);
- if (fillStringValues(&loader->values,
- "/foo/bar",
- "/tmp/my_path",
- NULL) < 0)
- return -1;
-
- cpu->hostPassthrough = true;
- cpu->hostModel = virCPUDefCopy(&host);
- if (!(cpu->custom = virDomainCapsCPUModelsNew(3)) ||
- virDomainCapsCPUModelsAdd(cpu->custom, "Model1", -1,
- VIR_DOMCAPS_CPU_USABLE_UNKNOWN, NULL) < 0 ||
- virDomainCapsCPUModelsAdd(cpu->custom, "Model2", -1,
- VIR_DOMCAPS_CPU_USABLE_NO, NULL) < 0 ||
- virDomainCapsCPUModelsAdd(cpu->custom, "Model3", -1,
- VIR_DOMCAPS_CPU_USABLE_YES, NULL) < 0)
- return -1;
-
- disk->supported = true;
- SET_ALL_BITS(disk->diskDevice);
- SET_ALL_BITS(disk->bus);
-
- graphics->supported = true;
- SET_ALL_BITS(graphics->type);
-
- video->supported = true;
- SET_ALL_BITS(video->modelType);
-
- hostdev->supported = true;
- SET_ALL_BITS(hostdev->mode);
- SET_ALL_BITS(hostdev->startupPolicy);
- SET_ALL_BITS(hostdev->subsysType);
- SET_ALL_BITS(hostdev->capsType);
- SET_ALL_BITS(hostdev->pciBackend);
- return 0;
-}
-
-
#if WITH_QEMU
# include "testutilsqemu.h"
# include "testutilshostcpus.h"
@@ -257,7 +194,6 @@ fillBhyveCaps(virDomainCapsPtr domCaps, unsigned int *bhyve_caps)
enum testCapsType {
CAPS_NONE,
- CAPS_ALL,
CAPS_QEMU,
CAPS_LIBXL,
CAPS_BHYVE,
@@ -296,11 +232,6 @@ test_virDomainCapsFormat(const void *opaque)
case CAPS_NONE:
break;
- case CAPS_ALL:
- if (fillAllCaps(domCaps) < 0)
- goto cleanup;
- break;
-
case CAPS_QEMU:
#if WITH_QEMU
if (fillQemuCaps(domCaps, data->capsName, data->arch, data->machine,
@@ -406,9 +337,6 @@ mymain(void)
ret = -1; \
} while (0)
- DO_TEST("full", "/bin/emulatorbin", "my-machine-type",
- "x86_64", VIR_DOMAIN_VIRT_KVM, CAPS_ALL);
-
#define DO_TEST_BHYVE(Name, Emulator, BhyveCaps, Type) \
do { \
char *name = NULL; \
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 04/10] conf: domcaps: Add single line formatting macro
Similar to the macros we have for formatting enums, add a macro to
simplify formatting the pattern:
<FOO supported='yes|no'/>
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/conf/domain_capabilities.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 6352eda343..4539281ff5 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -384,6 +384,12 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
virBufferAddLit(buf, "</" #item ">\n"); \
} while (0)
+#define FORMAT_SINGLE(name, supported) \
+ do { \
+ virBufferAsprintf(&buf, "<%s supported='%s'/>\n",
name, \
+ supported ? "yes" : "no"); \
+ } while (0)
+
#define ENUM_PROCESS(master, capsEnum, valToStr) \
do { \
virDomainCapsEnumFormat(buf, &master->capsEnum, \
@@ -593,8 +599,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps)
if (caps->maxvcpus)
virBufferAsprintf(&buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
- virBufferAsprintf(&buf, "<iothreads supported='%s'/>\n",
- caps->iothreads ? "yes" : "no");
+ FORMAT_SINGLE("iothreads", caps->iothreads);
virDomainCapsOSFormat(&buf, &caps->os);
virDomainCapsCPUFormat(&buf, &caps->cpu);
@@ -614,11 +619,8 @@ virDomainCapsFormat(virDomainCapsPtr const caps)
virBufferAdjustIndent(&buf, 2);
virDomainCapsFeatureGICFormat(&buf, &caps->gic);
- virBufferAsprintf(&buf, "<vmcoreinfo
supported='%s'/>\n",
- caps->vmcoreinfo ? "yes" : "no");
-
- virBufferAsprintf(&buf, "<genid supported='%s'/>\n",
- caps->genid ? "yes" : "no");
+ FORMAT_SINGLE("vmcoreinfo", caps->vmcoreinfo);
+ FORMAT_SINGLE("genid", caps->genid);
virDomainCapsFeatureSEVFormat(&buf, caps->sev);
virBufferAdjustIndent(&buf, -2);
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 05/10] conf: domcaps: use virTristateBool for 'supported'
Switch most 'supported' handling to use virTristateBool, so eventually
we can handle the ABSENT state.
For now the XML formatter treats ABSENT the same as FALSE, so there's
no functional output change. This will be addressed in later patches
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/bhyve/bhyve_capabilities.c | 10 +++++-----
src/conf/domain_capabilities.c | 8 ++++----
src/conf/domain_capabilities.h | 20 ++++++++++----------
src/libxl/libxl_capabilities.c | 12 ++++++------
src/qemu/qemu_capabilities.c | 24 +++++++++++++-----------
5 files changed, 38 insertions(+), 36 deletions(-)
diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 6feaded2ad..79d7659da3 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -75,7 +75,7 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps,
unsigned int bhyvecaps,
virDomainCapsStringValuesPtr firmwares)
{
- caps->disk.supported = true;
+ caps->disk.supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(caps->disk.diskDevice,
VIR_DOMAIN_DISK_DEVICE_DISK,
VIR_DOMAIN_DISK_DEVICE_CDROM);
@@ -84,10 +84,10 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps,
VIR_DOMAIN_DISK_BUS_SATA,
VIR_DOMAIN_DISK_BUS_VIRTIO);
- caps->os.supported = true;
+ caps->os.supported = VIR_TRISTATE_BOOL_YES;
if (bhyvecaps & BHYVE_CAP_LPC_BOOTROM) {
- caps->os.loader.supported = true;
+ caps->os.loader.supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(caps->os.loader.type,
VIR_DOMAIN_LOADER_TYPE_PFLASH);
VIR_DOMAIN_CAPS_ENUM_SET(caps->os.loader.readonly,
@@ -99,8 +99,8 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps,
if (bhyvecaps & BHYVE_CAP_FBUF) {
- caps->graphics.supported = true;
- caps->video.supported = true;
+ caps->graphics.supported = VIR_TRISTATE_BOOL_YES;
+ caps->video.supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(caps->graphics.type, VIR_DOMAIN_GRAPHICS_TYPE_VNC);
VIR_DOMAIN_CAPS_ENUM_SET(caps->video.modelType, VIR_DOMAIN_VIDEO_TYPE_GOP);
}
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 4539281ff5..6ae18515b6 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -371,9 +371,9 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
#define FORMAT_PROLOGUE(item) \
do { \
virBufferAsprintf(buf, "<" #item "
supported='%s'%s\n", \
- item->supported ? "yes" : "no", \
- item->supported ? ">" : "/>"); \
- if (!item->supported) \
+ (item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" :
"no", \
+ (item->supported == VIR_TRISTATE_BOOL_YES) ? ">" :
"/>"); \
+ if (item->supported != VIR_TRISTATE_BOOL_YES) \
return; \
virBufferAdjustIndent(buf, 2); \
} while (0)
@@ -387,7 +387,7 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
#define FORMAT_SINGLE(name, supported) \
do { \
virBufferAsprintf(&buf, "<%s supported='%s'/>\n",
name, \
- supported ? "yes" : "no"); \
+ (supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no");
\
} while (0)
#define ENUM_PROCESS(master, capsEnum, valToStr) \
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 15e065359b..1ebdb01716 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -46,7 +46,7 @@ struct _virDomainCapsStringValues {
typedef struct _virDomainCapsLoader virDomainCapsLoader;
typedef virDomainCapsLoader *virDomainCapsLoaderPtr;
struct _virDomainCapsLoader {
- bool supported;
+ virTristateBool supported;
virDomainCapsStringValues values; /* Info about values for the element */
virDomainCapsEnum type; /* Info about virDomainLoader */
virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */
@@ -55,14 +55,14 @@ struct _virDomainCapsLoader {
typedef struct _virDomainCapsOS virDomainCapsOS;
typedef virDomainCapsOS *virDomainCapsOSPtr;
struct _virDomainCapsOS {
- bool supported;
+ virTristateBool supported;
virDomainCapsLoader loader; /* Info about virDomainLoaderDef */
};
typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk;
typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr;
struct _virDomainCapsDeviceDisk {
- bool supported;
+ virTristateBool supported;
virDomainCapsEnum diskDevice; /* Info about virDomainDiskDevice enum values */
virDomainCapsEnum bus; /* Info about virDomainDiskBus enum values */
/* add new fields here */
@@ -71,21 +71,21 @@ struct _virDomainCapsDeviceDisk {
typedef struct _virDomainCapsDeviceGraphics virDomainCapsDeviceGraphics;
typedef virDomainCapsDeviceGraphics *virDomainCapsDeviceGraphicsPtr;
struct _virDomainCapsDeviceGraphics {
- bool supported;
+ virTristateBool supported;
virDomainCapsEnum type; /* virDomainGraphicsType */
};
typedef struct _virDomainCapsDeviceVideo virDomainCapsDeviceVideo;
typedef virDomainCapsDeviceVideo *virDomainCapsDeviceVideoPtr;
struct _virDomainCapsDeviceVideo {
- bool supported;
+ virTristateBool supported;
virDomainCapsEnum modelType; /* virDomainVideoType */
};
typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr;
struct _virDomainCapsDeviceHostdev {
- bool supported;
+ virTristateBool supported;
virDomainCapsEnum mode; /* Info about virDomainHostdevMode */
virDomainCapsEnum startupPolicy; /* Info about virDomainStartupPolicy */
virDomainCapsEnum subsysType; /* Info about virDomainHostdevSubsysType */
@@ -97,7 +97,7 @@ struct _virDomainCapsDeviceHostdev {
typedef struct _virDomainCapsFeatureGIC virDomainCapsFeatureGIC;
typedef virDomainCapsFeatureGIC *virDomainCapsFeatureGICPtr;
struct _virDomainCapsFeatureGIC {
- bool supported;
+ virTristateBool supported;
virDomainCapsEnum version; /* Info about virGICVersion */
};
@@ -155,7 +155,7 @@ struct _virDomainCaps {
/* Some machine specific info */
int maxvcpus;
- bool iothreads; /* Whether I/O threads are supported or not. */
+ virTristateBool iothreads; /* Whether I/O threads are supported or not. */
virDomainCapsOS os;
virDomainCapsCPU cpu;
@@ -166,8 +166,8 @@ struct _virDomainCaps {
/* add new domain devices here */
virDomainCapsFeatureGIC gic;
- bool vmcoreinfo;
- bool genid;
+ virTristateBool vmcoreinfo;
+ virTristateBool genid;
virSEVCapabilityPtr sev;
/* add new domain features here */
};
diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
index cc42dc6987..385b08be38 100644
--- a/src/libxl/libxl_capabilities.c
+++ b/src/libxl/libxl_capabilities.c
@@ -603,12 +603,12 @@ libxlMakeDomainOSCaps(const char *machine,
virDomainCapsLoaderPtr capsLoader = &os->loader;
size_t i;
- os->supported = true;
+ os->supported = VIR_TRISTATE_BOOL_YES;
if (STREQ(machine, "xenpv") || STREQ(machine, "xenpvh"))
return 0;
- capsLoader->supported = true;
+ capsLoader->supported = VIR_TRISTATE_BOOL_YES;
if (VIR_ALLOC_N(capsLoader->values.values, nfirmwares) < 0)
return -1;
@@ -631,7 +631,7 @@ libxlMakeDomainOSCaps(const char *machine,
static int
libxlMakeDomainDeviceDiskCaps(virDomainCapsDeviceDiskPtr dev)
{
- dev->supported = true;
+ dev->supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(dev->diskDevice,
VIR_DOMAIN_DISK_DEVICE_DISK,
@@ -648,7 +648,7 @@ libxlMakeDomainDeviceDiskCaps(virDomainCapsDeviceDiskPtr dev)
static int
libxlMakeDomainDeviceGraphicsCaps(virDomainCapsDeviceGraphicsPtr dev)
{
- dev->supported = true;
+ dev->supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(dev->type,
VIR_DOMAIN_GRAPHICS_TYPE_SDL,
@@ -661,7 +661,7 @@ libxlMakeDomainDeviceGraphicsCaps(virDomainCapsDeviceGraphicsPtr dev)
static int
libxlMakeDomainDeviceVideoCaps(virDomainCapsDeviceVideoPtr dev)
{
- dev->supported = true;
+ dev->supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType,
VIR_DOMAIN_VIDEO_TYPE_VGA,
@@ -683,7 +683,7 @@ bool libxlCapsHasPVUSB(void)
static int
libxlMakeDomainDeviceHostdevCaps(virDomainCapsDeviceHostdevPtr dev)
{
- dev->supported = true;
+ dev->supported = VIR_TRISTATE_BOOL_YES;
/* VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES is for containers only */
VIR_DOMAIN_CAPS_ENUM_SET(dev->mode,
VIR_DOMAIN_HOSTDEV_MODE_SUBSYS);
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9d9c8096ba..ea75177f39 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5119,7 +5119,7 @@ virQEMUCapsFillDomainLoaderCaps(virDomainCapsLoaderPtr capsLoader,
{
size_t i;
- capsLoader->supported = true;
+ capsLoader->supported = VIR_TRISTATE_BOOL_YES;
if (VIR_ALLOC_N(capsLoader->values.values, nfirmwares) < 0)
return -1;
@@ -5159,7 +5159,7 @@ virQEMUCapsFillDomainOSCaps(virDomainCapsOSPtr os,
{
virDomainCapsLoaderPtr capsLoader = &os->loader;
- os->supported = true;
+ os->supported = VIR_TRISTATE_BOOL_YES;
if (virQEMUCapsFillDomainLoaderCaps(capsLoader, firmwares, nfirmwares) < 0)
return -1;
return 0;
@@ -5212,7 +5212,8 @@ static int
virQEMUCapsFillDomainIOThreadCaps(virQEMUCapsPtr qemuCaps,
virDomainCapsPtr domCaps)
{
- domCaps->iothreads = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD);
+ domCaps->iothreads = virTristateBoolFromBool(
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD));
return 0;
}
@@ -5223,7 +5224,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps,
const char *machine,
virDomainCapsDeviceDiskPtr disk)
{
- disk->supported = true;
+ disk->supported = VIR_TRISTATE_BOOL_YES;
/* QEMU supports all of these */
VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice,
VIR_DOMAIN_DISK_DEVICE_DISK,
@@ -5258,7 +5259,7 @@ static int
virQEMUCapsFillDomainDeviceGraphicsCaps(virQEMUCapsPtr qemuCaps,
virDomainCapsDeviceGraphicsPtr dev)
{
- dev->supported = true;
+ dev->supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(dev->type, VIR_DOMAIN_GRAPHICS_TYPE_SDL);
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC))
@@ -5274,7 +5275,7 @@ static int
virQEMUCapsFillDomainDeviceVideoCaps(virQEMUCapsPtr qemuCaps,
virDomainCapsDeviceVideoPtr dev)
{
- dev->supported = true;
+ dev->supported = VIR_TRISTATE_BOOL_YES;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_VGA);
@@ -5298,7 +5299,7 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps,
bool supportsPassthroughKVM = qemuHostdevHostSupportsPassthroughLegacy();
bool supportsPassthroughVFIO = qemuHostdevHostSupportsPassthroughVFIO();
- hostdev->supported = true;
+ hostdev->supported = VIR_TRISTATE_BOOL_YES;
/* VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES is for containers only */
VIR_DOMAIN_CAPS_ENUM_SET(hostdev->mode,
VIR_DOMAIN_HOSTDEV_MODE_SUBSYS);
@@ -5413,7 +5414,7 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps,
version))
continue;
- gic->supported = true;
+ gic->supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(gic->version,
version);
}
@@ -5484,10 +5485,11 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps,
domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus);
}
- domCaps->vmcoreinfo = virQEMUCapsGet(qemuCaps,
- QEMU_CAPS_DEVICE_VMCOREINFO);
+ domCaps->vmcoreinfo = virTristateBoolFromBool(
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMCOREINFO));
- domCaps->genid = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID);
+ domCaps->genid = virTristateBoolFromBool(
+ virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID));
if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 ||
virQEMUCapsFillDomainCPUCaps(caps, qemuCaps, domCaps) < 0 ||
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 06/10] qemu: domcaps: fill in explicit supported BOOL_NO
Only gic->supported needs an explicit BOOL_NO setting, all other
'supported' values are handling things correctly
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ea75177f39..bcf3bec03e 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5403,6 +5403,8 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps,
virDomainCapsFeatureGICPtr gic = &domCaps->gic;
virGICVersion version;
+ gic->supported = VIR_TRISTATE_BOOL_NO;
+
if (!qemuDomainMachineIsARMVirt(domCaps->machine, domCaps->arch))
return 0;
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 07/10] libxl: domcaps: fill in explicit supported BOOL_NO
None of the <feature> bits are supported, and the <loader> piece
is only conditionally supported
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/libxl/libxl_capabilities.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c
index 385b08be38..672c1c7c66 100644
--- a/src/libxl/libxl_capabilities.c
+++ b/src/libxl/libxl_capabilities.c
@@ -604,6 +604,7 @@ libxlMakeDomainOSCaps(const char *machine,
size_t i;
os->supported = VIR_TRISTATE_BOOL_YES;
+ capsLoader->supported = VIR_TRISTATE_BOOL_NO;
if (STREQ(machine, "xenpv") || STREQ(machine, "xenpvh"))
return 0;
@@ -773,6 +774,11 @@ libxlMakeDomainCapabilities(virDomainCapsPtr domCaps,
libxlMakeDomainDeviceHostdevCaps(hostdev) < 0)
return -1;
+ domCaps->iothreads = VIR_TRISTATE_BOOL_NO;
+ domCaps->vmcoreinfo = VIR_TRISTATE_BOOL_NO;
+ domCaps->genid = VIR_TRISTATE_BOOL_NO;
+ domCaps->gic.supported = VIR_TRISTATE_BOOL_NO;
+
return 0;
}
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 08/10] bhyve: domcaps: fill in explicit supported BOOL_NO
<hostdev> and <features> are not supported. <loader>, <graphics>,
and <video> are supported conditionally
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
This patch is untested. If someone has a bhyve build setup I'd
appreciate a test
src/bhyve/bhyve_capabilities.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 79d7659da3..ff78023895 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -86,6 +86,7 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps,
caps->os.supported = VIR_TRISTATE_BOOL_YES;
+ caps->os.loader.supported = VIR_TRISTATE_BOOL_NO;
if (bhyvecaps & BHYVE_CAP_LPC_BOOTROM) {
caps->os.loader.supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(caps->os.loader.type,
@@ -98,12 +99,21 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps,
}
+ caps->graphics.supported = VIR_TRISTATE_BOOL_NO;
+ caps->video.supported = VIR_TRISTATE_BOOL_NO;
if (bhyvecaps & BHYVE_CAP_FBUF) {
caps->graphics.supported = VIR_TRISTATE_BOOL_YES;
caps->video.supported = VIR_TRISTATE_BOOL_YES;
VIR_DOMAIN_CAPS_ENUM_SET(caps->graphics.type, VIR_DOMAIN_GRAPHICS_TYPE_VNC);
VIR_DOMAIN_CAPS_ENUM_SET(caps->video.modelType, VIR_DOMAIN_VIDEO_TYPE_GOP);
}
+
+ caps->hostdev.supported = VIR_TRISTATE_BOOL_NO;
+ caps->iothreads = VIR_TRISTATE_BOOL_NO;
+ caps->vmcoreinfo = VIR_TRISTATE_BOOL_NO;
+ caps->genid = VIR_TRISTATE_BOOL_NO;
+ caps->gic.supported = VIR_TRISTATE_BOOL_NO;
+
return 0;
}
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 09/10] schemas: domcaps: Make more elements optional
Upcoming changes will make outputting these subelements optional.
While we are here drop the useless interleave: since this is an output
only format the elements are always in the same order
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
docs/schemas/domaincaps.rng | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 7d80693d38..3c42cb8075 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -142,12 +142,18 @@
<define name='devices'>
<element name='devices'>
- <interleave>
+ <optional>
<ref name='disk'/>
+ </optional>
+ <optional>
<ref name='graphics'/>
+ </optional>
+ <optional>
<ref name='video'/>
+ </optional>
+ <optional>
<ref name='hostdev'/>
- </interleave>
+ </optional>
</element>
</define>
@@ -181,12 +187,18 @@
<define name='features'>
<element name='features'>
- <interleave>
+ <optional>
<ref name='gic'/>
+ </optional>
+ <optional>
<ref name='vmcoreinfo'/>
+ </optional>
+ <optional>
<ref name='vmgenid'/>
+ </optional>
+ <optional>
<ref name='sev'/>
- </interleave>
+ </optional>
</element>
</define>
--
2.20.1
2:09 p.m.
New subject: [libvirt] [PATCH 10/10] conf: domcaps: Don't output XML on tristate ABSENT
Change domcaps to skip formatting XML if the default
TRISTATE_BOOL_ABSENT is found. Now when domcaps is extended, driver
XML output won't change until an explicit TRISTATE_BOOL value is set
in driver code.
Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
---
src/conf/domain_capabilities.c | 10 +++++++---
tests/domaincapsschemadata/empty.xml | 9 ---------
2 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 6ae18515b6..a64f10842d 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -370,10 +370,12 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
#define FORMAT_PROLOGUE(item) \
do { \
+ if (item->supported == VIR_TRISTATE_BOOL_ABSENT) \
+ return; \
virBufferAsprintf(buf, "<" #item "
supported='%s'%s\n", \
(item->supported == VIR_TRISTATE_BOOL_YES) ? "yes" :
"no", \
(item->supported == VIR_TRISTATE_BOOL_YES) ? ">" :
"/>"); \
- if (item->supported != VIR_TRISTATE_BOOL_YES) \
+ if (item->supported == VIR_TRISTATE_BOOL_NO) \
return; \
virBufferAdjustIndent(buf, 2); \
} while (0)
@@ -386,8 +388,10 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
#define FORMAT_SINGLE(name, supported) \
do { \
- virBufferAsprintf(&buf, "<%s supported='%s'/>\n",
name, \
- (supported == VIR_TRISTATE_BOOL_YES) ? "yes" : "no");
\
+ if (supported != VIR_TRISTATE_BOOL_ABSENT) { \
+ virBufferAsprintf(&buf, "<%s supported='%s'/>\n",
name, \
+ (supported == VIR_TRISTATE_BOOL_YES) ? "yes" :
"no"); \
+ } \
} while (0)
#define ENUM_PROCESS(master, capsEnum, valToStr) \
diff --git a/tests/domaincapsschemadata/empty.xml b/tests/domaincapsschemadata/empty.xml
index 2b2e97d3b3..6c3f5f54fd 100644
--- a/tests/domaincapsschemadata/empty.xml
+++ b/tests/domaincapsschemadata/empty.xml
@@ -3,23 +3,14 @@
<domain>kvm</domain>
<machine>my-machine-type</machine>
<arch>x86_64</arch>
- <iothreads supported='no'/>
- <os supported='no'/>
<cpu>
<mode name='host-passthrough' supported='no'/>
<mode name='host-model' supported='no'/>
<mode name='custom' supported='no'/>
</cpu>
<devices>
- <disk supported='no'/>
- <graphics supported='no'/>
- <video supported='no'/>
- <hostdev supported='no'/>
</devices>
<features>
- <gic supported='no'/>
- <vmcoreinfo supported='no'/>
- <genid supported='no'/>
<sev supported='no'/>
</features>
</domainCapabilities>
--
2.20.1
10:33 a.m.
On 2/19/19 3:09 PM, Cole Robinson wrote:
Extending domaincapabilities with new XML schema is currently a bit
of
a maintenance pain. Consider the case of adding a new enum for listing
<sound> models. I want to output this info for the qemu driver.
Internally in the domaincapabilities plumbing, whether a <device> is
supported= is tracked with boolean true/false. If I extend that
pattern for <sound> devices and fill in data for the qemu driver, the
other domcaps implementations will now automatically output a new XML
element:
<sound supported='no'>
Now, for bhyve I can 'git grep' confirm that it doesn't have any
<sound> support, but for xen/libxl it _is_ supported. So if I don't
fill in accurate support in the xen driver, I've just made their
domcaps report blatantly incorrect info.
Ideally I would make these <sound> changes and the other drivers output
would _not_ change. xen output would now be incomplete, but not
obviously wrong, which is easier on me the developer, and safer for the
API consumer.
This moves domcaps plumbing in that direction. It switches most
internal 'supported' fields to virTristateBool so we can track an
ABSENT state and map that to outputting no XML. Explicit supported='no'
values are filled in where needed to ensure existing driver XML doesn't
change. cpu and sev supported= values are left unconverted, but they
require semi-special handling anyways so aren't really affected by the
problem I laid out above.
Cole Robinson (10):
tests: domcaps: Add a default 'empty' test
tests: domcaps: Remove unused typedef
tests: domcaps: Remove 'full' test
conf: domcaps: Add single line formatting macro
conf: domcaps: use virTristateBool for 'supported'
qemu: domcaps: fill in explicit supported BOOL_NO
libxl: domcaps: fill in explicit supported BOOL_NO
bhyve: domcaps: fill in explicit supported BOOL_NO
schemas: domcaps: Make more elements optional
conf: domcaps: Don't output XML on tristate ABSENT
docs/schemas/domaincaps.rng | 20 +++++--
src/bhyve/bhyve_capabilities.c | 20 +++++--
src/conf/domain_capabilities.c | 26 ++++++----
src/conf/domain_capabilities.h | 20 +++----
src/libxl/libxl_capabilities.c | 18 ++++---
src/qemu/qemu_capabilities.c | 26 ++++++----
tests/domaincapsschemadata/empty.xml | 16 ++++++
tests/domaincapstest.c | 78 ++--------------------------
8 files changed, 103 insertions(+), 121 deletions(-)
create mode 100644 tests/domaincapsschemadata/empty.xml
I think in patch3, you probably should remove the full.xml file too.
Logically, it seems things work; however, I am curious what happens
if/when this is applied to bhvye and libxl environments. I think it
would be good to get someone to apply this there and be sure there's no
unexpected (for you) failures.
Interesting "view" for <cpu> on "empty.xml" - digging deeper
shows that
"mode" could be optional (at least that's how I read the formatdomain
text). So rather than "no" for all 3, there would be nothing.
Similarly for SEV the "no" just is some default/optional value matching
your RNG changes. Also, even though it wouldn't necessarily be a
"feature" of perhaps Intel, someone running on AMD could I assume get a
different result than you got. IOW: Same problem I ran into with that 2
patch series trying to "fake" SEV output.
If cpu, devices, and features have no subelements - should they even be
displayed? Leaving purely just the path, domain, machine, and arch output?
Even with all that - is there any thought that perhaps some application
has made use of the existing "functionality" to spit out "no" for
those
undefined/missing (e.g. ABSENT) values. IOW: It seems via the RNG we
would have made claims that certain output exists that we're now making
optional. I conceptually don't have an issue with it, I'm just thinking
terms of what API output contract we may have made. Yes, those
applications should be able to handle missing fields, but at the very
least we probably would need to update the news.xml to let the consumer
know.
John
4:19 p.m.
On 2/25/19 11:33 AM, John Ferlan wrote:
On 2/19/19 3:09 PM, Cole Robinson wrote:
> Extending domaincapabilities with new XML schema is currently a bit of
> a maintenance pain. Consider the case of adding a new enum for listing
> <sound> models. I want to output this info for the qemu driver.
>
> Internally in the domaincapabilities plumbing, whether a <device> is
> supported= is tracked with boolean true/false. If I extend that
> pattern for <sound> devices and fill in data for the qemu driver, the
> other domcaps implementations will now automatically output a new XML
> element:
>
> <sound supported='no'>
>
> Now, for bhyve I can 'git grep' confirm that it doesn't have any
> <sound> support, but for xen/libxl it _is_ supported. So if I don't
> fill in accurate support in the xen driver, I've just made their
> domcaps report blatantly incorrect info.
>
> Ideally I would make these <sound> changes and the other drivers output
> would _not_ change. xen output would now be incomplete, but not
> obviously wrong, which is easier on me the developer, and safer for the
> API consumer.
>
> This moves domcaps plumbing in that direction. It switches most
> internal 'supported' fields to virTristateBool so we can track an
> ABSENT state and map that to outputting no XML. Explicit supported='no'
> values are filled in where needed to ensure existing driver XML doesn't
> change. cpu and sev supported= values are left unconverted, but they
> require semi-special handling anyways so aren't really affected by the
> problem I laid out above.
>
> Cole Robinson (10):
> tests: domcaps: Add a default 'empty' test
> tests: domcaps: Remove unused typedef
> tests: domcaps: Remove 'full' test
> conf: domcaps: Add single line formatting macro
> conf: domcaps: use virTristateBool for 'supported'
> qemu: domcaps: fill in explicit supported BOOL_NO
> libxl: domcaps: fill in explicit supported BOOL_NO
> bhyve: domcaps: fill in explicit supported BOOL_NO
> schemas: domcaps: Make more elements optional
> conf: domcaps: Don't output XML on tristate ABSENT
>
> docs/schemas/domaincaps.rng | 20 +++++--
> src/bhyve/bhyve_capabilities.c | 20 +++++--
> src/conf/domain_capabilities.c | 26 ++++++----
> src/conf/domain_capabilities.h | 20 +++----
> src/libxl/libxl_capabilities.c | 18 ++++---
> src/qemu/qemu_capabilities.c | 26 ++++++----
> tests/domaincapsschemadata/empty.xml | 16 ++++++
> tests/domaincapstest.c | 78 ++--------------------------
> 8 files changed, 103 insertions(+), 121 deletions(-)
> create mode 100644 tests/domaincapsschemadata/empty.xml
>
I think in patch3, you probably should remove the full.xml file too.
Logically, it seems things work; however, I am curious what happens
if/when this is applied to bhvye and libxl environments. I think it
would be good to get someone to apply this there and be sure there's no
unexpected (for you) failures.
Interesting "view" for <cpu> on "empty.xml" - digging deeper
shows that
"mode" could be optional (at least that's how I read the formatdomain
text). So rather than "no" for all 3, there would be nothing.
Similarly for SEV the "no" just is some default/optional value matching
your RNG changes. Also, even though it wouldn't necessarily be a
"feature" of perhaps Intel, someone running on AMD could I assume get a
different result than you got. IOW: Same problem I ran into with that 2
patch series trying to "fake" SEV output.
Yes, cpu and sev bits would ideally get similar treatment, that's an
idea for a follow up series for sure.
If cpu, devices, and features have no subelements - should they even
be
displayed? Leaving purely just the path, domain, machine, and arch output?
Would certainly make for nicer XML. Not a priority for me in the near
term though. FWIW my goal for the short/medium term with this stuff is
making it as simple as possible to extend domaincapabilities with the
multitude of data it is lacking.
Even with all that - is there any thought that perhaps some
application
has made use of the existing "functionality" to spit out "no" for
those
undefined/missing (e.g. ABSENT) values. IOW: It seems via the RNG we
would have made claims that certain output exists that we're now making
optional. I conceptually don't have an issue with it, I'm just thinking
terms of what API output contract we may have made. Yes, those
applications should be able to handle missing fields, but at the very
least we probably would need to update the news.xml to let the consumer
know.
It's not impossible I suppose. But if you parse the XML like that then
you are already restricting your ability to handle older libvirt domcaps
XML which may not output the specific field at all: think before <sev>
bits were added. I think in general with libvirt if you are parsing XML
such that you are always expecting a value to be present, you are doing
it wrong, because almost every XML bit in the domain schema at least is
optionally missing.
Regardless I can extend the docs to try and clarify how I think the API
should be consumed:
"Some XML elements may be entirely omitted from the domaincapabilities
XML, depending on what the libvirt driver has filled in. Applications
should only act on what is explicitly reported in the domaincapabilities
XML. For example, if <disk supported='yes'/> is present, you can safely
assume the driver supports <disk> devices. If <disk supported='no'/>
is
present, you can safely assume the driver does NOT support <disk>
devices. If the <disk> block is omitted entirely, the driver is not
indicating one way or the other whether it supports <disk> devices, and
applications should not interpret the missing block to mean any thing in
particular."
Thanks,
Cole
2122
days inactive
2137
days old
12 comments
2 participants
participants (2)
-
Cole Robinson -
John Ferlan