[libvirt] [PATCH] LXC: Fix the output units of ram filesystem size

[Daniel P. Berrange]

On Wed, Oct 09, 2013 at 11:40:35AM +0200, Ján Tomko wrote:
> On 10/08/2013 07:43 PM, Eric Blake wrote:
> > On 10/08/2013 11:28 AM, Ján Tomko wrote:
> >> Commit 76b644c added support for adding RAM filesystems to LXC
> >> domains, with a syntax of:
> >> <source usage='10' unit='MiB'/>
> >> where default unit would be KiB per documentation, however the
> >> XML parser treated sizes without units as bytes.
> >>
> >> When formatting the XML, this was divided by 1024, but the KiB units
> >> were put inside the 'units' attribute, as opposed to the 'unit'
> >> attribute the parser looks for.
> >>
> >> The code generating the mount options assumed the size in the domain
> >> definition to be in KiB, despite it being parsed as B. This worked
> >> as long as exaclty one re-format of the XML happened (for domains that
> >> were just created).
> >>
> >> Change the XML output to bytes and fix the documentation.
> > 
> > We still have libvirt in the wild that outputs bogus units= in the XML;
> > that should still pass the RelaxNG grammar even if it is otherwise ignored.
> > 
> >>          <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 in bytes.
> > 
> > If the RelaxNG is fixed to parse the broken output, then this could
> > document that <code>units</code> is ignored.
> > 
> 
> >> +++ b/src/conf/domain_conf.c
> >> @@ -14764,8 +14764,8 @@ virDomainFSDefFormat(virBufferPtr buf,
> >>          break;
> >>  
> >>      case VIR_DOMAIN_FS_TYPE_RAM:
> >> -        virBufferAsprintf(buf, "      <source usage='%lld' units='KiB'/>\n",
> >> -                          def->usage / 1024);
> >> +        virBufferAsprintf(buf, "      <source usage='%lld' unit='B'/>\n",
> >> +                          def->usage);
> > 
> > Wait.  This says older libvirt was outputting k, not bytes.  If that's
> > true, then we MUST continue to output k.
> > 
> 
> I agree. Even though the number was output in to MiB, GiB, ... after the
> definition was copied, but the output was in KiB for domains that had a chance
> of working correclty (transient and freshly-defined persistent domains seem to
> be).
> 
> >> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> >> index b1f429c..7c722cc 100644
> >> --- a/src/lxc/lxc_container.c
> >> +++ b/src/lxc/lxc_container.c
> >> @@ -1428,7 +1428,7 @@ static int lxcContainerMountFSTmpfs(virDomainFSDefPtr fs,
> >>      VIR_DEBUG("usage=%lld sec=%s", fs->usage, sec_mount_options);
> >>  
> >>      if (virAsprintf(&data,
> >> -                    "size=%lldk%s", fs->usage, sec_mount_options) < 0)
> >> +                    "size=%lld%s", fs->usage, sec_mount_options) < 0)
> > 
> > I'm also not convince on this change - if the XML contains k by default,
> > then we must continue to pass k to the mount command.
> > 
> 
> This does not depend on the XML default, but on the internal unit.
> 
> Libvirtd outputs the size in KiB, but since 'units' is ignored by the parser,
> libvirt_lxc just puts the raw number in 'usage', making KiB its internal unit.
> 
> If we fix the formatter and/or parser to correctly read the value it just
> printed, we will need to either get rid of this 'k' or store it in KiB internally.


This code was added for the benefit of libvirt-sandbox and that is the
main (possibly only) user of this feature currently.

It passes in XML that says

    <filesystem type="ram" accessmode="passthrough">
      <source usage="10485760" units="bytes"/>
      <target dir="/run"/>
    </filesystem>

and this translates to a sandbox with a 10MB mount

  # cat /proc/mounts  | grep /run
  tmpfs /run tmpfs rw,context=system_u:object_r:svirt_sandbox_file_t:s0,nosuid,nodev,relatime,size=10240k 0 0


And this is reflected in the output XML

    <filesystem type='ram' accessmode='passthrough'>
      <source usage='10240' units='KiB'/>
      <target dir='/run'/>
    </filesystem>


So IMHO we need to fix the parser to honour the 'units' attribute.

Regards,
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 :|