Re: [PATCHv5 05/13] Add compatibility attribute to memballoon

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

 



On Wed, Aug 24, 2016 at 00:20:47 +0200, Ján Tomko wrote:
> A new attribute to alter the virtio revision:
> <memballoon model='virtio'>
>   <driver compatibility='transitional'/>
> </memballoon>
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1227354
> ---
>  docs/formatdomain.html.in                          |  8 ++++
>  docs/schemas/domaincommon.rng                      | 15 ++++++
>  src/conf/domain_conf.c                             | 40 ++++++++++++++++
>  src/conf/domain_conf.h                             | 10 ++++
>  .../qemuxml2argv-virtio-revision.xml               | 53 ++++++++++++++++++++++
>  .../qemuxml2xmlout-virtio-revision.xml             | 53 ++++++++++++++++++++++
>  tests/qemuxml2xmltest.c                            |  2 +
>  7 files changed, 181 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index bfbb0f2..56dddbd 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -6373,6 +6373,7 @@ qemu-kvm -net nic,model=? /dev/null
>      &lt;memballoon model='virtio'&gt;
>        &lt;address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/&gt;
>        &lt;stats period='10'/&gt;
> +      &lt;driver compatibility='transitional'/&gt;

Hmm, this is a though one. Since virtio is not entirely meant to be qemu
specific but rather a standard the setting here makes sense.

>      &lt;/memballoon&gt;
>    &lt;/devices&gt;
>  &lt;/domain&gt;</pre>
> @@ -6417,6 +6418,13 @@ qemu-kvm -net nic,model=? /dev/null
>            <span class='since'>Since 1.1.1, requires QEMU 1.5</span>
>          </p>
>        </dd>
> +      <dt><code>driver</code></dt>
> +      <dd>
> +        The <code>compatibility</code> attribute can be used to specify the
> +        compatibility of virtio devices. Allowed values are <code>legacy</code>,
> +        <code>transitional</code> and <code>modern</code>.
> +        <span class="since">Since 2.2.0</span>.

I'd like to see a more specific description of the individual values.
Since virtio is meant to be a standard, we should describe the vales a
bit more IMO.

> +      </dd>
>      </dl>
>      <h4><a name="elementsRng">Random number generator device</a></h4>
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 052f28c..8ed4b9d 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3612,6 +3612,11 @@
>              </attribute>
>            </element>
>          </optional>
> +        <optional>
> +          <element name="driver">
> +            <ref name="compatibility"/>
> +          </element>
> +        </optional>
>        </interleave>
>      </element>
>    </define>
> @@ -4830,6 +4835,16 @@
>      </element>
>    </define>
>  
> +  <define name="compatibility">

I'd add a <optional> block here since the attribute should not be
mandatory.

> +    <attribute name="compatibility">
> +      <choice>
> +        <value>legacy</value>
> +        <value>transitional</value>
> +        <value>modern</value>
> +      </choice>
> +    </attribute>
> +  </define>
> +
>    <define name="usbmaster">
>      <element name="master">
>        <attribute name="startport">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1e694fd..53c3453 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -834,6 +834,13 @@ VIR_ENUM_IMPL(virDomainLoader,
>                "rom",
>                "pflash")
>  
> +VIR_ENUM_IMPL(virDomainDriverCompatibility,
> +              VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST,
> +              "default",
> +              "legacy",
> +              "transitional",
> +              "modern")
> +
>  /* Internal mapping: subset of block job types that can be present in
>   * <mirror> XML (remaining types are not two-phase). */
>  VIR_ENUM_DECL(virDomainBlockJob)
> @@ -1059,6 +1066,31 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr xmlopt)
>      return &xmlopt->ns;
>  }
>  
> +static int
> +virDomainDriverCompatibilityParseXML(xmlXPathContextPtr ctxt,
> +                                     virDomainDriverCompatibility *res)
> +{
> +    char *str = NULL;
> +    int ret = -1;
> +    int val;
> +
> +    if (!(str = virXPathString("string(./driver/@compatibility)", ctxt)))
> +        return 0;
> +
> +    if ((val = virDomainDriverCompatibilityTypeFromString(str)) < 0) {

This still allows to use the "default" setting which is not documented
though.

> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("unable to parse the compatibility attribute"));
> +        goto cleanup;
> +    }
> +
> +    *res = val;
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(str);
> +    return ret;
> +}
> +
>  
>  void
>  virBlkioDeviceArrayClear(virBlkioDevicePtr devices,
> @@ -12065,6 +12097,9 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
>      else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
>          goto error;
>  
> +    if (virDomainDriverCompatibilityParseXML(ctxt, &def->compatibility) < 0)
> +        goto error;
> +
>   cleanup:
>      VIR_FREE(model);
>      VIR_FREE(deflate);
> @@ -21506,6 +21541,11 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>          return -1;
>      }
>  
> +    if (def->compatibility) {

"default" is not formatted though.

> +        virBufferAsprintf(&childrenBuf, "<driver compatibility='%s'/>\n",
> +                          virDomainDriverCompatibilityTypeToString(def->compatibility));
> +    }
> +
>      if (!virBufferUse(&childrenBuf)) {
>          virBufferAddLit(buf, "/>\n");
>      } else {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8b26724..ac9f552 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -154,6 +154,14 @@ typedef virDomainTPMDef *virDomainTPMDefPtr;
>  typedef struct _virDomainIOMMUDef virDomainIOMMUDef;
>  typedef virDomainIOMMUDef *virDomainIOMMUDefPtr;
>  
> +typedef enum {
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_DEFAULT,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_LEGACY,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_TRANSITIONAL,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_MODERN,
> +    VIR_DOMAIN_DRIVER_COMPATIBILITY_LAST,
> +} virDomainDriverCompatibility;
> +
>  /* Flags for the 'type' field in virDomainDeviceDef */
>  typedef enum {
>      VIR_DOMAIN_DEVICE_NONE = 0,
> @@ -1546,6 +1554,7 @@ struct _virDomainMemballoonDef {
>      virDomainDeviceInfo info;
>      int period; /* seconds between collections */
>      int autodeflate; /* enum virTristateSwitch */
> +    virDomainDriverCompatibility compatibility;
>  };
>  
>  struct _virDomainNVRAMDef {
> @@ -3022,6 +3031,7 @@ VIR_ENUM_DECL(virDomainTPMBackend)
>  VIR_ENUM_DECL(virDomainMemoryModel)
>  VIR_ENUM_DECL(virDomainMemoryBackingModel)
>  VIR_ENUM_DECL(virDomainIOMMUModel)
> +VIR_ENUM_DECL(virDomainDriverCompatibility)
>  /* from libvirt.h */
>  VIR_ENUM_DECL(virDomainState)
>  VIR_ENUM_DECL(virDomainNostateReason)

ACK if you clarify the docs from me. I think this is the least worst
approach we can use and still hide the very weird tuple of qemu
arguments used to configure this.

Peter

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]