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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list