On Wed, Mar 18, 2015 at 10:32:54AM +0000, Daniel P. Berrange wrote:
> From: Nehal J Wani <nehaljw.kkd1@xxxxxxxxx>
>
> examples/Makefile.am:
> * Add new file domipaddrs.py
>
> examples/README:
> * Add documentation for the python example
>
> libvirt-override-api.xml:
> * Add new symbol for virDomainInterfacesAddresses
>
> libvirt-override.c:
> * Hand written python api
>
> Example:
> $ python examples/domipaddrs.py qemu:///system f18
> Interface MAC address Protocol Address
> vnet0 52:54:00:20:70:3d ipv4 192.168.105.240/16
>
> In v11:
> - Cope with hwaddr being NULL by filling in PY_NONE
> ---
> MANIFEST.in | 1 +
> examples/README | 1 +
> examples/domipaddrs.py | 57 ++++++++++++++++++++++++
> generator.py | 2 +
> libvirt-override-api.xml | 9 +++-
> libvirt-override.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++
> sanitytest.py | 3 ++
> 7 files changed, 182 insertions(+), 1 deletion(-)
> create mode 100755 examples/domipaddrs.py
>
[...]
> diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
> index 4660c9f..b197639 100644
> --- a/libvirt-override-api.xml
> +++ b/libvirt-override-api.xml
> @@ -678,5 +678,12 @@
> <arg name='flags' type='unsigned int' info='unused, pass 0'/>
> <return type='char *' info="list of mounted filesystems information"/>
> </function>
> - </symbols>
> + <function name='virDomainInterfaceAddresses' file='python'>
> + <info>returns a dictionary of domain interfaces along with their MAC and IP addresses</info>
> + <arg name='dom' type='virDomainPtr' info='pointer to the domain'/>
> + <arg name='source' type='unsigned int' info='the data source'/>
> + <arg name='flags' type='unsigned int' info='extra flags; not used yet, so callers should always pass 0'/>
> + <return type='char *' info="dictionary of domain interfaces along with their MAC and IP addresses"/>
> + </function>
> +</symbols>
Probably just a mistake with the </symbols> change.
> </api>
> diff --git a/libvirt-override.c b/libvirt-override.c
> index 1241305..548be24 100644
> --- a/libvirt-override.c
> +++ b/libvirt-override.c
> @@ -5120,6 +5120,113 @@ cleanup:
> return py_retval;
> }
>
> +
> +static PyObject *
> +libvirt_virDomainInterfaceAddresses(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args)
> +{
> + PyObject *py_retval = VIR_PY_NONE;
> + virDomainPtr domain;
> + PyObject *pyobj_domain;
> + unsigned int source;
> + unsigned int flags;
> + virDomainInterfacePtr *ifaces = NULL;
> + int ifaces_count = 0;
> + size_t i, j;
> + int ret;
> + bool full_free = true;
> +
> + if (!PyArg_ParseTuple(args, (char *) "Oii:virDomainInterfacePtr",
> + &pyobj_domain, &source, &flags))
> + return NULL;
You cannot return NULL without Py_DECREF(py_retval) because VIR_PY_NONE takes
reference to Py_None.
> +
> + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + ifaces_count = virDomainInterfaceAddresses(domain, &ifaces, source, flags);
> + ret = ifaces_count;
> + LIBVIRT_END_ALLOW_THREADS;
> + if (ret < 0)
> + goto cleanup;
The ret variable is pointless here.
> +
> + if (!(py_retval = PyDict_New()))
> + goto no_memory;
> +
> + for (i = 0; i < ifaces_count; i++) {
> + virDomainInterfacePtr iface = ifaces[i];
> + PyObject *py_addrs = NULL;
> + PyObject *py_iface = NULL;
> +
> + if (!(py_iface = PyDict_New()))
> + goto no_memory;
> +
> + if (iface->naddrs) {
> + if (!(py_addrs = PyList_New(iface->naddrs))) {
> + Py_DECREF(py_iface);
> + goto no_memory;
> + }
> + } else {
> + py_addrs = VIR_PY_NONE;
> + }
> +
> + for (j = 0; j < iface->naddrs; j++) {
> + virDomainIPAddressPtr addr = &(iface->addrs[j]);
> + PyObject *py_addr = PyDict_New();
> + int type = addr->type;
> +
> + if (!addr) {
You've probably meant py_addr.
> + Py_DECREF(py_iface);
> + Py_DECREF(py_addrs);
> + goto no_memory;
> + }
> +
> + PyDict_SetItem(py_addr, libvirt_constcharPtrWrap("addr"),
> + libvirt_constcharPtrWrap(addr->addr));
> + PyDict_SetItem(py_addr, libvirt_constcharPtrWrap("prefix"),
> + libvirt_intWrap(addr->prefix));
> + PyDict_SetItem(py_addr, libvirt_constcharPtrWrap("type"),
> + libvirt_intWrap(type));
> +
All the libvirt_*Wrap and Py*_SetItem functions can fail. In that case they
returns NULL/-1 and set an exception and we should check that and in case of
error return NULL.
> + PyList_SetItem(py_addrs, j, py_addr);
> + }
> +
> + PyDict_SetItem(py_iface, libvirt_constcharPtrWrap("addrs"),
> + py_addrs);
> + if (iface->hwaddr) {
> + PyDict_SetItem(py_iface, libvirt_constcharPtrWrap("hwaddr"),
> + libvirt_constcharPtrWrap(iface->hwaddr));
> + } else {
> + PyDict_SetItem(py_iface, libvirt_constcharPtrWrap("hwaddr"),
> + VIR_PY_NONE);
> + }
> +
> + PyDict_SetItem(py_retval, libvirt_charPtrWrap(iface->name),
> + py_iface);
> + }
> +
> + full_free = false;
> +
> +cleanup:
> + if (ifaces && ifaces_count > 0) {
> + for (i = 0; full_free && i < ifaces_count; i++) {
> + /*
> + * We don't want to free values we've just shared with python variables unless
> + * there was an error and hence we are returning PY_NONE or equivalent
> + */
Actually I don't think that this statement is true. All the values that we've
shared with python are copied and therefore can be freed.
> + virDomainInterfaceFree(ifaces[i]);
> + }
> + }
> + VIR_FREE(ifaces);
> +
> + return py_retval;
> +
> +no_memory:
> + Py_XDECREF(py_retval);
> + py_retval = PyErr_NoMemory();
All the paths leading to no_memory already set python exception and we should
not just overwrite it. Keep the original exception instead as it doesn't have
to be always NoMemmory.
> + goto cleanup;
> +}
> +
> +
> /*******************************************
> * Helper functions to avoid importing modules
> * for every callback
> @@ -8750,6 +8857,9 @@ static PyMethodDef libvirtMethods[] = {
> #if LIBVIR_CHECK_VERSION(1, 2, 11)
> {(char *) "virDomainGetFSInfo", libvirt_virDomainGetFSInfo, METH_VARARGS, NULL},
> #endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */
> +#if LIBVIR_CHECK_VERSION(1, 2, 14)
> + {(char *) "virDomainInterfaceAddresses", libvirt_virDomainInterfaceAddresses, METH_VARARGS, NULL},
> +#endif /* LIBVIR_CHECK_VERSION(1, 2, 14) */
> {NULL, NULL, 0, NULL}
> };
>
> diff --git a/sanitytest.py b/sanitytest.py
> index 0e6e0e5..2b609a9 100644
> --- a/sanitytest.py
> +++ b/sanitytest.py
> @@ -145,6 +145,9 @@ for cname in wantfunctions:
> if name[0:26] == "virDomainIOThreadsInfoFree":
> continue
>
> + if name[0:33] == "virDomainInterfaceFree":
> + continue
You've probably just hit a wrong key as it should be 22.
> +
> if name[0:21] == "virDomainListGetStats":
> name = "virConnectDomainListGetStats"
>
> --
> 2.1.0
>
I'll send a v12 with all the changes for review. The rewrite also uses implicit
free by assigning all created python objects to the main py_retval dict. Then
the Py_XDECREF will free everything for us.
Pavel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list