[libvirt] [PATCH] libvirt-override: fix flags mutually exclusize
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@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; if (i_retval < 0) { -- 1.9.5.msysgit.1
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@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. CC'ing Dan what he thinks about the python API doing some extra checking to compensate for the old server's behaviour. Regards, Erik
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@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/ :|
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 [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@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.
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/17/2017 08:26 AM, Daniel P. Berrange wrote:
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.
Is it worth improving the C code to do relaxed parsing (if the C type is int, but the caller passes a long long with a value that fits in an int, to accept the caller instead of rejecting it as a type mismatch)? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Feb 17, 2017 at 10:20:34AM -0600, Eric Blake wrote:
On 02/17/2017 08:26 AM, Daniel P. Berrange wrote:
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.
Is it worth improving the C code to do relaxed parsing (if the C type is int, but the caller passes a long long with a value that fits in an int, to accept the caller instead of rejecting it as a type mismatch)?
I don't see much benefit in that - any app or language binding that relied on such a feature would find itself needlessly broken when run against older libvirt. 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/ :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Erik Skultety -
Jie Wang