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.
>
> 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(a)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