On 10/12/2017 08:48 AM, Erik Skultety wrote:

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@redhat.com> ---

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...)

First off - I'm OK w/ the NACK. Still this isn't unlike some of the virdomainobjlist code... Still having a separate callback for each is something I went with originally. Then going through the domainobj code recently made me reconsider my decision. Also there's the virobject patches I've had on list for a while which could do the combined callback.

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?

@elems is a list of "elements" that are copied back to the caller. Typically a list of obj->name, but IIRC there's one uuidstr list (secrets). Still if you consider from an abstract viewpoint - it's just a list of something. I went with items to be consistent between drivers, but it's just as easy to change it to data->names. The data->elems is a list of char*'s used by the connectList[Defined]Networks for the virConnectList*Networks API's which are connected to "older" style API's (e.g. pre 0.10.2) that were used to fetch and populate the virsh list style output. Go read any of the virsh source code and see what the sequence is. The data->nnets is a list of virNetworkPtr's which is the newer (everything is relative) and sexier way to get the list of network names plus some other stuff in one call. I think there's still a bit of historical cruft tied to maxelems, but mostly because at one time you'd have to call GetNum in order to get the number of elements... then allocate your buffer, then call GetNames, but that could mean maxelems would be off. Still true today actually depending on the client's code.

...

+ 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.

Well... I understand your viewpoint; however, it does hold data - it's just opaque data that I decided to fill in "consistently" with the same name fields between the various modules.

...

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.

This was me be super cautious and overly complete, but right it isn't necessary since we don't change size; whereas, the Num/Names processing could have had a change the nelems between calls, so maxelems ensures only a set number are collected.

...

+ + 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?

Absolutely = virNetworkObjListGetHelper calls VIR_STRDUP and sets error = true on failure. Similarly, virNetworkObjListPopulate will call virGetNetwork and if it fails, then error = true. It's an avoidance system to ensure we don't continue to fail, but don't force virHashForEach to fail IIUC.

...

- - 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;

True for all, but

...

- - 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.

active is only for NumOf and Names, but not for Export

...

- 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.

I don't understand. You asked about elems earlier - this is that code and it is already.

...

- 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...

Well not really... That's used for the networkStateInitialize, AutoStart, Reload, Refresh, and ReloadFirewallRule calls from the driver. Those are quite different. They're not collecting data about the objects, but rather performing actions on them. The virNetworkObjListForEachCb is as pointed out in the commit message meant to combined the NumOf, Names, and Export - the data fetch APIs.

...

if (data.error) - goto cleanup; + goto error;

you could drop ^this then.

Why? If there was a failure we need to return -1 and free whatever we've partially collected - yes, we're using the callers buffer.

...

- 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.

Nope.

...

- 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

I think I understand what you've written, but perhaps you have a change of view point once you've read the response. If you think about - the change here is essentially to expand virNetworkObjListGetHelper to include fetching of Export data as well. The Export function is made a bit tricker by the presence of a passed @nets pointer. Without it, it's essentially the NumOf call without the active/inactive check. John