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

[Peter Krempa]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160425/983d6d46/attachment-0001.sig>