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