12:28 p.m.
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1015689
---
docs/formatdomain.html.in | 5 ++--
src/conf/domain_conf.c | 4 ++--
src/lxc/lxc_container.c | 2 +-
tests/lxcxml2xmldata/lxc-filesystem-ram.xml | 33 ++++++++++++++++++++++++++
tests/lxcxml2xmloutdata/lxc-filesystem-ram.xml | 33 ++++++++++++++++++++++++++
tests/lxcxml2xmltest.c | 1 +
6 files changed, 73 insertions(+), 5 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..bfe0ae4 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 bytes, unless units
+ are specified by the attribute <code>unit</code>. Only used
by LXC driver.
<span class="since"> (since 0.9.13)</span></dd>
<dt><code>type='bind'</code></dt>
@@ -2279,7 +2280,7 @@
<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.
</dd>
<dt><code>target</code></dt>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0d63845..2c67966 100644
--- a/src/conf/domain_conf.c
+++ 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);
break;
}
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)
goto cleanup;
if (virFileMakePath(fs->dst) < 0) {
diff --git a/tests/lxcxml2xmldata/lxc-filesystem-ram.xml
b/tests/lxcxml2xmldata/lxc-filesystem-ram.xml
new file mode 100644
index 0000000..404429b
--- /dev/null
+++ b/tests/lxcxml2xmldata/lxc-filesystem-ram.xml
@@ -0,0 +1,33 @@
+<domain type='lxc'>
+ <name>demo</name>
+ <uuid>8369f1ac-7e46-e869-4ca5-759d51478066</uuid>
+ <memory unit='KiB'>500000</memory>
+ <currentMemory unit='KiB'>500000</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64'>exe</type>
+ <init>/bin/sh</init>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/libexec/libvirt_lxc</emulator>
+ <filesystem type='ram'>
+ <source usage='1048576'/>
+ <target dir='/mnt/mississippi'/>
+ </filesystem>
+ <filesystem type='ram'>
+ <source usage='1048576' unit='b'/>
+ <target dir='/mnt/titicaca'/>
+ </filesystem>
+ <filesystem type='ram'>
+ <source usage='1024' unit='KiB'/>
+ <target dir='/mnt/orinoco'/>
+ </filesystem>
+ <console type='pty'>
+ <target type='lxc' port='0'/>
+ </console>
+ </devices>
+</domain>
diff --git a/tests/lxcxml2xmloutdata/lxc-filesystem-ram.xml
b/tests/lxcxml2xmloutdata/lxc-filesystem-ram.xml
new file mode 100644
index 0000000..431b8cb
--- /dev/null
+++ b/tests/lxcxml2xmloutdata/lxc-filesystem-ram.xml
@@ -0,0 +1,33 @@
+<domain type='lxc'>
+ <name>demo</name>
+ <uuid>8369f1ac-7e46-e869-4ca5-759d51478066</uuid>
+ <memory unit='KiB'>500000</memory>
+ <currentMemory unit='KiB'>500000</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='x86_64'>exe</type>
+ <init>/bin/sh</init>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/libexec/libvirt_lxc</emulator>
+ <filesystem type='ram' accessmode='passthrough'>
+ <source usage='1048576' unit='B'/>
+ <target dir='/mnt/mississippi'/>
+ </filesystem>
+ <filesystem type='ram' accessmode='passthrough'>
+ <source usage='1048576' unit='B'/>
+ <target dir='/mnt/titicaca'/>
+ </filesystem>
+ <filesystem type='ram' accessmode='passthrough'>
+ <source usage='1048576' unit='B'/>
+ <target dir='/mnt/orinoco'/>
+ </filesystem>
+ <console type='pty'>
+ <target type='lxc' port='0'/>
+ </console>
+ </devices>
+</domain>
diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c
index 5846ab0..1692e4b 100644
--- a/tests/lxcxml2xmltest.c
+++ b/tests/lxcxml2xmltest.c
@@ -136,6 +136,7 @@ mymain(void)
DO_TEST("systemd");
DO_TEST("hostdev");
DO_TEST("disk-formats");
+ DO_TEST_DIFFERENT("filesystem-ram");
virObjectUnref(caps);
virObjectUnref(xmlopt);
--
1.8.1.5
12:43 p.m.
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.
break;
}
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:40 a.m.
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
4:49 a.m.
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 :|
4063
days inactive
4064
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko