[libvirt] [PATCH 22/49] list: Add helpers to list network objects

Osier Yang jyang at redhat.com
Tue Jul 24 05:09:08 UTC 2012


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




More information about the libvir-list mailing list