[libvirt] [PATCHv2 05/15] xml: output memory unit for clarity

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120306/005fe272/attachment-0001.sig>