On 2012年07月25日 01:22, Laine Stump wrote:
On 07/24/2012 01:09 AM, Osier Yang wrote:
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.
"function point of view" how? Because they all build a list of pointers
to objects? That argument doesn't hold - should all functions in libvirt
that build a list of pointers to objects be put into the same source
file? To be hyperbolic about it - should we put all functions that
acquire and release a lock in the same file?
Organizing the code in the manner you propose leads to a source tree
where adding a new API requires touching an incredibly long list of
files, and makes it difficult to split out pieces for use elsewhere.
(I know that's already the case for several other pieces of libvirt, but
why make the list longer? Each separate subsystem should be as
self-contained as possible.)
Too much questions, and guess I will be trapped if saying no. So you
win.
If you're concerned about maintenance of multiple functions that are all
mostly a copy-paste of each other, rather than grouping all those
functions together in one file, refactor it so that they all call a
common function with appropriate args (which may require some
callbacks). If the resulting utility function is more difficult to
understand than the current straightforward functions, then it's not
worth the bother.
I thought in that way when doing the patches, and don't think it worths.
Or you prefer to have separate files like virDomainList.[ch],
virStorageList.[ch], ..etc?
No, I think that's overkill.
Yeah, that was the mainly reason for I intended to created a common
file.
Or folder the functions into
conf/domain_conf.[ch], conf/storage_conf.[ch], etc?
Where is the source data structure defined? If the data structure that
is used to list of domains/networks/storage pools|volumes is defined in
conf/*_conf.h, then the function that operates on that data structure to
produce the output list should be defined in conf/*_conf.c. If the data
structure is defined in ${driver}.h, then the function that operates on
it should be defined in ${driver}.c. (I think for all examples in this
current patchset, the answer is *_conf.c)
Well. You are a bit late (may be on vacation). virdomainlist.[ch]
is already there. I'd want to see a 3rd opinion on this, if finally
we want them be foldered into *_conf.c, I will be fine with it.
We need to get rid of the existed virdomainlist.[ch] first.
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++;
Never mind - I read through the code too quickly, and somehow mistakenly
saw the else clause at the level of the Match, rather than one step
further in (my brain had seen it as nnets being incremented even in the
case of no match).
I do prefer your modified version above, though.
+ }
+ }
+ 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list