[libvirt] [PATCH] interface: list all interfaces with flags == 0

[Jiri Denemark]

On Tue, May 21, 2013 at 17:05:09 +0800, Guan Nan Ren wrote:
> virConnectListAllInterfaces should support to list all of
> interfaces when the value of flags is 0. The behaviour is
> consistent with other virConnectListAll* APIs
> ---
> diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c
> index cbba4fd..c6e069a 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
...
> @@ -361,16 +359,17 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
>          /* 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++;
> +        if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
> +            !((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
> +               (status & NETCF_IFACE_ACTIVE)) ||
> +              (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
> +               (status & NETCF_IFACE_INACTIVE))))
> +            continue;

IMHO this is much harder to read. I'd just rewrite the original
condition as (i.e., !MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE)
is the part that was missing):

    if (!MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) ||
        (MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
         (status & NETCF_IFACE_ACTIVE)) ||
        (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
         (status & NETCF_IFACE_INACTIVE)))

Moreover, your version is wrong as it skips ncf_if_free(iface) in case
iface should be skipped.

> +
> +        if (ifaces) {
> +            iface_obj = virGetInterface(conn, ncf_if_name(iface),
> +                                        ncf_if_mac_string(iface));
> +            tmp_iface_objs[niface_objs++] = iface_obj;
>          }
>  
>          ncf_if_free(iface);
> diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c
> index 1fd7d46..242fc15 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
...
> @@ -363,18 +362,15 @@ udevConnectListAllInterfaces(virConnectPtr conn,
>          status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
>  
>          /* Filter the results */
> -        if (status && (flags & VIR_CONNECT_LIST_INTERFACES_ACTIVE))
> -            add_to_list = 1;
> -        else if (!status && (flags & VIR_CONNECT_LIST_INTERFACES_INACTIVE))
> -            add_to_list = 1;
> +        if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
> +            !((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && status) ||
> +              (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && !status)))
> +            continue;

Same bug in skipping udev_device_unref(dev). And similarly hard to read
test :-)

>  
>          /* If we matched a filter, then add it */
> -        if (add_to_list) {
> -            if (ifaces) {
> -                iface_obj = virGetInterface(conn, name, macaddr);
> -                ifaces_list[count] = iface_obj;
> -            }
> -            count++;
> +        if (ifaces) {
> +            iface_obj = virGetInterface(conn, name, macaddr);
> +            ifaces_list[count++] = iface_obj;
>          }
>          udev_device_unref(dev);
>      }

Jirka