[libvirt] [PATCH] interface: list all interfaces with flags == 0
virConnectListAllInterfaces should support to list all of interfaces when the value of flags is 0. The behaviour is consistent with other virConnectListAll* APIs --- src/conf/interface_conf.h | 4 ++++ src/interface/interface_backend_netcf.c | 27 +++++++++++++-------------- src/interface/interface_backend_udev.c | 23 ++++++++++------------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index e636c35..ae93811 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -211,4 +211,8 @@ char *virInterfaceDefFormat(const virInterfaceDefPtr def); void virInterfaceObjLock(virInterfaceObjPtr obj); void virInterfaceObjUnlock(virInterfaceObjPtr obj); +#define VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE \ + (VIR_CONNECT_LIST_INTERFACES_ACTIVE | \ + VIR_CONNECT_LIST_INTERFACES_INACTIVE) + #endif /* __INTERFACE_CONF_H__ */ 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 @@ -260,6 +260,7 @@ static int netcfConnectListDefinedInterfaces(virConnectPtr conn, char **const na } +#define MATCH(FLAG) (flags & (FLAG)) static int netcfConnectListAllInterfaces(virConnectPtr conn, virInterfacePtr **ifaces, @@ -276,8 +277,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn, int ret = -1; char **names = NULL; - virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | - VIR_CONNECT_LIST_INTERFACES_INACTIVE, -1); + virCheckFlags(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE, -1); interfaceDriverLock(driver); @@ -293,7 +293,6 @@ netcfConnectListAllInterfaces(virConnectPtr conn, _("failed to get number of host interfaces: %s%s%s"), errmsg, details ? " - " : "", details ? details : ""); - ret = -1; goto cleanup; } @@ -304,7 +303,6 @@ netcfConnectListAllInterfaces(virConnectPtr conn, if (VIR_ALLOC_N(names, count) < 0) { virReportOOMError(); - ret = -1; goto cleanup; } @@ -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; + + 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 @@ -282,6 +282,7 @@ udevConnectListDefinedInterfaces(virConnectPtr conn, VIR_UDEV_IFACE_INACTIVE); } +#define MATCH(FLAG) (flags & (FLAG)) static int udevConnectListAllInterfaces(virConnectPtr conn, virInterfacePtr **ifaces, @@ -299,8 +300,7 @@ udevConnectListAllInterfaces(virConnectPtr conn, int status = 0; int ret; - virCheckFlags(VIR_CONNECT_LIST_INTERFACES_ACTIVE | - VIR_CONNECT_LIST_INTERFACES_INACTIVE, -1); + virCheckFlags(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE, -1); /* Grab a udev reference */ udev = udev_ref(driverState->udev); @@ -354,7 +354,6 @@ udevConnectListAllInterfaces(virConnectPtr conn, const char *path; const char *name; const char *macaddr; - int add_to_list = 0; path = udev_list_entry_get_name(dev_entry); dev = udev_device_new_from_syspath(udev, path); @@ -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; /* 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); } @@ -387,6 +383,7 @@ udevConnectListAllInterfaces(virConnectPtr conn, if (ifaces) { ignore_value(VIR_REALLOC_N(ifaces_list, count + 1)); *ifaces = ifaces_list; + ifaces_list = NULL; } return count; -- 1.8.1.4
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
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
On 05/21/2013 06:49 AM, Guannan Ren wrote: Guannan's logic says when to drop:
+ 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;
Jirka's logic says when to keep; so it would need a ! around the overall expression if we want to turn it into the condition that represents when to drop.
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)))
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.
Let's compare what happens for all four combinations of the two flags [active is flag 1, inactive is flag 2]: flags status Guannan's [0 means keep] Jirka's [1 means keep] 0 active 0&&!(0||0)=keep !0||0||0=keep 0 inactive 0&&!(0||0)=keep !0||0||0=keep 1 active 1&&!(1||0)=keep !1||1||0=keep 1 inactive 1&&!(0||0)=drop !1||0||0=drop 2 active 1&&!(0||0)=drop !1||0||0=drop 2 inactive 1&&!(0||1)=keep !1||0||1=keep 3 active 1&&!(1||0)=keep !1||1||0=keep 3 inactive 1&&!(0||1)=keep !1||0||1=keep Ultimately, the two expressions are equivalent (you can use deMorgan's law to prove the equivalence), but I find Jirka's positive logic a bit easier to reason with than Guannan's negative logic, even if Guannan's style copied from how we did it on other API. I do agree that for purposes of adding future filter groups, as well as minimizing nested conditionals, that the logic looks better in terms of drop checks: foreach item: if (filter group 1 drops) continue; if (filter group 2 drops) continue; item was kept by all filters, add it to return rather than logic in terms of keep checks: foreach item: if (filter group 1 keeps) if (filter group 2 keeps) add it to return -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Impressive. :) ----- Original Message ----- From: "Eric Blake" <eblake@redhat.com> To: "Guannan Ren" <gren@redhat.com> Cc: libvir-list@redhat.com Sent: Tuesday, May 21, 2013 10:49:57 PM Subject: Re: [libvirt] [PATCH] interface: list all interfaces with flags == 0 On 05/21/2013 06:49 AM, Guannan Ren wrote: Guannan's logic says when to drop:
+ 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;
Jirka's logic says when to keep; so it would need a ! around the overall expression if we want to turn it into the condition that represents when to drop.
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)))
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.
Let's compare what happens for all four combinations of the two flags [active is flag 1, inactive is flag 2]: flags status Guannan's [0 means keep] Jirka's [1 means keep] 0 active 0&&!(0||0)=keep !0||0||0=keep 0 inactive 0&&!(0||0)=keep !0||0||0=keep 1 active 1&&!(1||0)=keep !1||1||0=keep 1 inactive 1&&!(0||0)=drop !1||0||0=drop 2 active 1&&!(0||0)=drop !1||0||0=drop 2 inactive 1&&!(0||1)=keep !1||0||1=keep 3 active 1&&!(1||0)=keep !1||1||0=keep 3 inactive 1&&!(0||1)=keep !1||0||1=keep Ultimately, the two expressions are equivalent (you can use deMorgan's law to prove the equivalence), but I find Jirka's positive logic a bit easier to reason with than Guannan's negative logic, even if Guannan's style copied from how we did it on other API. I do agree that for purposes of adding future filter groups, as well as minimizing nested conditionals, that the logic looks better in terms of drop checks: foreach item: if (filter group 1 drops) continue; if (filter group 2 drops) continue; item was kept by all filters, add it to return rather than logic in terms of keep checks: foreach item: if (filter group 1 keeps) if (filter group 2 keeps) add it to return -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Guan Nan Ren -
Guannan Ren
-
Jiri Denemark