On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
> Signed-off-by: Giuseppe Scrivano <gscrivan@xxxxxxxxxx>
> ---
> python/generator.py | 2 +-
> python/libvirt-override-api.xml | 7 ++++++
> python/libvirt-override.c | 52 +++++++++++++++++++++++++++++++++++++++++
> python/libvirt-override.py | 11 +++++++++
> 4 files changed, 71 insertions(+), 1 deletion(-)
>
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> +
> + c_retval = virConnectGetCPUModelNames(conn, arch, &models, flags);
> +
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (c_retval == -1)
> + return VIR_PY_INT_FAIL;
> +
> + if ((rv = PyList_New(c_retval)) == NULL)
> + goto error;
> +
> + for (i = 0; i < c_retval; i++) {
> + PyObject *str;
> + if ((str = PyString_FromString(models[i])) == NULL)
> + goto error;
> +
> + PyList_SET_ITEM(rv, i, str);
Elsewhere, we've used PyList_New(0)/PyList_Append() rather than
PyList_New(count)/PyList_SET_ITEM(); but that's not universal; also, I
see uses of PyList_SetItem but not PyList_SET_ITEM; what's the difference?
More importantly, you STILL have a data leak. If c_retval is 2 but
PyString_FromString(models[1]) fails (typically only possible on OOM),
then we goto error and never free models.
Maybe you can get some inspiration from
libvirt_virDomainSnapshotListNames for how to properly have a single
cleanup: label (instead of done:/error:), while handling error cases and
setting up proper transfer semantics from an array of strings to a
python list of strings.
I'm looking forward to v4; the changes I suggested squashing in have all
worked in my testing, and it is merely this patch and patch 2 that need
actual fixes that I have not written.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list