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.
Jan