[libvirt] [PATCH 2/3] interface: Introduce virInterfaceObjGetNames

[John Ferlan]

On 04/10/2017 04:11 AM, Michal Privoznik wrote:
> On 04/06/2017 04:08 PM, John Ferlan wrote:
>> Unlike other drivers, this is a test driver only API. Still combining
>> the logic of testConnectListInterfaces and
>> testConnectListDefinedInterfaces
>> makes things a bit easier in the long run.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/virinterfaceobj.c | 34 +++++++++++++++++++++++++++++
>>  src/conf/virinterfaceobj.h |  6 ++++++
>>  src/libvirt_private.syms   |  1 +
>>  src/test/test_driver.c     | 54
>> +++++++++++-----------------------------------
>>  4 files changed, 53 insertions(+), 42 deletions(-)
>>
>> diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c
>> index 0407c1f..229226a 100644
>> --- a/src/conf/virinterfaceobj.c
>> +++ b/src/conf/virinterfaceobj.c
>> @@ -235,3 +235,37 @@
>> virInterfaceObjNumOfInterfaces(virInterfaceObjListPtr interfaces,
>>
>>      return ninterfaces;
>>  }
>> +
>> +
>> +int
>> +virInterfaceObjGetNames(virInterfaceObjListPtr interfaces,
>> +                        bool wantActive,
>> +                        char **const names,
>> +                        int maxnames)
>> +{
>> +    int nnames = 0;
>> +    size_t i;
>> +
>> +    for (i = 0; i < interfaces->count && nnames < maxnames; i++) {
>> +        virInterfaceObjPtr obj = interfaces->objs[i];
>> +        virInterfaceObjLock(obj);
>> +        if ((wantActive && virInterfaceObjIsActive(obj)) ||
>> +            (!wantActive && !virInterfaceObjIsActive(obj))) {
> 
> Again. if (wantActive == virInterfaceObjIsActive()) ...
> 
>> +            if (VIR_STRDUP(names[nnames], obj->def->name) < 0) {
>> +                virInterfaceObjUnlock(obj);
>> +                goto failure;
>> +            }
>> +            nnames++;
>> +        }
>> +        virInterfaceObjUnlock(obj);
>> +    }
>> +
>> +    return nnames;
>> +
>> + failure:
>> +    while (--nnames >= 0)
>> +        VIR_FREE(names[nnames]);
>> +    memset(names, 0, maxnames * sizeof(*names));
> 
> This isn't necessary. Firstly, VIR_FREE() already sets all those items
> in the array we've touched to zero. Secondly, the callers already clear
> our the array (even those items we haven't touched). But mostly, in
> general, if an error is returned, callers should not rely on anything
> else that is returned. For instance, should virStrToLong_*(s, &ret,
> base, &result) return -1, the @result is undefined.
> 

OK - that's fine... While going through this exercise of convergence I
have found some places that did this (nwfilter, storage pool/volume) and
others that don't.  The "goal" was to be consistent for all.

I will remove it -

>> +
>> +    return -1;
>> +}
>> diff --git a/src/conf/virinterfaceobj.h b/src/conf/virinterfaceobj.h
>> index 2f07174..5b0527d 100644
>> --- a/src/conf/virinterfaceobj.h
>> +++ b/src/conf/virinterfaceobj.h
>> @@ -85,4 +85,10 @@ int
>>  virInterfaceObjNumOfInterfaces(virInterfaceObjListPtr interfaces,
>>                                 bool wantActive);
>>
>> +int
>> +virInterfaceObjGetNames(virInterfaceObjListPtr interfaces,
>> +                        bool wantActive,
>> +                        char **const names,
>> +                        int maxnames);
>> +
>>  #endif /* __VIRINTERFACEOBJ_H__ */
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 96aacaa..88e530c 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -935,6 +935,7 @@ virDomainObjListRename;
>>  virInterfaceObjAssignDef;
>>  virInterfaceObjFindByMACString;
>>  virInterfaceObjFindByName;
>> +virInterfaceObjGetNames;
>>  virInterfaceObjListClone;
>>  virInterfaceObjListFree;
>>  virInterfaceObjLock;
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 6910681..4e10eb2 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -3657,33 +3657,18 @@ static int
>> testConnectNumOfInterfaces(virConnectPtr conn)
>>      return ninterfaces;
>>  }
>>
>> -static int testConnectListInterfaces(virConnectPtr conn, char **const
>> names, int nnames)
>> +static int testConnectListInterfaces(virConnectPtr conn, char **const
>> names, int maxnames)
>>  {
>>      testDriverPtr privconn = conn->privateData;
>> -    int n = 0;
>> -    size_t i;
>> +    int nnames;
>> +
>> +    memset(names, 0, maxnames * sizeof(*names));
> 
> I don't think this is necessary either, but I don't care that much.
> 

True, but similar to above it's more of a try to be consistent. In the
long run - all the driver specific functions would use a single common
function withe the details of driver data (e.g. ->def and 'filter'
checks) being handled by specific code.

Similarly I will remove it.

tks -

John

>>
>>      testDriverLock(privconn);
>> -    memset(names, 0, sizeof(*names)*nnames);
>> -    for (i = 0; (i < privconn->ifaces.count) && (n < nnames); i++) {
>> -        virInterfaceObjLock(privconn->ifaces.objs[i]);
>> -        if (virInterfaceObjIsActive(privconn->ifaces.objs[i])) {
>> -            if (VIR_STRDUP(names[n++],
>> privconn->ifaces.objs[i]->def->name) < 0) {
>> -                virInterfaceObjUnlock(privconn->ifaces.objs[i]);
>> -                goto error;
>> -            }
>> -        }
>> -        virInterfaceObjUnlock(privconn->ifaces.objs[i]);
>> -    }
>> +    nnames = virInterfaceObjGetNames(&privconn->ifaces, true, names,
>> maxnames);
>>      testDriverUnlock(privconn);
>>
>> -    return n;
>> -
>> - error:
>> -    for (n = 0; n < nnames; n++)
>> -        VIR_FREE(names[n]);
>> -    testDriverUnlock(privconn);
>> -    return -1;
>> +    return nnames;
> 
> ACK
> 
> Michal