[libvirt] [PATCH 4/6 v5] list: Implement listAllInterfaces

[Laine Stump]

On 09/11/2012 11:37 PM, Osier Yang wrote:
> This is not that ideal as API for other objects, as it's still
> O(n). Because interface driver uses netcf APIs to manage the
> stuffs, instead of by itself. And netcf APIs don't return a object.
> It provides APIs like old libvirt APIs:
>
>    ncf_number_of_interfaces
>    ncf_list_interfaces
>    ncf_lookup_by_name
>    ......
>
> Perhaps we should further improve netcf to let it provide an API
> to return the object, but it could be a later patch. And anyway,
> we will still benefit from the new API for the simplification,
> and no race like the old APIs.
>
> src/interface/netcf_driver.c: Implement listAllInterfaces
>
> v4 - v5:
>   * Ignore the NETCF_NOERROR, in case of the iface could be deleted
>     by other management apps as a race.
>
>   * Fix the memory leak caught by Laine.
>
>   * The version number fix.
>
> ---
>  src/interface/netcf_driver.c |  143 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 143 insertions(+), 0 deletions(-)
>
> diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c
> index 935be66..26d25d7 100644
> --- a/src/interface/netcf_driver.c
> +++ b/src/interface/netcf_driver.c
> @@ -30,6 +30,7 @@
>  #include "netcf_driver.h"
>  #include "interface_conf.h"
>  #include "memory.h"
> +#include "logging.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_INTERFACE
>  
> @@ -259,6 +260,147 @@ static int interfaceListDefinedInterfaces(virConnectPtr conn, char **const names
>  
>  }
>  
> +static int
> +interfaceListAllInterfaces(virConnectPtr conn,
> +                           virInterfacePtr **ifaces,
> +                           unsigned int flags)
> +{
> +    struct interface_driver *driver = conn->interfacePrivateData;
> +    int count;
> +    int i;
> +    struct netcf_if *iface = NULL;
> +    virInterfacePtr *tmp_iface_objs = NULL;
> +    virInterfacePtr iface_obj = NULL;
> +    unsigned int status;
> +    int niface_objs = 0;
> +    int ret = -1;
> +    char **names;
> +
> +    virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE |
> +                  VIR_CONNECT_LIST_INTERFACES_INACTIVE, -1);
> +
> +    interfaceDriverLock(driver);
> +
> +    /* List all interfaces, in case of we might support new filter flags
> +     * except active|inactive in future.
> +     */
> +    count = ncf_num_of_interfaces(driver->netcf, NETCF_IFACE_ACTIVE |
> +                                  NETCF_IFACE_INACTIVE);
> +    if (count < 0) {
> +        const char *errmsg, *details;
> +        int errcode = ncf_error(driver->netcf, &errmsg, &details);
> +        virReportError(netcf_to_vir_err(errcode),
> +                       _("failed to get number of host interfaces: %s%s%s"),
> +                       errmsg, details ? " - " : "",
> +                       details ? details : "");
> +        return -1;


Ooh! I just noticed that you have several return paths here that don't
call interfaceDriverUnlock()!!

If you initialize names = NULL, then qualify the loop to free names with
"if (names) { ... }", you should be able to just to "ret = X; goto
cleanup;" instead of return.

> +    }
> +
> +    if (count == 0)
> +        return 0;
> +
> +    if (VIR_ALLOC_N(names, count) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    if ((count = ncf_list_interfaces(driver->netcf, count, names,
> +                                     NETCF_IFACE_ACTIVE |
> +                                     NETCF_IFACE_INACTIVE)) < 0) {
> +        const char *errmsg, *details;
> +        int errcode = ncf_error(driver->netcf, &errmsg, &details);
> +        virReportError(netcf_to_vir_err(errcode),
> +                       _("failed to list host interfaces: %s%s%s"),
> +                       errmsg, details ? " - " : "",
> +                       details ? details : "");
> +        goto cleanup;
> +    }
> +
> +    if (ifaces) {
> +        if (VIR_ALLOC_N(tmp_iface_objs, count + 1) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +    }
> +
> +    for (i = 0; i < count; i++) {
> +        iface = ncf_lookup_by_name(driver->netcf, names[i]);
> +        if (!iface) {
> +            const char *errmsg, *details;
> +            int errcode = ncf_error(driver->netcf, &errmsg, &details);
> +            if (errcode != NETCF_NOERROR) {
> +                virReportError(netcf_to_vir_err(errcode),
> +                               _("couldn't find interface named '%s': %s%s%s"),
> +                               names[i], errmsg,
> +                               details ? " - " : "", details ? details : "");
> +                goto cleanup;
> +            } else {
> +                /* Ignore the NETCF_NOERROR, as the interface is very likely
> +                 * deleted by other management apps (e.g. virt-manager).
> +                 */
> +                VIR_WARN("couldn't find interface named '%s', might be "
> +                         "deleted by other process", names[i]);


Yeah, I guess this is uncommon enough (as in "I bet this will never
happen, unless I actually put money on the bet" :-) that it's okay to
print a warning.


> +                continue;
> +            }
> +        }
> +
> +        if (ncf_if_status(iface, &status) < 0) {
> +            const char *errmsg, *details;
> +            int errcode = ncf_error(driver->netcf, &errmsg, &details);
> +            virReportError(netcf_to_vir_err(errcode),
> +                           _("failed to get status of interface %s: %s%s%s"),
> +                           names[i], errmsg, details ? " - " : "",
> +                           details ? details : "");
> +            goto cleanup;
> +        }
> +
> +        /* XXX: Filter the result, need to be splitted once new filter flags
> +         * except active|inactive are supported.
> +         */
> +        if (((status & NETCF_IFACE_ACTIVE) &&
> +             (flags & VIR_CONNECT_LIST_INTERFACES_ACTIVE)) ||
> +            ((status & NETCF_IFACE_INACTIVE) &&
> +             (flags & VIR_CONNECT_LIST_INTERFACES_INACTIVE))) {
> +            if (ifaces) {
> +                iface_obj = virGetInterface(conn, ncf_if_name(iface),
> +                                            ncf_if_mac_string(iface));
> +                tmp_iface_objs[niface_objs] = iface_obj;
> +            }
> +            niface_objs++;
> +        }
> +
> +        ncf_if_free(iface);
> +        iface = NULL;
> +    }
> +
> +    if (tmp_iface_objs) {
> +        /* trim the array to the final size */
> +        ignore_value(VIR_REALLOC_N(tmp_iface_objs, niface_objs + 1));
> +        *ifaces = tmp_iface_objs;
> +        tmp_iface_objs = NULL;
> +    }
> +
> +    ret = niface_objs;
> +
> +cleanup:
> +    ncf_if_free(iface);
> +
> +    for (i = 0; i < count; i++)
> +        VIR_FREE(names[i]);
> +    VIR_FREE(names);
> +
> +    if (tmp_iface_objs) {
> +        for (i = 0; i < niface_objs; i++) {
> +            if (tmp_iface_objs[i])
> +                virInterfaceFree(tmp_iface_objs[i]);
> +        }
> +    }
> +
> +    interfaceDriverUnlock(driver);
> +    return ret;
> +}
> +
> +
>  static virInterfacePtr interfaceLookupByName(virConnectPtr conn,
>                                               const char *name)
>  {
> @@ -642,6 +784,7 @@ static virInterfaceDriver interfaceDriver = {
>      .listInterfaces = interfaceListInterfaces, /* 0.7.0 */
>      .numOfDefinedInterfaces = interfaceNumOfDefinedInterfaces, /* 0.7.0 */
>      .listDefinedInterfaces = interfaceListDefinedInterfaces, /* 0.7.0 */
> +    .listAllInterfaces = interfaceListAllInterfaces, /* 0.10.2 */
>      .interfaceLookupByName = interfaceLookupByName, /* 0.7.0 */
>      .interfaceLookupByMACString = interfaceLookupByMACString, /* 0.7.0 */
>      .interfaceGetXMLDesc = interfaceGetXMLDesc, /* 0.7.0 */

ACK with the interfaceDriverUnlock() problem fixed.