[libvirt] [PATCH] LXC: Fix the output units of ram filesystem size
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
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
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
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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko