virNodeInfo.memory is in KiB

[libvirt] [PATCH libvirt-python] virNodeInfo.memory is in KiB

Posted by Philipp Hahn 5 years, 11 months ago

but the Python library does an extra left shift of 10 bits returning MiB
instead:

> # cat y.c
> #include <stdlib.h>
> #include <stdio.h>
> #include <libvirt.h>
> int main(void) {
>         virConnectPtr conn = virConnectOpen("qemu:///system");
>         virNodeInfo info;
>         int rv = virNodeGetInfo(conn, &info);
>         printf("%ld\n", info.memory);
>         return rv;
> }
> # gcc y.c -I/usr/include/libvirt -lvirt
> # ./a.out
> 4041088

> # python -c 'import libvirt;c=libvirt.open("qemu:///system");print(c.getInfo()[1])'
> 3946

Fixes: 197153c6
Signed-off-by: Philipp Hahn <hahn@univention.de>
---
 libvirt-override.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index f7b2f6b..616fa1c 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -2740,7 +2740,7 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED,
     VIR_PY_LIST_SET_GOTO(py_retval, 0,
                          libvirt_constcharPtrWrap(&info.model[0]), error);
     VIR_PY_LIST_SET_GOTO(py_retval, 1,
-                         libvirt_longWrap((long) info.memory >> 10), error);
+                         libvirt_longWrap((long) info.memory), error);
     VIR_PY_LIST_SET_GOTO(py_retval, 2, libvirt_intWrap((int) info.cpus), error);
     VIR_PY_LIST_SET_GOTO(py_retval, 3, libvirt_intWrap((int) info.mhz), error);
     VIR_PY_LIST_SET_GOTO(py_retval, 4, libvirt_intWrap((int) info.nodes), error);
-- 
2.11.0

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

Re: [libvirt] [PATCH libvirt-python] virNodeInfo.memory is in KiB

Posted by Daniel P. Berrangé 5 years, 11 months ago

On Wed, Dec 05, 2018 at 01:01:13PM +0100, Philipp Hahn wrote:
> but the Python library does an extra left shift of 10 bits returning MiB
> instead:
> 
> > # cat y.c
> > #include <stdlib.h>
> > #include <stdio.h>
> > #include <libvirt.h>
> > int main(void) {
> >         virConnectPtr conn = virConnectOpen("qemu:///system");
> >         virNodeInfo info;
> >         int rv = virNodeGetInfo(conn, &info);
> >         printf("%ld\n", info.memory);
> >         return rv;
> > }
> > # gcc y.c -I/usr/include/libvirt -lvirt
> > # ./a.out
> > 4041088
> 
> > # python -c 'import libvirt;c=libvirt.open("qemu:///system");print(c.getInfo()[1])'
> > 3946
> 
> Fixes: 197153c6

Not sure why you're quoting that commit has as it is unrelated.

This use of MB is from pretty much day 1 in 506fb7d8

> Signed-off-by: Philipp Hahn <hahn@univention.de>
> ---
>  libvirt-override.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libvirt-override.c b/libvirt-override.c
> index f7b2f6b..616fa1c 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -2740,7 +2740,7 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED,
>      VIR_PY_LIST_SET_GOTO(py_retval, 0,
>                           libvirt_constcharPtrWrap(&info.model[0]), error);
>      VIR_PY_LIST_SET_GOTO(py_retval, 1,
> -                         libvirt_longWrap((long) info.memory >> 10), error);
> +                         libvirt_longWrap((long) info.memory), error);

We can't change this as it would break every single existing user of this
API which have been written to expect this to be a MB value.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH libvirt-python] virNodeInfo.memory is in KiB

Posted by Philipp Hahn 5 years, 11 months ago

Hello Daniel,


Am 05.12.18 um 13:10 schrieb Daniel P. Berrangé:
> On Wed, Dec 05, 2018 at 01:01:13PM +0100, Philipp Hahn wrote:
>> but the Python library does an extra left shift of 10 bits returning MiB
>> instead:
...
>>> # ./a.out
>>> 4041088
>>
>>> # python -c 'import libvirt;c=libvirt.open("qemu:///system");print(c.getInfo()[1])'
>>> 3946
>>
>> Fixes: 197153c6
> 
> Not sure why you're quoting that commit has as it is unrelated.
> 
> This use of MB is from pretty much day 1 in 506fb7d8

Yes, you're right, that was the wrong commit hash and 506fb7d8 is the
correct one.

>> Signed-off-by: Philipp Hahn <hahn@univention.de>
>> ---
>>  libvirt-override.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libvirt-override.c b/libvirt-override.c
>> index f7b2f6b..616fa1c 100644
>> --- a/libvirt-override.c
>> +++ b/libvirt-override.c
>> @@ -2740,7 +2740,7 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED,
>>      VIR_PY_LIST_SET_GOTO(py_retval, 0,
>>                           libvirt_constcharPtrWrap(&info.model[0]), error);
>>      VIR_PY_LIST_SET_GOTO(py_retval, 1,
>> -                         libvirt_longWrap((long) info.memory >> 10), error);
>> +                         libvirt_longWrap((long) info.memory), error);
> 
> We can't change this as it would break every single existing user of this
> API which have been written to expect this to be a MB value.

Okay. This is already documented in libvirt-override-api.xml:

> 102     <function name='virNodeGetInfo' file='python'>                                                                                                                             
> 103       <info>Extract hardware information about the Node. Note that the memory size is reported in MiB instead of KiB.</info>

I was only looking at the C-API documentation.
So my patch can go to >/dev/null.

Sorry for the noise.

Philipp

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