[libvirt] [PATCHv4 2/3] conf: Add new xml elements for file memorybacking support

[Michal Privoznik]

On 13.12.2016 13:12, Jaroslav Safka wrote:
> This first change introduces new xml elements for file based
> memorybacking support and their parsing.
> It allows vhost-user to be used without hugepages.
> 
> New xml elements:
> <memoryBacking>
>   <source type="file|anonymous"/>
>   <access mode="shared|private"/>
>   <allocation mode="immediate|ondemand"/>
> </memoryBacking>
> ---
>  docs/formatdomain.html.in                          |   9 ++
>  docs/schemas/domaincommon.rng                      |  30 +++++
>  src/conf/domain_conf.c                             | 133 ++++++++++++++++-----
>  src/conf/domain_conf.h                             |  22 ++++
>  .../qemuxml2argv-memorybacking-set.xml             |  24 ++++
>  .../qemuxml2argv-memorybacking-unset.xml           |  24 ++++
>  .../qemuxml2xmlout-memorybacking-set.xml           |  32 +++++
>  .../qemuxml2xmlout-memorybacking-unset.xml         |  32 +++++
>  tests/qemuxml2xmltest.c                            |   3 +
>  9 files changed, 276 insertions(+), 33 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-set.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-memorybacking-unset.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-set.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-memorybacking-unset.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 9218eec..fc194bf 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -903,6 +903,9 @@
>      </hugepages>
>      <nosharepages/>
>      <locked/>
> +    <source type="file|anonymous"/>
> +    <access mode="shared|private"/>
> +    <allocation mode="immediate|ondemand"/>
>    </memoryBacking>
>    ...
>  </domain>
> @@ -942,6 +945,12 @@
>          most of the host's memory). Doing so may be dangerous to both the
>          domain and the host itself since the host's kernel may run out of
>          memory. <span class="since">Since 1.0.6</span></dd>
> +       <dt><code>source</code></dt>
> +       <dd>In this attribute you can switch to file memorybacking or keep default anonymous.</dd>
> +       <dt><code>access</code></dt>
> +       <dd>Specify if memory is shared or private. This can be overridden per numa node by <code>memAccess</code></dd>
> +       <dt><code>allocation</code></dt>
> +       <dd>Specify when allocate the memory</dd>
>      </dl>
>  
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index c004233..363ee2f 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -560,6 +560,36 @@
>                  <empty/>
>                </element>
>              </optional>
> +            <optional>
> +              <element name="source">
> +                <attribute name="type">
> +                  <choice>
> +                    <value>file</value>
> +                    <value>anonymous</value>
> +                  </choice>
> +                </attribute>
> +              </element>
> +            </optional>
> +            <optional>
> +              <element name="access">
> +                <attribute name="mode">
> +                  <choice>
> +                    <value>shared</value>
> +                    <value>private</value>
> +                  </choice>
> +                </attribute>
> +              </element>
> +            </optional>
> +            <optional>
> +              <element name="allocation">
> +                <attribute name="mode">
> +                  <choice>
> +                    <value>immediate</value>
> +                    <value>ondemand</value>
> +                  </choice>
> +                </attribute>
> +              </element>
> +            </optional>
>            </interleave>
>          </element>
>        </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b0bd38d..c54fc97 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -837,6 +837,16 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
>                "abort",
>                "pivot")
>  
> +VIR_ENUM_IMPL(virDomainMemorySource, VIR_DOMAIN_MEMORY_SOURCE_LAST,
> +              "none",
> +              "file",
> +              "anonymous")
> +
> +VIR_ENUM_IMPL(virDomainMemoryAllocation, VIR_DOMAIN_MEMORY_ALLOCATION_LAST,
> +              "none",
> +              "immediate",
> +              "ondemand")
> +
>  VIR_ENUM_IMPL(virDomainLoader,
>                VIR_DOMAIN_LOADER_TYPE_LAST,
>                "rom",
> @@ -16528,48 +16538,93 @@ virDomainDefParseXML(xmlDocPtr xml,
>      }
>      VIR_FREE(tmp);
>  
> -    if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("cannot extract hugepages nodes"));
> -        goto error;
> +    tmp = virXPathString("string(./memoryBacking/source/@type)", ctxt);
> +    if (tmp) {
> +        if ((def->mem.source = virDomainMemorySourceTypeFromString(tmp)) < 0) {

This will not fly. def->mem.source is typeof enum virDomainMemorySource
which can have values 0, 1 or 2. Now, should
virDomainMemorySourceTypeFromString() return an error (-1) (because of
unknown @type value), this gets squeezed into the enum range, which
always falls into 0-2 range. IOW, this condition will never be true.
That's why we have all those:

  int blah; /* enum virMyAwesomeEnum */

lines in the definition. Had the @source been an int, this starts
working. Except, we might want to disallow "none" and thus the condition
should be "<= 0". This also apply for the other two variables.

> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown memoryBacking/source/type '%s'"), tmp);
> +            goto error;
> +        }
> +        VIR_FREE(tmp);
>      }

ACK with that change.

Michal