Re: [libvirt] [PATCH] Return right error code for baselineCPU
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(a)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.
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 :|