[libvirt] [PATCH] domaincaps: Expose UEFI binary path, if it exists
Check to see if the UEFI binary mentioned in qemu.conf actually exists, and if so expose it in domcapabilities like <loader ...> <value>/path/to/ovmf</value> </loader> We introduce some generic domcaps infrastructure for handling a dynamic list of string values, it may be of use for future bits. --- docs/formatdomaincaps.html.in | 6 ++++ docs/schemas/domaincaps.rng | 17 ++++++--- src/conf/domain_capabilities.c | 23 ++++++++++++ src/conf/domain_capabilities.h | 8 +++++ src/qemu/qemu_driver.c | 24 +++++++++++++ tests/domaincapsschemadata/domaincaps-full.xml | 1 + tests/domaincapstest.c | 49 +++++++++++++++++++++----- 7 files changed, 115 insertions(+), 13 deletions(-) diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 34d746d..6959dfe 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -105,6 +105,7 @@ ... <os supported='yes'> <loader supported='yes'> + <value>/usr/share/OVMF/OVMF_CODE.fd</value> <enum name='type'> <value>rom</value> <value>pflash</value> @@ -122,6 +123,11 @@ <p>For the <code>loader</code> element, the following can occur:</p> <dl> + <dt>value</dt> + <dd>List of known loader paths. Currently this is only used + to advertise known locations of OVMF binaries for qemu. Binaries + will only be listed if they actually exist on disk.</dd> + <dt>type</dt> <dd>Whether loader is a typical BIOS (<code>rom</code>) or an UEFI binary (<code>pflash</code>). This refers to diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index ad8d966..dfdb9b9 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -46,6 +46,9 @@ <define name='loader'> <element name='loader'> + <optional> + <ref name='value'/> + </optional> <ref name='supported'/> <ref name='enum'/> </element> @@ -85,6 +88,14 @@ </element> </define> + <define name='value'> + <zeroOrMore> + <element name='value'> + <text/> + </element> + </zeroOrMore> + </define> + <define name='supported'> <attribute name='supported'> <choice> @@ -100,11 +111,7 @@ <attribute name='name'> <text/> </attribute> - <zeroOrMore> - <element name='value'> - <text/> - </element> - </zeroOrMore> + <ref name='value'/> </element> </zeroOrMore> </define> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 5a3c8e7..44e422a 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -46,6 +46,15 @@ static int virDomainCapsOnceInit(void) VIR_ONCE_GLOBAL_INIT(virDomainCaps) +static void +virDomainCapsValuesFree(virDomainCapsValuesPtr values) +{ + size_t i; + + for (i = 0; i < values->nvalues; i++) { + VIR_FREE(values->values[i]); + } +} static void virDomainCapsDispose(void *obj) @@ -54,6 +63,8 @@ virDomainCapsDispose(void *obj) VIR_FREE(caps->path); VIR_FREE(caps->machine); + + virDomainCapsValuesFree(&caps->os.loader.values); } @@ -156,6 +167,17 @@ virDomainCapsEnumFormat(virBufferPtr buf, return ret; } +static void +virDomainCapsValuesFormat(virBufferPtr buf, + virDomainCapsValuesPtr values) +{ + size_t i; + + for (i = 0; i < values->nvalues; i++) { + virBufferAsprintf(buf, "<value>%s</value>\n", values->values[i]); + } +} + #define FORMAT_PROLOGUE(item) \ do { \ virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ @@ -185,6 +207,7 @@ virDomainCapsLoaderFormat(virBufferPtr buf, { FORMAT_PROLOGUE(loader); + virDomainCapsValuesFormat(buf, &loader->values); ENUM_PROCESS(loader, type, virDomainLoaderTypeToString); ENUM_PROCESS(loader, readonly, virTristateBoolTypeToString); diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 768646b..3d5aaa3 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -37,6 +37,13 @@ struct _virDomainCapsEnum { unsigned int values; /* Bitmask of values supported in the corresponding enum */ }; +typedef struct _virDomainCapsValues virDomainCapsValues; +typedef virDomainCapsValues *virDomainCapsValuesPtr; +struct _virDomainCapsValues { + char **values; /* raw string values */ + size_t nvalues; /* number of strings */ +}; + typedef struct _virDomainCapsDevice virDomainCapsDevice; typedef virDomainCapsDevice *virDomainCapsDevicePtr; struct _virDomainCapsDevice { @@ -47,6 +54,7 @@ typedef struct _virDomainCapsLoader virDomainCapsLoader; typedef virDomainCapsLoader *virDomainCapsLoaderPtr; struct _virDomainCapsLoader { virDomainCapsDevice device; + virDomainCapsValues values; virDomainCapsEnum type; /* Info about virDomainLoader */ virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */ }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6008aeb..4dd9d14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17282,6 +17282,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, int virttype; /* virDomainVirtType */ virDomainCapsPtr domCaps = NULL; int arch = virArchFromHost(); /* virArch */ + virQEMUDriverConfigPtr cfg = NULL; + size_t i; virCheckFlags(0, ret); @@ -17356,8 +17358,30 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, virQEMUCapsFillDomainCaps(domCaps, qemuCaps); + cfg = virQEMUDriverGetConfig(driver); + for (i = 0; i < cfg->nloader; i++) { + char *filename = cfg->loader[i]; + + if (access(filename, F_OK) == 0) { + if (VIR_REALLOC_N(domCaps->os.loader.values.values, + domCaps->os.loader.values.nvalues + 1) < 0) { + goto cleanup; + } + domCaps->os.loader.values.nvalues++; + + if (VIR_STRDUP(domCaps->os.loader.values.values[ + domCaps->os.loader.values.nvalues - 1], + filename) < 0) { + goto cleanup; + } + } else { + VIR_DEBUG("loader filename=%s doesn't exist", filename); + } + } + ret = virDomainCapsFormat(domCaps); cleanup: + virObjectUnref(cfg); virObjectUnref(domCaps); virObjectUnref(qemuCaps); return ret; diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 9722772..e7f41d7 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -6,6 +6,7 @@ <vcpu max='255'/> <os supported='yes'> <loader supported='yes'> + <value>/foo/test</value> <enum name='type'> <value>rom</value> <value>pflash</value> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index f240643..ddc4d02 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -28,16 +28,37 @@ #define VIR_FROM_THIS VIR_FROM_NONE -typedef void (*virDomainCapsFill)(virDomainCapsPtr domCaps, - void *opaque); +typedef int (*virDomainCapsFill)(virDomainCapsPtr domCaps, + void *opaque); #define SET_ALL_BITS(x) \ memset(&(x.values), 0xff, sizeof(x.values)) -static void +static int +fillValues(virDomainCapsValuesPtr values) +{ + int ret = -1; + + if (VIR_ALLOC_N(values->values, 1) < 0) { + goto cleanup; + } + values->nvalues = 1; + + if (VIR_STRDUP(values->values[0], "/foo/test") < 0) { + goto cleanup; + } + + ret = 0; + cleanup: + return ret; +} + +static int fillAll(virDomainCapsPtr domCaps, void *opaque ATTRIBUTE_UNUSED) { + int ret = -1; + virDomainCapsOSPtr os = &domCaps->os; virDomainCapsLoaderPtr loader = &os->loader; virDomainCapsDeviceDiskPtr disk = &domCaps->disk; @@ -49,6 +70,9 @@ fillAll(virDomainCapsPtr domCaps, loader->device.supported = true; SET_ALL_BITS(loader->type); SET_ALL_BITS(loader->readonly); + if (fillValues(&loader->values) < 0) { + goto cleanup; + } disk->device.supported = true; SET_ALL_BITS(disk->diskDevice); @@ -60,12 +84,16 @@ fillAll(virDomainCapsPtr domCaps, SET_ALL_BITS(hostdev->subsysType); SET_ALL_BITS(hostdev->capsType); SET_ALL_BITS(hostdev->pciBackend); + + ret = 0; + cleanup: + return ret; } #ifdef WITH_QEMU # include "testutilsqemu.h" -static void +static int fillQemuCaps(virDomainCapsPtr domCaps, void *opaque) { @@ -82,6 +110,8 @@ fillQemuCaps(virDomainCapsPtr domCaps, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO); + + return 0; } #endif /* WITH_QEMU */ @@ -94,16 +124,19 @@ buildVirDomainCaps(const char *emulatorbin, virDomainCapsFill fillFunc, void *opaque) { - virDomainCapsPtr domCaps; + virDomainCapsPtr domCaps, ret = NULL; if (!(domCaps = virDomainCapsNew(emulatorbin, machine, arch, type))) goto cleanup; - if (fillFunc) - fillFunc(domCaps, opaque); + if (fillFunc && fillFunc(domCaps, opaque) < 0) { + virObjectUnref(domCaps); + goto cleanup; + } + ret = domCaps; cleanup: - return domCaps; + return ret; } struct test_virDomainCapsFormatData { -- 2.1.0
Hi Cole, I'm not subscribed to the list; please CC me on UEFI-related patches. Michal pinged me to review this one. Some comments: On 09/17/14 01:52, Cole Robinson wrote:
Check to see if the UEFI binary mentioned in qemu.conf actually exists, and if so expose it in domcapabilities like
<loader ...> <value>/path/to/ovmf</value> </loader>
We introduce some generic domcaps infrastructure for handling a dynamic list of string values, it may be of use for future bits. --- docs/formatdomaincaps.html.in | 6 ++++ docs/schemas/domaincaps.rng | 17 ++++++--- src/conf/domain_capabilities.c | 23 ++++++++++++ src/conf/domain_capabilities.h | 8 +++++ src/qemu/qemu_driver.c | 24 +++++++++++++ tests/domaincapsschemadata/domaincaps-full.xml | 1 + tests/domaincapstest.c | 49 +++++++++++++++++++++----- 7 files changed, 115 insertions(+), 13 deletions(-)
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 34d746d..6959dfe 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -105,6 +105,7 @@ ... <os supported='yes'> <loader supported='yes'> + <value>/usr/share/OVMF/OVMF_CODE.fd</value> <enum name='type'> <value>rom</value> <value>pflash</value> @@ -122,6 +123,11 @@ <p>For the <code>loader</code> element, the following can occur:</p>
<dl> + <dt>value</dt> + <dd>List of known loader paths. Currently this is only used + to advertise known locations of OVMF binaries for qemu. Binaries + will only be listed if they actually exist on disk.</dd> + <dt>type</dt> <dd>Whether loader is a typical BIOS (<code>rom</code>) or an UEFI binary (<code>pflash</code>). This refers to diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index ad8d966..dfdb9b9 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -46,6 +46,9 @@
<define name='loader'> <element name='loader'> + <optional> + <ref name='value'/> + </optional> <ref name='supported'/> <ref name='enum'/> </element> @@ -85,6 +88,14 @@ </element> </define>
+ <define name='value'> + <zeroOrMore> + <element name='value'> + <text/> + </element> + </zeroOrMore> + </define> + <define name='supported'> <attribute name='supported'> <choice> @@ -100,11 +111,7 @@ <attribute name='name'> <text/> </attribute> - <zeroOrMore> - <element name='value'> - <text/> - </element> - </zeroOrMore> + <ref name='value'/> </element> </zeroOrMore> </define> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 5a3c8e7..44e422a 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -46,6 +46,15 @@ static int virDomainCapsOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virDomainCaps)
+static void +virDomainCapsValuesFree(virDomainCapsValuesPtr values) +{ + size_t i; + + for (i = 0; i < values->nvalues; i++) { + VIR_FREE(values->values[i]); + } +}
static void virDomainCapsDispose(void *obj) @@ -54,6 +63,8 @@ virDomainCapsDispose(void *obj)
VIR_FREE(caps->path); VIR_FREE(caps->machine); + + virDomainCapsValuesFree(&caps->os.loader.values); }
(1) This leaks the caps->os.loader.values.values array. (Which is a dynamically allocated array of pointers.) virDomainCapsValuesFree() should VIR_FREE() it too.
@@ -156,6 +167,17 @@ virDomainCapsEnumFormat(virBufferPtr buf, return ret; }
+static void +virDomainCapsValuesFormat(virBufferPtr buf, + virDomainCapsValuesPtr values) +{ + size_t i; + + for (i = 0; i < values->nvalues; i++) { + virBufferAsprintf(buf, "<value>%s</value>\n", values->values[i]); + } +}
(2) virBufferEscapeString() would probably useful here; the filename shouldn't be plainly embedded into an XML fragment. I'm not sure if we paid attention to this with the nvram patches... Hm yes; I rechecked Michal's commits now, and they seem to use virBufferEscapeString().
+ #define FORMAT_PROLOGUE(item) \ do { \ virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ @@ -185,6 +207,7 @@ virDomainCapsLoaderFormat(virBufferPtr buf, { FORMAT_PROLOGUE(loader);
+ virDomainCapsValuesFormat(buf, &loader->values); ENUM_PROCESS(loader, type, virDomainLoaderTypeToString); ENUM_PROCESS(loader, readonly, virTristateBoolTypeToString);
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 768646b..3d5aaa3 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -37,6 +37,13 @@ struct _virDomainCapsEnum { unsigned int values; /* Bitmask of values supported in the corresponding enum */ };
+typedef struct _virDomainCapsValues virDomainCapsValues; +typedef virDomainCapsValues *virDomainCapsValuesPtr; +struct _virDomainCapsValues { + char **values; /* raw string values */ + size_t nvalues; /* number of strings */ +}; + typedef struct _virDomainCapsDevice virDomainCapsDevice; typedef virDomainCapsDevice *virDomainCapsDevicePtr; struct _virDomainCapsDevice { @@ -47,6 +54,7 @@ typedef struct _virDomainCapsLoader virDomainCapsLoader; typedef virDomainCapsLoader *virDomainCapsLoaderPtr; struct _virDomainCapsLoader { virDomainCapsDevice device; + virDomainCapsValues values; virDomainCapsEnum type; /* Info about virDomainLoader */ virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */ }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6008aeb..4dd9d14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17282,6 +17282,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, int virttype; /* virDomainVirtType */ virDomainCapsPtr domCaps = NULL; int arch = virArchFromHost(); /* virArch */ + virQEMUDriverConfigPtr cfg = NULL; + size_t i;
virCheckFlags(0, ret);
@@ -17356,8 +17358,30 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
virQEMUCapsFillDomainCaps(domCaps, qemuCaps);
+ cfg = virQEMUDriverGetConfig(driver); + for (i = 0; i < cfg->nloader; i++) { + char *filename = cfg->loader[i]; + + if (access(filename, F_OK) == 0) { + if (VIR_REALLOC_N(domCaps->os.loader.values.values, + domCaps->os.loader.values.nvalues + 1) < 0) { + goto cleanup; + } + domCaps->os.loader.values.nvalues++; + + if (VIR_STRDUP(domCaps->os.loader.values.values[ + domCaps->os.loader.values.nvalues - 1], + filename) < 0) { + goto cleanup; + } + } else { + VIR_DEBUG("loader filename=%s doesn't exist", filename); + } + } +
(3) I propose to simply preallocate "domCaps->os.loader.values.values" with VIR_ALLOC_N(). There are several reasons: - This saves you on a bunch of VIR_REALLOC_N() calls. Just preallocate for cfg->nloader elements, and only populate (and bump "nvalues") only for loader files that exist. A few extra, NULL filled, unused elements at the end of the array won't hurt. - More importantly, the patch as proposed will cause undefined behavior when VIR_STRDUP() fails and we jump to the cleanup. Namely, at that point you will have reallocated the array -- and VIR_REALLOC_N() does *not* zero-fill -- and also incremented nvalues. This will result, on the error path, the virDomainCapsValuesFree() function to VIR_FREE() a pointer with indeterminate value. If, however, you preallocate with VIR_ALLOC_N(), all entries will be NULL, and VIR_FREE() can handle that. You might also want to consider bumping nvalues *after* VIR_STRDUP() succeeds.
ret = virDomainCapsFormat(domCaps); cleanup: + virObjectUnref(cfg); virObjectUnref(domCaps); virObjectUnref(qemuCaps); return ret; diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 9722772..e7f41d7 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -6,6 +6,7 @@ <vcpu max='255'/> <os supported='yes'> <loader supported='yes'> + <value>/foo/test</value> <enum name='type'> <value>rom</value> <value>pflash</value> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index f240643..ddc4d02 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -28,16 +28,37 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-typedef void (*virDomainCapsFill)(virDomainCapsPtr domCaps, - void *opaque); +typedef int (*virDomainCapsFill)(virDomainCapsPtr domCaps, + void *opaque);
#define SET_ALL_BITS(x) \ memset(&(x.values), 0xff, sizeof(x.values))
-static void +static int +fillValues(virDomainCapsValuesPtr values) +{ + int ret = -1; + + if (VIR_ALLOC_N(values->values, 1) < 0) { + goto cleanup; + } + values->nvalues = 1; + + if (VIR_STRDUP(values->values[0], "/foo/test") < 0) { + goto cleanup; + } + + ret = 0; + cleanup: + return ret; +}
For example, this seems to be safe, even if VIR_STRDUP() fails, because you use VIR_ALLOC_N().
+ +static int fillAll(virDomainCapsPtr domCaps, void *opaque ATTRIBUTE_UNUSED) { + int ret = -1; + virDomainCapsOSPtr os = &domCaps->os; virDomainCapsLoaderPtr loader = &os->loader; virDomainCapsDeviceDiskPtr disk = &domCaps->disk; @@ -49,6 +70,9 @@ fillAll(virDomainCapsPtr domCaps, loader->device.supported = true; SET_ALL_BITS(loader->type); SET_ALL_BITS(loader->readonly); + if (fillValues(&loader->values) < 0) { + goto cleanup; + }
... I'll trust that the automatic dispose functions / destructors will take care of not leaking anything...
disk->device.supported = true; SET_ALL_BITS(disk->diskDevice); @@ -60,12 +84,16 @@ fillAll(virDomainCapsPtr domCaps, SET_ALL_BITS(hostdev->subsysType); SET_ALL_BITS(hostdev->capsType); SET_ALL_BITS(hostdev->pciBackend); + + ret = 0; + cleanup: + return ret; }
#ifdef WITH_QEMU # include "testutilsqemu.h" -static void +static int fillQemuCaps(virDomainCapsPtr domCaps, void *opaque) { @@ -82,6 +110,8 @@ fillQemuCaps(virDomainCapsPtr domCaps, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO); + + return 0; } #endif /* WITH_QEMU */
@@ -94,16 +124,19 @@ buildVirDomainCaps(const char *emulatorbin, virDomainCapsFill fillFunc, void *opaque) { - virDomainCapsPtr domCaps; + virDomainCapsPtr domCaps, ret = NULL;
if (!(domCaps = virDomainCapsNew(emulatorbin, machine, arch, type))) goto cleanup;
- if (fillFunc) - fillFunc(domCaps, opaque); + if (fillFunc && fillFunc(domCaps, opaque) < 0) { + virObjectUnref(domCaps); + goto cleanup; + }
+ ret = domCaps; cleanup: - return domCaps; + return ret; }
struct test_virDomainCapsFormatData {
These look okay to me. Please consider fixing (1) to (3). Thanks, Laszlo
On 17.09.2014 01:52, Cole Robinson wrote:
Check to see if the UEFI binary mentioned in qemu.conf actually exists, and if so expose it in domcapabilities like
<loader ...> <value>/path/to/ovmf</value> </loader>
We introduce some generic domcaps infrastructure for handling a dynamic list of string values, it may be of use for future bits. --- docs/formatdomaincaps.html.in | 6 ++++ docs/schemas/domaincaps.rng | 17 ++++++--- src/conf/domain_capabilities.c | 23 ++++++++++++ src/conf/domain_capabilities.h | 8 +++++ src/qemu/qemu_driver.c | 24 +++++++++++++ tests/domaincapsschemadata/domaincaps-full.xml | 1 + tests/domaincapstest.c | 49 +++++++++++++++++++++----- 7 files changed, 115 insertions(+), 13 deletions(-)
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index 34d746d..6959dfe 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -105,6 +105,7 @@ ... <os supported='yes'> <loader supported='yes'> + <value>/usr/share/OVMF/OVMF_CODE.fd</value> <enum name='type'> <value>rom</value> <value>pflash</value> @@ -122,6 +123,11 @@ <p>For the <code>loader</code> element, the following can occur:</p>
<dl> + <dt>value</dt> + <dd>List of known loader paths. Currently this is only used + to advertise known locations of OVMF binaries for qemu. Binaries + will only be listed if they actually exist on disk.</dd> + <dt>type</dt> <dd>Whether loader is a typical BIOS (<code>rom</code>) or an UEFI binary (<code>pflash</code>). This refers to diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index ad8d966..dfdb9b9 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -46,6 +46,9 @@
<define name='loader'> <element name='loader'> + <optional> + <ref name='value'/> + </optional> <ref name='supported'/>
I know it doesn't really matter, but I prefer attributes to be defined before any child elements. So I'd move 'supported' a few lines up.
<ref name='enum'/> </element> @@ -85,6 +88,14 @@ </element> </define>
+ <define name='value'> + <zeroOrMore> + <element name='value'> + <text/> + </element> + </zeroOrMore> + </define> + <define name='supported'> <attribute name='supported'> <choice> @@ -100,11 +111,7 @@ <attribute name='name'> <text/> </attribute> - <zeroOrMore> - <element name='value'> - <text/> - </element> - </zeroOrMore> + <ref name='value'/> </element> </zeroOrMore> </define> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 5a3c8e7..44e422a 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -46,6 +46,15 @@ static int virDomainCapsOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virDomainCaps)
+static void +virDomainCapsValuesFree(virDomainCapsValuesPtr values) +{ + size_t i; + + for (i = 0; i < values->nvalues; i++) { + VIR_FREE(values->values[i]); + } +}
static void virDomainCapsDispose(void *obj) @@ -54,6 +63,8 @@ virDomainCapsDispose(void *obj)
VIR_FREE(caps->path); VIR_FREE(caps->machine); + + virDomainCapsValuesFree(&caps->os.loader.values); }
@@ -156,6 +167,17 @@ virDomainCapsEnumFormat(virBufferPtr buf, return ret; }
+static void +virDomainCapsValuesFormat(virBufferPtr buf, + virDomainCapsValuesPtr values) +{ + size_t i; + + for (i = 0; i < values->nvalues; i++) { + virBufferAsprintf(buf, "<value>%s</value>\n", values->values[i]); + } +} + #define FORMAT_PROLOGUE(item) \ do { \ virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ @@ -185,6 +207,7 @@ virDomainCapsLoaderFormat(virBufferPtr buf, { FORMAT_PROLOGUE(loader);
+ virDomainCapsValuesFormat(buf, &loader->values); ENUM_PROCESS(loader, type, virDomainLoaderTypeToString); ENUM_PROCESS(loader, readonly, virTristateBoolTypeToString);
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 768646b..3d5aaa3 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -37,6 +37,13 @@ struct _virDomainCapsEnum { unsigned int values; /* Bitmask of values supported in the corresponding enum */ };
+typedef struct _virDomainCapsValues virDomainCapsValues; +typedef virDomainCapsValues *virDomainCapsValuesPtr; +struct _virDomainCapsValues { + char **values; /* raw string values */ + size_t nvalues; /* number of strings */ +};
While this works, I'd rename this to virDomainCapsStringValues so that it's clear what values do we have in mind. Moreover, if we ever introduce other values (say list of integers) the environment is ready and we don't have to do the renaming then.
+ typedef struct _virDomainCapsDevice virDomainCapsDevice; typedef virDomainCapsDevice *virDomainCapsDevicePtr; struct _virDomainCapsDevice { @@ -47,6 +54,7 @@ typedef struct _virDomainCapsLoader virDomainCapsLoader; typedef virDomainCapsLoader *virDomainCapsLoaderPtr; struct _virDomainCapsLoader { virDomainCapsDevice device; + virDomainCapsValues values; virDomainCapsEnum type; /* Info about virDomainLoader */ virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */ }; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6008aeb..4dd9d14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17282,6 +17282,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, int virttype; /* virDomainVirtType */ virDomainCapsPtr domCaps = NULL; int arch = virArchFromHost(); /* virArch */ + virQEMUDriverConfigPtr cfg = NULL; + size_t i;
virCheckFlags(0, ret);
@@ -17356,8 +17358,30 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
virQEMUCapsFillDomainCaps(domCaps, qemuCaps);
+ cfg = virQEMUDriverGetConfig(driver); + for (i = 0; i < cfg->nloader; i++) { + char *filename = cfg->loader[i]; + + if (access(filename, F_OK) == 0) { + if (VIR_REALLOC_N(domCaps->os.loader.values.values, + domCaps->os.loader.values.nvalues + 1) < 0) { + goto cleanup; + } + domCaps->os.loader.values.nvalues++; + + if (VIR_STRDUP(domCaps->os.loader.values.values[ + domCaps->os.loader.values.nvalues - 1], + filename) < 0) { + goto cleanup; + } + } else { + VIR_DEBUG("loader filename=%s doesn't exist", filename); + } + } +
My idea was to keep implementation on qemu_driver.c as small as possible and fill everything in virQEMUCapsFillDomainCaps() or its children.
ret = virDomainCapsFormat(domCaps); cleanup: + virObjectUnref(cfg); virObjectUnref(domCaps); virObjectUnref(qemuCaps); return ret; diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 9722772..e7f41d7 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -6,6 +6,7 @@ <vcpu max='255'/> <os supported='yes'> <loader supported='yes'> + <value>/foo/test</value> <enum name='type'> <value>rom</value> <value>pflash</value> diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index f240643..ddc4d02 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -28,16 +28,37 @@
#define VIR_FROM_THIS VIR_FROM_NONE
-typedef void (*virDomainCapsFill)(virDomainCapsPtr domCaps, - void *opaque); +typedef int (*virDomainCapsFill)(virDomainCapsPtr domCaps, + void *opaque);
#define SET_ALL_BITS(x) \ memset(&(x.values), 0xff, sizeof(x.values))
-static void +static int +fillValues(virDomainCapsValuesPtr values) +{ + int ret = -1; + + if (VIR_ALLOC_N(values->values, 1) < 0) { + goto cleanup; + } + values->nvalues = 1; + + if (VIR_STRDUP(values->values[0], "/foo/test") < 0) {
I know this is only tests, but @values->values is leaked if VIR_STRDUP() fails.
+ goto cleanup; + } + + ret = 0; + cleanup: + return ret; +} + +static int fillAll(virDomainCapsPtr domCaps, void *opaque ATTRIBUTE_UNUSED) { + int ret = -1; + virDomainCapsOSPtr os = &domCaps->os; virDomainCapsLoaderPtr loader = &os->loader; virDomainCapsDeviceDiskPtr disk = &domCaps->disk; @@ -49,6 +70,9 @@ fillAll(virDomainCapsPtr domCaps, loader->device.supported = true; SET_ALL_BITS(loader->type); SET_ALL_BITS(loader->readonly); + if (fillValues(&loader->values) < 0) { + goto cleanup; + }
disk->device.supported = true; SET_ALL_BITS(disk->diskDevice); @@ -60,12 +84,16 @@ fillAll(virDomainCapsPtr domCaps, SET_ALL_BITS(hostdev->subsysType); SET_ALL_BITS(hostdev->capsType); SET_ALL_BITS(hostdev->pciBackend); + + ret = 0; + cleanup: + return ret; }
#ifdef WITH_QEMU # include "testutilsqemu.h" -static void +static int fillQemuCaps(virDomainCapsPtr domCaps, void *opaque) { @@ -82,6 +110,8 @@ fillQemuCaps(virDomainCapsPtr domCaps, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM, VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO); + + return 0; } #endif /* WITH_QEMU */
@@ -94,16 +124,19 @@ buildVirDomainCaps(const char *emulatorbin, virDomainCapsFill fillFunc, void *opaque) { - virDomainCapsPtr domCaps; + virDomainCapsPtr domCaps, ret = NULL;
if (!(domCaps = virDomainCapsNew(emulatorbin, machine, arch, type))) goto cleanup;
- if (fillFunc) - fillFunc(domCaps, opaque); + if (fillFunc && fillFunc(domCaps, opaque) < 0) { + virObjectUnref(domCaps); + goto cleanup; + }
+ ret = domCaps; cleanup: - return domCaps; + return ret; }
struct test_virDomainCapsFormatData {
The only concern I have is: if we ever allow <loader/> in domain XML to have children (and we've done that in other cases), what will happen to the <loader/> in domain capabilities XML? Let me update the patch and propose v2. Michal
participants (3)
-
Cole Robinson -
Laszlo Ersek -
Michal Privoznik