[libvirt] [PATCH] Return right error code for baselineCPU

[Don Dugger]

On Mon, Nov 25, 2013 at 04:02:37PM +0000, Daniel P. Berrange wrote:
> On Mon, Nov 25, 2013 at 08:55:12AM -0700, Don Dugger wrote:
> > On Mon, Nov 25, 2013 at 03:48:45PM +0000, Daniel P. Berrange wrote:
> > > On Mon, Nov 25, 2013 at 08:40:41AM -0700, Don Dugger wrote:
> > > > On Mon, Nov 25, 2013 at 10:45:38AM +0000, Daniel P. Berrange wrote:
> > > > > On Sun, Nov 24, 2013 at 10:46:13AM -0600, Doug Goldstein wrote:
> > > > > > On Sat, Nov 23, 2013 at 3:15 PM, Don Dugger <n0ano at n0ano.com> wrote:
> > > > > > >
> > > > > > > This Python interface code is returning a -1 on errors for the
> > > > > > > `baselineCPU' API.  Since this API is supposed to return a pointer
> > > > > > > the error return value should really be VIR_PY_NONE.
> > > > ...
> > > > > > >
> > > > > > 
> > > > > > ACK. This is correct. But it obviously changes our API so I'm not
> > > > > > really sure how we should handle this, (e.g. document the API as is as
> > > > > > note that its broken or fix it).
> > > > > 
> > > > > The implicit expectation with python APIs is that they all raise an
> > > > > exception if the libvirt call fails. So ACK to this bug fix & we
> > > > > should put it in maint branches.
> > > > 
> > > > Much as I hate to raise the issue this assumption is true for pointer
> > > > APIs but APIs that return an integer don't raise an exception, they
> > > > just return -1.  Obviously, changing this behavior would be way too
> > > > invasive but documenting this behavior should be done somewhere.
> > > 
> > > What APIs in particular ?  Any API which results in a libvirt error
> > > being set should be translated into an exception in the python. This
> > > is done whether they're APIs returning NULL pointers or -1 ints. If
> > > there are other exceptions to the rule they must be fixed too.
> > 
> > I'm basing this on code inspection so I could be wrong but if you look
> > at the Python interface code for libvirt_virDomainSetSchedulerParameters
> > it checks the return value from virDomainSetSchedulerParameters and,
> > if it is <0, then the Pythong code returns -1, without raising an
> > exception.
> 
> That code is the low-level module (called libvirtmod) which interfaces
> to the C library, and is not called directly by applications.
> 
> Applications call the main module (called libvirt) which translates
> to the lowlevel module. eg in this case it does:
> 
>     def setSchedulerParametersFlags(self, params, flags=0):
>         """Change the scheduler parameters """
>         ret = libvirtmod.virDomainSetSchedulerParametersFlags(self._o, params, flags)
>         if ret == -1: raise libvirtError ('virDomainSetSchedulerParametersFlags() failed', dom=self)
>         return ret
> 
> so the -1 error is turned into an exception for applications.
> 
> 
> The flaw the original patch in this thread was fixing is that the low
> level libvirtmod was returning NULL, where the high level libvirt
> expected to see -1, so the exception translation was missed.

Eric/Daniel-

That's why I distrust code inspection, there's always a hidden gotcha.  Tnx
for the clarification.

> 
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
> 

-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
n0ano at n0ano.com
Ph: 303/443-3786