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@xxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
