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

Jie Wang posted 1 patch 4 years, 2 months ago
Failed in applying to current master (apply log)
libvirt-override.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

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

Posted by Jie Wang 4 years, 2 months ago
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




--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 4 years, 2 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrange 4 years, 2 months ago
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/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 4 years, 2 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Eric Blake 4 years, 2 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrange 4 years, 2 months ago
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/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list