On Tue, Oct 10, 2017 at 04:20:06PM -0400, John Ferlan wrote:
> Rather than separate callbacks for the NumOfNetworks, GetNames, and
> Export functions, let's combine them all into one multi-purpose callback.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
NACK to the idea of consolidating all of them into a single callback, making it
ambiguous, much harder to read and unmaintainable (not that we're about to
introduce more getters for which another combination of elements within
virNetworkObjForEachData would determine the action, but still...)
However, we could still improve the existing code base, see below.
> src/conf/virnetworkobj.c | 153 +++++++++++++++++++----------------------------
> 1 file changed, 62 insertions(+), 91 deletions(-)
>
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index 8cd1b62c1c..40e3eb5ea0 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -1309,47 +1309,61 @@ virNetworkMatch(virNetworkObjPtr obj,
> #undef MATCH
>
>
> -struct virNetworkObjListData {
> +typedef bool (*virNetworkObjMatch)(virNetworkObjPtr obj, unsigned int flags);
> +struct _virNetworkObjForEachData {
> virConnectPtr conn;
> - virNetworkPtr *nets;
> virNetworkObjListFilter filter;
> + virNetworkObjMatch match;
> unsigned int flags;
> - int nnets;
> bool error;
> + bool checkActive;
> + bool active;
> + int nElems;
> + int maxElems;
> + char **const elems;
> + virNetworkPtr *nets;
> };
>
> static int
> -virNetworkObjListPopulate(void *payload,
> - const void *name ATTRIBUTE_UNUSED,
> - void *opaque)
> +virNetworkObjListForEachCb(void *payload,
> + const void *name ATTRIBUTE_UNUSED,
> + void *opaque)
> {
> - struct virNetworkObjListData *data = opaque;
> + struct _virNetworkObjForEachData *data = opaque;
> virNetworkObjPtr obj = payload;
> virNetworkPtr net = NULL;
>
> if (data->error)
> return 0;
>
> + if (data->maxElems >= 0 && data->nElems == data->maxElems)
> + return 0;
> +
> virObjectLock(obj);
>
> - if (data->filter &&
> - !data->filter(data->conn, obj->def))
> + if (data->filter && !data->filter(data->conn, obj->def))
> goto cleanup;
>
> - if (!virNetworkMatch(obj, data->flags))
> + if (data->match && !virNetworkMatch(obj, data->flags))
> goto cleanup;
>
> - if (!data->nets) {
> - data->nnets++;
> + if (data->checkActive && data->active != virNetworkObjIsActive(obj))
> goto cleanup;
> - }
>
> - if (!(net = virGetNetwork(data->conn, obj->def->name, obj->def->uuid))) {
> - data->error = true;
> - goto cleanup;
> + if (data->elems) {
This is an example of the ambiguity I'm talking about, what are elems? it's a
generic term that could describe anything, even @nets, yet @nets can't be
@elems (right below this text), only @maxnames can, so what's the meaning of and
restrictions tied to @elems?
> + if (VIR_STRDUP(data->elems[data->nElems], obj->def->name) < 0) {
> + data->error = true;
> + goto cleanup;
> + }
> + } else if (data->nets) {
> + if (!(net = virGetNetwork(data->conn, obj->def->name, obj->def->uuid))) {
> + data->error = true;
> + goto cleanup;
> + }
> + data->nets[data->nElems] = net;
> }
>
> - data->nets[data->nnets++] = net;
> + data->nElems++;
>
> cleanup:
> virObjectUnlock(obj);
> @@ -1365,31 +1379,36 @@ virNetworkObjListExport(virConnectPtr conn,
> unsigned int flags)
> {
> int ret = -1;
> - struct virNetworkObjListData data = {
> - .conn = conn, .nets = NULL, .filter = filter, .flags = flags,
> - .nnets = 0, .error = false };
> + struct _virNetworkObjForEachData data = {
> + .conn = conn, .filter = filter, .match = virNetworkMatch,
> + .flags = flags, .checkActive = false, .active = false, .error = false,
> + .nElems = 0, .maxElems = -1, .elems = NULL, .nets = NULL };
>
^The structure isn't holding actual data, just a bunch of switches the
combination of which further determines the operation with the object and
that's where I see the problem.
> virObjectRWLockRead(netobjs);
> if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0)
> goto cleanup;
>
> - virHashForEach(netobjs->objs, virNetworkObjListPopulate, &data);
> + if (data.nets)
> + data.maxElems = virHashSize(netobjs->objs) + 1;
What's the purpose of ^this assignment, okay, I can see that it's supposed to
be an array boundary check, but we're holding the read lock to whole time, so
no writer can update the overall number of objects in the meantime, so both
with our without this assignment we're looping through the whole list, nothing
more, nothing less.
> +
> + virHashForEach(netobjs->objs, virNetworkObjListForEachCb, &data);
>
> if (data.error)
> goto cleanup;
>
> if (data.nets) {
> /* trim the array to the final size */
> - ignore_value(VIR_REALLOC_N(data.nets, data.nnets + 1));
> + ignore_value(VIR_REALLOC_N(data.nets, data.nElems + 1));
> *nets = data.nets;
> data.nets = NULL;
> }
>
> - ret = data.nnets;
> + ret = data.nElems;
> +
> cleanup:
> virObjectRWUnlock(netobjs);
> - while (data.nets && data.nnets)
> - virObjectUnref(data.nets[--data.nnets]);
> + while (data.nets && data.nElems)
> + virObjectUnref(data.nets[--data.nElems]);
>
> VIR_FREE(data.nets);
> return ret;
> @@ -1442,53 +1461,6 @@ virNetworkObjListForEach(virNetworkObjListPtr nets,
> }
>
>
> -struct virNetworkObjListGetHelperData {
> - virConnectPtr conn;
> - virNetworkObjListFilter filter;
> - char **names;
> - int nnames;
> - int maxnames;
> - bool active;
> - bool error;
> -};
> -
> -static int
> -virNetworkObjListGetHelper(void *payload,
> - const void *name ATTRIBUTE_UNUSED,
> - void *opaque)
> -{
> - struct virNetworkObjListGetHelperData *data = opaque;
> - virNetworkObjPtr obj = payload;
> -
> - if (data->error)
> - return 0;
Could this have actually ever been true?
> -
> - if (data->maxnames >= 0 &&
> - data->nnames == data->maxnames)
> - return 0;
> -
> - virObjectLock(obj);
> -
Starting here...
> - if (data->filter &&
> - !data->filter(data->conn, obj->def))
> - goto cleanup;
> -
> - if ((data->active && virNetworkObjIsActive(obj)) ||
> - (!data->active && !virNetworkObjIsActive(obj))) {
and ending here, this could become a helper on its own, we reuse this pattern
among all the getters you're trying to change.
> - if (data->names &&
> - VIR_STRDUP(data->names[data->nnames], obj->def->name) < 0) {
> - data->error = true;
> - goto cleanup;
> - }
This hunk should be part of a new virNetworkObjListGetName helper.
> - data->nnames++;
> - }
> -
> - cleanup:
> - virObjectUnlock(obj);
> - return 0;
> -}
> -
> -
> int
> virNetworkObjListGetNames(virNetworkObjListPtr nets,
> bool active,
> @@ -1497,26 +1469,24 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets,
> virNetworkObjListFilter filter,
> virConnectPtr conn)
> {
> - int ret = -1;
> -
> - struct virNetworkObjListGetHelperData data = {
> - .conn = conn, .filter = filter, .names = names, .nnames = 0,
> - .maxnames = maxnames, .active = active, .error = false};
> + struct _virNetworkObjForEachData data = {
> + .conn = conn, .filter = filter, .match = NULL,
> + .flags = 0, .checkActive = true, .active = active, .error = false,
> + .nElems = 0, .maxElems = maxnames, .elems = names, .nets = NULL };
>
> virObjectRWLockRead(nets);
> - virHashForEach(nets->objs, virNetworkObjListGetHelper, &data);
> + virHashForEach(nets->objs, virNetworkObjListForEachCb, &data);
> virObjectRWUnlock(nets);
We have virNetworkObjListForEach for ^this...
>
> if (data.error)
> - goto cleanup;
> + goto error;
you could drop ^this then.
>
> - ret = data.nnames;
> - cleanup:
> - if (ret < 0) {
> - while (data.nnames)
> - VIR_FREE(data.names[--data.nnames]);
> - }
> - return ret;
> + return data.nElems;
> +
> + error:
> + while (data.nElems)
> + VIR_FREE(data.elems[--data.nElems]);
> + return -1;
> }
>
>
> @@ -1526,15 +1496,16 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets,
> virNetworkObjListFilter filter,
> virConnectPtr conn)
> {
> - struct virNetworkObjListGetHelperData data = {
> - .conn = conn, .filter = filter, .names = NULL, .nnames = 0,
> - .maxnames = -1, .active = active, .error = false};
> + struct _virNetworkObjForEachData data = {
> + .conn = conn, .filter = filter, .match = NULL,
> + .flags = 0, .checkActive = true, .active = active, .error = false,
> + .nElems = 0, .maxElems = -1, .elems = NULL, .nets = NULL };
>
> virObjectRWLockRead(nets);
> - virHashForEach(nets->objs, virNetworkObjListGetHelper, &data);
> + virHashForEach(nets->objs, virNetworkObjListForEachCb, &data);
> virObjectRWUnlock(nets);
Again, virNetworkObjListForEach can be used instead.
>
> - return data.nnames;
> + return data.nElems;
> }
To sum it up (feel free to use different names), you could replace
virNetworkObjListGetHelper with virNetworkObjListFilterHelper
(or something similar) which would be responsible for all filtering, IOW
determines whether the obj should or should not be used as part of the current
action over the list. This filter helper would contain the 2 hunks I marked
above and would be called from all the *ForEachCallbacks.
Then you'd have *ListNumOfNetworks calling something like *ObjListGetCount
(again, first calling the filter helper). Similarly, *GetNames could be
adjusted in this fashion.
I admit that preparing a patch myself might have actually been easier in order
to express my idea more clearly :/.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
