On 09/04/2012 11:55 AM, Osier Yang wrote:

This is to list the network objects, supported filtering flags are: active|inactive, persistent|transient, autostart|no-autostart.

include/libvirt/libvirt.h.in: Declare enum virConnectListAllNetworkFlags and virConnectListAllNetworks. python/generator.py: Skip auto-generating src/driver.h: (virDrvConnectListAllNetworks) src/libvirt.c: Implement the public API src/libvirt_public.syms: Export the symbol to public --- include/libvirt/libvirt.h.in | 20 ++++++++++ python/generator.py | 1 + src/driver.h | 5 ++ src/libvirt.c | 86 +++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 1 + 5 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 58cd6ff..50d865c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2279,6 +2279,26 @@ int virConnectNumOfDefinedNetworks (virConnectPtr conn); int virConnectListDefinedNetworks (virConnectPtr conn, char **const names, int maxnames); +/* + * virConnectListAllNetworks: + * + * Flags used to filter the returned networks. Flags in each group + * are exclusive attributes of a network. + */ +typedef enum { + VIR_CONNECT_LIST_NETWORKS_INACTIVE = 1 << 0, + VIR_CONNECT_LIST_NETWORKS_ACTIVE = 1 << 1, + + VIR_CONNECT_LIST_NETWORKS_PERSISTENT = 1 << 2, + VIR_CONNECT_LIST_NETWORKS_TRANSIENT = 1 << 3, + + VIR_CONNECT_LIST_NETWORKS_AUTOSTART = 1 << 4, + VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART = 1 << 5, +} virConnectListAllNetworksFlags; + +int virConnectListAllNetworks (virConnectPtr conn, + virNetworkPtr **nets, + unsigned int flags);

/* * Lookup network by name or uuid diff --git a/python/generator.py b/python/generator.py index 276b4ff..a9a401b 100755 --- a/python/generator.py +++ b/python/generator.py @@ -462,6 +462,7 @@ skip_function = ( 'virDomainSnapshotListAllChildren', # overridden in virDomainSnapshot.py 'virConnectListAllStoragePools', # overridden in virConnect.py 'virStoragePoolListAllVolumes', # overridden in virStoragePool.py + 'virConnectListAllNetworks', # overridden in virConnect.py

'virStreamRecvAll', # Pure python libvirt-override-virStream.py 'virStreamSendAll', # Pure python libvirt-override-virStream.py diff --git a/src/driver.h b/src/driver.h index 1bdfd29..534da05 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1082,6 +1082,10 @@ typedef int (*virDrvListDefinedNetworks) (virConnectPtr conn, char **const names, int maxnames); +typedef int + (*virDrvListAllNetworks) (virConnectPtr conn, + virNetworkPtr **nets, + unsigned int flags); typedef virNetworkPtr (*virDrvNetworkLookupByUUID) (virConnectPtr conn, const unsigned char *uuid); @@ -1140,6 +1144,7 @@ struct _virNetworkDriver { virDrvListNetworks listNetworks; virDrvNumOfDefinedNetworks numOfDefinedNetworks; virDrvListDefinedNetworks listDefinedNetworks; + virDrvListAllNetworks listAllNetworks; virDrvNetworkLookupByUUID networkLookupByUUID; virDrvNetworkLookupByName networkLookupByName; virDrvNetworkCreateXML networkCreateXML; diff --git a/src/libvirt.c b/src/libvirt.c index 700dbef..686af8a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -9799,6 +9799,76 @@ virNetworkGetConnect (virNetworkPtr net) }

/** + * virConnectListAllNetworks: + * @conn: Pointer to the hypervisor connection. + * @nets: Pointer to a variable to store the array containing the network + * objects or NULL if the list is not required (just returns number + * of networks). + * @flags: bitwise-OR of virConnectListAllNetworksFlags. + * + * Collect the list of networks, and allocate an array to store those + * objects. This API solves the race inherent between virConnectListNetworks + * and virConnectListDefinedNetworks. + * + * Normally, all networks are returned; however, @flags can be used to + * filter the results for a smaller list of targeted networks. The valid + * flags are divided into groups, where each group contains bits that + * describe mutually exclusive attributes of a network, and where all bits + * within a group describe all possible networks. + * + * The first group of @flags is VIR_CONNECT_LIST_NETWORKS_ACTIVE (up) and + * VIR_CONNECT_LIST_NETWORKS_INACTIVE (down) to filter the networks by state. + * + * The second group of @flags is VIR_CONNECT_LIST_NETWORKS_PERSISTENT (defined) + * and VIR_CONNECT_LIST_NETWORKS_TRANSIENT (running but not defined), to filter + * the networks by whether they have persistent config or not. + * + * The third group of @flags is VIR_CONNECT_LIST_NETWORKS_AUTOSTART + * and VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART, to filter the networks by + * whether they are marked as autostart or not. + * + * Returns the number of networks found or -1 and sets @nets to NULL in case + * of error. On success, the array stored into @nets is guaranteed to have an + * extra allocated element set to NULL but not included in the return count, + * to make iteration easier. The caller is responsible for calling + * virNetworkFree() on each array element, then calling free() on @nets. + */ +int +virConnectListAllNetworks(virConnectPtr conn, + virNetworkPtr **nets, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, nets=%p, flags=%x", conn, nets, flags); + + virResetLastError(); + + if (nets) + *nets = NULL; + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (conn->networkDriver && + conn->networkDriver->listAllNetworks) { + int ret; + ret = conn->networkDriver->listAllNetworks(conn, nets, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + + +/** * virConnectNumOfNetworks: * @conn: pointer to the hypervisor connection * @@ -9842,7 +9912,13 @@ error: * * Collect the list of active networks, and store their names in @names * - * Returns the number of networks found or -1 in case of error + * For more control over the results, see virConnectListAllNetworks(). + * + * Returns the number of networks found or -1 in case of error. Note that + * this command is inherently racy; a network can be started between a call + * to virConnectNumOfNetworks() and this call; you are only guaranteed that + * all currently active networks were listed if the return is less than + * @maxnames. */ int virConnectListNetworks(virConnectPtr conn, char **const names, int maxnames) @@ -9919,7 +9995,13 @@ error: * * list the inactive networks, stores the pointers to the names in @names * - * Returns the number of names provided in the array or -1 in case of error + * For more control over the results, see virConnectListAllNetworks(). + * + * Returns the number of names provided in the array or -1 in case of error. + * Note that this command is inherently racy; a network can be defined between + * a call to virConnectNumOfDefinedNetworks() and this call; you are only + * guaranteed that all currently defined networks were listed if the return + * is less than @maxnames. The client must call free() on each returned name. */ int virConnectListDefinedNetworks(virConnectPtr conn, char **const names, diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 53db37f..6ff5a77 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -556,6 +556,7 @@ LIBVIRT_0.10.0 {

LIBVIRT_0.10.2 { global: + virConnectListAllNetworks; virConnectListAllStoragePools; virStoragePoolListAllVolumes; } LIBVIRT_0.10.0;

ACK. I think this horse has been beaten enough :-)

On 09/04/2012 11:55 AM, Osier Yang wrote:

The RPC generator doesn't support returning list of object, this patch do the work manually.

* daemon/remote.c: Implemente the server side handler remoteDispatchConnectListAllNetworks.

* src/remote/remote_driver.c: Add remote driver handler remoteConnectListAllNetworks.

* src/remote/remote_protocol.x: New RPC procedure REMOTE_PROC_CONNECT_LIST_ALL_NETWORKS and structs to represent the args and ret for it.

* src/remote_protocol-structs: Likewise. --- daemon/remote.c | 55 ++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 64 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 13 ++++++++- src/remote_protocol-structs | 12 ++++++++ 4 files changed, 143 insertions(+), 1 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c index 8208c71..dba51a7 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4211,6 +4211,61 @@ cleanup: return rv; }

+static int +remoteDispatchConnectListAllNetworks(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_list_all_networks_args *args, + remote_connect_list_all_networks_ret *ret) +{ + virNetworkPtr *nets = NULL; + int nnets = 0; + int i; + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if ((nnets = virConnectListAllNetworks(priv->conn, + args->need_results ? &nets : NULL, + args->flags)) < 0) + goto cleanup; + + if (nets && nnets) { + if (VIR_ALLOC_N(ret->nets.nets_val, nnets) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret->nets.nets_len = nnets; + + for (i = 0; i < nnets; i++) + make_nonnull_network(ret->nets.nets_val + i, nets[i]); + } else { + ret->nets.nets_len = 0; + ret->nets.nets_val = NULL; + } + + ret->ret = nnets; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (nets) { + for (i = 0; i < nnets; i++) + virNetworkFree(nets[i]); + VIR_FREE(nets); + } + return rv; +} + + /*----- Helpers. -----*/

/* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8d4420e..56e50a6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2718,6 +2718,69 @@ done: return rv; }

+static int +remoteConnectListAllNetworks(virConnectPtr conn, + virNetworkPtr **nets, + unsigned int flags) +{ + int rv = -1; + int i; + virNetworkPtr *tmp_nets = NULL; + remote_connect_list_all_networks_args args; + remote_connect_list_all_networks_ret ret; + + struct private_data *priv = conn->privateData; + + remoteDriverLock(priv); + + args.need_results = !!nets; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + if (call(conn, + priv, + 0, + REMOTE_PROC_CONNECT_LIST_ALL_NETWORKS, + (xdrproc_t) xdr_remote_connect_list_all_networks_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_list_all_networks_ret, + (char *) &ret) == -1) + goto done; + + if (nets) { + if (VIR_ALLOC_N(tmp_nets, ret.nets.nets_len + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < ret.nets.nets_len; i++) { + tmp_nets[i] = get_nonnull_network (conn, ret.nets.nets_val[i]); + if (!tmp_nets[i]) { + virReportOOMError(); + goto cleanup; + } + } + *nets = tmp_nets; + tmp_nets = NULL; + } + + rv = ret.ret; + +cleanup: + if (tmp_nets) { + for (i = 0; i < ret.nets.nets_len; i++) + if (tmp_nets[i]) + virNetworkFree(tmp_nets[i]); + VIR_FREE(tmp_nets); + } + + xdr_free((xdrproc_t) xdr_remote_connect_list_all_networks_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} +

/*----------------------------------------------------------------------*/

@@ -5696,6 +5759,7 @@ static virNetworkDriver network_driver = { .listNetworks = remoteListNetworks, /* 0.3.0 */ .numOfDefinedNetworks = remoteNumOfDefinedNetworks, /* 0.3.0 */ .listDefinedNetworks = remoteListDefinedNetworks, /* 0.3.0 */ + .listAllNetworks = remoteConnectListAllNetworks, /* 0.10.0 */

0.10.2

.networkLookupByUUID = remoteNetworkLookupByUUID, /* 0.3.0 */ .networkLookupByName = remoteNetworkLookupByName, /* 0.3.0 */ .networkCreateXML = remoteNetworkCreateXML, /* 0.3.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index e540167..12ecfd7 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2579,6 +2579,16 @@ struct remote_storage_pool_list_all_volumes_ret { unsigned int ret; };

+struct remote_connect_list_all_networks_args { + int need_results; + unsigned int flags; +}; + +struct remote_connect_list_all_networks_ret { + remote_nonnull_network nets<>; + unsigned int ret; +}; + /*----- Protocol. -----*/

/* Define the program number, protocol version and procedure numbers here. */ @@ -2911,7 +2921,8 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_EMULATOR_PIN_INFO = 280, /* skipgen skipgen */

REMOTE_PROC_CONNECT_LIST_ALL_STORAGE_POOLS = 281, /* skipgen skipgen priority:high */ - REMOTE_PROC_STORAGE_POOL_LIST_ALL_VOLUMES = 282 /* skipgen skipgen priority:high */ + REMOTE_PROC_STORAGE_POOL_LIST_ALL_VOLUMES = 282, /* skipgen skipgen priority:high */ + REMOTE_PROC_CONNECT_LIST_ALL_NETWORKS = 283 /* skipgen skipgen priority:high */

/* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index f105a38..45262f8 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2034,6 +2034,17 @@ struct remote_storage_pool_list_all_volumes_ret { } vols; u_int ret; }; +struct remote_list_all_networks_args { + int need_results; + u_int flags; +}; +struct remote_list_all_networks_ret { + struct { + u_int nets_len; + remote_nonnull_network * nets_val; + } nets; + u_int ret; +}; enum remote_procedure { REMOTE_PROC_OPEN = 1, REMOTE_PROC_CLOSE = 2, @@ -2317,4 +2328,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_GET_EMULATOR_PIN_INFO = 280, REMOTE_PROC_CONNECT_LIST_ALL_STORAGE_POOLS = 281, REMOTE_PROC_STORAGE_POOL_LIST_ALL_VOLUMES = 282, + REMOTE_PROC_CONNECT_LIST_ALL_NETWORKS = 283, };

ACK aside from the one small version number change.

On 09/04/2012 11:55 AM, Osier Yang wrote:

src/conf/network_conf.c: Add virNetworkMatch to filter the networks; and virNetworkList to iterate over all the networks with the filter.

src/conf/network_conf.h: Declare virNetworkList and define the macros for filters.

src/libvirt_private.syms: Export virNetworkList. --- src/conf/network_conf.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 22 +++++++++++ src/libvirt_private.syms | 1 + 3 files changed, 114 insertions(+), 0 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9d53d8e..1ee0951 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2056,3 +2056,94 @@ void virNetworkObjUnlock(virNetworkObjPtr obj) { virMutexUnlock(&obj->lock); } + +#define MATCH(FLAG) (flags & (FLAG)) +static bool +virNetworkMatch (virNetworkObjPtr netobj, + unsigned int flags) +{ + /* filter by active state */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) && + !((MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE) && + virNetworkObjIsActive(netobj)) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_INACTIVE) && + !virNetworkObjIsActive(netobj)))) + return false; + + /* filter by persistence */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT) && + !((MATCH(VIR_CONNECT_LIST_NETWORKS_PERSISTENT) && + netobj->persistent) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_TRANSIENT) && + !netobj->persistent))) + return false; + + /* filter by autostart option */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART) && + !((MATCH(VIR_CONNECT_LIST_NETWORKS_AUTOSTART) && + netobj->autostart) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART) && + !netobj->autostart))) + return false; + + return true; +} +#undef MATCH + +int +virNetworkList(virConnectPtr conn, + virNetworkObjList netobjs, + virNetworkPtr **nets, + unsigned int flags) +{ + virNetworkPtr *tmp_nets = NULL; + virNetworkPtr net = NULL; + int nnets = 0; + int ret = -1; + int i; + + if (nets) { + if (VIR_ALLOC_N(tmp_nets, netobjs.count + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + } + + for (i = 0; i < netobjs.count; i++) { + virNetworkObjPtr netobj = netobjs.objs[i]; + virNetworkObjLock(netobj); + if (virNetworkMatch(netobj, flags)) { + if (nets) { + if (!(net = virGetNetwork(conn, + netobj->def->name, + netobj->def->uuid))) { + virNetworkObjUnlock(netobj); + goto cleanup; + } + tmp_nets[nnets] = net; + } + nnets++; + } + virNetworkObjUnlock(netobj); + } + + if (tmp_nets) { + /* trim the array to the final size */ + ignore_value(VIR_REALLOC_N(tmp_nets, nnets + 1)); + *nets = tmp_nets; + tmp_nets = NULL; + } + + ret = nnets; + +cleanup: + if (tmp_nets) { + for (i = 0; i < nnets; i++) { + if (tmp_nets[i]) + virNetworkFree(tmp_nets[i]); + } + } + + VIR_FREE(tmp_nets); + return ret; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index f49c367..5dbf50d 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -321,4 +321,26 @@ void virNetworkObjUnlock(virNetworkObjPtr obj);

VIR_ENUM_DECL(virNetworkForward)

+# define VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE \ + (VIR_CONNECT_LIST_NETWORKS_ACTIVE | \ + VIR_CONNECT_LIST_NETWORKS_INACTIVE) + +# define VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT \ + (VIR_CONNECT_LIST_NETWORKS_PERSISTENT | \ + VIR_CONNECT_LIST_NETWORKS_TRANSIENT) + +# define VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART \ + (VIR_CONNECT_LIST_NETWORKS_AUTOSTART | \ + VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART) + +# define VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL \ + (VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE | \ + VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT | \ + VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART) + +int virNetworkList(virConnectPtr conn, + virNetworkObjList netobjs, + virNetworkPtr **nets, + unsigned int flags); + #endif /* __NETWORK_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index abfee47..7464c59 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -837,6 +837,7 @@ virNetworkFindByName; virNetworkFindByUUID; virNetworkIpDefNetmask; virNetworkIpDefPrefix; +virNetworkList; virNetworkLoadAllConfigs; virNetworkObjIsDuplicate; virNetworkObjListFree;

ACK.

On 09/04/2012 11:55 AM, Osier Yang wrote:

src/network/bridge_driver.c: Implement listAllNetworks. --- src/network/bridge_driver.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 53eebed..73ad43f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2263,6 +2263,22 @@ static int networkListDefinedNetworks(virConnectPtr conn, char **const names, in return -1; }

+static int +networkListAllNetworks(virConnectPtr conn, + virNetworkPtr **nets, + unsigned int flags) +{ + struct network_driver *driver = conn->networkPrivateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); + + networkDriverLock(driver); + ret = virNetworkList(conn, driver->networks, nets, flags); + networkDriverUnlock(driver); + + return ret; +}

static int networkIsActive(virNetworkPtr net) { @@ -2793,6 +2809,7 @@ static virNetworkDriver networkDriver = { .listNetworks = networkListNetworks, /* 0.2.0 */ .numOfDefinedNetworks = networkNumDefinedNetworks, /* 0.2.0 */ .listDefinedNetworks = networkListDefinedNetworks, /* 0.2.0 */ + .listAllNetworks = networkListAllNetworks, /* 0.10.0 */

version number again. Otherwise ACK.

.networkLookupByUUID = networkLookupByUUID, /* 0.2.0 */ .networkLookupByName = networkLookupByName, /* 0.2.0 */ .networkCreateXML = networkCreate, /* 0.2.0 */

On 09/04/2012 11:55 AM, Osier Yang wrote:

src/test/test_driver.c: Implement listAllNetworks. --- src/test/test_driver.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 8d93129..821cf72 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3039,6 +3039,22 @@ no_memory: return -1; }

+static int +testNetworkListAllNetworks(virConnectPtr conn, + virNetworkPtr **nets, + unsigned int flags) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); + + testDriverLock(privconn); + ret = virNetworkList(conn, privconn->networks, nets, flags); + testDriverUnlock(privconn); + + return ret; +}

static int testNetworkIsActive(virNetworkPtr net) { @@ -5698,6 +5714,7 @@ static virNetworkDriver testNetworkDriver = { .listNetworks = testListNetworks, /* 0.3.2 */ .numOfDefinedNetworks = testNumDefinedNetworks, /* 0.3.2 */ .listDefinedNetworks = testListDefinedNetworks, /* 0.3.2 */ + .listAllNetworks = testNetworkListAllNetworks, /* 0.10.0 */

I think I can stop pointing this out now, and just assume you'll recheck all the version numbers before you push :-)

.networkLookupByUUID = testLookupNetworkByUUID, /* 0.3.2 */ .networkLookupByName = testLookupNetworkByName, /* 0.3.2 */ .networkCreateXML = testNetworkCreate, /* 0.3.2 */

ACK.

On 09/04/2012 11:55 AM, Osier Yang wrote:

tools/virsh-network.c: * vshNetworkSorter to sort networks by name

* vshNetworkListFree to free the network objects list.

* vshNetworkListCollect to collect the network objects, trying to use new API first, fall back to older APIs if it's not supported.

* New options --persistent, --transient, --autostart, --no-autostart, for net-list, and new field 'Persistent' for its output.

tools/virsh.pod: * Add documents for the new options. --- tools/virsh-network.c | 352 ++++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 12 ++- 2 files changed, 275 insertions(+), 89 deletions(-)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c index db204af..f6623ff 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/network_conf.h"

I've gotta say that (as discussed before) I don't like including something from the conf directory here. I think it's the case that this is only being done so that virsh can provide the new functionality even when only the old API is available, but I think it should be done in a self-contained manner, at least partly because people will look to virsh as an example of how to use the libvirt API. I guess I'm okay with leaving it this way for now, but I think it really needs to be cleaned up.

virNetworkPtr vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, @@ -342,6 +343,225 @@ cmdNetworkInfo(vshControl *ctl, const vshCmd *cmd) return true; }

+static int +vshNetworkSorter(const void *a, const void *b) +{ + virNetworkPtr *na = (virNetworkPtr *) a; + virNetworkPtr *nb = (virNetworkPtr *) b; + + if (*na && !*nb) + return -1; + + if (!*na) + return *nb != NULL; + + return vshStrcasecmp(virNetworkGetName(*na), + virNetworkGetName(*nb));

This use of strcasecmp mmade me go back to verify that case *is* significant in the rest of the network_conf code (so, for example, you can have both a network named "ABC" and one named "Abc"). It's still useful to have strcasecmp here, though, as it makes the ordering ignore case, which is a "good thing"(tm).

+} + +struct vshNetworkList { + virNetworkPtr *nets; + size_t nnets; +}; +typedef struct vshNetworkList *vshNetworkListPtr;

The fact that you're doing this leaves me wondering if maybe virNetworkList should be a part of the API (along with a "virNetworkListFree()" as I've previously mentioned). After all, the fact that this first example of using the new API is doing this is a reasonably good indicator that future users will be copying the same code.

+ +static void +vshNetworkListFree(vshNetworkListPtr list) +{ + int i; + + if (list && list->nnets) { + for (i = 0; i < list->nnets; i++) { + if (list->nets[i]) + virNetworkFree(list->nets[i]); + } + VIR_FREE(list->nets); + } + VIR_FREE(list); +} + +static vshNetworkListPtr +vshNetworkListCollect(vshControl *ctl, + unsigned int flags) +{ + vshNetworkListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virNetworkPtr net; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActiveNets = 0; + int nInactiveNets = 0; + int nAllNets = 0; + + /* try the list with flags support (0.10.0 and later) */ + if ((ret = virConnectListAllNetworks(ctl->conn, + &list->nets, + flags)) >= 0) { + list->nnets = ret; + goto finished; + } + + /* check if the command is actually supported */ + if (last_error && last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + goto fallback; + } + + if (last_error && last_error->code == VIR_ERR_INVALID_ARG) { + /* try the new API again but mask non-guaranteed flags */ + unsigned int newflags = flags & (VIR_CONNECT_LIST_NETWORKS_ACTIVE | + VIR_CONNECT_LIST_NETWORKS_INACTIVE); + + vshResetLibvirtError(); + if ((ret = virConnectListAllNetworks(ctl->conn, &list->nets, + newflags)) >= 0) { + list->nnets = ret; + goto filter; + } + } + + /* there was an error during the first or second call */ + vshError(ctl, "%s", _("Failed to list networks")); + goto cleanup; + + +fallback: + /* fall back to old method (0.9.13 and older) */

Well, okay, I'll mention this incorrect version number, because it's a different number than the others.

+ vshResetLibvirtError();

As was pointed out in the nwfilter patches, this 2nd vshResetLibvirtError() is redundant; you only need it in one place or the other, but not both.

+ + /* Get the number of active networks */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) { + if ((nActiveNets = virConnectNumOfNetworks(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to get the number of active networks")); + goto cleanup; + } + } + + /* Get the number of inactive networks */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_INACTIVE)) { + if ((nInactiveNets = virConnectNumOfDefinedNetworks(ctl->conn)) < 0) { + vshError(ctl, "%s", _("Failed to get the number of inactive networks")); + goto cleanup; + } + } + + nAllNets = nActiveNets + nInactiveNets; + + if (nAllNets == 0) + return list; + + names = vshMalloc(ctl, sizeof(char *) * nAllNets); + + /* Retrieve a list of active network names */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) { + if (virConnectListNetworks(ctl->conn, + names, nActiveNets) < 0) { + vshError(ctl, "%s", _("Failed to list active networks")); + goto cleanup; + } + } + + /* Add the inactive networks to the end of the name list */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) { + if (virConnectListDefinedNetworks(ctl->conn, + &names[nActiveNets], + nInactiveNets) < 0) { + vshError(ctl, "%s", _("Failed to list inactive networks")); + goto cleanup; + } + } + + list->nets = vshMalloc(ctl, sizeof(virNetworkPtr) * (nAllNets)); + list->nnets = 0; + + /* get active networks */ + for (i = 0; i < nActiveNets; i++) { + if (!(net = virNetworkLookupByName(ctl->conn, names[i]))) + continue; + list->nets[list->nnets++] = net; + } + + /* get inactive networks */ + for (i = 0; i < nInactiveNets; i++) { + if (!(net = virNetworkLookupByName(ctl->conn, names[i]))) + continue; + list->nets[list->nnets++] = net; + } + + /* truncate networks that weren't found */ + deleted = nAllNets - list->nnets; + +filter: + /* filter list the list if the list was acquired by fallback means */ + for (i = 0; i < list->nnets; i++) { + net = list->nets[i]; + + /* persistence filter */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT)) { + if ((persistent = virNetworkIsPersistent(net)) < 0) { + vshError(ctl, "%s", _("Failed to get network persistence info")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_PERSISTENT) && persistent) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_TRANSIENT) && !persistent))) + goto remove_entry; + } + + /* autostart filter */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART)) { + if (virNetworkGetAutostart(net, &autostart) < 0) { + vshError(ctl, "%s", _("Failed to get network autostart state")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_AUTOSTART) && autostart) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART) && !autostart))) + goto remove_entry; + } + /* the pool matched all filters, it may stay */ + continue; + +remove_entry: + /* the pool has to be removed as it failed one of the filters */ + virNetworkFree(list->nets[i]); + list->nets[i] = NULL; + deleted++; + } + +finished: + /* sort the list */ + if (list->nets && list->nnets) + qsort(list->nets, list->nnets, + sizeof(*list->nets), vshNetworkSorter); + + /* truncate the list if filter simulation deleted entries */ + if (deleted) + VIR_SHRINK_N(list->nets, list->nnets, deleted); + + success = true; + +cleanup: + for (i = 0; i < nAllNets; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + + if (!success) { + vshNetworkListFree(list); + list = NULL; + } + + return list; +} + /* * "net-list" command */ @@ -354,114 +574,70 @@ static const vshCmdInfo info_network_list[] = { static const vshCmdOptDef opts_network_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive networks")}, {"all", VSH_OT_BOOL, 0, N_("list inactive & active networks")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent networks")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient networks")}, + {"autostart", VSH_OT_BOOL, 0, N_("list networks with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list networks with autostart disabled")}, {NULL, 0, 0, NULL} };

static bool cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + vshNetworkListPtr list = NULL; + int i; bool inactive = vshCommandOptBool(cmd, "inactive"); bool all = vshCommandOptBool(cmd, "all"); - bool active = !inactive || all; - int maxactive = 0, maxinactive = 0, i; - char **activeNames = NULL, **inactiveNames = NULL; - inactive |= all; - - if (active) { - maxactive = virConnectNumOfNetworks(ctl->conn); - if (maxactive < 0) { - vshError(ctl, "%s", _("Failed to list active networks")); - return false; - } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); - - if ((maxactive = virConnectListNetworks(ctl->conn, activeNames, - maxactive)) < 0) { - vshError(ctl, "%s", _("Failed to list active networks")); - VIR_FREE(activeNames); - return false; - } + bool persistent = vshCommandOptBool(cmd, "persistent"); + bool transient = vshCommandOptBool(cmd, "transient"); + bool autostart = vshCommandOptBool(cmd, "autostart"); + bool no_autostart = vshCommandOptBool(cmd, "no-autostart"); + unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;

- qsort(&activeNames[0], maxactive, sizeof(char *), vshNameSorter); - } - } - if (inactive) { - maxinactive = virConnectNumOfDefinedNetworks(ctl->conn); - if (maxinactive < 0) { - vshError(ctl, "%s", _("Failed to list inactive networks")); - VIR_FREE(activeNames); - return false; - } - if (maxinactive) { - inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive); - - if ((maxinactive = - virConnectListDefinedNetworks(ctl->conn, inactiveNames, - maxinactive)) < 0) { - vshError(ctl, "%s", _("Failed to list inactive networks")); - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return false; - } + if (inactive) + flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;

- qsort(&inactiveNames[0], maxinactive, sizeof(char*), vshNameSorter); - } - } - vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"), - _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); + if (all) + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE | + VIR_CONNECT_LIST_NETWORKS_INACTIVE;

- for (i = 0; i < maxactive; i++) { - virNetworkPtr network = - virNetworkLookupByName(ctl->conn, activeNames[i]); - const char *autostartStr; - int autostart = 0; + if (persistent) + flags |= VIR_CONNECT_LIST_NETWORKS_PERSISTENT;

- /* this kind of work with networks is not atomic operation */ - if (!network) { - VIR_FREE(activeNames[i]); - continue; - } + if (transient) + flags |= VIR_CONNECT_LIST_NETWORKS_TRANSIENT;

- if (virNetworkGetAutostart(network, &autostart) < 0) - autostartStr = _("no autostart"); - else - autostartStr = autostart ? _("yes") : _("no"); + if (autostart) + flags |= VIR_CONNECT_LIST_NETWORKS_AUTOSTART;

- vshPrint(ctl, "%-20s %-10s %-10s\n", - virNetworkGetName(network), - _("active"), - autostartStr); - virNetworkFree(network); - VIR_FREE(activeNames[i]); - } - for (i = 0; i < maxinactive; i++) { - virNetworkPtr network = virNetworkLookupByName(ctl->conn, inactiveNames[i]); - const char *autostartStr; - int autostart = 0; + if (no_autostart) + flags |= VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART;

- /* this kind of work with networks is not atomic operation */ - if (!network) { - VIR_FREE(inactiveNames[i]); - continue; - } + if (!(list = vshNetworkListCollect(ctl, flags))) + return false; + + vshPrintExtra(ctl, "%-20s %-10s %-13s %s\n", _("Name"), _("State"), + _("Autostart"), _("Persistent")); + vshPrintExtra(ctl, "--------------------------------------------------\n");

Do you think we need to worry about adding a new column onto the output having an adverse affect on existing scripts that parse the output?

- if (virNetworkGetAutostart(network, &autostart) < 0) + for (i = 0; i < list->nnets; i++) { + virNetworkPtr network = list->nets[i]; + const char *autostartStr; + int is_autostart = 0; + + if (virNetworkGetAutostart(network, &is_autostart) < 0) autostartStr = _("no autostart"); else - autostartStr = autostart ? _("yes") : _("no"); - - vshPrint(ctl, "%-20s %-10s %-10s\n", - inactiveNames[i], - _("inactive"), - autostartStr); + autostartStr = is_autostart ? _("yes") : _("no");

- virNetworkFree(network); - VIR_FREE(inactiveNames[i]); + vshPrint(ctl, "%-20s %-10s %-13s %s\n", + virNetworkGetName(network), + virNetworkIsActive(network) ? _("active") : _("inactive"), + autostartStr, + virNetworkIsPersistent(network) ? _("yes") : _("no")); } - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); + + vshNetworkListFree(list); return true; }

diff --git a/tools/virsh.pod b/tools/virsh.pod index 9aeb47e..c806335 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1933,10 +1933,20 @@ variables, and defaults to C<vi>. Returns basic information about the I<network> object.

=item B<net-list> [I<--inactive> | I<--all>] + [I<--persistent>] [<--transient>] + [I<--autostart>] [<--no-autostart>]

Returns the list of active networks, if I<--all> is specified this will also include defined but inactive networks, if I<--inactive> is specified only the -inactive ones will be listed. +inactive ones will be listed. You may also want to filter the returned networks +by I<--persistent> to list the persitent ones, I<--transient> to list the +transient ones, I<--autostart> to list the ones with autostart enabled, and +I<--no-autostart> to list the ones with autostart disabled. + +NOTE: When talking to older servers, this command is forced to use a series of +API calls with an inherent race, where a pool might not be listed or might appear +more than once if it changed state between calls while the list was being +collected. Newer servers do not have this problem.

=item B<net-name> I<network-UUID>

In the interest of having the API in the release, I would be okay with pushing as-is, but would rather have the questions above cleared up first.

On 09/10/2012 12:33 PM, Daniel P. Berrange wrote:

On Mon, Sep 10, 2012 at 12:29:43PM -0400, Laine Stump wrote:

...

On 09/04/2012 11:55 AM, Osier Yang wrote:

...

tools/virsh-network.c: * vshNetworkSorter to sort networks by name

* vshNetworkListFree to free the network objects list.

* vshNetworkListCollect to collect the network objects, trying to use new API first, fall back to older APIs if it's not supported.

* New options --persistent, --transient, --autostart, --no-autostart, for net-list, and new field 'Persistent' for its output.

tools/virsh.pod: * Add documents for the new options. --- tools/virsh-network.c | 352 ++++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 12 ++- 2 files changed, 275 insertions(+), 89 deletions(-)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c index db204af..f6623ff 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/network_conf.h"

I've gotta say that (as discussed before) I don't like including something from the conf directory here. I think it's the case that this is only being done so that virsh can provide the new functionality even when only the old API is available, but I think it should be done in a self-contained manner, at least partly because people will look to virsh as an example of how to use the libvirt API. I guess I'm okay with leaving it this way for now, but I think it really needs to be cleaned up. I don't see why the fallback code needs to use this include either, since it must surely still be just using older public APIs, not internal code.

I haven't investigated but have an inkling that it's likely used to avoid retyping some #defines of combined flags or something like that.

On 2012年09月11日 00:29, Laine Stump wrote:

On 09/04/2012 11:55 AM, Osier Yang wrote:

...

tools/virsh-network.c: * vshNetworkSorter to sort networks by name

* vshNetworkListFree to free the network objects list.

* vshNetworkListCollect to collect the network objects, trying to use new API first, fall back to older APIs if it's not supported.

* New options --persistent, --transient, --autostart, --no-autostart, for net-list, and new field 'Persistent' for its output.

tools/virsh.pod: * Add documents for the new options. --- tools/virsh-network.c | 352 ++++++++++++++++++++++++++++++++++++------------ tools/virsh.pod | 12 ++- 2 files changed, 275 insertions(+), 89 deletions(-)

diff --git a/tools/virsh-network.c b/tools/virsh-network.c index db204af..f6623ff 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -36,6 +36,7 @@ #include "memory.h" #include "util.h" #include "xml.h" +#include "conf/network_conf.h"

I've gotta say that (as discussed before) I don't like including something from the conf directory here. I think it's the case that this is only being done so that virsh can provide the new functionality even when only the old API is available, but I think it should be done in a self-contained manner, at least partly because people will look to virsh as an example of how to use the libvirt API. I guess I'm okay with leaving it this way for now, but I think it really needs to be cleaned up.

Yes, that's same problem in the whole series. I will cleanup later.

...

virNetworkPtr vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd, @@ -342,6 +343,225 @@ cmdNetworkInfo(vshControl *ctl, const vshCmd *cmd) return true; }

+static int +vshNetworkSorter(const void *a, const void *b) +{ + virNetworkPtr *na = (virNetworkPtr *) a; + virNetworkPtr *nb = (virNetworkPtr *) b; + + if (*na&& !*nb) + return -1; + + if (!*na) + return *nb != NULL; + + return vshStrcasecmp(virNetworkGetName(*na), + virNetworkGetName(*nb));

This use of strcasecmp mmade me go back to verify that case *is* significant in the rest of the network_conf code (so, for example, you can have both a network named "ABC" and one named "Abc"). It's still useful to have strcasecmp here, though, as it makes the ordering ignore case, which is a "good thing"(tm).

Okay. :-)

...

+} + +struct vshNetworkList { + virNetworkPtr *nets; + size_t nnets; +}; +typedef struct vshNetworkList *vshNetworkListPtr;

The fact that you're doing this leaves me wondering if maybe virNetworkList should be a part of the API (along with a "virNetworkListFree()" as I've previously mentioned). After all, the fact that this first example of using the new API is doing this is a reasonably good indicator that future users will be copying the same code.

I still think it should be the work of the client apps (e.g. virsh here).

...

+ +static void +vshNetworkListFree(vshNetworkListPtr list) +{ + int i; + + if (list&& list->nnets) { + for (i = 0; i< list->nnets; i++) { + if (list->nets[i]) + virNetworkFree(list->nets[i]); + } + VIR_FREE(list->nets); + } + VIR_FREE(list); +} + +static vshNetworkListPtr +vshNetworkListCollect(vshControl *ctl, + unsigned int flags) +{ + vshNetworkListPtr list = vshMalloc(ctl, sizeof(*list)); + int i; + int ret; + char **names = NULL; + virNetworkPtr net; + bool success = false; + size_t deleted = 0; + int persistent; + int autostart; + int nActiveNets = 0; + int nInactiveNets = 0; + int nAllNets = 0; + + /* try the list with flags support (0.10.0 and later) */ + if ((ret = virConnectListAllNetworks(ctl->conn, +&list->nets, + flags))>= 0) { + list->nnets = ret; + goto finished; + } + + /* check if the command is actually supported */ + if (last_error&& last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + goto fallback; + } + + if (last_error&& last_error->code == VIR_ERR_INVALID_ARG) { + /* try the new API again but mask non-guaranteed flags */ + unsigned int newflags = flags& (VIR_CONNECT_LIST_NETWORKS_ACTIVE | + VIR_CONNECT_LIST_NETWORKS_INACTIVE); + + vshResetLibvirtError(); + if ((ret = virConnectListAllNetworks(ctl->conn,&list->nets, + newflags))>= 0) { + list->nnets = ret; + goto filter; + } + } + + /* there was an error during the first or second call */ + vshError(ctl, "%s", _("Failed to list networks")); + goto cleanup; + + +fallback: + /* fall back to old method (0.9.13 and older) */

Well, okay, I'll mention this incorrect version number, because it's a different number than the others.

Okay. Will update when pushing.

...

+ vshResetLibvirtError();

As was pointed out in the nwfilter patches, this 2nd vshResetLibvirtError() is redundant; you only need it in one place or the other, but not both.

Okay. Will ... pushing.

...

+ + /* Get the number of active networks */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) { + if ((nActiveNets = virConnectNumOfNetworks(ctl->conn))< 0) { + vshError(ctl, "%s", _("Failed to get the number of active networks")); + goto cleanup; + } + } + + /* Get the number of inactive networks */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_INACTIVE)) { + if ((nInactiveNets = virConnectNumOfDefinedNetworks(ctl->conn))< 0) { + vshError(ctl, "%s", _("Failed to get the number of inactive networks")); + goto cleanup; + } + } + + nAllNets = nActiveNets + nInactiveNets; + + if (nAllNets == 0) + return list; + + names = vshMalloc(ctl, sizeof(char *) * nAllNets); + + /* Retrieve a list of active network names */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) { + if (virConnectListNetworks(ctl->conn, + names, nActiveNets)< 0) { + vshError(ctl, "%s", _("Failed to list active networks")); + goto cleanup; + } + } + + /* Add the inactive networks to the end of the name list */ + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) || + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) { + if (virConnectListDefinedNetworks(ctl->conn, +&names[nActiveNets], + nInactiveNets)< 0) { + vshError(ctl, "%s", _("Failed to list inactive networks")); + goto cleanup; + } + } + + list->nets = vshMalloc(ctl, sizeof(virNetworkPtr) * (nAllNets)); + list->nnets = 0; + + /* get active networks */ + for (i = 0; i< nActiveNets; i++) { + if (!(net = virNetworkLookupByName(ctl->conn, names[i]))) + continue; + list->nets[list->nnets++] = net; + } + + /* get inactive networks */ + for (i = 0; i< nInactiveNets; i++) { + if (!(net = virNetworkLookupByName(ctl->conn, names[i]))) + continue; + list->nets[list->nnets++] = net; + } + + /* truncate networks that weren't found */ + deleted = nAllNets - list->nnets; + +filter: + /* filter list the list if the list was acquired by fallback means */ + for (i = 0; i< list->nnets; i++) { + net = list->nets[i]; + + /* persistence filter */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT)) { + if ((persistent = virNetworkIsPersistent(net))< 0) { + vshError(ctl, "%s", _("Failed to get network persistence info")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_PERSISTENT)&& persistent) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_TRANSIENT)&& !persistent))) + goto remove_entry; + } + + /* autostart filter */ + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART)) { + if (virNetworkGetAutostart(net,&autostart)< 0) { + vshError(ctl, "%s", _("Failed to get network autostart state")); + goto cleanup; + } + + if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_AUTOSTART)&& autostart) || + (MATCH(VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART)&& !autostart))) + goto remove_entry; + } + /* the pool matched all filters, it may stay */ + continue; + +remove_entry: + /* the pool has to be removed as it failed one of the filters */ + virNetworkFree(list->nets[i]); + list->nets[i] = NULL; + deleted++; + } + +finished: + /* sort the list */ + if (list->nets&& list->nnets) + qsort(list->nets, list->nnets, + sizeof(*list->nets), vshNetworkSorter); + + /* truncate the list if filter simulation deleted entries */ + if (deleted) + VIR_SHRINK_N(list->nets, list->nnets, deleted); + + success = true; + +cleanup: + for (i = 0; i< nAllNets; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + + if (!success) { + vshNetworkListFree(list); + list = NULL; + } + + return list; +} + /* * "net-list" command */ @@ -354,114 +574,70 @@ static const vshCmdInfo info_network_list[] = { static const vshCmdOptDef opts_network_list[] = { {"inactive", VSH_OT_BOOL, 0, N_("list inactive networks")}, {"all", VSH_OT_BOOL, 0, N_("list inactive& active networks")}, + {"persistent", VSH_OT_BOOL, 0, N_("list persistent networks")}, + {"transient", VSH_OT_BOOL, 0, N_("list transient networks")}, + {"autostart", VSH_OT_BOOL, 0, N_("list networks with autostart enabled")}, + {"no-autostart", VSH_OT_BOOL, 0, N_("list networks with autostart disabled")}, {NULL, 0, 0, NULL} };

static bool cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) { + vshNetworkListPtr list = NULL; + int i; bool inactive = vshCommandOptBool(cmd, "inactive"); bool all = vshCommandOptBool(cmd, "all"); - bool active = !inactive || all; - int maxactive = 0, maxinactive = 0, i; - char **activeNames = NULL, **inactiveNames = NULL; - inactive |= all; - - if (active) { - maxactive = virConnectNumOfNetworks(ctl->conn); - if (maxactive< 0) { - vshError(ctl, "%s", _("Failed to list active networks")); - return false; - } - if (maxactive) { - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive); - - if ((maxactive = virConnectListNetworks(ctl->conn, activeNames, - maxactive))< 0) { - vshError(ctl, "%s", _("Failed to list active networks")); - VIR_FREE(activeNames); - return false; - } + bool persistent = vshCommandOptBool(cmd, "persistent"); + bool transient = vshCommandOptBool(cmd, "transient"); + bool autostart = vshCommandOptBool(cmd, "autostart"); + bool no_autostart = vshCommandOptBool(cmd, "no-autostart"); + unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;

- qsort(&activeNames[0], maxactive, sizeof(char *), vshNameSorter); - } - } - if (inactive) { - maxinactive = virConnectNumOfDefinedNetworks(ctl->conn); - if (maxinactive< 0) { - vshError(ctl, "%s", _("Failed to list inactive networks")); - VIR_FREE(activeNames); - return false; - } - if (maxinactive) { - inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive); - - if ((maxinactive = - virConnectListDefinedNetworks(ctl->conn, inactiveNames, - maxinactive))< 0) { - vshError(ctl, "%s", _("Failed to list inactive networks")); - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); - return false; - } + if (inactive) + flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;

- qsort(&inactiveNames[0], maxinactive, sizeof(char*), vshNameSorter); - } - } - vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"), - _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n"); + if (all) + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE | + VIR_CONNECT_LIST_NETWORKS_INACTIVE;

- for (i = 0; i< maxactive; i++) { - virNetworkPtr network = - virNetworkLookupByName(ctl->conn, activeNames[i]); - const char *autostartStr; - int autostart = 0; + if (persistent) + flags |= VIR_CONNECT_LIST_NETWORKS_PERSISTENT;

- /* this kind of work with networks is not atomic operation */ - if (!network) { - VIR_FREE(activeNames[i]); - continue; - } + if (transient) + flags |= VIR_CONNECT_LIST_NETWORKS_TRANSIENT;

- if (virNetworkGetAutostart(network,&autostart)< 0) - autostartStr = _("no autostart"); - else - autostartStr = autostart ? _("yes") : _("no"); + if (autostart) + flags |= VIR_CONNECT_LIST_NETWORKS_AUTOSTART;

- vshPrint(ctl, "%-20s %-10s %-10s\n", - virNetworkGetName(network), - _("active"), - autostartStr); - virNetworkFree(network); - VIR_FREE(activeNames[i]); - } - for (i = 0; i< maxinactive; i++) { - virNetworkPtr network = virNetworkLookupByName(ctl->conn, inactiveNames[i]); - const char *autostartStr; - int autostart = 0; + if (no_autostart) + flags |= VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART;

- /* this kind of work with networks is not atomic operation */ - if (!network) { - VIR_FREE(inactiveNames[i]); - continue; - } + if (!(list = vshNetworkListCollect(ctl, flags))) + return false; + + vshPrintExtra(ctl, "%-20s %-10s %-13s %s\n", _("Name"), _("State"), + _("Autostart"), _("Persistent")); + vshPrintExtra(ctl, "--------------------------------------------------\n");

Do you think we need to worry about adding a new column onto the output having an adverse affect on existing scripts that parse the output?

Yes, we need to consider the existed script. But compared to the old output, we only add a new field "Persistent" at the end, not breaking the old field sequense.

...

- vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"), - _("Autostart")); - vshPrintExtra(ctl, "-----------------------------------------\n");

So I think it's fine.

...

- if (virNetworkGetAutostart(network,&autostart)< 0) + for (i = 0; i< list->nnets; i++) { + virNetworkPtr network = list->nets[i]; + const char *autostartStr; + int is_autostart = 0; + + if (virNetworkGetAutostart(network,&is_autostart)< 0) autostartStr = _("no autostart"); else - autostartStr = autostart ? _("yes") : _("no"); - - vshPrint(ctl, "%-20s %-10s %-10s\n", - inactiveNames[i], - _("inactive"), - autostartStr); + autostartStr = is_autostart ? _("yes") : _("no");

- virNetworkFree(network); - VIR_FREE(inactiveNames[i]); + vshPrint(ctl, "%-20s %-10s %-13s %s\n", + virNetworkGetName(network), + virNetworkIsActive(network) ? _("active") : _("inactive"), + autostartStr, + virNetworkIsPersistent(network) ? _("yes") : _("no")); } - VIR_FREE(activeNames); - VIR_FREE(inactiveNames); + + vshNetworkListFree(list); return true; }

diff --git a/tools/virsh.pod b/tools/virsh.pod index 9aeb47e..c806335 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1933,10 +1933,20 @@ variables, and defaults to C<vi>. Returns basic information about the I<network> object.

=item B<net-list> [I<--inactive> | I<--all>] + [I<--persistent>] [<--transient>] + [I<--autostart>] [<--no-autostart>]

Returns the list of active networks, if I<--all> is specified this will also include defined but inactive networks, if I<--inactive> is specified only the -inactive ones will be listed. +inactive ones will be listed. You may also want to filter the returned networks +by I<--persistent> to list the persitent ones, I<--transient> to list the +transient ones, I<--autostart> to list the ones with autostart enabled, and +I<--no-autostart> to list the ones with autostart disabled. + +NOTE: When talking to older servers, this command is forced to use a series of +API calls with an inherent race, where a pool might not be listed or might appear +more than once if it changed state between calls while the list was being +collected. Newer servers do not have this problem.

=item B<net-name> I<network-UUID>

In the interest of having the API in the release, I would be okay with pushing as-is, but would rather have the questions above cleared up first.

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list