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