[libvirt] [libvirt-python][PATCH] sanitytest: Sanitize VIR_TYPED_PARAM_*

[Michal Privoznik]

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 at 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