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

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(a)huawei.com&gt; > > --- > > 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/ :| -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list