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