[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] domaincaps: Expose UEFI binary path, if it exists



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 @@
>    ...
>    &lt;os supported='yes'&gt;
>      &lt;loader supported='yes'&gt;
> +      &lt;value&gt;/usr/share/OVMF/OVMF_CODE.fd&lt;/value&gt;
>        &lt;enum name='type'&gt;
>          &lt;value&gt;rom&lt;/value&gt;
>          &lt;value&gt;pflash&lt;/value&gt;
> @@ -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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]