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

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.