On 07/20/2012 10:25 AM, Osier Yang wrote:
> src/conf/virobjectlist.c: Add virNetworkMatch to filter the networks;
> and virNetworkList to iterate over all the networks with the filter.
>
> src/conf/virobjectlist.h: Declare virNetworkList and define the macros
> for filters.
Before anything else - as I've said in a couple of earlier responses to
this series (and I won't say it for the other iterations - you can just
assume the same comment for all :-) - I don't think that driver-specific
functions (virNetworkMatch, virNetworkList) should be in a common source
file. If these functions have something in common, factor out that
common part and put *that* in a common file. If a function is specific
enough that it needs the name of the driver in the name, then it
shouldn't be in a common file.
I'll review the rest, ignoring that for the moment.
>
> src/libvirt_private.syms: Export virNetworkList.
> ---
> src/conf/virobjectlist.c | 90
> ++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/virobjectlist.h | 23 ++++++++++++
> src/libvirt_private.syms | 1 +
> 3 files changed, 114 insertions(+), 0 deletions(-)
>
> diff --git a/src/conf/virobjectlist.c b/src/conf/virobjectlist.c
> index fb5f974..83b0d9c 100644
> --- a/src/conf/virobjectlist.c
> +++ b/src/conf/virobjectlist.c
> @@ -191,6 +191,37 @@ virStoragePoolMatch (virStoragePoolObjPtr poolobj,
>
> return true;
> }
> +
> +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
> @@ -340,3 +371,62 @@ cleanup:
> VIR_FREE(tmp_pools);
> return ret;
> }
> +
> +int
> +virNetworkList(virConnectPtr conn,
> + virNetworkObjList netobjs,
Minor point - to be consistent with naming in existing network driver
code, why not call it "networks" rather than "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;
> + } else {
> + nnets++;
I don't think you want this else clause. the index on netobjs (i) is
incremented by the for(), and you only want the index on the output list
to be incremented when you actually add something to it.
> + }
> + }
> + 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 < netobjs.count; i++) {
You only want to do this nnets times, not netobjs.count. Since it's
NULL-initialized, there's no harm, but it may foster a misunderstanding
of the code in the future).
> + if (tmp_nets[i])
> + virNetworkFree(tmp_nets[i]);
> + }
> + }
> +
> + VIR_FREE(tmp_nets);
> + return ret;
> +}
> diff --git a/src/conf/virobjectlist.h b/src/conf/virobjectlist.h
> index b93cd19..431635e 100644
> --- a/src/conf/virobjectlist.h
> +++ b/src/conf/virobjectlist.h
> @@ -28,6 +28,7 @@
> # include "virhash.h"
> # include "domain_conf.h"
> # include "storage_conf.h"
> +# include "network_conf.h"
>
> # define VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE \
> (VIR_CONNECT_LIST_DOMAINS_ACTIVE | \
> @@ -105,6 +106,23 @@
> VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_AUTOSTART | \
> VIR_CONNECT_LIST_STORAGE_POOLS_FILTERS_POOL_TYPE)
>
> +# 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 virDomainList(virConnectPtr conn, virHashTablePtr domobjs,
> virDomainPtr **domains, unsigned int flags);
>
> @@ -119,4 +137,9 @@ int virStoragePoolList(virConnectPtr conn,
> virStoragePoolPtr **pools,
> unsigned int flags);
>
> +int virNetworkList(virConnectPtr conn,
> + virNetworkObjList netobjs,
> + virNetworkPtr **nets,
> + unsigned int flags);
> +
> #endif /* __VIR_OBJECT_LIST_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 18b3185..647ecf0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1246,6 +1246,7 @@ virDBusGetSystemBus;
> # virobjectlist.h
> virDomainList;
> virDomainListSnapshots;
> +virNetworkList;
> virStoragePoolList;
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
