[libvirt] [PATCH v2 00/16] domcaps: use virTristateBool
v1 posting: https://www.redhat.com/archives/libvir-list/2019-February/msg01088.html v2 changes: - Rebase to master - Remove the full.xml test in patch #3 - Add virCapsEnum 'format' and use it - Extend docs to explain optional XML v1 cover letter: 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. In v2, I additionally added a mechanism to make <enum> values optionally formatted. Right now whenever a new <enum> is added, if the parent bit is supported (like <disk supported='yes'/>), the new <enum> is automatically formatted as well. This has the same problem described above with the @supported bit. Now drivers are required to set a virCapsPtr.report = true if they want the <enum> to be formatted. Existing drives have this value filled in to maintain back compat. Again, bhyve changes are untested. If someone can give them a spin that would be appreciated, otherwise I will try to get a freebsd build setup. Cole Robinson (16): 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 conf: domcaps: Add virCapsEnum 'report' qemu: fill in virCapsEnum 'report' libxl: fill in virCapsEnum 'report' bhyve: fill in virCapsEnum 'report' conf: domcaps: Don't format XML on report=false docs: formatdomaincaps: Describe optional XML changes docs/formatdomaincaps.html.in | 11 +++ docs/schemas/domaincaps.rng | 20 ++++- src/bhyve/bhyve_capabilities.c | 27 ++++-- src/conf/domain_capabilities.c | 31 ++++--- src/conf/domain_capabilities.h | 21 ++--- src/libxl/libxl_capabilities.c | 31 +++++-- src/qemu/qemu_capabilities.c | 41 ++++++--- tests/domaincapsschemadata/empty.xml | 16 ++++ tests/domaincapsschemadata/full.xml | 123 --------------------------- tests/domaincapstest.c | 79 +---------------- 10 files changed, 155 insertions(+), 245 deletions(-) create mode 100644 tests/domaincapsschemadata/empty.xml delete mode 100644 tests/domaincapsschemadata/full.xml -- 2.20.1
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@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 3be2806a48..b5bf4c234f 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -433,6 +433,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
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/domaincapstest.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index b5bf4c234f..02f33d9f00 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
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@redhat.com> --- tests/domaincapsschemadata/full.xml | 123 ---------------------------- tests/domaincapstest.c | 73 ----------------- 2 files changed, 196 deletions(-) delete mode 100644 tests/domaincapsschemadata/full.xml diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml deleted file mode 100644 index 28263466a4..0000000000 --- a/tests/domaincapsschemadata/full.xml +++ /dev/null @@ -1,123 +0,0 @@ -<domainCapabilities> - <path>/bin/emulatorbin</path> - <domain>kvm</domain> - <machine>my-machine-type</machine> - <arch>x86_64</arch> - <vcpu max='255'/> - <iothreads supported='no'/> - <os supported='yes'> - <loader supported='yes'> - <value>/foo/bar</value> - <value>/tmp/my_path</value> - <enum name='type'> - <value>rom</value> - <value>pflash</value> - </enum> - <enum name='readonly'> - <value>default</value> - <value>yes</value> - <value>no</value> - </enum> - </loader> - </os> - <cpu> - <mode name='host-passthrough' supported='yes'/> - <mode name='host-model' supported='yes'> - <model>host</model> - <vendor>CPU Vendorrr</vendor> - </mode> - <mode name='custom' supported='yes'> - <model usable='unknown'>Model1</model> - <model usable='no'>Model2</model> - <model usable='yes'>Model3</model> - </mode> - </cpu> - <devices> - <disk supported='yes'> - <enum name='diskDevice'> - <value>disk</value> - <value>cdrom</value> - <value>floppy</value> - <value>lun</value> - </enum> - <enum name='bus'> - <value>ide</value> - <value>fdc</value> - <value>scsi</value> - <value>virtio</value> - <value>xen</value> - <value>usb</value> - <value>uml</value> - <value>sata</value> - <value>sd</value> - </enum> - <enum name='model'> - <value>default</value> - <value>virtio</value> - <value>virtio-transitional</value> - <value>virtio-non-transitional</value> - </enum> - </disk> - <graphics supported='yes'> - <enum name='type'> - <value>sdl</value> - <value>vnc</value> - <value>rdp</value> - <value>desktop</value> - <value>spice</value> - <value>egl-headless</value> - </enum> - </graphics> - <video supported='yes'> - <enum name='modelType'> - <value>default</value> - <value>vga</value> - <value>cirrus</value> - <value>vmvga</value> - <value>xen</value> - <value>vbox</value> - <value>qxl</value> - <value>parallels</value> - <value>virtio</value> - <value>gop</value> - <value>none</value> - </enum> - </video> - <hostdev supported='yes'> - <enum name='mode'> - <value>subsystem</value> - <value>capabilities</value> - </enum> - <enum name='startupPolicy'> - <value>default</value> - <value>mandatory</value> - <value>requisite</value> - <value>optional</value> - </enum> - <enum name='subsysType'> - <value>usb</value> - <value>pci</value> - <value>scsi</value> - <value>scsi_host</value> - <value>mdev</value> - </enum> - <enum name='capsType'> - <value>storage</value> - <value>misc</value> - <value>net</value> - </enum> - <enum name='pciBackend'> - <value>default</value> - <value>kvm</value> - <value>vfio</value> - <value>xen</value> - </enum> - </hostdev> - </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 02f33d9f00..1aa8c023a2 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,67 +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); - SET_ALL_BITS(disk->model); - - 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" @@ -258,7 +194,6 @@ fillBhyveCaps(virDomainCapsPtr domCaps, unsigned int *bhyve_caps) enum testCapsType { CAPS_NONE, - CAPS_ALL, CAPS_QEMU, CAPS_LIBXL, CAPS_BHYVE, @@ -297,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, @@ -407,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
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@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 a6104920ab..5a26329176 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, \ @@ -594,8 +600,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); @@ -615,11 +620,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
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@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 5a26329176..081549eefb 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 b5387916a1..3282b47d52 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 */ virDomainCapsEnum model; /* Info about virDomainDiskModel enum values */ @@ -72,21 +72,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 */ @@ -98,7 +98,7 @@ struct _virDomainCapsDeviceHostdev { typedef struct _virDomainCapsFeatureGIC virDomainCapsFeatureGIC; typedef virDomainCapsFeatureGIC *virDomainCapsFeatureGICPtr; struct _virDomainCapsFeatureGIC { - bool supported; + virTristateBool supported; virDomainCapsEnum version; /* Info about virGICVersion */ }; @@ -156,7 +156,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; @@ -167,8 +167,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 c9700193fd..6a0ec0d1bd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4918,7 +4918,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; @@ -4958,7 +4958,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; @@ -5011,7 +5011,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; } @@ -5022,7 +5023,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, @@ -5067,7 +5068,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)) @@ -5083,7 +5084,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); @@ -5107,7 +5108,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); @@ -5222,7 +5223,7 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps, version)) continue; - gic->supported = true; + gic->supported = VIR_TRISTATE_BOOL_YES; VIR_DOMAIN_CAPS_ENUM_SET(gic->version, version); } @@ -5293,10 +5294,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
Only gic->supported needs an explicit BOOL_NO setting, all other 'supported' values are handling things correctly Signed-off-by: Cole Robinson <crobinso@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 6a0ec0d1bd..3611fb92d8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5212,6 +5212,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
None of the <feature> bits are supported, and the <loader> piece is only conditionally supported Signed-off-by: Cole Robinson <crobinso@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
<hostdev> and <features> are not supported. <loader>, <graphics>, and <video> are supported conditionally Signed-off-by: Cole Robinson <crobinso@redhat.com> --- 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
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@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
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@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 081549eefb..f45a3bcc3d 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
virCapsEnum report is an internal bool indicating whether we should format the enum in the XML at all. This is unused for now but will be handled in future patches. We use a plain bool instead of tristate because the case here is a bit different than the explicit @supported output. We already report the equivalent of supported=YES|NO based on what enum values are filled in. This adds report=false to handle the ABSENT case. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_capabilities.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 3282b47d52..26f4b8c394 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -33,6 +33,7 @@ typedef virDomainCaps *virDomainCapsPtr; typedef struct _virDomainCapsEnum virDomainCapsEnum; typedef virDomainCapsEnum *virDomainCapsEnumPtr; struct _virDomainCapsEnum { + bool report; /* Whether the format the enum at all */ unsigned int values; /* Bitmask of values supported in the corresponding enum */ }; -- 2.20.1
Set report=true for all enums currently formatted in the XML Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_capabilities.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3611fb92d8..c18af5fb44 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4919,6 +4919,8 @@ virQEMUCapsFillDomainLoaderCaps(virDomainCapsLoaderPtr capsLoader, size_t i; capsLoader->supported = VIR_TRISTATE_BOOL_YES; + capsLoader->type.report = true; + capsLoader->readonly.report = true; if (VIR_ALLOC_N(capsLoader->values.values, nfirmwares) < 0) return -1; @@ -5024,6 +5026,10 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, virDomainCapsDeviceDiskPtr disk) { disk->supported = VIR_TRISTATE_BOOL_YES; + disk->diskDevice.report = true; + disk->bus.report = true; + disk->model.report = true; + /* QEMU supports all of these */ VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_DISK, @@ -5069,6 +5075,7 @@ virQEMUCapsFillDomainDeviceGraphicsCaps(virQEMUCapsPtr qemuCaps, virDomainCapsDeviceGraphicsPtr dev) { dev->supported = VIR_TRISTATE_BOOL_YES; + dev->type.report = true; VIR_DOMAIN_CAPS_ENUM_SET(dev->type, VIR_DOMAIN_GRAPHICS_TYPE_SDL); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNC)) @@ -5085,6 +5092,7 @@ virQEMUCapsFillDomainDeviceVideoCaps(virQEMUCapsPtr qemuCaps, virDomainCapsDeviceVideoPtr dev) { dev->supported = VIR_TRISTATE_BOOL_YES; + dev->modelType.report = true; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_VGA); @@ -5109,6 +5117,12 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps, bool supportsPassthroughVFIO = qemuHostdevHostSupportsPassthroughVFIO(); hostdev->supported = VIR_TRISTATE_BOOL_YES; + hostdev->mode.report = true; + hostdev->startupPolicy.report = true; + hostdev->subsysType.report = true; + hostdev->capsType.report = true; + hostdev->pciBackend.report = true; + /* VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES is for containers only */ VIR_DOMAIN_CAPS_ENUM_SET(hostdev->mode, VIR_DOMAIN_HOSTDEV_MODE_SUBSYS); @@ -5226,6 +5240,7 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps, continue; gic->supported = VIR_TRISTATE_BOOL_YES; + gic->version.report = true; VIR_DOMAIN_CAPS_ENUM_SET(gic->version, version); } -- 2.20.1
Set report=true for all enums currently formatted in the XML Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libxl/libxl_capabilities.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index 672c1c7c66..19f90ba181 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -605,6 +605,8 @@ libxlMakeDomainOSCaps(const char *machine, os->supported = VIR_TRISTATE_BOOL_YES; capsLoader->supported = VIR_TRISTATE_BOOL_NO; + capsLoader->type.report = true; + capsLoader->readonly.report = true; if (STREQ(machine, "xenpv") || STREQ(machine, "xenpvh")) return 0; @@ -633,6 +635,9 @@ static int libxlMakeDomainDeviceDiskCaps(virDomainCapsDeviceDiskPtr dev) { dev->supported = VIR_TRISTATE_BOOL_YES; + dev->diskDevice.report = true; + dev->bus.report = true; + dev->model.report = true; VIR_DOMAIN_CAPS_ENUM_SET(dev->diskDevice, VIR_DOMAIN_DISK_DEVICE_DISK, @@ -650,6 +655,7 @@ static int libxlMakeDomainDeviceGraphicsCaps(virDomainCapsDeviceGraphicsPtr dev) { dev->supported = VIR_TRISTATE_BOOL_YES; + dev->type.report = true; VIR_DOMAIN_CAPS_ENUM_SET(dev->type, VIR_DOMAIN_GRAPHICS_TYPE_SDL, @@ -663,6 +669,7 @@ static int libxlMakeDomainDeviceVideoCaps(virDomainCapsDeviceVideoPtr dev) { dev->supported = VIR_TRISTATE_BOOL_YES; + dev->modelType.report = true; VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_VGA, @@ -685,6 +692,12 @@ static int libxlMakeDomainDeviceHostdevCaps(virDomainCapsDeviceHostdevPtr dev) { dev->supported = VIR_TRISTATE_BOOL_YES; + dev->mode.report = true; + dev->startupPolicy.report = true; + dev->subsysType.report = true; + dev->capsType.report = true; + dev->pciBackend.report = true; + /* VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES is for containers only */ VIR_DOMAIN_CAPS_ENUM_SET(dev->mode, VIR_DOMAIN_HOSTDEV_MODE_SUBSYS); -- 2.20.1
Set report=true for all enums currently formatted in the XML Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/bhyve/bhyve_capabilities.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index ff78023895..27fa8ee5ef 100644 --- a/src/bhyve/bhyve_capabilities.c +++ b/src/bhyve/bhyve_capabilities.c @@ -76,6 +76,9 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps, virDomainCapsStringValuesPtr firmwares) { caps->disk.supported = VIR_TRISTATE_BOOL_YES; + caps->disk.diskDevice.report = true; + caps->disk.bus.report = true; + caps->disk.model.report = true; VIR_DOMAIN_CAPS_ENUM_SET(caps->disk.diskDevice, VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_DEVICE_CDROM); @@ -88,6 +91,8 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps, caps->os.loader.supported = VIR_TRISTATE_BOOL_NO; if (bhyvecaps & BHYVE_CAP_LPC_BOOTROM) { + caps->os.loader.type.report = true; + caps->os.loader.readonly.report = true; caps->os.loader.supported = VIR_TRISTATE_BOOL_YES; VIR_DOMAIN_CAPS_ENUM_SET(caps->os.loader.type, VIR_DOMAIN_LOADER_TYPE_PFLASH); @@ -103,7 +108,9 @@ virBhyveDomainCapsFill(virDomainCapsPtr caps, caps->video.supported = VIR_TRISTATE_BOOL_NO; if (bhyvecaps & BHYVE_CAP_FBUF) { caps->graphics.supported = VIR_TRISTATE_BOOL_YES; + caps->graphics.type.report = true; caps->video.supported = VIR_TRISTATE_BOOL_YES; + caps->video.modelType.report = true; 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); } -- 2.20.1
After this, newly added enums will not automatically show up in driver output unless the driver code specifically sets report=true Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_capabilities.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index f45a3bcc3d..5a8f48da61 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -330,6 +330,11 @@ virDomainCapsEnumFormat(virBufferPtr buf, int ret = -1; size_t i; + if (!capsEnum->report) { + ret = 0; + goto cleanup; + } + virBufferAsprintf(buf, "<enum name='%s'", capsEnumName); if (!capsEnum->values) { virBufferAddLit(buf, "/>\n"); -- 2.20.1
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/formatdomaincaps.html.in | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index ba48daab2d..2583f9bead 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -37,6 +37,17 @@ management application to choose an appropriate mode for a pass-through host device as well as which adapter to utilize.</p> + <p>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.</p> + <h2><a id="elements">Element and attribute overview</a></h2> <p> A new query interface was added to the virConnect API's to retrieve the -- 2.20.1
On 3/6/19 6:36 PM, Cole Robinson wrote:
v1 posting: https://www.redhat.com/archives/libvir-list/2019-February/msg01088.html
v2 changes: - Rebase to master - Remove the full.xml test in patch #3 - Add virCapsEnum 'format' and use it - Extend docs to explain optional XML
v1 cover letter: 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.
In v2, I additionally added a mechanism to make <enum> values optionally formatted. Right now whenever a new <enum> is added, if the parent bit is supported (like <disk supported='yes'/>), the new <enum> is automatically formatted as well. This has the same problem described above with the @supported bit. Now drivers are required to set a virCapsPtr.report = true if they want the <enum> to be formatted. Existing drives have this value filled in to maintain back compat.
Again, bhyve changes are untested. If someone can give them a spin that would be appreciated, otherwise I will try to get a freebsd build setup.
I have confirmed that after applying this series: * bhyve builds and tests pass on freebsd 12.0 * runtime libxl domaincapabilities output on f29 does not change for both xenpv and xenfv - Cole
On 3/7/19 12:36 AM, Cole Robinson wrote:
v1 posting: https://www.redhat.com/archives/libvir-list/2019-February/msg01088.html
v2 changes: - Rebase to master - Remove the full.xml test in patch #3 - Add virCapsEnum 'format' and use it - Extend docs to explain optional XML
v1 cover letter: 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.
In v2, I additionally added a mechanism to make <enum> values optionally formatted. Right now whenever a new <enum> is added, if the parent bit is supported (like <disk supported='yes'/>), the new <enum> is automatically formatted as well. This has the same problem described above with the @supported bit. Now drivers are required to set a virCapsPtr.report = true if they want the <enum> to be formatted. Existing drives have this value filled in to maintain back compat.
What if we'd have virDomainCapsEnumSet() also set enum.report = true? That should still work as if a driver doesn't support given enum it won't call the function, would it? Otherwise the patches look good. Michal
On 3/18/19 9:28 AM, Michal Privoznik wrote:
On 3/7/19 12:36 AM, Cole Robinson wrote:
v1 posting: https://www.redhat.com/archives/libvir-list/2019-February/msg01088.html
v2 changes: - Rebase to master - Remove the full.xml test in patch #3 - Add virCapsEnum 'format' and use it - Extend docs to explain optional XML
v1 cover letter: 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.
In v2, I additionally added a mechanism to make <enum> values optionally formatted. Right now whenever a new <enum> is added, if the parent bit is supported (like <disk supported='yes'/>), the new <enum> is automatically formatted as well. This has the same problem described above with the @supported bit. Now drivers are required to set a virCapsPtr.report = true if they want the <enum> to be formatted. Existing drives have this value filled in to maintain back compat.
What if we'd have virDomainCapsEnumSet() also set enum.report = true? That should still work as if a driver doesn't support given enum it won't call the function, would it?
I thought about this case. The downside is in an example like virQEMUCapsFillDomainDeviceVideoCaps. Every video model we report is dependent on a QEMU_CAPS check. If though a qemu binary doesn't have any of those devices, we want to report an empty enum to reflect that. If we implicitly depend on setting 'report = true' via VIR_DOMAIN_CAPS_ENUM_SET, then we won't get the empty enum in this case, instead report no enum at all. Keeping the 'report' bit explicit makes it harder to accidentally introduce an issue like that. It's a corner case but that was the motivation Thanks, Cole - Cole
On 3/18/19 2:53 PM, Cole Robinson wrote:
On 3/18/19 9:28 AM, Michal Privoznik wrote:
On 3/7/19 12:36 AM, Cole Robinson wrote:
v1 posting: https://www.redhat.com/archives/libvir-list/2019-February/msg01088.html
v2 changes: - Rebase to master - Remove the full.xml test in patch #3 - Add virCapsEnum 'format' and use it - Extend docs to explain optional XML
v1 cover letter: 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.
In v2, I additionally added a mechanism to make <enum> values optionally formatted. Right now whenever a new <enum> is added, if the parent bit is supported (like <disk supported='yes'/>), the new <enum> is automatically formatted as well. This has the same problem described above with the @supported bit. Now drivers are required to set a virCapsPtr.report = true if they want the <enum> to be formatted. Existing drives have this value filled in to maintain back compat.
What if we'd have virDomainCapsEnumSet() also set enum.report = true? That should still work as if a driver doesn't support given enum it won't call the function, would it?
I thought about this case. The downside is in an example like virQEMUCapsFillDomainDeviceVideoCaps. Every video model we report is dependent on a QEMU_CAPS check. If though a qemu binary doesn't have any of those devices, we want to report an empty enum to reflect that. If we implicitly depend on setting 'report = true' via VIR_DOMAIN_CAPS_ENUM_SET, then we won't get the empty enum in this case, instead report no enum at all.
Keeping the 'report' bit explicit makes it harder to accidentally introduce an issue like that. It's a corner case but that was the motivation
Fair enough. ACK series. Michal
participants (2)
-
Cole Robinson -
Michal Privoznik