4:05 a.m.
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
7:15 a.m.
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
7:49 a.m.
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
9:49 a.m.
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
11:20 a.m.
Impressive. :)
----- Original Message -----
From: "Eric Blake" <eblake(a)redhat.com>
To: "Guannan Ren" <gren(a)redhat.com>
Cc: libvir-list(a)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
4203
days inactive
4203
days old
4 comments
4 participants
participants (4)
-
Eric Blake -
Guan Nan Ren -
Guannan Ren
-
Jiri Denemark