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.)
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.
>
> Or you prefer to have separate files like virDomainList.[ch],
> virStorageList.[ch], ..etc?
No, I think that's overkill.
> 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)
> 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