On Fri, Feb 17, 2017 at 02:26:21PM +0000, Daniel P. Berrange wrote: > 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 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@xxxxxxxxxx> > > > --- > > > 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. Noted, thanks for clarification, in that case, there are a few identical issues with SetNumaParameters, SetMemoryParameters, SetInterfaceParameters, SetSchedulerParametersFlags, so it would be awesome if we could fix them at the same time, since we would hit the very same issue you're trying to fix with all of those. Could you send a v2 fixing all the above-mentioned occurrences? Erik > > 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
