[libvirt] [PATCH 22/49] list: Add helpers to list network objects
Osier Yang
jyang at redhat.com
Wed Jul 25 02:52:47 UTC 2012
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list