8:28 p.m.
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(a)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