[libvirt] [libvirt-python][PATCH] sanitytest: Sanitize VIR_TYPED_PARAM_*
This test checks whether all enums that we produce in libvirt.py have some reasonable value. But because of some crazy backcompat we have the following in libvirt-domain.h: VIR_DOMAIN_SCHED_FIELD_UINT = VIR_TYPED_PARAM_UINT VIR_DOMAIN_SCHED_FIELD_ULLONG = VIR_TYPED_PARAM_ULLONG with repetitions for other types. Now, when generating libvirt.py those values are special cased and thus assigned correct integer values. But sanitytest is missing the special treatment of VIR_TYPED_PARAM_* and therefore produces following error: /usr/bin/python sanitytest.py build/lib.linux-x86_64-2.7 /home/jenkins/build/libvirt/share/libvirt/api/libvirt-api.xml Cannot get a value of enum VIR_TYPED_PARAM_UINT (originally VIR_DOMAIN_SCHED_FIELD_UINT) Cannot get a value of enum VIR_TYPED_PARAM_ULLONG (originally VIR_DOMAIN_SCHED_FIELD_ULLONG) While the test is technically correct, "VIR_TYPED_PARAM_UINT" is not an integer value, it's a name for an enum value. Yet again, special handling is missing here, as VIR_DOMAIN_SCHED_FIELD_* has correct integer value in libvirt.py. Same applies for VIR_DOMAIN_BLKIO_PARAM_* and VIR_DOMAIN_MEMORY_PARAM_* which are fixed by this too. This has been exposed by previous commit of 3026a0593bd040 which started to generate values for symbols from libvirt-common.h too. The symbols I'm mentioning above live just in that very file. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- sanitytest.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) mode change 100644 => 100755 sanitytest.py diff --git a/sanitytest.py b/sanitytest.py old mode 100644 new mode 100755 index faabccb..23163f1 --- a/sanitytest.py +++ b/sanitytest.py @@ -22,6 +22,21 @@ def get_libvirt_api_xml_path(): sys.exit(proc.returncode) return stdout.splitlines()[0] +def sanitize_enum_val(value): + if value == 'VIR_TYPED_PARAM_INT': + value = 1 + elif value == 'VIR_TYPED_PARAM_UINT': + value = 2 + elif value == 'VIR_TYPED_PARAM_LLONG': + value = 3 + elif value == 'VIR_TYPED_PARAM_ULLONG': + value = 4 + elif value == 'VIR_TYPED_PARAM_DOUBLE': + value = 5 + elif value == 'VIR_TYPED_PARAM_BOOLEAN': + value = 6 + return value + # Path to the libvirt API XML file if len(sys.argv) >= 3: xml = sys.argv[2] @@ -66,7 +81,7 @@ for n in set: for n in second_pass: typ = n.attrib['type'] name = n.attrib['name'] - val = n.attrib['value'] + val = sanitize_enum_val(n.attrib['value']) for v in enumvals.values(): if val in v: -- 2.7.3
On Fri, Apr 22, 2016 at 11:09:52 +0200, Michal Privoznik wrote:
This test checks whether all enums that we produce in libvirt.py have some reasonable value. But because of some crazy backcompat we have the following in libvirt-domain.h:
VIR_DOMAIN_SCHED_FIELD_UINT = VIR_TYPED_PARAM_UINT VIR_DOMAIN_SCHED_FIELD_ULLONG = VIR_TYPED_PARAM_ULLONG
Yes that is true. We have this also for basically every API that takes virDomainModificationImpact flags combined with something private.
with repetitions for other types. Now, when generating libvirt.py those values are special cased and thus assigned correct integer values. But sanitytest is missing the special treatment of VIR_TYPED_PARAM_* and therefore produces following error:
/usr/bin/python sanitytest.py build/lib.linux-x86_64-2.7 /home/jenkins/build/libvirt/share/libvirt/api/libvirt-api.xml Cannot get a value of enum VIR_TYPED_PARAM_UINT (originally VIR_DOMAIN_SCHED_FIELD_UINT) Cannot get a value of enum VIR_TYPED_PARAM_ULLONG (originally VIR_DOMAIN_SCHED_FIELD_ULLONG)
This was caused by commit f5d9c5d00cfc0c in libvirt.git. The commit moved the enum that declares VIR_TYPED_PARAM_* into libvirt-common.h which was NOT handled by apibuild.py from libvirt.git. sanitytest.py (from libvirt-python.git) then takes 'libvirt-api.xml' wich was generated by apibuild.xml (from libvirt.git) and verifies that all enum fields are represented in the python binding.
While the test is technically correct, "VIR_TYPED_PARAM_UINT" is not an integer value, it's a name for an enum value. Yet again, special handling is missing here, as VIR_DOMAIN_SCHED_FIELD_* has correct integer value in libvirt.py.
And the test is able to correctly infer the type in a second pass if you have VIR_TYPED_PARAM_* macros present in libvirt-api.xml.
Same applies for VIR_DOMAIN_BLKIO_PARAM_* and VIR_DOMAIN_MEMORY_PARAM_* which are fixed by this too.
This has been exposed by previous commit of 3026a0593bd040 which started to generate values for symbols from libvirt-common.h too. The symbols I'm mentioning above live just in that very file.
No. This is not true. That commit is part of two fixes to libvirt.git and libvirt-python.git that are set to fix the very issue
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- sanitytest.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) mode change 100644 => 100755 sanitytest.py
diff --git a/sanitytest.py b/sanitytest.py old mode 100644 new mode 100755 index faabccb..23163f1 --- a/sanitytest.py +++ b/sanitytest.py @@ -22,6 +22,21 @@ def get_libvirt_api_xml_path(): sys.exit(proc.returncode) return stdout.splitlines()[0]
+def sanitize_enum_val(value): + if value == 'VIR_TYPED_PARAM_INT': + value = 1 + elif value == 'VIR_TYPED_PARAM_UINT': + value = 2 + elif value == 'VIR_TYPED_PARAM_LLONG': + value = 3 + elif value == 'VIR_TYPED_PARAM_ULLONG': + value = 4 + elif value == 'VIR_TYPED_PARAM_DOUBLE': + value = 5 + elif value == 'VIR_TYPED_PARAM_BOOLEAN': + value = 6 + return value + # Path to the libvirt API XML file if len(sys.argv) >= 3: xml = sys.argv[2] @@ -66,7 +81,7 @@ for n in set: for n in second_pass: typ = n.attrib['type'] name = n.attrib['name'] - val = n.attrib['value'] + val = sanitize_enum_val(n.attrib['value'])
for v in enumvals.values(): if val in v:
NACK, this works the error around by doing a local lookup rather than fixing the real issue which was already fixed by commit 99283874733b809a962df51c33ede4446f70bfe9 in libvirt.git. Peter
On 22.04.2016 13:55, Peter Krempa wrote:
On Fri, Apr 22, 2016 at 11:09:52 +0200, Michal Privoznik wrote:
This test checks whether all enums that we produce in libvirt.py have some reasonable value. But because of some crazy backcompat we have the following in libvirt-domain.h:
VIR_DOMAIN_SCHED_FIELD_UINT = VIR_TYPED_PARAM_UINT VIR_DOMAIN_SCHED_FIELD_ULLONG = VIR_TYPED_PARAM_ULLONG
Yes that is true. We have this also for basically every API that takes virDomainModificationImpact flags combined with something private.
But there's a huge difference between these two. While virDomainModificationImpact flags are exposed in python, typed params are not due to our special handling (associative arrays). In other words, if you have two enums, you can map one to another and our sanity check script would be happy about. Except typed params.
with repetitions for other types. Now, when generating libvirt.py those values are special cased and thus assigned correct integer values. But sanitytest is missing the special treatment of VIR_TYPED_PARAM_* and therefore produces following error:
/usr/bin/python sanitytest.py build/lib.linux-x86_64-2.7 /home/jenkins/build/libvirt/share/libvirt/api/libvirt-api.xml Cannot get a value of enum VIR_TYPED_PARAM_UINT (originally VIR_DOMAIN_SCHED_FIELD_UINT) Cannot get a value of enum VIR_TYPED_PARAM_ULLONG (originally VIR_DOMAIN_SCHED_FIELD_ULLONG)
This was caused by commit f5d9c5d00cfc0c in libvirt.git. The commit moved the enum that declares VIR_TYPED_PARAM_* into libvirt-common.h which was NOT handled by apibuild.py from libvirt.git.
sanitytest.py (from libvirt-python.git) then takes 'libvirt-api.xml' wich was generated by apibuild.xml (from libvirt.git) and verifies that all enum fields are represented in the python binding.
While the test is technically correct, "VIR_TYPED_PARAM_UINT" is not an integer value, it's a name for an enum value. Yet again, special handling is missing here, as VIR_DOMAIN_SCHED_FIELD_* has correct integer value in libvirt.py.
And the test is able to correctly infer the type in a second pass if you have VIR_TYPED_PARAM_* macros present in libvirt-api.xml.
Same applies for VIR_DOMAIN_BLKIO_PARAM_* and VIR_DOMAIN_MEMORY_PARAM_* which are fixed by this too.
This has been exposed by previous commit of 3026a0593bd040 which started to generate values for symbols from libvirt-common.h too. The symbols I'm mentioning above live just in that very file.
No. This is not true. That commit is part of two fixes to libvirt.git and libvirt-python.git that are set to fix the very issue
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- sanitytest.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) mode change 100644 => 100755 sanitytest.py
diff --git a/sanitytest.py b/sanitytest.py old mode 100644 new mode 100755 index faabccb..23163f1 --- a/sanitytest.py +++ b/sanitytest.py @@ -22,6 +22,21 @@ def get_libvirt_api_xml_path(): sys.exit(proc.returncode) return stdout.splitlines()[0]
+def sanitize_enum_val(value): + if value == 'VIR_TYPED_PARAM_INT': + value = 1 + elif value == 'VIR_TYPED_PARAM_UINT': + value = 2 + elif value == 'VIR_TYPED_PARAM_LLONG': + value = 3 + elif value == 'VIR_TYPED_PARAM_ULLONG': + value = 4 + elif value == 'VIR_TYPED_PARAM_DOUBLE': + value = 5 + elif value == 'VIR_TYPED_PARAM_BOOLEAN': + value = 6 + return value + # Path to the libvirt API XML file if len(sys.argv) >= 3: xml = sys.argv[2] @@ -66,7 +81,7 @@ for n in set: for n in second_pass: typ = n.attrib['type'] name = n.attrib['name'] - val = n.attrib['value'] + val = sanitize_enum_val(n.attrib['value'])
for v in enumvals.values(): if val in v:
NACK, this works the error around by doing a local lookup rather than fixing the real issue which was already fixed by commit 99283874733b809a962df51c33ede4446f70bfe9 in libvirt.git.
I don't think it is fixed. Have you tried running: ./setup.py check with the latest libvirt HEAD installed (which already contains the commit you're referring to)? Point is, build of python bindings is broken, so either we fix it or revert whatever patch caused it. Michal
On Sun, Apr 24, 2016 at 10:35:30 +0200, Michal Privoznik wrote:
On 22.04.2016 13:55, Peter Krempa wrote:
On Fri, Apr 22, 2016 at 11:09:52 +0200, Michal Privoznik wrote:
This test checks whether all enums that we produce in libvirt.py have some reasonable value. But because of some crazy backcompat we have the following in libvirt-domain.h:
VIR_DOMAIN_SCHED_FIELD_UINT = VIR_TYPED_PARAM_UINT VIR_DOMAIN_SCHED_FIELD_ULLONG = VIR_TYPED_PARAM_ULLONG
Yes that is true. We have this also for basically every API that takes virDomainModificationImpact flags combined with something private.
But there's a huge difference between these two. While virDomainModificationImpact flags are exposed in python, typed params are not due to our special handling (associative arrays). In other
Not really. They were exposed in python and stopped to be once commit f5d9c5d00cfc0c moved them away into a file that was not handled. I explained it a few lines below [1].
words, if you have two enums, you can map one to another and our sanity check script would be happy about. Except typed params.
The script is happy as long as the first of the enums maps directly to integers or the second referenced enum maps directly to integers. Just the two passes. I also think we can't just stop exposing the VIR_TYPED_PARAM_* constants in python despite the fact that they are not really useful there.
with repetitions for other types. Now, when generating libvirt.py those values are special cased and thus assigned correct integer values. But sanitytest is missing the special treatment of VIR_TYPED_PARAM_* and therefore produces following error:
/usr/bin/python sanitytest.py build/lib.linux-x86_64-2.7 /home/jenkins/build/libvirt/share/libvirt/api/libvirt-api.xml Cannot get a value of enum VIR_TYPED_PARAM_UINT (originally VIR_DOMAIN_SCHED_FIELD_UINT) Cannot get a value of enum VIR_TYPED_PARAM_ULLONG (originally VIR_DOMAIN_SCHED_FIELD_ULLONG)
This was caused by commit f5d9c5d00cfc0c in libvirt.git. The commit moved the enum that declares VIR_TYPED_PARAM_* into libvirt-common.h which was NOT handled by apibuild.py from libvirt.git.
sanitytest.py (from libvirt-python.git) then takes 'libvirt-api.xml' wich was generated by apibuild.xml (from libvirt.git) and verifies that all enum fields are represented in the python binding.
[1]
While the test is technically correct, "VIR_TYPED_PARAM_UINT" is not an integer value, it's a name for an enum value. Yet again, special handling is missing here, as VIR_DOMAIN_SCHED_FIELD_* has correct integer value in libvirt.py.
And the test is able to correctly infer the type in a second pass if you have VIR_TYPED_PARAM_* macros present in libvirt-api.xml.
Same applies for VIR_DOMAIN_BLKIO_PARAM_* and VIR_DOMAIN_MEMORY_PARAM_* which are fixed by this too.
This has been exposed by previous commit of 3026a0593bd040 which started to generate values for symbols from libvirt-common.h too. The symbols I'm mentioning above live just in that very file.
No. This is not true. That commit is part of two fixes to libvirt.git and libvirt-python.git that are set to fix the very issue
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
[...]
for v in enumvals.values(): if val in v:
NACK, this works the error around by doing a local lookup rather than fixing the real issue which was already fixed by commit 99283874733b809a962df51c33ede4446f70bfe9 in libvirt.git.
I don't think it is fixed. Have you tried running:
./setup.py check
I presume you meant ./setup.py test, since that is the part that is running sanitytest.py. And I sure did run it since I was fixing the exact issue as this patch is supposed to fix: http://www.redhat.com/archives/libvir-list/2016-April/msg01464.html Here's my output if you don't trust me: # /home/pipo/libvirt.git/run ./setup.py check running check # /home/pipo/libvirt.git/run ./setup.py test running test /usr/bin/python sanitytest.py build/lib.linux-x86_64-2.7 /home/pipo/libvirt.git/docs/libvirt-api.xml /usr/bin/python /usr/bin/nosetests .. ---------------------------------------------------------------------- Ran 2 tests in 0.025s OK That is with current master branches of libvirt.git and libvirt-python.git as of writing this email.
with the latest libvirt HEAD installed (which already contains the commit you're referring to)?
Point is, build of python bindings is broken, so either we fix it or revert whatever patch caused it.
That would be basically the whole recent admin API work.
Michal
participants (2)
-
Michal Privoznik -
Peter Krempa