[libvirt] [PATCH v11] Expose virDomainInterfacesAddresses to python binding

Pavel Hrdina phrdina at redhat.com
Thu Mar 19 13:14:29 UTC 2015


On Wed, Mar 18, 2015 at 10:32:54AM +0000, Daniel P. Berrange wrote:
> From: Nehal J Wani <nehaljw.kkd1 at gmail.com>
> 
> 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




More information about the libvir-list mailing list