Let's use the RWObjectLockable for the various list lock mgmt. Only time need Write lock will be for Add, Remove, and Prune logic. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/virnetworkobj.c | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 20f846db5e..8cd1b62c1c 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -61,7 +61,7 @@ struct _virNetworkObj { }; struct _virNetworkObjList { - virObjectLockable parent; + virObjectRWLockable parent; virHashTablePtr objs; }; @@ -80,7 +80,7 @@ virNetworkObjOnceInit(void) virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectRWLockable(), "virNetworkObjList", sizeof(virNetworkObjList), virNetworkObjListDispose))) @@ -337,7 +337,7 @@ virNetworkObjListNew(void) if (virNetworkObjInitialize() < 0) return NULL; - if (!(nets = virObjectLockableNew(virNetworkObjListClass))) + if (!(nets = virObjectRWLockableNew(virNetworkObjListClass))) return NULL; if (!(nets->objs = virHashCreate(50, virObjectFreeHashData))) { @@ -381,9 +381,9 @@ virNetworkObjFindByUUID(virNetworkObjListPtr nets, { virNetworkObjPtr obj; - virObjectLock(nets); + virObjectRWLockRead(nets); obj = virNetworkObjFindByUUIDLocked(nets, uuid); - virObjectUnlock(nets); + virObjectRWUnlock(nets); if (obj) virObjectLock(obj); return obj; @@ -435,9 +435,9 @@ virNetworkObjFindByName(virNetworkObjListPtr nets, { virNetworkObjPtr obj; - virObjectLock(nets); + virObjectRWLockRead(nets); obj = virNetworkObjFindByNameLocked(nets, name); - virObjectUnlock(nets); + virObjectRWUnlock(nets); if (obj) virObjectLock(obj); return obj; @@ -638,9 +638,9 @@ virNetworkObjAssignDef(virNetworkObjListPtr nets, { virNetworkObjPtr obj; - virObjectLock(nets); + virObjectRWLockWrite(nets); obj = virNetworkObjAssignDefLocked(nets, def, flags); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return obj; } @@ -789,10 +789,10 @@ virNetworkObjRemoveInactive(virNetworkObjListPtr nets, virUUIDFormat(obj->def->uuid, uuidstr); virObjectRef(obj); virObjectUnlock(obj); - virObjectLock(nets); + virObjectRWLockWrite(nets); virObjectLock(obj); virHashRemoveEntry(nets->objs, uuidstr); - virObjectUnlock(nets); + virObjectRWUnlock(nets); virObjectUnref(obj); } @@ -1180,9 +1180,9 @@ virNetworkObjBridgeInUse(virNetworkObjListPtr nets, virNetworkObjPtr obj; struct virNetworkObjBridgeInUseHelperData data = {bridge, skipname}; - virObjectLock(nets); + virObjectRWLockRead(nets); obj = virHashSearch(nets->objs, virNetworkObjBridgeInUseHelper, &data, NULL); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return obj != NULL; } @@ -1369,7 +1369,7 @@ virNetworkObjListExport(virConnectPtr conn, .conn = conn, .nets = NULL, .filter = filter, .flags = flags, .nnets = 0, .error = false }; - virObjectLock(netobjs); + virObjectRWLockRead(netobjs); if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0) goto cleanup; @@ -1387,7 +1387,7 @@ virNetworkObjListExport(virConnectPtr conn, ret = data.nnets; cleanup: - virObjectUnlock(netobjs); + virObjectRWUnlock(netobjs); while (data.nets && data.nnets) virObjectUnref(data.nets[--data.nnets]); @@ -1435,9 +1435,9 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, { struct virNetworkObjListForEachHelperData data = { .callback = callback, .opaque = opaque, .ret = 0}; - virObjectLock(nets); + virObjectRWLockRead(nets); virHashForEach(nets->objs, virNetworkObjListForEachHelper, &data); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return data.ret; } @@ -1503,9 +1503,9 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, .conn = conn, .filter = filter, .names = names, .nnames = 0, .maxnames = maxnames, .active = active, .error = false}; - virObjectLock(nets); + virObjectRWLockRead(nets); virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - virObjectUnlock(nets); + virObjectRWUnlock(nets); if (data.error) goto cleanup; @@ -1530,9 +1530,9 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, .conn = conn, .filter = filter, .names = NULL, .nnames = 0, .maxnames = -1, .active = active, .error = false}; - virObjectLock(nets); + virObjectRWLockRead(nets); virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - virObjectUnlock(nets); + virObjectRWUnlock(nets); return data.nnames; } @@ -1572,7 +1572,7 @@ virNetworkObjListPrune(virNetworkObjListPtr nets, { struct virNetworkObjListPruneHelperData data = {flags}; - virObjectLock(nets); + virObjectRWLockWrite(nets); virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); - virObjectUnlock(nets); + virObjectRWUnlock(nets); } -- 2.13.6

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&gt; --- 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) { + 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 }; 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; + + 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; - - if (data->maxnames >= 0 && - data->nnames == data->maxnames) - return 0; - - virObjectLock(obj); - - if (data->filter && - !data->filter(data->conn, obj->def)) - goto cleanup; - - if ((data->active && virNetworkObjIsActive(obj)) || - (!data->active && !virNetworkObjIsActive(obj))) { - if (data->names && - VIR_STRDUP(data->names[data->nnames], obj->def->name) < 0) { - data->error = true; - goto cleanup; - } - 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); if (data.error) - goto cleanup; + goto error; - 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); - return data.nnames; + return data.nElems; } -- 2.13.6

On 10/12/2017 08:48 AM, Erik Skultety wrote: 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. @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. 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. 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. 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. True for all, but active is only for NumOf and Names, but not for Export I don't understand. You asked about elems earlier - this is that code and it is already. 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. 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. Nope. 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