[libvirt] [PATCH] libvirt-override: fix flags mutually exclusize

[Daniel P. Berrange]

On Fri, Feb 17, 2017 at 03:22:12PM +0100, Erik Skultety wrote:
> On Fri, Feb 17, 2017 at 07:19:25PM +0800, Jie Wang wrote:
> 
> Just a nit that we tend to prefix the patch with [libvirt-python] instead of
> just [libvirt] so it's absolutely clear which repository you're sending the
> patch against and in this case it's a python bindings fix.
> 
> > As virDomainGetBlkioParameters is called in libvirt_virDomainSetBlkioParameters,
> > it will result in the two flags 'VIR_DOMAIN_AFFECT_LIVE' and 'VIR_DOMAIN_AFFECT_CONFIG'
> > are mutually exclusive in libvirt_virDomainSetBlkioParameters, it's unreasonable,
> > So ues this patch to fix it.
> > 
> > Signed-off-by: Jie Wang <wangjie88 at huawei.com>
> > ---
> >  libvirt-override.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libvirt-override.c b/libvirt-override.c
> > index 9e40f00..7bdc09c 100644
> > --- a/libvirt-override.c
> > +++ b/libvirt-override.c
> > @@ -727,7 +727,7 @@ libvirt_virDomainSetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
> >      }
> >  
> >      LIBVIRT_BEGIN_ALLOW_THREADS;
> > -    i_retval = virDomainGetBlkioParameters(domain, NULL, &nparams, flags);
> > +    i_retval = virDomainGetBlkioParameters(domain, NULL, &nparams, 0);
> >      LIBVIRT_END_ALLOW_THREADS;
> >  
> >      if (i_retval < 0)
> > @@ -743,7 +743,7 @@ libvirt_virDomainSetBlkioParameters(PyObject *self ATTRIBUTE_UNUSED,
> >          return PyErr_NoMemory();
> >  
> >      LIBVIRT_BEGIN_ALLOW_THREADS;
> > -    i_retval = virDomainGetBlkioParameters(domain, params, &nparams, flags);
> > +    i_retval = virDomainGetBlkioParameters(domain, params, &nparams, 0);
> >      LIBVIRT_END_ALLOW_THREADS;
> >  
> 
> IMHO the correct fix would be to get rid of the getters completely as that is
> something the python API adds as an 'extra' compared to the plain C API - it
> never performed such a check and it never will since it's been done on the
> server side for quite some time. So, if someone used the old C API with a new
> client and an old server using some unsupported typed params, the old server
> would happily interpret all the ones it knew and fail at the first unknown one,
> however, there was no rollback involved. Similarly, the python API should behave
> exactly the same, irregardless of the correctness of the old server's behaviour.

It isn't that simple - this isn't a mere matter of checking data. The use
of the getters during the setter method is a fundamental requirement. At
the C layer we have many distinct data types - eg int, unsigned int,
long long, unsigned long long, which all map to the same python data
type.

When we set the parameters, we have no idea which data VIR_TYPED_PARAM_XXXX
data type to use. You need to thus call the getter to fetch the list of all
known types parameters & their data types. You now know what C data type to
convert the python type into.

Anyway, the patch above is correct - when calling the getters from a setter,
the flags parameter should always be zero.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|