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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list