12:09 a.m.
On 2012年07月24日 04:53, Laine Stump wrote:
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.
Thanks for the reviewing, Laine.
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 see what you concern, and agree with you driver specific stuffs
should be separated into each driver.
But for these list stuffs, I insist that keeping those function in
a common file is better. They are of specific drivers indeed, but
in common from function point of view.
Or you prefer to have separate files like virDomainList.[ch],
virStorageList.[ch], ..etc? Or folder the functions into
conf/domain_conf.[ch], conf/storage_conf.[ch], etc? IMO either
is not better than keeping them in common place. :-)
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"?
I intended to keep consistent with other funcs in virobjectlist.c.
If finally we prefer to have driver specific funcs separately,
I will update it to "networks".
> + 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.
One design of the list APIs is to only return the object number. So
the index is always needed. But this could be optimized as:
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< 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).
Agreed.
Regards,
Osier