[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/21/2013 08:15 PM, Jiri Denemark wrote:
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.

yes, currently, there are only one group of flags with two values(active/inactive),
        the way of yours is better to read.
if in future, there are more than one group of flags which are going to support, the way of my version will be better. virConnectListAllDomains use this way to
        filter list of domains.

        Guannan


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]