5:46 a.m.
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(a)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