Re: [PATCHv2] LXC: Fix handling of RAM filesystem size units

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

 



On Wed, Oct 09, 2013 at 03:15:36PM +0200, Ján Tomko wrote:
> Since 76b644c when the support for RAM filesystems was introduced,
> libvirt accepted the following XML:
> <source usage='1024' unit='KiB'/>
> 
> This was parsed correctly and internally stored in bytes, but it
> was formatted as (with an extra 's'):
> <source usage='1024' units='KiB'/>
> When read again, this was treated as if the units were missing,
> meaning libvirt was unable to parse its own XML correctly.
> 
> The usage attribute was documented as being in KiB, but it was not
> scaled if the unit was missing. Transient domains still worked,
> because this was balanced by an extra 'k' in the mount options.
> 
> This patch:
> Outputs the units via the 'unit' attribute (fixing persistent domains),
> but accepts both 'unit' and 'units' (for compatibility with older
> libvirt and libvirt-sandbox).
> 
> Removes the extra 'k' from the tmpfs mount options, which is needed
> because now we parse our own XML correctly.
> 
> Changes the default unit to KiB to match documentation, fixing:
> https://bugzilla.redhat.com/show_bug.cgi?id=1015689
> ---
> v1: https://www.redhat.com/archives/libvir-list/2013-October/msg00361.html
> v2: keeps the output unit KiB
>     changes the default input unit to KiB
>     accepts 'units' as well as 'unit' in input XML
> 
>  docs/formatdomain.html.in                      |  6 ++--
>  docs/schemas/domaincommon.rng                  |  5 ++++
>  src/conf/domain_conf.c                         | 12 ++++++--
>  src/lxc/lxc_container.c                        |  2 +-
>  tests/domainschematest                         |  2 +-
>  tests/lxcxml2xmldata/lxc-filesystem-ram.xml    | 41 ++++++++++++++++++++++++++
>  tests/lxcxml2xmloutdata/lxc-filesystem-ram.xml | 41 ++++++++++++++++++++++++++
>  tests/lxcxml2xmltest.c                         |  1 +
>  8 files changed, 103 insertions(+), 7 deletions(-)
>  create mode 100644 tests/lxcxml2xmldata/lxc-filesystem-ram.xml
>  create mode 100644 tests/lxcxml2xmloutdata/lxc-filesystem-ram.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 3689399..dfd11bf 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2213,7 +2213,8 @@
>          <dd>
>            An in-memory filesystem, using memory from the host OS.
>            The source element has a single attribute <code>usage</code>
> -          which gives the memory usage limit in kibibytes. Only used
> +          which gives the memory usage limit in KiB, unless units
> +          are specified by the <code>unit</code> attribute. Only used
>            by LXC driver.
>            <span class="since"> (since 0.9.13)</span></dd>
>          <dt><code>type='bind'</code></dt>
> @@ -2279,7 +2280,8 @@
>          <code>name</code> attribute must be used with
>          <code>type='template'</code>, and the <code>dir</code> attribute must
>          be used with <code>type='mount'</code>. The <code>usage</code> attribute
> -        is used with <code>type='ram'</code> to set the memory limit in KB.
> +        is used with <code>type='ram'</code> to set the memory limit KiB, unless
> +        units are specified by the <code>unit</code> attribute.
>        </dd>
>  
>        <dt><code>target</code></dt>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 5c5301d..582d4c3 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1736,6 +1736,11 @@
>                    <ref name='unit'/>
>                  </attribute>
>                </optional>
> +              <optional>
> +                <attribute name='units'>
> +                  <ref name='unit'/>
> +                </attribute>
> +              </optional>
>                <empty/>
>              </element>
>            </interleave>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0d63845..0870047 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5917,6 +5917,7 @@ virDomainFSDefParseXML(xmlNodePtr node,
>      char *wrpolicy = NULL;
>      char *usage = NULL;
>      char *unit = NULL;
> +    char *units = NULL;
>  
>      ctxt->node = node;
>  
> @@ -5973,6 +5974,7 @@ virDomainFSDefParseXML(xmlNodePtr node,
>                  else if (def->type == VIR_DOMAIN_FS_TYPE_RAM) {
>                      usage = virXMLPropString(cur, "usage");
>                      unit = virXMLPropString(cur, "unit");
> +                    units = virXMLPropString(cur, "units");

We should just be dropping the 'unit' attribute - it was undocumented
and broken.

>                  }
>              } else if (!target &&
>                         xmlStrEqual(cur->name, BAD_CAST "target")) {
> @@ -6042,8 +6044,11 @@ virDomainFSDefParseXML(xmlNodePtr node,
>                             usage);
>              goto error;
>          }
> -        if (unit &&
> -            virScaleInteger(&def->usage, unit,
> +        /* Older libvirt only parsed 'unit' here, despite
> +         * printing it in the 'units' attribute.
> +         *
> +         * The default was bytes when 'unit' wasn't present */
> +        if (virScaleInteger(&def->usage, units ? units : unit,
>                              1024, ULLONG_MAX) < 0)
>              goto error;
>      }
> @@ -6066,6 +6071,7 @@ cleanup:
>      VIR_FREE(wrpolicy);
>      VIR_FREE(usage);
>      VIR_FREE(unit);
> +    VIR_FREE(units);
>      VIR_FREE(format);
>  
>      return def;
> @@ -14764,7 +14770,7 @@ virDomainFSDefFormat(virBufferPtr buf,
>          break;
>  
>      case VIR_DOMAIN_FS_TYPE_RAM:
> -        virBufferAsprintf(buf, "      <source usage='%lld' units='KiB'/>\n",
> +        virBufferAsprintf(buf, "      <source usage='%lld' unit='KiB'/>\n",
>                            def->usage / 1024);
>          break;

No, you should not be changing the attribute name here.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
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]