On 03/06/2012 08:14 AM, Peter Krempa wrote:
On 03/06/2012 01:34 AM, Eric Blake wrote:
Make it obvious to 'dumpxml' readers what unit we are using, since our default of KiB for memory (1024) differs from qemu's default of MiB; while we use bytes for storage.
Tests were updated via:
+++ b/docs/schemas/basictypes.rng @@ -140,8 +140,16 @@
<define name='unit'> <data type='string'> -<param name='pattern'>[kKmMgGtTpPeE]</param> +<param name='pattern'>(bytes)|[kKmMgGtTpPeE]</param>
Looking at this again. Don't you want to be able to specify the unit as "KiB" or just only as "k"?
Yes, and that gets fixed later in the series (see 6/15 and 10/15). This was the minimum change to get 'make check' to pass at this stage of the series before I start adding new parsing abilities in later patches. Also, note that domaincommon.rng _isn't_ using <ref name='unit'/> until patch 10/15; in this patch, domaincommon.rng is using an open-coded RNG that accepts _only_ <attribute name='unit'><value>KiB</value></attribute>.
- virBufferAsprintf(buf, "<currentMemory>%lu</currentMemory>\n", + virBufferAsprintf(buf, "<memory unit='KiB'>%lu</memory>\n", + def->mem.max_balloon); + virBufferAsprintf(buf, "<currentMemory unit='KiB'>%lu</currentMemory>\n", def->mem.cur_balloon);
I'm not quite sure about this. When we print the unit and somebody tries to use the xml as a template for a new machine or simply to change the memory allocation for the domain using virsh edit, he might be tempted to change the unit to mibibytes and expect the amount of memory to be used. Instead of that he will get that amount in kilobytes and no warning about this. We probably should parse the unit and at least warn the user about this happening.
Yes, that happens in 10/15.
With this patch applied to current upstream you will need to: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml index 65cd55d..b6bf1d4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml @@ -1,8 +1,8 @@ <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory>219136</memory> - <currentMemory>219136</currentMemory> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory>
Ah, new tests added in the meantime. Yes, when I rebase, I'll make sure things still work.
to pass the tests.
I'm not comfortable ACKing this one :(. I'd like to have some feedback from others on printing units without parsing them. Otherwise looks fine and passes the tests with that patch applied.
If anything, I can modify the commit message to mention that a later patch will parse units. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org