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

[Peter Krempa]

On 03/06/2012 04:14 PM, 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:
>>
>> $ find tests/*data tests/*out -name '*.xml' | \
>> xargs sed -i
>> 's/<\(memory\|currentMemory\|hard_limit\|soft_limit\|min_guarantee\|swap_hard_limit\)>/<\1
>> unit='"'KiB'>/"
>> $ find tests/*data tests/*out -name '*.xml' | \
>> xargs sed -i 's/<\(capacity\|allocation\|available\)>/<\1
>> unit='"'bytes'>/"
>>
>> followed by a few fixes for the stragglers.
>>
>> * docs/schemas/basictypes.rng (unit): Add 'bytes'.
>> (scaledInteger): New define.
>> * docs/schemas/storagevol.rng (sizing): Use it.
>> * docs/schemas/storagepool.rng (sizing): Likewise.
>> * docs/schemas/domaincommon.rng (memoryKBElement): New define; use
>> for memory elements.
>> * src/conf/storage_conf.c (virStoragePoolDefFormat)
>> (virStorageVolDefFormat): Likewise.
>> * src/conf/domain_conf.h (_virDomainDef): Document unit used
>> internally.
>> * src/conf/storage_conf.h (_virStoragePoolDef, _virStorageVolDef):
>> Likewise.
>> * tests/*data/*.xml: Update all tests.
>> * tests/*out/*.xml: Likewise.
>> * tests/define-dev-segfault: Likewise.
>> * tests/openvzutilstest.c (testReadNetworkConf): Likewise.
>> * tests/qemuargv2xmltest.c (blankProblemElements): Likewise.
>> ---
>>
>> corresponds to memory v1 1/3;
>> v2: also output units for storage, use 'unit=' not 'units=', use
>> common RNG
>>
>> Portions of this patch elided to reduce mail size; see cover letter
>> for git repo to see entire patch.
>
> I didn't read the cover letter thoroughly enough to notice the repo, at
> first :(
>
>>
>> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
>> index 4f16fa7..a50349c 100644
>> --- a/docs/schemas/basictypes.rng
>> +++ 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"?
>
>> </data>
>> </define>
>> +<define name='scaledInteger'>
>> +<optional>
>> +<attribute name='unit'>
>> +<ref name='unit'/>
>> +</attribute>
>> +</optional>
>> +<ref name='unsignedLong'/>
>> +</define>
>>
>> </grammar>
>>
>> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index f9654f1..331d923 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -11638,8 +11638,9 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>> xmlIndentTreeOutput = oldIndentTreeOutput;
>> }
>>
>> - virBufferAsprintf(buf, "<memory>%lu</memory>\n", def->mem.max_balloon);
>> - 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.
>
> 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>
> <vcpu>1</vcpu>
> <os>
> <type arch='i686' machine='pc'>hvm</type>
>
>
> 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.
>
> Peter

After reviewing 10/15 I'm now comfortable with ACKing this one if 10/15 
gets in :)

Peter

>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list