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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list