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

[Peter Krempa]

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