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

[Martin Kletzander]

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.
> >
> > NB:  I've checked all the other APIs in this file and this is the
> > only pointer API that is returning -1.
> >
> > Signed-off-by: Don Dugger <donald.d.dugger at intel.com>
> > ---
> >  python/libvirt-override.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> > index c60747d..b471605 100644
> > --- a/python/libvirt-override.c
> > +++ b/python/libvirt-override.c
> > @@ -4471,13 +4471,13 @@ libvirt_virConnectBaselineCPU(PyObject *self ATTRIBUTE_UNUSED,
> >
> >          ncpus = PyList_Size(list);
> >          if (VIR_ALLOC_N_QUIET(xmlcpus, ncpus) < 0)
> > -            return VIR_PY_INT_FAIL;
> > +            return VIR_PY_NONE;
> >
> >          for (i = 0; i < ncpus; i++) {
> >              xmlcpus[i] = PyString_AsString(PyList_GetItem(list, i));
> >              if (xmlcpus[i] == NULL) {
> >                  VIR_FREE(xmlcpus);
> > -                return VIR_PY_INT_FAIL;
> > +                return VIR_PY_NONE;
> >              }
> >          }
> >      }
> > @@ -4489,13 +4489,13 @@ libvirt_virConnectBaselineCPU(PyObject *self ATTRIBUTE_UNUSED,
> >      VIR_FREE(xmlcpus);
> >
> >      if (base_cpu == NULL)
> > -        return VIR_PY_INT_FAIL;
> > +        return VIR_PY_NONE;
> >
> >      pybase_cpu = PyString_FromString(base_cpu);
> >      VIR_FREE(base_cpu);
> >
> >      if (pybase_cpu == NULL)
> > -        return VIR_PY_INT_FAIL;
> > +        return VIR_PY_NONE;
> >
> >      return pybase_cpu;
> >  }
> > --
> > 1.7.10.4
> >
> 
> 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).
> 

I'd say this _also_ depends also on how things are documented.  Since
I'm not aware of any documentation explicitly mentioning that this
particular (an only python) API should return -1 in case of allocation
error, I'd say this was an error all along.  I'd also say go ahead
with it since this is the proper behavior and I can't imagine a project
where one would rely on this API to return -1 for allocation errors
without a OOM exception instead of all others.

OTOH, though, there are places where similar behavior was already many
times said to be "kept for consistency" (e.g. public *Free() functions
segfaulting with NULL parameter, even though the HACKING file says
they should be no-op in that case), so I must say this is a thing I
cannot clearly resolve with my own logic.  In case of voting, I'm all
for cleaning this 3yo mistake.

My $0.02,
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131125/d7fc08c6/attachment-0001.sig>