[libvirt] [PATCH v1 00/31] Drop network driver lock
The patchset starts slowly, with small patches, and small changes, accelerating towards the end, with big changes. I know that network driver is not used as intensely as domain drivers, but hey, if we remove 100 lines (and substitute some others) we get fine grained locking! And cleaner code, of course. Oh, and this is meant for next release, obviously :) Michal Privoznik (31): networkLookupByUUID: Improve error message bridge_driver: Don't check network active unlocked testNetworkUpdate: Unlock network at the end networkGetNetworkAddress: Drop empty 'error' label virNetworkObjIsDuplicate: s/@doms/@nets/ virNetworkObjListFree: Accept NULL virNetworkObjListExport: Pass virNetworkObjListPtr test_driver: s/virNetworkObjList/virNetworkObjListPtr/ parallels: s/virNetworkObjList/virNetworkObjListPtr/ bridge_driver: s/virNetworkObjList/virNetworkObjListPtr/ conf: s/virNetworkFindByUUID/virNetworkObjFindByUUID/ conf: s/virNetworkFindByUUID/virNetworkObjFindByUUID/ network_conf: Introduce virNetworkObjListForEach network_conf: Introduce virNetworkObjListGetNames network_conf: Introduce virNetworkObjListNumOfNetworks bridge_driver: Adapt to new virNetworkObjList accessors test_driver: Adapt to new virNetworkObjList accessors parallels_network: Adapt to new virNetworkObjList accessors network_conf: Turn virNetworkObjList into virObject network_conf: Turn struct _virNetworkObjList private network_conf: Make virNetworkObj actually virObject virNetworkObjList: Derive from virObjectLockableClass network_conf: Introduce virNetworkObjEndAPI bridge_driver: Use virNetworkObjEndAPI test_driver: Use virNetworkObjEndAPI parallels_network: Use virNetworkObjEndAPI virNetworkObjListPtr: Make APIs self-locking virNetworkObjFindBy*: Return an reference to found object bridge_driver: Drop networkDriverLock() from almost everywhere test_driver: Drop testDriverLock() from almost everywhere parallels_network: Drop parallelsDriverLock() from everywhere. cfg.mk | 3 - src/conf/network_conf.c | 372 +++++++++++++++++++------- src/conf/network_conf.h | 51 ++-- src/libvirt_private.syms | 14 +- src/network/bridge_driver.c | 502 ++++++++++++++--------------------- src/network/bridge_driver_platform.h | 2 +- src/parallels/parallels_driver.c | 5 +- src/parallels/parallels_network.c | 144 +++------- src/parallels/parallels_utils.h | 2 +- src/test/test_driver.c | 223 ++++------------ tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 12 files changed, 612 insertions(+), 712 deletions(-) -- 2.0.5
We have this function networkObjFromNetwork() which for given virNetworkPtr tries to find corresponding virNetworkObjPtr. If no object is found, a nice error message is printed out: no network with matching uuid '$uuid' ($name) Let's improve the error message produced by networkLookupByUUID to follow that logic. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1209609..522ccc2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2475,7 +2475,8 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + _("no network with matching uuid '%s'"), + uuid); goto cleanup; } -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:10 +0100, Michal Privoznik wrote:
We have this function networkObjFromNetwork() which for given virNetworkPtr tries to find corresponding virNetworkObjPtr. If no object is found, a nice error message is printed out:
no network with matching uuid '$uuid' ($name)
Let's improve the error message produced by networkLookupByUUID to follow that logic.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1209609..522ccc2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2475,7 +2475,8 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + _("no network with matching uuid '%s'"), + uuid);
UUID is the binary representation. You need to convert it to a string first. Peter
Okay, this is mainly for educational purposes, since is called from single point only, with all the possible locks held. So there's no way for other thread to hop in and do something wrong. Nevertheless, we should not give bad example. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 522ccc2..f2026fb 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -353,10 +353,11 @@ networkUpdateAllState(void) for (i = 0; i < driver->networks.count; i++) { virNetworkObjPtr obj = driver->networks.objs[i]; - if (!obj->active) - continue; - virNetworkObjLock(obj); + if (!virNetworkObjIsActive(obj)) { + virNetworkObjUnlock(obj); + continue; + } switch (obj->def->forward.type) { case VIR_NETWORK_FORWARD_NONE: -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:11 +0100, Michal Privoznik wrote:
Okay, this is mainly for educational purposes, since is called from single point only, with all the possible locks held. So
Drop the last two commas in the sentence.
there's no way for other thread to hop in and do something wrong. Nevertheless, we should not give bad example.
And this too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
ACK, Peter
Silly this bug went unnoticed so long. At the beginning we try to find the passed network in the list of network objects. If found, it's locked and real work takes place. Then, in the end, the network object is never unlocked. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a386270..2bed3f2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3835,6 +3835,8 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: + if (network) + virNetworkObjUnlock(network); testDriverUnlock(privconn); return ret; } -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:12 +0100, Michal Privoznik wrote:
Silly this bug went unnoticed so long. At the beginning we try to find the passed network in the list of network objects. If found, it's locked and real work takes place. Then, in the end, the network object is never unlocked.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 2 ++ 1 file changed, 2 insertions(+)
The reason this went unnoticed is probably as the test driver is always reinitialized :). ACK Peter
Moreover, there are two points within the function, where we're missing 'goto cleanup'. Fix this too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f2026fb..019cb41 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4560,7 +4560,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), netname); - goto error; + goto cleanup; } netdef = network->def; @@ -4574,7 +4574,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' doesn't have an IPv4 address"), netdef->name); - break; + goto cleanup; } addrptr = &ipdef->address; break; @@ -4596,19 +4596,20 @@ networkGetNetworkAddress(const char *netname, char **netaddr) virReportError(VIR_ERR_INTERNAL_ERROR, _("network '%s' has no associated interface or bridge"), netdef->name); + goto cleanup; } break; } if (dev_name) { if (virNetDevGetIPv4Address(dev_name, &addr) < 0) - goto error; + goto cleanup; addrptr = &addr; } if (!(addrptr && (*netaddr = virSocketAddrFormat(addrptr)))) { - goto error; + goto cleanup; } ret = 0; @@ -4616,9 +4617,6 @@ networkGetNetworkAddress(const char *netname, char **netaddr) if (network) virNetworkObjUnlock(network); return ret; - - error: - goto cleanup; } /** -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:13 +0100, Michal Privoznik wrote:
Moreover, there are two points within the function, where we're missing 'goto cleanup'. Fix this too.
Control flow is okay even in that case, but this makes it more clear.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK Peter
This is probably a copy-paste error from virDomainObj* counterpart. But when speaking of virNetworkObj we should use variable @nets for an array of networks, rather than @doms. It's just confusing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 34 +++++++++++++++++----------------- src/conf/network_conf.h | 4 ++-- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 0f4fc1e..154a9bc 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4130,7 +4130,7 @@ virNetworkObjUpdate(virNetworkObjPtr network, /* * virNetworkObjIsDuplicate: - * @doms : virNetworkObjListPtr to search + * @nets : virNetworkObjListPtr to search * @def : virNetworkDefPtr definition of network to lookup * @check_active: If true, ensure that network is not active * @@ -4139,32 +4139,32 @@ virNetworkObjUpdate(virNetworkObjPtr network, * 1 if network is a duplicate */ int -virNetworkObjIsDuplicate(virNetworkObjListPtr doms, +virNetworkObjIsDuplicate(virNetworkObjListPtr nets, virNetworkDefPtr def, bool check_active) { int ret = -1; - virNetworkObjPtr vm = NULL; + virNetworkObjPtr net = NULL; - /* See if a VM with matching UUID already exists */ - vm = virNetworkFindByUUID(doms, def->uuid); - if (vm) { + /* See if a network with matching UUID already exists */ + net = virNetworkFindByUUID(nets, def->uuid); + if (net) { /* UUID matches, but if names don't match, refuse it */ - if (STRNEQ(vm->def->name, def->name)) { + if (STRNEQ(net->def->name, def->name)) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); + virUUIDFormat(net->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("network '%s' is already defined with uuid %s"), - vm->def->name, uuidstr); + net->def->name, uuidstr); goto cleanup; } if (check_active) { - /* UUID & name match, but if VM is already active, refuse it */ - if (virNetworkObjIsActive(vm)) { + /* UUID & name match, but if network is already active, refuse it */ + if (virNetworkObjIsActive(net)) { virReportError(VIR_ERR_OPERATION_INVALID, _("network is already active as '%s'"), - vm->def->name); + net->def->name); goto cleanup; } } @@ -4172,10 +4172,10 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr doms, ret = 1; } else { /* UUID does not match, but if a name matches, refuse it */ - vm = virNetworkFindByName(doms, def->name); - if (vm) { + net = virNetworkFindByName(nets, def->name); + if (net) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); + virUUIDFormat(net->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("network '%s' already exists with uuid %s"), def->name, uuidstr); @@ -4185,8 +4185,8 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr doms, } cleanup: - if (vm) - virNetworkObjUnlock(vm); + if (net) + virNetworkObjUnlock(net); return ret; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 484522e..70e6eb5 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -309,7 +309,7 @@ virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); -void virNetworkObjListFree(virNetworkObjListPtr vms); +void virNetworkObjListFree(virNetworkObjListPtr nets); typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, @@ -413,7 +413,7 @@ virNetworkObjUpdate(virNetworkObjPtr obj, const char *xml, unsigned int flags); /* virNetworkUpdateFlags */ -int virNetworkObjIsDuplicate(virNetworkObjListPtr doms, +int virNetworkObjIsDuplicate(virNetworkObjListPtr nets, virNetworkDefPtr def, bool check_active); -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:14 +0100, Michal Privoznik wrote:
This is probably a copy-paste error from virDomainObj* counterpart. But when speaking of virNetworkObj we should use variable @nets for an array of networks, rather than @doms. It's just confusing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 34 +++++++++++++++++----------------- src/conf/network_conf.h | 4 ++-- 2 files changed, 19 insertions(+), 19 deletions(-)
ACK, Peter
All of our vir*Free() functions should accept NULL, even though that there's no way of actually passing NULL with current code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 154a9bc..9734a7f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -279,6 +279,9 @@ void virNetworkObjListFree(virNetworkObjListPtr nets) { size_t i; + if (!nets) + return; + for (i = 0; i < nets->count; i++) virNetworkObjFree(nets->objs[i]); -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:15 +0100, Michal Privoznik wrote:
All of our vir*Free() functions should accept NULL, even though that there's no way of actually passing NULL with current code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 154a9bc..9734a7f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -279,6 +279,9 @@ void virNetworkObjListFree(virNetworkObjListPtr nets) { size_t i;
+ if (!nets) + return; + for (i = 0; i < nets->count; i++) virNetworkObjFree(nets->objs[i]);
Since the function does not free @nets at the end it should really be called virNetworkObjListClear. But that was present previously too. ACK as is. Peter
Instead of copying the whole object onto stack when calling the function, just pass the pointer to the object and save up some space on the stack. Moreover, this prepares the code to hide the virNetworkObjList structure into network_conf.c and use accessors only. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 8 ++++---- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 2 +- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9734a7f..00be8b6 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4239,7 +4239,7 @@ virNetworkMatch(virNetworkObjPtr netobj, int virNetworkObjListExport(virConnectPtr conn, - virNetworkObjList netobjs, + virNetworkObjListPtr netobjs, virNetworkPtr **nets, virNetworkObjListFilter filter, unsigned int flags) @@ -4250,11 +4250,11 @@ virNetworkObjListExport(virConnectPtr conn, int ret = -1; size_t i; - if (nets && VIR_ALLOC_N(tmp_nets, netobjs.count + 1) < 0) + if (nets && VIR_ALLOC_N(tmp_nets, netobjs->count + 1) < 0) goto cleanup; - for (i = 0; i < netobjs.count; i++) { - virNetworkObjPtr netobj = netobjs.objs[i]; + for (i = 0; i < netobjs->count; i++) { + virNetworkObjPtr netobj = netobjs->objs[i]; virNetworkObjLock(netobj); if ((!filter || filter(conn, netobj->def)) && virNetworkMatch(netobj, flags)) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 70e6eb5..74665e0 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -440,7 +440,7 @@ VIR_ENUM_DECL(virNetworkForward) VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART) int virNetworkObjListExport(virConnectPtr conn, - virNetworkObjList netobjs, + virNetworkObjListPtr netobjs, virNetworkPtr **nets, virNetworkObjListFilter filter, unsigned int flags); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 019cb41..edc4ed4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2638,7 +2638,7 @@ networkConnectListAllNetworks(virConnectPtr conn, goto cleanup; networkDriverLock(); - ret = virNetworkObjListExport(conn, driver->networks, nets, + ret = virNetworkObjListExport(conn, &driver->networks, nets, virConnectListAllNetworksCheckACL, flags); networkDriverUnlock(); diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 3e7087d..960bd50 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -449,7 +449,7 @@ static int parallelsConnectListAllNetworks(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); parallelsDriverLock(privconn); - ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); + ret = virNetworkObjListExport(conn, &privconn->networks, nets, NULL, flags); parallelsDriverUnlock(privconn); return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2bed3f2..e2da1e2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3636,7 +3636,7 @@ testConnectListAllNetworks(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); testDriverLock(privconn); - ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); + ret = virNetworkObjListExport(conn, &privconn->networks, nets, NULL, flags); testDriverUnlock(privconn); return ret; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:16 +0100, Michal Privoznik wrote:
Instead of copying the whole object onto stack when calling the function, just pass the pointer to the object and save up some space on the stack. Moreover, this prepares the code to hide the virNetworkObjList structure into network_conf.c and use accessors only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 8 ++++---- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 2 +- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-)
ACK, Peter
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 106 ++++++++++++++++++++++++------------------------- 1 file changed, 51 insertions(+), 55 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e2da1e2..90df0e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -101,7 +101,7 @@ struct _testConn { virDomainXMLOptionPtr xmlopt; virNodeInfo nodeInfo; virDomainObjListPtr domains; - virNetworkObjList networks; + virNetworkObjListPtr networks; virInterfaceObjList ifaces; bool transaction_running; virInterfaceObjList backupIfaces; @@ -115,7 +115,7 @@ struct _testConn { virObjectEventStatePtr eventState; }; typedef struct _testConn testConn; -typedef struct _testConn *testConnPtr; +typedef testConn *testConnPtr; static testConn defaultConn; static int defaultConnections; @@ -724,7 +724,8 @@ testOpenDefault(virConnectPtr conn) if (!(privconn->eventState = virObjectEventStateNew())) goto error; - if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) goto error; memmove(&privconn->nodeInfo, &defaultNodeInfo, sizeof(defaultNodeInfo)); @@ -781,7 +782,7 @@ testOpenDefault(virConnectPtr conn) if (!(netdef = virNetworkDefParseString(defaultNetworkXML))) goto error; - if (!(netobj = virNetworkAssignDef(&privconn->networks, netdef, false))) { + if (!(netobj = virNetworkAssignDef(privconn->networks, netdef, false))) { virNetworkDefFree(netdef); goto error; } @@ -829,7 +830,8 @@ testOpenDefault(virConnectPtr conn) error: virObjectUnref(privconn->domains); - virNetworkObjListFree(&privconn->networks); + virNetworkObjListFree(privconn->networks); + VIR_FREE(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virNodeDeviceObjListFree(&privconn->devs); @@ -1148,7 +1150,7 @@ testParseNetworks(testConnPtr privconn, if (!def) goto error; - if (!(obj = virNetworkAssignDef(&privconn->networks, def, false))) { + if (!(obj = virNetworkAssignDef(privconn->networks, def, false))) { virNetworkDefFree(def); goto error; } @@ -1411,7 +1413,8 @@ testOpenFromFile(virConnectPtr conn, const char *file) testDriverLock(privconn); conn->privateData = privconn; - if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) goto error; if (!(privconn->caps = testBuildCapabilities(conn))) @@ -1463,7 +1466,8 @@ testOpenFromFile(virConnectPtr conn, const char *file) xmlXPathFreeContext(ctxt); xmlFreeDoc(doc); virObjectUnref(privconn->domains); - virNetworkObjListFree(&privconn->networks); + virNetworkObjListFree(privconn->networks); + VIR_FREE(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); VIR_FREE(privconn->path); @@ -1589,7 +1593,8 @@ static int testConnectClose(virConnectPtr conn) virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); virNodeDeviceObjListFree(&privconn->devs); - virNetworkObjListFree(&privconn->networks); + virNetworkObjListFree(privconn->networks); + VIR_FREE(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virObjectEventStateFree(privconn->eventState); @@ -3494,7 +3499,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, virNetworkPtr ret = NULL; testDriverLock(privconn); - net = virNetworkFindByUUID(&privconn->networks, uuid); + net = virNetworkFindByUUID(privconn->networks, uuid); testDriverUnlock(privconn); if (net == NULL) { @@ -3518,7 +3523,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, virNetworkPtr ret = NULL; testDriverLock(privconn); - net = virNetworkFindByName(&privconn->networks, name); + net = virNetworkFindByName(privconn->networks, name); testDriverUnlock(privconn); if (net == NULL) { @@ -3542,11 +3547,11 @@ static int testConnectNumOfNetworks(virConnectPtr conn) size_t i; testDriverLock(privconn); - for (i = 0; i < privconn->networks.count; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (virNetworkObjIsActive(privconn->networks.objs[i])) + for (i = 0; i < privconn->networks->count; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (virNetworkObjIsActive(privconn->networks->objs[i])) numActive++; - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } testDriverUnlock(privconn); @@ -3560,14 +3565,14 @@ static int testConnectListNetworks(virConnectPtr conn, char **const names, int n testDriverLock(privconn); memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->networks.count && n < nnames; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (virNetworkObjIsActive(privconn->networks.objs[i]) && - VIR_STRDUP(names[n++], privconn->networks.objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks.objs[i]); + for (i = 0; i < privconn->networks->count && n < nnames; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (virNetworkObjIsActive(privconn->networks->objs[i]) && + VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { + virNetworkObjUnlock(privconn->networks->objs[i]); goto error; } - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } testDriverUnlock(privconn); @@ -3587,11 +3592,11 @@ static int testConnectNumOfDefinedNetworks(virConnectPtr conn) size_t i; testDriverLock(privconn); - for (i = 0; i < privconn->networks.count; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (!virNetworkObjIsActive(privconn->networks.objs[i])) + for (i = 0; i < privconn->networks->count; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (!virNetworkObjIsActive(privconn->networks->objs[i])) numInactive++; - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } testDriverUnlock(privconn); @@ -3605,14 +3610,14 @@ static int testConnectListDefinedNetworks(virConnectPtr conn, char **const names testDriverLock(privconn); memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->networks.count && n < nnames; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (!virNetworkObjIsActive(privconn->networks.objs[i]) && - VIR_STRDUP(names[n++], privconn->networks.objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks.objs[i]); + for (i = 0; i < privconn->networks->count && n < nnames; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (!virNetworkObjIsActive(privconn->networks->objs[i]) && + VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { + virNetworkObjUnlock(privconn->networks->objs[i]); goto error; } - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } testDriverUnlock(privconn); @@ -3636,7 +3641,7 @@ testConnectListAllNetworks(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); testDriverLock(privconn); - ret = virNetworkObjListExport(conn, &privconn->networks, nets, NULL, flags); + ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); testDriverUnlock(privconn); return ret; @@ -3649,7 +3654,7 @@ static int testNetworkIsActive(virNetworkPtr net) int ret = -1; testDriverLock(privconn); - obj = virNetworkFindByUUID(&privconn->networks, net->uuid); + obj = virNetworkFindByUUID(privconn->networks, net->uuid); testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -3670,7 +3675,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) int ret = -1; testDriverLock(privconn); - obj = virNetworkFindByUUID(&privconn->networks, net->uuid); + obj = virNetworkFindByUUID(privconn->networks, net->uuid); testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -3697,7 +3702,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; - if (!(net = virNetworkAssignDef(&privconn->networks, def, true))) + if (!(net = virNetworkAssignDef(privconn->networks, def, true))) goto cleanup; def = NULL; net->active = 1; @@ -3731,7 +3736,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; - if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) goto cleanup; def = NULL; @@ -3759,8 +3764,7 @@ static int testNetworkUndefine(virNetworkPtr network) virObjectEventPtr event = NULL; testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -3777,8 +3781,7 @@ static int testNetworkUndefine(virNetworkPtr network) VIR_NETWORK_EVENT_UNDEFINED, 0); - virNetworkRemoveInactive(&privconn->networks, - privnet); + virNetworkRemoveInactive(privconn->networks, privnet); privnet = NULL; ret = 0; @@ -3809,7 +3812,7 @@ testNetworkUpdate(virNetworkPtr net, testDriverLock(privconn); - network = virNetworkFindByUUID(&privconn->networks, net->uuid); + network = virNetworkFindByUUID(privconn->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -3849,8 +3852,7 @@ static int testNetworkCreate(virNetworkPtr network) virObjectEventPtr event = NULL; testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -3886,8 +3888,7 @@ static int testNetworkDestroy(virNetworkPtr network) virObjectEventPtr event = NULL; testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -3899,8 +3900,7 @@ static int testNetworkDestroy(virNetworkPtr network) VIR_NETWORK_EVENT_STOPPED, 0); if (!privnet->persistent) { - virNetworkRemoveInactive(&privconn->networks, - privnet); + virNetworkRemoveInactive(privconn->networks, privnet); privnet = NULL; } ret = 0; @@ -3924,8 +3924,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, virCheckFlags(0, NULL); testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -3947,8 +3946,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { virNetworkObjPtr privnet; testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -3979,8 +3977,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, int ret = -1; testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -4005,8 +4002,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, int ret = -1; testDriverLock(privconn); - privnet = virNetworkFindByName(&privconn->networks, - network->name); + privnet = virNetworkFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:17 +0100, Michal Privoznik wrote:
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 106 ++++++++++++++++++++++++------------------------- 1 file changed, 51 insertions(+), 55 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e2da1e2..90df0e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -101,7 +101,7 @@ struct _testConn { virDomainXMLOptionPtr xmlopt; virNodeInfo nodeInfo; virDomainObjListPtr domains; - virNetworkObjList networks; + virNetworkObjListPtr networks; virInterfaceObjList ifaces; bool transaction_running; virInterfaceObjList backupIfaces; @@ -115,7 +115,7 @@ struct _testConn { virObjectEventStatePtr eventState; }; typedef struct _testConn testConn; -typedef struct _testConn *testConnPtr; +typedef testConn *testConnPtr;
static testConn defaultConn; static int defaultConnections; @@ -724,7 +724,8 @@ testOpenDefault(virConnectPtr conn) if (!(privconn->eventState = virObjectEventStateNew())) goto error;
- if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) goto error;
Since you are going to convert the network structure to an objec wouldn't it be better to add a constructor for the object right away instead of having to do it later once you hide the struct? Otherwise looks good. Peter
On Thu, Feb 26, 2015 at 16:46:26 +0100, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:17 +0100, Michal Privoznik wrote:
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 106 ++++++++++++++++++++++++------------------------- 1 file changed, 51 insertions(+), 55 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e2da1e2..90df0e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -101,7 +101,7 @@ struct _testConn { virDomainXMLOptionPtr xmlopt; virNodeInfo nodeInfo; virDomainObjListPtr domains; - virNetworkObjList networks; + virNetworkObjListPtr networks; virInterfaceObjList ifaces; bool transaction_running; virInterfaceObjList backupIfaces; @@ -115,7 +115,7 @@ struct _testConn { virObjectEventStatePtr eventState; }; typedef struct _testConn testConn; -typedef struct _testConn *testConnPtr; +typedef testConn *testConnPtr;
static testConn defaultConn; static int defaultConnections; @@ -724,7 +724,8 @@ testOpenDefault(virConnectPtr conn) if (!(privconn->eventState = virObjectEventStateNew())) goto error;
- if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) goto error;
Since you are going to convert the network structure to an objec wouldn't it be better to add a constructor for the object right away instead of having to do it later once you hide the struct?
Otherwise looks good.
Fair enough. Patch 19 of this series does the conversion. ACK
Peter
On 26.02.2015 16:46, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:17 +0100, Michal Privoznik wrote:
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 106 ++++++++++++++++++++++++------------------------- 1 file changed, 51 insertions(+), 55 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e2da1e2..90df0e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -101,7 +101,7 @@ struct _testConn { virDomainXMLOptionPtr xmlopt; virNodeInfo nodeInfo; virDomainObjListPtr domains; - virNetworkObjList networks; + virNetworkObjListPtr networks; virInterfaceObjList ifaces; bool transaction_running; virInterfaceObjList backupIfaces; @@ -115,7 +115,7 @@ struct _testConn { virObjectEventStatePtr eventState; }; typedef struct _testConn testConn; -typedef struct _testConn *testConnPtr; +typedef testConn *testConnPtr;
static testConn defaultConn; static int defaultConnections; @@ -724,7 +724,8 @@ testOpenDefault(virConnectPtr conn) if (!(privconn->eventState = virObjectEventStateNew())) goto error;
- if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) goto error;
Since you are going to convert the network structure to an objec wouldn't it be better to add a constructor for the object right away instead of having to do it later once you hide the struct?
That's just cosmetics IMO. The only benefit would be that the commit turning the struct into virObject would be a few lines shorter. /me goes and check how many VIR_ALLOCs are added in these patches. Er, two for virNetworkObj, and 4 for virNetworkObjList. So in the end we would save up 6 lines. I can do that if you think it's worth it though. Michal
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_driver.c | 5 +++- src/parallels/parallels_network.c | 60 +++++++++++++++++++-------------------- src/parallels/parallels_utils.h | 2 +- 3 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index c9338b5..32f2ede 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -207,7 +207,8 @@ parallelsOpenDefault(virConnectPtr conn) NULL, NULL))) goto error; - if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) goto error; if (!(privconn->domainEventState = virObjectEventStateNew())) @@ -225,6 +226,7 @@ parallelsOpenDefault(virConnectPtr conn) error: virObjectUnref(privconn->domains); + VIR_FREE(privconn->networks); virObjectUnref(privconn->caps); virStoragePoolObjListFree(&privconn->pools); virObjectEventStateFree(privconn->domainEventState); @@ -283,6 +285,7 @@ parallelsConnectClose(virConnectPtr conn) virObjectUnref(privconn->caps); virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); + VIR_FREE(privconn->networks); virObjectEventStateFree(privconn->domainEventState); prlsdkDisconnect(privconn); conn->privateData = NULL; diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 960bd50..bfa7432 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -226,7 +226,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; } - if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) goto cleanup; net->active = 1; net->autostart = 1; @@ -259,7 +259,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } def->uuid_specified = 1; - if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) { + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) { virNetworkDefFree(def); goto cleanup; } @@ -337,7 +337,7 @@ int parallelsNetworkClose(virConnectPtr conn) { parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - virNetworkObjListFree(&privconn->networks); + virNetworkObjListFree(privconn->networks); parallelsDriverUnlock(privconn); return 0; } @@ -349,11 +349,11 @@ static int parallelsConnectNumOfNetworks(virConnectPtr conn) parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks.count; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (virNetworkObjIsActive(privconn->networks.objs[i])) + for (i = 0; i < privconn->networks->count; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (virNetworkObjIsActive(privconn->networks->objs[i])) nactive++; - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } parallelsDriverUnlock(privconn); @@ -369,16 +369,16 @@ static int parallelsConnectListNetworks(virConnectPtr conn, size_t i; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks.count && got < nnames; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (virNetworkObjIsActive(privconn->networks.objs[i])) { - if (VIR_STRDUP(names[got], privconn->networks.objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks.objs[i]); + for (i = 0; i < privconn->networks->count && got < nnames; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (virNetworkObjIsActive(privconn->networks->objs[i])) { + if (VIR_STRDUP(names[got], privconn->networks->objs[i]->def->name) < 0) { + virNetworkObjUnlock(privconn->networks->objs[i]); goto cleanup; } got++; } - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } parallelsDriverUnlock(privconn); @@ -398,11 +398,11 @@ static int parallelsConnectNumOfDefinedNetworks(virConnectPtr conn) parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks.count; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (!virNetworkObjIsActive(privconn->networks.objs[i])) + for (i = 0; i < privconn->networks->count; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (!virNetworkObjIsActive(privconn->networks->objs[i])) ninactive++; - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } parallelsDriverUnlock(privconn); @@ -418,16 +418,16 @@ static int parallelsConnectListDefinedNetworks(virConnectPtr conn, size_t i; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks.count && got < nnames; i++) { - virNetworkObjLock(privconn->networks.objs[i]); - if (!virNetworkObjIsActive(privconn->networks.objs[i])) { - if (VIR_STRDUP(names[got], privconn->networks.objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks.objs[i]); + for (i = 0; i < privconn->networks->count && got < nnames; i++) { + virNetworkObjLock(privconn->networks->objs[i]); + if (!virNetworkObjIsActive(privconn->networks->objs[i])) { + if (VIR_STRDUP(names[got], privconn->networks->objs[i]->def->name) < 0) { + virNetworkObjUnlock(privconn->networks->objs[i]); goto cleanup; } got++; } - virNetworkObjUnlock(privconn->networks.objs[i]); + virNetworkObjUnlock(privconn->networks->objs[i]); } parallelsDriverUnlock(privconn); return got; @@ -449,7 +449,7 @@ static int parallelsConnectListAllNetworks(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); parallelsDriverLock(privconn); - ret = virNetworkObjListExport(conn, &privconn->networks, nets, NULL, flags); + ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); parallelsDriverUnlock(privconn); return ret; @@ -463,7 +463,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, virNetworkPtr ret = NULL; parallelsDriverLock(privconn); - network = virNetworkFindByUUID(&privconn->networks, uuid); + network = virNetworkFindByUUID(privconn->networks, uuid); parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -487,7 +487,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, virNetworkPtr ret = NULL; parallelsDriverLock(privconn); - network = virNetworkFindByName(&privconn->networks, name); + network = virNetworkFindByName(privconn->networks, name); parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -513,7 +513,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, virCheckFlags(VIR_NETWORK_XML_INACTIVE, NULL); parallelsDriverLock(privconn); - network = virNetworkFindByUUID(&privconn->networks, net->uuid); + network = virNetworkFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!network) { @@ -537,7 +537,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) int ret = -1; parallelsDriverLock(privconn); - obj = virNetworkFindByUUID(&privconn->networks, net->uuid); + obj = virNetworkFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -558,7 +558,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) int ret = -1; parallelsDriverLock(privconn); - obj = virNetworkFindByUUID(&privconn->networks, net->uuid); + obj = virNetworkFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -580,7 +580,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, int ret = -1; parallelsDriverLock(privconn); - network = virNetworkFindByUUID(&privconn->networks, net->uuid); + network = virNetworkFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h index bebf841..ead5586 100644 --- a/src/parallels/parallels_utils.h +++ b/src/parallels/parallels_utils.h @@ -55,7 +55,7 @@ struct _parallelsConn { PRL_HANDLE server; PRL_UINT32 jobTimeout; virStoragePoolObjList pools; - virNetworkObjList networks; + virNetworkObjListPtr networks; virCapsPtr caps; virDomainXMLOptionPtr xmlopt; virObjectEventStatePtr domainEventState; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:18 +0100, Michal Privoznik wrote:
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_driver.c | 5 +++- src/parallels/parallels_network.c | 60 +++++++++++++++++++-------------------- src/parallels/parallels_utils.h | 2 +- 3 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index c9338b5..32f2ede 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -207,7 +207,8 @@ parallelsOpenDefault(virConnectPtr conn) NULL, NULL))) goto error;
- if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0)
This is a bit confusing. The network object is allocated in the VM driver open function ...
goto error;
if (!(privconn->domainEventState = virObjectEventStateNew())) @@ -225,6 +226,7 @@ parallelsOpenDefault(virConnectPtr conn)
error: virObjectUnref(privconn->domains); + VIR_FREE(privconn->networks); virObjectUnref(privconn->caps); virStoragePoolObjListFree(&privconn->pools); virObjectEventStateFree(privconn->domainEventState); @@ -283,6 +285,7 @@ parallelsConnectClose(virConnectPtr conn) virObjectUnref(privconn->caps); virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); + VIR_FREE(privconn->networks);
And deleted in the VM driver ...
virObjectEventStateFree(privconn->domainEventState); prlsdkDisconnect(privconn); conn->privateData = NULL; diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 960bd50..bfa7432 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -226,7 +226,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; }
- if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) goto cleanup; net->active = 1; net->autostart = 1; @@ -259,7 +259,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } def->uuid_specified = 1;
- if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) { + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) { virNetworkDefFree(def); goto cleanup; } @@ -337,7 +337,7 @@ int parallelsNetworkClose(virConnectPtr conn) { parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - virNetworkObjListFree(&privconn->networks); + virNetworkObjListFree(privconn->networks);
But is cleared in the network subdriver clenup function.
parallelsDriverUnlock(privconn); return 0; }
I think it should be put into parallelsNetworkOpen. Peter
26.02.2015 18:56, Peter Krempa пишет:
On Thu, Feb 26, 2015 at 15:17:18 +0100, Michal Privoznik wrote:
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_driver.c | 5 +++- src/parallels/parallels_network.c | 60 +++++++++++++++++++-------------------- src/parallels/parallels_utils.h | 2 +- 3 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index c9338b5..32f2ede 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -207,7 +207,8 @@ parallelsOpenDefault(virConnectPtr conn) NULL, NULL))) goto error;
- if (!(privconn->domains = virDomainObjListNew())) + if (!(privconn->domains = virDomainObjListNew()) || + VIR_ALLOC(privconn->networks) < 0) This is a bit confusing. The network object is allocated in the VM driver open function ...
goto error;
if (!(privconn->domainEventState = virObjectEventStateNew())) @@ -225,6 +226,7 @@ parallelsOpenDefault(virConnectPtr conn)
error: virObjectUnref(privconn->domains); + VIR_FREE(privconn->networks); virObjectUnref(privconn->caps); virStoragePoolObjListFree(&privconn->pools); virObjectEventStateFree(privconn->domainEventState); @@ -283,6 +285,7 @@ parallelsConnectClose(virConnectPtr conn) virObjectUnref(privconn->caps); virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); + VIR_FREE(privconn->networks);
And deleted in the VM driver ...
virObjectEventStateFree(privconn->domainEventState); prlsdkDisconnect(privconn); conn->privateData = NULL; diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 960bd50..bfa7432 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -226,7 +226,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; }
- if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) goto cleanup; net->active = 1; net->autostart = 1; @@ -259,7 +259,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } def->uuid_specified = 1;
- if (!(net = virNetworkAssignDef(&privconn->networks, def, false))) { + if (!(net = virNetworkAssignDef(privconn->networks, def, false))) { virNetworkDefFree(def); goto cleanup; } @@ -337,7 +337,7 @@ int parallelsNetworkClose(virConnectPtr conn) { parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - virNetworkObjListFree(&privconn->networks); + virNetworkObjListFree(privconn->networks);
But is cleared in the network subdriver clenup function.
parallelsDriverUnlock(privconn); return 0; }
I think it should be put into parallelsNetworkOpen.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list I think Peter's notes are reasonable. It is worth moving allocation/freeing of privconn->networks to parallelsNetworkOpen.
Maxim Nestratov
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 98 +++++++++++++++++++----------------- src/network/bridge_driver_platform.h | 2 +- 2 files changed, 52 insertions(+), 48 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index edc4ed4..1011984 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -130,7 +130,7 @@ networkObjFromNetwork(virNetworkPtr net) char uuidstr[VIR_UUID_STRING_BUFLEN]; networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, net->uuid); + network = virNetworkFindByUUID(driver->networks, net->uuid); networkDriverUnlock(); if (!network) { @@ -303,7 +303,7 @@ networkRemoveInactive(virNetworkObjPtr net) unlink(statusfile); /* remove the network definition */ - virNetworkRemoveInactive(&driver->networks, net); + virNetworkRemoveInactive(driver->networks, net); ret = 0; @@ -350,8 +350,8 @@ networkUpdateAllState(void) { size_t i; - for (i = 0; i < driver->networks.count; i++) { - virNetworkObjPtr obj = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count; i++) { + virNetworkObjPtr obj = driver->networks->objs[i]; virNetworkObjLock(obj); if (!virNetworkObjIsActive(obj)) { @@ -411,8 +411,8 @@ networkUpdateAllState(void) /* remove inactive transient networks */ i = 0; - while (i < driver->networks.count) { - virNetworkObjPtr obj = driver->networks.objs[i]; + while (i < driver->networks->count) { + virNetworkObjPtr obj = driver->networks->objs[i]; virNetworkObjLock(obj); if (!obj->persistent && !obj->active) { @@ -431,15 +431,15 @@ networkAutostartConfigs(void) { size_t i; - for (i = 0; i < driver->networks.count; i++) { - virNetworkObjLock(driver->networks.objs[i]); - if (driver->networks.objs[i]->autostart && - !virNetworkObjIsActive(driver->networks.objs[i])) { - if (networkStartNetwork(driver->networks.objs[i]) < 0) { + for (i = 0; i < driver->networks->count; i++) { + virNetworkObjLock(driver->networks->objs[i]); + if (driver->networks->objs[i]->autostart && + !virNetworkObjIsActive(driver->networks->objs[i])) { + if (networkStartNetwork(driver->networks->objs[i]) < 0) { /* failed to start but already logged */ } } - virNetworkObjUnlock(driver->networks.objs[i]); + virNetworkObjUnlock(driver->networks->objs[i]); } } @@ -638,11 +638,14 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driver->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); - if (virNetworkLoadAllState(&driver->networks, + if (VIR_ALLOC(driver->networks) < 0) + goto error; + + if (virNetworkLoadAllState(driver->networks, driver->stateDir) < 0) goto error; - if (virNetworkLoadAllConfigs(&driver->networks, + if (virNetworkLoadAllConfigs(driver->networks, driver->networkConfigDir, driver->networkAutostartDir) < 0) goto error; @@ -723,9 +726,9 @@ networkStateReload(void) return 0; networkDriverLock(); - virNetworkLoadAllState(&driver->networks, + virNetworkLoadAllState(driver->networks, driver->stateDir); - virNetworkLoadAllConfigs(&driver->networks, + virNetworkLoadAllConfigs(driver->networks, driver->networkConfigDir, driver->networkAutostartDir); networkReloadFirewallRules(); @@ -752,7 +755,8 @@ networkStateCleanup(void) virObjectEventStateFree(driver->networkEventState); /* free inactive networks */ - virNetworkObjListFree(&driver->networks); + virNetworkObjListFree(driver->networks); + VIR_FREE(driver->networks); VIR_FREE(driver->networkConfigDir); VIR_FREE(driver->networkAutostartDir); @@ -1751,8 +1755,8 @@ networkRefreshDaemons(void) VIR_INFO("Refreshing network daemons"); - for (i = 0; i < driver->networks.count; i++) { - virNetworkObjPtr network = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count; i++) { + virNetworkObjPtr network = driver->networks->objs[i]; virNetworkObjLock(network); if (virNetworkObjIsActive(network) && @@ -1779,8 +1783,8 @@ networkReloadFirewallRules(void) VIR_INFO("Reloading iptables rules"); - for (i = 0; i < driver->networks.count; i++) { - virNetworkObjPtr network = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count; i++) { + virNetworkObjPtr network = driver->networks->objs[i]; virNetworkObjLock(network); if (virNetworkObjIsActive(network) && @@ -2472,7 +2476,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkPtr ret = NULL; networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, uuid); + network = virNetworkFindByUUID(driver->networks, uuid); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -2499,7 +2503,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, virNetworkPtr ret = NULL; networkDriverLock(); - network = virNetworkFindByName(&driver->networks, name); + network = virNetworkFindByName(driver->networks, name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -2527,8 +2531,8 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) return -1; networkDriverLock(); - for (i = 0; i < driver->networks.count; i++) { - virNetworkObjPtr obj = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count; i++) { + virNetworkObjPtr obj = driver->networks->objs[i]; virNetworkObjLock(obj); if (virConnectNumOfNetworksCheckACL(conn, obj->def) && virNetworkObjIsActive(obj)) @@ -2548,8 +2552,8 @@ static int networkConnectListNetworks(virConnectPtr conn, char **const names, in return -1; networkDriverLock(); - for (i = 0; i < driver->networks.count && got < nnames; i++) { - virNetworkObjPtr obj = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count && got < nnames; i++) { + virNetworkObjPtr obj = driver->networks->objs[i]; virNetworkObjLock(obj); if (virConnectListNetworksCheckACL(conn, obj->def) && virNetworkObjIsActive(obj)) { @@ -2581,8 +2585,8 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) return -1; networkDriverLock(); - for (i = 0; i < driver->networks.count; i++) { - virNetworkObjPtr obj = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count; i++) { + virNetworkObjPtr obj = driver->networks->objs[i]; virNetworkObjLock(obj); if (virConnectNumOfDefinedNetworksCheckACL(conn, obj->def) && !virNetworkObjIsActive(obj)) @@ -2602,8 +2606,8 @@ static int networkConnectListDefinedNetworks(virConnectPtr conn, char **const na return -1; networkDriverLock(); - for (i = 0; i < driver->networks.count && got < nnames; i++) { - virNetworkObjPtr obj = driver->networks.objs[i]; + for (i = 0; i < driver->networks->count && got < nnames; i++) { + virNetworkObjPtr obj = driver->networks->objs[i]; virNetworkObjLock(obj); if (virConnectListDefinedNetworksCheckACL(conn, obj->def) && !virNetworkObjIsActive(obj)) { @@ -2638,7 +2642,7 @@ networkConnectListAllNetworks(virConnectPtr conn, goto cleanup; networkDriverLock(); - ret = virNetworkObjListExport(conn, &driver->networks, nets, + ret = virNetworkObjListExport(conn, driver->networks, nets, virConnectListAllNetworksCheckACL, flags); networkDriverUnlock(); @@ -2741,7 +2745,7 @@ networkValidate(virNetworkDefPtr def, bool usesInterface = false, usesAddress = false; /* check for duplicate networks */ - if (virNetworkObjIsDuplicate(&driver->networks, def, check_active) < 0) + if (virNetworkObjIsDuplicate(driver->networks, def, check_active) < 0) return -1; /* Only the three L3 network types that are configured by libvirt @@ -2751,7 +2755,7 @@ networkValidate(virNetworkDefPtr def, def->forward.type == VIR_NETWORK_FORWARD_NAT || def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { - if (virNetworkSetBridgeName(&driver->networks, def, 1)) + if (virNetworkSetBridgeName(driver->networks, def, 1)) return -1; virNetworkSetBridgeMacAddr(def); @@ -2969,12 +2973,12 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) * we assign the def with live = true in anticipation that it will * be started momentarily. */ - if (!(network = virNetworkAssignDef(&driver->networks, def, true))) + if (!(network = virNetworkAssignDef(driver->networks, def, true))) goto cleanup; def = NULL; if (networkStartNetwork(network) < 0) { - virNetworkRemoveInactive(&driver->networks, + virNetworkRemoveInactive(driver->networks, network); network = NULL; goto cleanup; @@ -3017,7 +3021,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (networkValidate(def, false) < 0) goto cleanup; - if (!(network = virNetworkAssignDef(&driver->networks, def, false))) + if (!(network = virNetworkAssignDef(driver->networks, def, false))) goto cleanup; /* def was assigned to network object */ @@ -3025,7 +3029,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { - virNetworkRemoveInactive(&driver->networks, network); + virNetworkRemoveInactive(driver->networks, network); network = NULL; goto cleanup; } @@ -3065,7 +3069,7 @@ networkUndefine(virNetworkPtr net) networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, net->uuid); + network = virNetworkFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -3138,7 +3142,7 @@ networkUpdate(virNetworkPtr net, networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, net->uuid); + network = virNetworkFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -3295,7 +3299,7 @@ static int networkCreate(virNetworkPtr net) virObjectEventPtr event = NULL; networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, net->uuid); + network = virNetworkFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3330,7 +3334,7 @@ static int networkDestroy(virNetworkPtr net) virObjectEventPtr event = NULL; networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, net->uuid); + network = virNetworkFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3456,7 +3460,7 @@ static int networkSetAutostart(virNetworkPtr net, int ret = -1; networkDriverLock(); - network = virNetworkFindByUUID(&driver->networks, net->uuid); + network = virNetworkFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3786,7 +3790,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, iface->data.network.actual = NULL; networkDriverLock(); - network = virNetworkFindByName(&driver->networks, iface->data.network.name); + network = virNetworkFindByName(driver->networks, iface->data.network.name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -4195,7 +4199,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, return 0; networkDriverLock(); - network = virNetworkFindByName(&driver->networks, iface->data.network.name); + network = virNetworkFindByName(driver->networks, iface->data.network.name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -4395,7 +4399,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, return 0; networkDriverLock(); - network = virNetworkFindByName(&driver->networks, iface->data.network.name); + network = virNetworkFindByName(driver->networks, iface->data.network.name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -4554,7 +4558,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) *netaddr = NULL; networkDriverLock(); - network = virNetworkFindByName(&driver->networks, netname); + network = virNetworkFindByName(driver->networks, netname); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 1e8264a..b7492e6 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -34,7 +34,7 @@ struct _virNetworkDriverState { virMutex lock; - virNetworkObjList networks; + virNetworkObjListPtr networks; char *networkConfigDir; char *networkAutostartDir; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:19 +0100, Michal Privoznik wrote:
In order to hide the object internals (and use just accessors everywhere), lets store a pointer to the object, instead of object itself.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 98 +++++++++++++++++++----------------- src/network/bridge_driver_platform.h | 2 +- 2 files changed, 52 insertions(+), 48 deletions(-)
ACK, Peter
It's returning virNetworkObjPtr after all. And it matches the pattern laid out by domain_conf.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 6 +++--- src/conf/network_conf.h | 4 ++-- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 14 +++++++------- src/parallels/parallels_network.c | 10 +++++----- src/test/test_driver.c | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 00be8b6..30fb309 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -86,8 +86,8 @@ virNetworkObjTaint(virNetworkObjPtr obj, return true; } -virNetworkObjPtr virNetworkFindByUUID(virNetworkObjListPtr nets, - const unsigned char *uuid) +virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, + const unsigned char *uuid) { size_t i; @@ -4150,7 +4150,7 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, virNetworkObjPtr net = NULL; /* See if a network with matching UUID already exists */ - net = virNetworkFindByUUID(nets, def->uuid); + net = virNetworkObjFindByUUID(nets, def->uuid); if (net) { /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(net->def->name, def->name)) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 74665e0..3298376 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -301,8 +301,8 @@ virNetworkObjIsActive(const virNetworkObj *net) bool virNetworkObjTaint(virNetworkObjPtr obj, virNetworkTaintFlags taint); -virNetworkObjPtr virNetworkFindByUUID(virNetworkObjListPtr nets, - const unsigned char *uuid); +virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, + const unsigned char *uuid); virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, const char *name); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ba05cc6..140f3f4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -559,13 +559,13 @@ virNetworkDefParseString; virNetworkDefUpdateSection; virNetworkDeleteConfig; virNetworkFindByName; -virNetworkFindByUUID; virNetworkForwardTypeToString; virNetworkIpDefNetmask; virNetworkIpDefPrefix; virNetworkLoadAllConfigs; virNetworkLoadAllState; virNetworkObjAssignDef; +virNetworkObjFindByUUID; virNetworkObjFree; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1011984..f67822a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -130,7 +130,7 @@ networkObjFromNetwork(virNetworkPtr net) char uuidstr[VIR_UUID_STRING_BUFLEN]; networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, net->uuid); + network = virNetworkObjFindByUUID(driver->networks, net->uuid); networkDriverUnlock(); if (!network) { @@ -2476,7 +2476,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkPtr ret = NULL; networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, uuid); + network = virNetworkObjFindByUUID(driver->networks, uuid); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3069,7 +3069,7 @@ networkUndefine(virNetworkPtr net) networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, net->uuid); + network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -3142,7 +3142,7 @@ networkUpdate(virNetworkPtr net, networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, net->uuid); + network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -3299,7 +3299,7 @@ static int networkCreate(virNetworkPtr net) virObjectEventPtr event = NULL; networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, net->uuid); + network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3334,7 +3334,7 @@ static int networkDestroy(virNetworkPtr net) virObjectEventPtr event = NULL; networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, net->uuid); + network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3460,7 +3460,7 @@ static int networkSetAutostart(virNetworkPtr net, int ret = -1; networkDriverLock(); - network = virNetworkFindByUUID(driver->networks, net->uuid); + network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index bfa7432..23eee89 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -463,7 +463,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, virNetworkPtr ret = NULL; parallelsDriverLock(privconn); - network = virNetworkFindByUUID(privconn->networks, uuid); + network = virNetworkObjFindByUUID(privconn->networks, uuid); parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -513,7 +513,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, virCheckFlags(VIR_NETWORK_XML_INACTIVE, NULL); parallelsDriverLock(privconn); - network = virNetworkFindByUUID(privconn->networks, net->uuid); + network = virNetworkObjFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!network) { @@ -537,7 +537,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) int ret = -1; parallelsDriverLock(privconn); - obj = virNetworkFindByUUID(privconn->networks, net->uuid); + obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -558,7 +558,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) int ret = -1; parallelsDriverLock(privconn); - obj = virNetworkFindByUUID(privconn->networks, net->uuid); + obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -580,7 +580,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, int ret = -1; parallelsDriverLock(privconn); - network = virNetworkFindByUUID(privconn->networks, net->uuid); + network = virNetworkObjFindByUUID(privconn->networks, net->uuid); parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 90df0e7..a747704 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3499,7 +3499,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, virNetworkPtr ret = NULL; testDriverLock(privconn); - net = virNetworkFindByUUID(privconn->networks, uuid); + net = virNetworkObjFindByUUID(privconn->networks, uuid); testDriverUnlock(privconn); if (net == NULL) { @@ -3654,7 +3654,7 @@ static int testNetworkIsActive(virNetworkPtr net) int ret = -1; testDriverLock(privconn); - obj = virNetworkFindByUUID(privconn->networks, net->uuid); + obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -3675,7 +3675,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) int ret = -1; testDriverLock(privconn); - obj = virNetworkFindByUUID(privconn->networks, net->uuid); + obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); @@ -3812,7 +3812,7 @@ testNetworkUpdate(virNetworkPtr net, testDriverLock(privconn); - network = virNetworkFindByUUID(privconn->networks, net->uuid); + network = virNetworkObjFindByUUID(privconn->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:20 +0100, Michal Privoznik wrote:
It's returning virNetworkObjPtr after all. And it matches the pattern laid out by domain_conf.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 6 +++--- src/conf/network_conf.h | 4 ++-- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 14 +++++++------- src/parallels/parallels_network.c | 10 +++++----- src/test/test_driver.c | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-)
ACK, Peter
It's returning virNetworkObjPtr after all. And it matches the pattern laid out by domain_conf.h. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 8 ++++---- src/conf/network_conf.h | 4 ++-- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 10 +++++----- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 16 ++++++++-------- 6 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 30fb309..d0e7e1b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -101,8 +101,8 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, return NULL; } -virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, - const char *name) +virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, + const char *name) { size_t i; @@ -370,7 +370,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, { virNetworkObjPtr network; - if ((network = virNetworkFindByName(nets, def->name))) { + if ((network = virNetworkObjFindByName(nets, def->name))) { virNetworkObjAssignDef(network, def, live); return network; } @@ -4175,7 +4175,7 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, ret = 1; } else { /* UUID does not match, but if a name matches, refuse it */ - net = virNetworkFindByName(nets, def->name); + net = virNetworkObjFindByName(nets, def->name); if (net) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(net->def->uuid, uuidstr); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3298376..16aea99 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -303,8 +303,8 @@ bool virNetworkObjTaint(virNetworkObjPtr obj, virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); -virNetworkObjPtr virNetworkFindByName(virNetworkObjListPtr nets, - const char *name); +virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, + const char *name); void virNetworkDefFree(virNetworkDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 140f3f4..cbba8a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -558,13 +558,13 @@ virNetworkDefParseNode; virNetworkDefParseString; virNetworkDefUpdateSection; virNetworkDeleteConfig; -virNetworkFindByName; virNetworkForwardTypeToString; virNetworkIpDefNetmask; virNetworkIpDefPrefix; virNetworkLoadAllConfigs; virNetworkLoadAllState; virNetworkObjAssignDef; +virNetworkObjFindByName; virNetworkObjFindByUUID; virNetworkObjFree; virNetworkObjGetPersistentDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f67822a..268af49 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2503,7 +2503,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, virNetworkPtr ret = NULL; networkDriverLock(); - network = virNetworkFindByName(driver->networks, name); + network = virNetworkObjFindByName(driver->networks, name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3790,7 +3790,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, iface->data.network.actual = NULL; networkDriverLock(); - network = virNetworkFindByName(driver->networks, iface->data.network.name); + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -4199,7 +4199,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, return 0; networkDriverLock(); - network = virNetworkFindByName(driver->networks, iface->data.network.name); + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -4399,7 +4399,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, return 0; networkDriverLock(); - network = virNetworkFindByName(driver->networks, iface->data.network.name); + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -4558,7 +4558,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) *netaddr = NULL; networkDriverLock(); - network = virNetworkFindByName(driver->networks, netname); + network = virNetworkObjFindByName(driver->networks, netname); networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 23eee89..94e77cc 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -487,7 +487,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, virNetworkPtr ret = NULL; parallelsDriverLock(privconn); - network = virNetworkFindByName(privconn->networks, name); + network = virNetworkObjFindByName(privconn->networks, name); parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a747704..ecb7bcd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3523,7 +3523,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, virNetworkPtr ret = NULL; testDriverLock(privconn); - net = virNetworkFindByName(privconn->networks, name); + net = virNetworkObjFindByName(privconn->networks, name); testDriverUnlock(privconn); if (net == NULL) { @@ -3764,7 +3764,7 @@ static int testNetworkUndefine(virNetworkPtr network) virObjectEventPtr event = NULL; testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -3852,7 +3852,7 @@ static int testNetworkCreate(virNetworkPtr network) virObjectEventPtr event = NULL; testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -3888,7 +3888,7 @@ static int testNetworkDestroy(virNetworkPtr network) virObjectEventPtr event = NULL; testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); @@ -3924,7 +3924,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, virCheckFlags(0, NULL); testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -3946,7 +3946,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { virNetworkObjPtr privnet; testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -3977,7 +3977,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, int ret = -1; testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { @@ -4002,7 +4002,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, int ret = -1; testDriverLock(privconn); - privnet = virNetworkFindByName(privconn->networks, network->name); + privnet = virNetworkObjFindByName(privconn->networks, network->name); testDriverUnlock(privconn); if (privnet == NULL) { -- 2.0.5
s/ByUUID/ByName/g in subject On Thu, Feb 26, 2015 at 15:17:21 +0100, Michal Privoznik wrote:
It's returning virNetworkObjPtr after all. And it matches the pattern laid out by domain_conf.h.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 8 ++++---- src/conf/network_conf.h | 4 ++-- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 10 +++++----- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 16 ++++++++-------- 6 files changed, 21 insertions(+), 21 deletions(-)
ACK with commit message subject fixed. Peter
This API will be used in the future to call passed callback over each network object in the list. It's slightly different to it virDomainObjListForEach counterpart, because virDomainObjList uses a hash table to store domain object, while virNetworkObjList uses an array. Therefore, iterating over the entries is slightly more complicated, as we want to allow the callback to remove the network object while still iterating. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 22 ++++++++++++++++++++++ src/conf/network_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d0e7e1b..bcfcccd 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4290,3 +4290,25 @@ virNetworkObjListExport(virConnectPtr conn, VIR_FREE(tmp_nets); return ret; } + +int +virNetworkObjListForEach(virNetworkObjListPtr nets, + virNetworkObjListIterator callback, + void *opaque) +{ + int ret = 0; + size_t i = 0, oldcount; + + /* Deliberately don't use for-loop here. We want to allow + * @callback to remove a network object, in which case + * nets->count is decremented. */ + while (i < nets->count) { + oldcount = nets->count; + if (callback(nets->objs[i], opaque) < 0) + ret = -1; + if (oldcount == nets->count) + i++; + } + + return ret; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 16aea99..b9acc0e 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -445,6 +445,12 @@ int virNetworkObjListExport(virConnectPtr conn, virNetworkObjListFilter filter, unsigned int flags); +typedef int (*virNetworkObjListIterator)(virNetworkObjPtr dom, + void *opaque); + +int virNetworkObjListForEach(virNetworkObjListPtr nets, + virNetworkObjListIterator callback, + void *opaque); /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cbba8a9..8577006 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -570,6 +570,7 @@ virNetworkObjFree; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; +virNetworkObjListForEach; virNetworkObjListFree; virNetworkObjLock; virNetworkObjReplacePersistentDef; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:22 +0100, Michal Privoznik wrote:
This API will be used in the future to call passed callback over each network object in the list. It's slightly different to it
s/to it/to it's/
virDomainObjListForEach counterpart, because virDomainObjList uses a hash table to store domain object, while virNetworkObjList uses an array. Therefore, iterating over the entries is slightly more complicated, as we want to allow the callback to remove the network object while still iterating.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 22 ++++++++++++++++++++++ src/conf/network_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+)
...
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 16aea99..b9acc0e 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -445,6 +445,12 @@ int virNetworkObjListExport(virConnectPtr conn, virNetworkObjListFilter filter, unsigned int flags);
+typedef int (*virNetworkObjListIterator)(virNetworkObjPtr dom, + void *opaque);
Is there a reason you don't pass the object list as you want to allow deletion from the list itself? Users will have to pull it through the opaque argument.
+ +int virNetworkObjListForEach(virNetworkObjListPtr nets, + virNetworkObjListIterator callback, + void *opaque); /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def,
ACK, Peter
An accessor following pattern laid out by virDomainObjList* APIs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 8 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 47 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index bcfcccd..84becb5 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4312,3 +4312,41 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, return ret; } + +int +virNetworkObjListGetNames(virNetworkObjListPtr nets, + bool active, + char **names, + int nnames, + virNetworkObjListFilter filter, + virConnectPtr conn) +{ + int got = 0; + size_t i; + + for (i = 0; i < nets->count && got < nnames; i++) { + virNetworkObjPtr obj = nets->objs[i]; + virNetworkObjLock(obj); + if (filter && !filter(conn, obj->def)) { + virNetworkObjUnlock(obj); + continue; + } + + if ((active && virNetworkObjIsActive(obj)) || + (!active && !virNetworkObjIsActive(obj))) { + if (VIR_STRDUP(names[got], obj->def->name) < 0) { + virNetworkObjUnlock(obj); + goto error; + } + got++; + } + virNetworkObjUnlock(obj); + } + + return got; + + error: + for (i = 0; i < got; i++) + VIR_FREE(names[i]); + return -1; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index b9acc0e..d357892 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -451,6 +451,14 @@ typedef int (*virNetworkObjListIterator)(virNetworkObjPtr dom, int virNetworkObjListForEach(virNetworkObjListPtr nets, virNetworkObjListIterator callback, void *opaque); + +int virNetworkObjListGetNames(virNetworkObjListPtr nets, + bool active, + char **names, + int nnames, + virNetworkObjListFilter filter, + virConnectPtr conn); + /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8577006..3e7e389 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -572,6 +572,7 @@ virNetworkObjIsDuplicate; virNetworkObjListExport; virNetworkObjListForEach; virNetworkObjListFree; +virNetworkObjListGetNames; virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:23 +0100, Michal Privoznik wrote:
An accessor following pattern laid out by virDomainObjList* APIs.
The domain object accessors are quite different in this case, but ...
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 38 ++++++++++++++++++++++++++++++++++++++ src/conf/network_conf.h | 8 ++++++++ src/libvirt_private.syms | 1 + 3 files changed, 47 insertions(+)
ACK, Peter
An accessor following pattern laid out by virDomainObjList* APIs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 26 ++++++++++++++++++++++++++ src/conf/network_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 32 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 84becb5..24a5f7c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -4350,3 +4350,29 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, VIR_FREE(names[i]); return -1; } + +int +virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, + bool active, + virNetworkObjListFilter filter, + virConnectPtr conn) +{ + int count = 0; + size_t i; + + for (i = 0; i < nets->count; i++) { + virNetworkObjPtr obj = nets->objs[i]; + virNetworkObjLock(obj); + if (filter && !filter(conn, obj->def)) { + virNetworkObjUnlock(obj); + continue; + } + + if ((active && virNetworkObjIsActive(obj)) || + (!active && !virNetworkObjIsActive(obj))) + count++; + virNetworkObjUnlock(obj); + } + + return count; +} diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index d357892..164fb1a 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -459,6 +459,11 @@ int virNetworkObjListGetNames(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn); +int virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, + bool active, + virNetworkObjListFilter filter, + virConnectPtr conn); + /* for testing */ int virNetworkDefUpdateSection(virNetworkDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e7e389..8aacc30 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -573,6 +573,7 @@ virNetworkObjListExport; virNetworkObjListForEach; virNetworkObjListFree; virNetworkObjListGetNames; +virNetworkObjListNumOfNetworks; virNetworkObjLock; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:24 +0100, Michal Privoznik wrote:
An accessor following pattern laid out by virDomainObjList* APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 26 ++++++++++++++++++++++++++ src/conf/network_conf.h | 5 +++++ src/libvirt_private.syms | 1 + 3 files changed, 32 insertions(+)
ACK, Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 333 ++++++++++++++++++++------------------------ 1 file changed, 148 insertions(+), 185 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 268af49..1c73342 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -342,105 +342,91 @@ networkBridgeDummyNicName(const char *brname) return nicname; } -/* Update the internal status of all allegedly active networks - * according to external conditions on the host (i.e. anything that - * isn't stored directly in each network's state file). */ -static void -networkUpdateAllState(void) +static int +networkUpdateAllState(virNetworkObjPtr obj, + void *opaque ATTRIBUTE_UNUSED) { - size_t i; + int ret = -1; - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; + virNetworkObjLock(obj); + if (!virNetworkObjIsActive(obj)) { + virNetworkObjUnlock(obj); + return 0; + } - virNetworkObjLock(obj); - if (!virNetworkObjIsActive(obj)) { - virNetworkObjUnlock(obj); - continue; - } + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge doesn't exist, then mark it inactive */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + obj->active = 0; + break; - switch (obj->def->forward.type) { - case VIR_NETWORK_FORWARD_NONE: - case VIR_NETWORK_FORWARD_NAT: - case VIR_NETWORK_FORWARD_ROUTE: - /* If bridge doesn't exist, then mark it inactive */ - if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) obj->active = 0; break; - - case VIR_NETWORK_FORWARD_BRIDGE: - if (obj->def->bridge) { - if (virNetDevExists(obj->def->bridge) != 1) - obj->active = 0; - break; - } - /* intentionally drop through to common case for all - * macvtap networks (forward='bridge' with no bridge - * device defined is macvtap using its 'bridge' mode) - */ - case VIR_NETWORK_FORWARD_PRIVATE: - case VIR_NETWORK_FORWARD_VEPA: - case VIR_NETWORK_FORWARD_PASSTHROUGH: - /* so far no extra checks */ - break; - - case VIR_NETWORK_FORWARD_HOSTDEV: - /* so far no extra checks */ - break; - } - - /* Try and read dnsmasq/radvd pids of active networks */ - if (obj->active && obj->def->ips && (obj->def->nips > 0)) { - char *radvdpidbase; - - ignore_value(virPidFileReadIfAlive(driver->pidDir, - obj->def->name, - &obj->dnsmasqPid, - dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - radvdpidbase = networkRadvdPidfileBasename(obj->def->name); - if (!radvdpidbase) - break; - ignore_value(virPidFileReadIfAlive(driver->pidDir, - radvdpidbase, - &obj->radvdPid, RADVD)); - VIR_FREE(radvdpidbase); } - - virNetworkObjUnlock(obj); + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + /* so far no extra checks */ + break; + + case VIR_NETWORK_FORWARD_HOSTDEV: + /* so far no extra checks */ + break; } - /* remove inactive transient networks */ - i = 0; - while (i < driver->networks->count) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - - if (!obj->persistent && !obj->active) { - networkRemoveInactive(obj); - continue; - } + /* Try and read dnsmasq/radvd pids of active networks */ + if (obj->active && obj->def->ips && (obj->def->nips > 0)) { + char *radvdpidbase; + + ignore_value(virPidFileReadIfAlive(driver->pidDir, + obj->def->name, + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + goto cleanup; + + ignore_value(virPidFileReadIfAlive(driver->pidDir, + radvdpidbase, + &obj->radvdPid, RADVD)); + VIR_FREE(radvdpidbase); + } + ret = 0; + cleanup: + if (!obj->persistent && !obj->active) + networkRemoveInactive(obj); + else virNetworkObjUnlock(obj); - i++; - } + return ret; } - -static void -networkAutostartConfigs(void) +static int +networkAutostartConfigs(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED) { - size_t i; + int ret = -1; - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjLock(driver->networks->objs[i]); - if (driver->networks->objs[i]->autostart && - !virNetworkObjIsActive(driver->networks->objs[i])) { - if (networkStartNetwork(driver->networks->objs[i]) < 0) { - /* failed to start but already logged */ - } - } - virNetworkObjUnlock(driver->networks->objs[i]); - } + virNetworkObjLock(net); + if (net->autostart && + !virNetworkObjIsActive(net) && + networkStartNetwork(net) < 0) + goto cleanup; + + ret = 0; + cleanup: + virNetworkObjUnlock(net); + return ret; } #if HAVE_FIREWALLD @@ -650,7 +636,14 @@ networkStateInitialize(bool privileged, driver->networkAutostartDir) < 0) goto error; - networkUpdateAllState(); + + /* Update the internal status of all allegedly active + * networks according to external conditions on the host + * (i.e. anything that isn't stored directly in each + * network's state file). */ + virNetworkObjListForEach(driver->networks, + networkUpdateAllState, + NULL); networkReloadFirewallRules(); networkRefreshDaemons(); @@ -709,7 +702,9 @@ networkStateAutoStart(void) return; networkDriverLock(); - networkAutostartConfigs(); + virNetworkObjListForEach(driver->networks, + networkAutostartConfigs, + NULL); networkDriverUnlock(); } @@ -733,7 +728,9 @@ networkStateReload(void) driver->networkAutostartDir); networkReloadFirewallRules(); networkRefreshDaemons(); - networkAutostartConfigs(); + virNetworkObjListForEach(driver->networks, + networkAutostartConfigs, + NULL); networkDriverUnlock(); return 0; } @@ -1745,62 +1742,70 @@ networkRestartRadvd(virNetworkObjPtr network) } #endif /* #if 0 */ +static int +networkRefreshDaemonsHelper(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED) +{ + + virNetworkObjLock(net); + if (virNetworkObjIsActive(net) && + ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || + (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || + (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + /* Only the three L3 network types that are configured by + * libvirt will have a dnsmasq or radvd daemon associated + * with them. Here we send a SIGHUP to an existing + * dnsmasq and/or radvd, or restart them if they've + * disappeared. + */ + networkRefreshDhcpDaemon(net); + networkRefreshRadvd(net); + } + virNetworkObjUnlock(net); + return 0; +} + /* SIGHUP/restart any dnsmasq or radvd daemons. * This should be called when libvirtd is restarted. */ static void networkRefreshDaemons(void) { - size_t i; - VIR_INFO("Refreshing network daemons"); + virNetworkObjListForEach(driver->networks, + networkRefreshDaemonsHelper, + NULL); +} - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr network = driver->networks->objs[i]; +static int +networkReloadFirewallRulesHelper(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED) +{ - virNetworkObjLock(network); - if (virNetworkObjIsActive(network) && - ((network->def->forward.type == VIR_NETWORK_FORWARD_NONE) || - (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { - /* Only the three L3 network types that are configured by - * libvirt will have a dnsmasq or radvd daemon associated - * with them. Here we send a SIGHUP to an existing - * dnsmasq and/or radvd, or restart them if they've - * disappeared. - */ - networkRefreshDhcpDaemon(network); - networkRefreshRadvd(network); + virNetworkObjLock(net); + if (virNetworkObjIsActive(net) && + ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || + (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || + (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + /* Only the three L3 network types that are configured by libvirt + * need to have iptables rules reloaded. + */ + networkRemoveFirewallRules(net->def); + if (networkAddFirewallRules(net->def) < 0) { + /* failed to add but already logged */ } - virNetworkObjUnlock(network); } + virNetworkObjUnlock(net); + return 0; } static void networkReloadFirewallRules(void) { - size_t i; - VIR_INFO("Reloading iptables rules"); - - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr network = driver->networks->objs[i]; - - virNetworkObjLock(network); - if (virNetworkObjIsActive(network) && - ((network->def->forward.type == VIR_NETWORK_FORWARD_NONE) || - (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (network->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { - /* Only the three L3 network types that are configured by libvirt - * need to have iptables rules reloaded. - */ - networkRemoveFirewallRules(network->def); - if (networkAddFirewallRules(network->def) < 0) { - /* failed to add but already logged */ - } - } - virNetworkObjUnlock(network); - } + virNetworkObjListForEach(driver->networks, + networkReloadFirewallRulesHelper, + NULL); } /* Enable IP Forwarding. Return 0 for success, -1 for failure. */ @@ -2524,21 +2529,16 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, static int networkConnectNumOfNetworks(virConnectPtr conn) { - int nactive = 0; - size_t i; + int nactive; if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectNumOfNetworksCheckACL(conn, obj->def) && - virNetworkObjIsActive(obj)) - nactive++; - virNetworkObjUnlock(obj); - } + nactive = virNetworkObjListNumOfNetworks(driver->networks, + true, + virConnectNumOfNetworksCheckACL, + conn); networkDriverUnlock(); return nactive; @@ -2546,53 +2546,32 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) static int networkConnectListNetworks(virConnectPtr conn, char **const names, int nnames) { int got = 0; - size_t i; if (virConnectListNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count && got < nnames; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectListNetworksCheckACL(conn, obj->def) && - virNetworkObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(obj); - } + got = virNetworkObjListGetNames(driver->networks, + true, names, nnames, + virConnectListNetworksCheckACL, + conn); networkDriverUnlock(); return got; - - cleanup: - networkDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) { int ninactive = 0; - size_t i; if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectNumOfDefinedNetworksCheckACL(conn, obj->def) && - !virNetworkObjIsActive(obj)) - ninactive++; - virNetworkObjUnlock(obj); - } + ninactive = virNetworkObjListNumOfNetworks(driver->networks, + false, + virConnectNumOfDefinedNetworksCheckACL, + conn); networkDriverUnlock(); return ninactive; @@ -2600,33 +2579,17 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) static int networkConnectListDefinedNetworks(virConnectPtr conn, char **const names, int nnames) { int got = 0; - size_t i; if (virConnectListDefinedNetworksEnsureACL(conn) < 0) return -1; networkDriverLock(); - for (i = 0; i < driver->networks->count && got < nnames; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - if (virConnectListDefinedNetworksCheckACL(conn, obj->def) && - !virNetworkObjIsActive(obj)) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(obj); - } + got = virNetworkObjListGetNames(driver->networks, + false, names, nnames, + virConnectListDefinedNetworksCheckACL, + conn); networkDriverUnlock(); return got; - - cleanup: - networkDriverUnlock(); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:25 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 333 ++++++++++++++++++++------------------------ 1 file changed, 148 insertions(+), 185 deletions(-)
Some bikeshedding below ...
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 268af49..1c73342 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -342,105 +342,91 @@ networkBridgeDummyNicName(const char *brname) return nicname; }
-/* Update the internal status of all allegedly active networks - * according to external conditions on the host (i.e. anything that - * isn't stored directly in each network's state file). */ -static void -networkUpdateAllState(void) +static int +networkUpdateAllState(virNetworkObjPtr obj,
This function now updates state of one network, thus the 'All' word is misleading.
+ void *opaque ATTRIBUTE_UNUSED) { - size_t i; + int ret = -1;
- for (i = 0; i < driver->networks->count; i++) { - virNetworkObjPtr obj = driver->networks->objs[i]; + virNetworkObjLock(obj); + if (!virNetworkObjIsActive(obj)) { + virNetworkObjUnlock(obj); + return 0; + }
- virNetworkObjLock(obj); - if (!virNetworkObjIsActive(obj)) { - virNetworkObjUnlock(obj); - continue; - } + switch (obj->def->forward.type) { + case VIR_NETWORK_FORWARD_NONE: + case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_ROUTE: + /* If bridge doesn't exist, then mark it inactive */ + if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + obj->active = 0; + break;
- switch (obj->def->forward.type) { - case VIR_NETWORK_FORWARD_NONE: - case VIR_NETWORK_FORWARD_NAT: - case VIR_NETWORK_FORWARD_ROUTE: - /* If bridge doesn't exist, then mark it inactive */ - if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) + case VIR_NETWORK_FORWARD_BRIDGE: + if (obj->def->bridge) { + if (virNetDevExists(obj->def->bridge) != 1) obj->active = 0; break; - - case VIR_NETWORK_FORWARD_BRIDGE: - if (obj->def->bridge) { - if (virNetDevExists(obj->def->bridge) != 1) - obj->active = 0; - break; - } - /* intentionally drop through to common case for all - * macvtap networks (forward='bridge' with no bridge - * device defined is macvtap using its 'bridge' mode) - */ - case VIR_NETWORK_FORWARD_PRIVATE: - case VIR_NETWORK_FORWARD_VEPA: - case VIR_NETWORK_FORWARD_PASSTHROUGH: - /* so far no extra checks */ - break; - - case VIR_NETWORK_FORWARD_HOSTDEV: - /* so far no extra checks */ - break; - } - - /* Try and read dnsmasq/radvd pids of active networks */ - if (obj->active && obj->def->ips && (obj->def->nips > 0)) { - char *radvdpidbase; - - ignore_value(virPidFileReadIfAlive(driver->pidDir, - obj->def->name, - &obj->dnsmasqPid, - dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); - radvdpidbase = networkRadvdPidfileBasename(obj->def->name); - if (!radvdpidbase) - break; - ignore_value(virPidFileReadIfAlive(driver->pidDir, - radvdpidbase, - &obj->radvdPid, RADVD)); - VIR_FREE(radvdpidbase); } - - virNetworkObjUnlock(obj); + /* intentionally drop through to common case for all + * macvtap networks (forward='bridge' with no bridge + * device defined is macvtap using its 'bridge' mode) + */ + case VIR_NETWORK_FORWARD_PRIVATE: + case VIR_NETWORK_FORWARD_VEPA: + case VIR_NETWORK_FORWARD_PASSTHROUGH: + /* so far no extra checks */ + break; + + case VIR_NETWORK_FORWARD_HOSTDEV: + /* so far no extra checks */ + break; }
- /* remove inactive transient networks */ - i = 0; - while (i < driver->networks->count) { - virNetworkObjPtr obj = driver->networks->objs[i]; - virNetworkObjLock(obj); - - if (!obj->persistent && !obj->active) { - networkRemoveInactive(obj); - continue; - } + /* Try and read dnsmasq/radvd pids of active networks */ + if (obj->active && obj->def->ips && (obj->def->nips > 0)) { + char *radvdpidbase; + + ignore_value(virPidFileReadIfAlive(driver->pidDir, + obj->def->name, + &obj->dnsmasqPid, + dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps))); + radvdpidbase = networkRadvdPidfileBasename(obj->def->name); + if (!radvdpidbase) + goto cleanup; + + ignore_value(virPidFileReadIfAlive(driver->pidDir, + radvdpidbase, + &obj->radvdPid, RADVD)); + VIR_FREE(radvdpidbase); + }
+ ret = 0; + cleanup: + if (!obj->persistent && !obj->active) + networkRemoveInactive(obj); + else virNetworkObjUnlock(obj); - i++; - } + return ret; }
- -static void -networkAutostartConfigs(void) +static int +networkAutostartConfigs(virNetworkObjPtr net, + void *opaque ATTRIBUTE_UNUSED)
Same here, plural may be misleading. ACK with or without naming fixed. Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 64 ++++++++++---------------------------------------- 1 file changed, 12 insertions(+), 52 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ecb7bcd..0a68283 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3543,16 +3543,11 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, static int testConnectNumOfNetworks(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - int numActive = 0; - size_t i; + int numActive; testDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) - numActive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + numActive = virNetworkObjListNumOfNetworks(privconn->networks, + true, NULL, conn); testDriverUnlock(privconn); return numActive; @@ -3560,44 +3555,24 @@ static int testConnectNumOfNetworks(virConnectPtr conn) static int testConnectListNetworks(virConnectPtr conn, char **const names, int nnames) { testConnPtr privconn = conn->privateData; - int n = 0; - size_t i; + int n; testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->networks->count && n < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i]) && - VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto error; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + n = virNetworkObjListGetNames(privconn->networks, + true, names, nnames, NULL, conn); testDriverUnlock(privconn); return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } static int testConnectNumOfDefinedNetworks(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - int numInactive = 0; - size_t i; + int numInactive; testDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) - numInactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + numInactive = virNetworkObjListNumOfNetworks(privconn->networks, + false, NULL, conn); testDriverUnlock(privconn); return numInactive; @@ -3605,29 +3580,14 @@ static int testConnectNumOfDefinedNetworks(virConnectPtr conn) static int testConnectListDefinedNetworks(virConnectPtr conn, char **const names, int nnames) { testConnPtr privconn = conn->privateData; - int n = 0; - size_t i; + int n; testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames); - for (i = 0; i < privconn->networks->count && n < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i]) && - VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto error; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + n = virNetworkObjListGetNames(privconn->networks, + false, names, nnames, NULL, conn); testDriverUnlock(privconn); return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; } static int -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:26 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 64 ++++++++++---------------------------------------- 1 file changed, 12 insertions(+), 52 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ecb7bcd..0a68283 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3543,16 +3543,11 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, static int testConnectNumOfNetworks(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - int numActive = 0; - size_t i; + int numActive;
testDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) - numActive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + numActive = virNetworkObjListNumOfNetworks(privconn->networks, + true, NULL, conn); testDriverUnlock(privconn);
return numActive; @@ -3560,44 +3555,24 @@ static int testConnectNumOfNetworks(virConnectPtr conn)
static int testConnectListNetworks(virConnectPtr conn, char **const names, int nnames) { testConnPtr privconn = conn->privateData; - int n = 0; - size_t i; + int n;
testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames);
The list will be no longer set to NULL pointers.
- for (i = 0; i < privconn->networks->count && n < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i]) && - VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto error; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + n = virNetworkObjListGetNames(privconn->networks, + true, names, nnames, NULL, conn); testDriverUnlock(privconn);
return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; }
static int testConnectNumOfDefinedNetworks(virConnectPtr conn) { testConnPtr privconn = conn->privateData; - int numInactive = 0; - size_t i; + int numInactive;
testDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) - numInactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + numInactive = virNetworkObjListNumOfNetworks(privconn->networks, + false, NULL, conn); testDriverUnlock(privconn);
return numInactive; @@ -3605,29 +3580,14 @@ static int testConnectNumOfDefinedNetworks(virConnectPtr conn)
static int testConnectListDefinedNetworks(virConnectPtr conn, char **const names, int nnames) { testConnPtr privconn = conn->privateData; - int n = 0; - size_t i; + int n;
testDriverLock(privconn); - memset(names, 0, sizeof(*names)*nnames);
Here too
- for (i = 0; i < privconn->networks->count && n < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i]) && - VIR_STRDUP(names[n++], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto error; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + n = virNetworkObjListGetNames(privconn->networks, + false, names, nnames, NULL, conn); testDriverUnlock(privconn);
return n; - - error: - for (n = 0; n < nnames; n++) - VIR_FREE(names[n]); - testDriverUnlock(privconn); - return -1; }
static int
But I don't think that should be a problem. ACK Peter
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 66 +++++++-------------------------------- 1 file changed, 12 insertions(+), 54 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 94e77cc..868e3f5 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -344,17 +344,12 @@ int parallelsNetworkClose(virConnectPtr conn) static int parallelsConnectNumOfNetworks(virConnectPtr conn) { - int nactive = 0; - size_t i; + int nactive; parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) - nactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + nactive = virNetworkObjListNumOfNetworks(privconn->networks, + true, NULL, conn); parallelsDriverUnlock(privconn); return nactive; @@ -365,45 +360,24 @@ static int parallelsConnectListNetworks(virConnectPtr conn, int nnames) { parallelsConnPtr privconn = conn->privateData; - int got = 0; - size_t i; + int got; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count && got < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (virNetworkObjIsActive(privconn->networks->objs[i])) { - if (VIR_STRDUP(names[got], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + got = virNetworkObjListGetNames(privconn->networks, + true, names, nnames, NULL, conn); parallelsDriverUnlock(privconn); return got; - - cleanup: - parallelsDriverUnlock(privconn); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int parallelsConnectNumOfDefinedNetworks(virConnectPtr conn) { - int ninactive = 0; - size_t i; + int ninactive; parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) - ninactive++; - virNetworkObjUnlock(privconn->networks->objs[i]); - } + ninactive = virNetworkObjListNumOfNetworks(privconn->networks, + false, NULL, conn); parallelsDriverUnlock(privconn); return ninactive; @@ -414,29 +388,13 @@ static int parallelsConnectListDefinedNetworks(virConnectPtr conn, int nnames) { parallelsConnPtr privconn = conn->privateData; - int got = 0; - size_t i; + int got; parallelsDriverLock(privconn); - for (i = 0; i < privconn->networks->count && got < nnames; i++) { - virNetworkObjLock(privconn->networks->objs[i]); - if (!virNetworkObjIsActive(privconn->networks->objs[i])) { - if (VIR_STRDUP(names[got], privconn->networks->objs[i]->def->name) < 0) { - virNetworkObjUnlock(privconn->networks->objs[i]); - goto cleanup; - } - got++; - } - virNetworkObjUnlock(privconn->networks->objs[i]); - } + got = virNetworkObjListGetNames(privconn->networks, + false, names, nnames, NULL, conn); parallelsDriverUnlock(privconn); return got; - - cleanup: - parallelsDriverUnlock(privconn); - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; } static int parallelsConnectListAllNetworks(virConnectPtr conn, -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:27 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 66 +++++++-------------------------------- 1 file changed, 12 insertions(+), 54 deletions(-)
ACK, Peter
Well, one day this will be self-locking object, but not today. But lets prepare the code for that! Moreover, virNetworkObjListFree() is no longer needed, so turn it into virNetworkObjListDispose(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/network_conf.c | 53 +++++++++++++++++++++++++++++---------- src/conf/network_conf.h | 11 ++++---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 5 ++-- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 13 ++++------ 8 files changed, 56 insertions(+), 33 deletions(-) diff --git a/cfg.mk b/cfg.mk index d72b039..07a794a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -250,7 +250,6 @@ useless_free_options = \ # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) # y virNetworkObjFree -# n virNetworkObjListFree FIXME # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 24a5f7c..4efad43 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -73,17 +73,33 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script"); -bool -virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint) +static virClassPtr virNetworkObjListClass; +static void virNetworkObjListDispose(void *obj); + +static int virNetworkObjOnceInit(void) +{ + if (!(virNetworkObjListClass = virClassNew(virClassForObject(), + "virNetworkObjList", + sizeof(virNetworkObjList), + virNetworkObjListDispose))) + return -1; + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virNetworkObj) + +virNetworkObjListPtr virNetworkObjListNew(void) { - unsigned int flag = (1 << taint); + virNetworkObjListPtr nets; + + if (virNetworkObjInitialize() < 0) + return NULL; - if (obj->taint & flag) - return false; + if (!(nets = virObjectNew(virNetworkObjListClass))) + return NULL; - obj->taint |= flag; - return true; + return nets; } virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, @@ -116,6 +132,19 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, return NULL; } +bool +virNetworkObjTaint(virNetworkObjPtr obj, + virNetworkTaintFlags taint) +{ + unsigned int flag = (1 << taint); + + if (obj->taint & flag) + return false; + + obj->taint |= flag; + return true; +} + static void virPortGroupDefClear(virPortGroupDefPtr def) @@ -275,18 +304,16 @@ void virNetworkObjFree(virNetworkObjPtr net) VIR_FREE(net); } -void virNetworkObjListFree(virNetworkObjListPtr nets) +static void +virNetworkObjListDispose(void *obj) { + virNetworkObjListPtr nets = obj; size_t i; - if (!nets) - return; - for (i = 0; i < nets->count; i++) virNetworkObjFree(nets->objs[i]); VIR_FREE(nets->objs); - nets->count = 0; } /* diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 164fb1a..5725258 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -40,6 +40,7 @@ # include "device_conf.h" # include "virbitmap.h" # include "networkcommon_conf.h" +# include "virobject.h" typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -277,6 +278,8 @@ struct _virNetworkObj { typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; struct _virNetworkObjList { + virObject parent; + size_t count; virNetworkObjPtr *objs; }; @@ -298,19 +301,17 @@ virNetworkObjIsActive(const virNetworkObj *net) return net->active; } -bool virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint); +virNetworkObjListPtr virNetworkObjListNew(void); virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name); - +bool virNetworkObjTaint(virNetworkObjPtr obj, + virNetworkTaintFlags taint); void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); -void virNetworkObjListFree(virNetworkObjListPtr nets); - typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8aacc30..a9ce2cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -571,8 +571,8 @@ virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; virNetworkObjListForEach; -virNetworkObjListFree; virNetworkObjListGetNames; +virNetworkObjListNew; virNetworkObjListNumOfNetworks; virNetworkObjLock; virNetworkObjReplacePersistentDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 1c73342..0bde89d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -624,7 +624,7 @@ networkStateInitialize(bool privileged, /* if this fails now, it will be retried later with dnsmasqCapsRefresh() */ driver->dnsmasqCaps = dnsmasqCapsNewFromBinary(DNSMASQ); - if (VIR_ALLOC(driver->networks) < 0) + if (!(driver->networks = virNetworkObjListNew())) goto error; if (virNetworkLoadAllState(driver->networks, @@ -752,8 +752,7 @@ networkStateCleanup(void) virObjectEventStateFree(driver->networkEventState); /* free inactive networks */ - virNetworkObjListFree(driver->networks); - VIR_FREE(driver->networks); + virObjectUnref(driver->networks); VIR_FREE(driver->networkConfigDir); VIR_FREE(driver->networkAutostartDir); diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 32f2ede..d8bcb4b 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -208,7 +208,7 @@ parallelsOpenDefault(virConnectPtr conn) goto error; if (!(privconn->domains = virDomainObjListNew()) || - VIR_ALLOC(privconn->networks) < 0) + !(privconn->networks = virNetworkObjListNew())) goto error; if (!(privconn->domainEventState = virObjectEventStateNew())) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 868e3f5..e626ff6 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -337,7 +337,7 @@ int parallelsNetworkClose(virConnectPtr conn) { parallelsConnPtr privconn = conn->privateData; parallelsDriverLock(privconn); - virNetworkObjListFree(privconn->networks); + virObjectUnref(privconn->networks); parallelsDriverUnlock(privconn); return 0; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0a68283..4b3aa24 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -725,7 +725,7 @@ testOpenDefault(virConnectPtr conn) goto error; if (!(privconn->domains = virDomainObjListNew()) || - VIR_ALLOC(privconn->networks) < 0) + !(privconn->networks = virNetworkObjListNew())) goto error; memmove(&privconn->nodeInfo, &defaultNodeInfo, sizeof(defaultNodeInfo)); @@ -830,8 +830,7 @@ testOpenDefault(virConnectPtr conn) error: virObjectUnref(privconn->domains); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virNodeDeviceObjListFree(&privconn->devs); @@ -1414,7 +1413,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) conn->privateData = privconn; if (!(privconn->domains = virDomainObjListNew()) || - VIR_ALLOC(privconn->networks) < 0) + !(privconn->networks = virNetworkObjListNew())) goto error; if (!(privconn->caps = testBuildCapabilities(conn))) @@ -1466,8 +1465,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) xmlXPathFreeContext(ctxt); xmlFreeDoc(doc); virObjectUnref(privconn->domains); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); VIR_FREE(privconn->path); @@ -1593,8 +1591,7 @@ static int testConnectClose(virConnectPtr conn) virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); virNodeDeviceObjListFree(&privconn->devs); - virNetworkObjListFree(privconn->networks); - VIR_FREE(privconn->networks); + virObjectUnref(privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); virObjectEventStateFree(privconn->eventState); -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:28 +0100, Michal Privoznik wrote:
Well, one day this will be self-locking object, but not today. But lets prepare the code for that! Moreover, virNetworkObjListFree() is no longer needed, so turn it into virNetworkObjListDispose().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/network_conf.c | 53 +++++++++++++++++++++++++++++---------- src/conf/network_conf.h | 11 ++++---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 5 ++-- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 13 ++++------ 8 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/cfg.mk b/cfg.mk index d72b039..07a794a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -250,7 +250,6 @@ useless_free_options = \ # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) # y virNetworkObjFree -# n virNetworkObjListFree FIXME # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 24a5f7c..4efad43 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -73,17 +73,33 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script");
-bool -virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint)
I don't see a reason to move this function ...
+static virClassPtr virNetworkObjListClass; +static void virNetworkObjListDispose(void *obj); + +static int virNetworkObjOnceInit(void) +{ + if (!(virNetworkObjListClass = virClassNew(virClassForObject(), + "virNetworkObjList", + sizeof(virNetworkObjList), + virNetworkObjListDispose))) + return -1; + return 0; +} + + +VIR_ONCE_GLOBAL_INIT(virNetworkObj) + +virNetworkObjListPtr virNetworkObjListNew(void) { - unsigned int flag = (1 << taint); + virNetworkObjListPtr nets; + + if (virNetworkObjInitialize() < 0) + return NULL;
- if (obj->taint & flag) - return false; + if (!(nets = virObjectNew(virNetworkObjListClass))) + return NULL;
- obj->taint |= flag; - return true; + return nets; }
virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, @@ -116,6 +132,19 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, return NULL; }
+bool +virNetworkObjTaint(virNetworkObjPtr obj, + virNetworkTaintFlags taint)
Here.
+{ + unsigned int flag = (1 << taint); + + if (obj->taint & flag) + return false; + + obj->taint |= flag; + return true; +} +
static void virPortGroupDefClear(virPortGroupDefPtr def) @@ -275,18 +304,16 @@ void virNetworkObjFree(virNetworkObjPtr net) VIR_FREE(net); }
-void virNetworkObjListFree(virNetworkObjListPtr nets) +static void +virNetworkObjListDispose(void *obj) { + virNetworkObjListPtr nets = obj; size_t i;
- if (!nets) - return; - for (i = 0; i < nets->count; i++) virNetworkObjFree(nets->objs[i]);
VIR_FREE(nets->objs); - nets->count = 0; }
/* diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 164fb1a..5725258 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -40,6 +40,7 @@ # include "device_conf.h" # include "virbitmap.h" # include "networkcommon_conf.h" +# include "virobject.h"
typedef enum { VIR_NETWORK_FORWARD_NONE = 0, @@ -277,6 +278,8 @@ struct _virNetworkObj { typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; struct _virNetworkObjList { + virObject parent; + size_t count; virNetworkObjPtr *objs; }; @@ -298,19 +301,17 @@ virNetworkObjIsActive(const virNetworkObj *net) return net->active; }
-bool virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint);
Nor this definition ...
+virNetworkObjListPtr virNetworkObjListNew(void);
virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name); - +bool virNetworkObjTaint(virNetworkObjPtr obj, + virNetworkTaintFlags taint);
Here.
void virNetworkDefFree(virNetworkDefPtr def); void virNetworkObjFree(virNetworkObjPtr net); -void virNetworkObjListFree(virNetworkObjListPtr nets); -
typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkDefPtr def);
ACK if you don't touch virNetworkObjTaint. Peter
On 03.03.2015 11:47, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:28 +0100, Michal Privoznik wrote:
Well, one day this will be self-locking object, but not today. But lets prepare the code for that! Moreover, virNetworkObjListFree() is no longer needed, so turn it into virNetworkObjListDispose().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/network_conf.c | 53 +++++++++++++++++++++++++++++---------- src/conf/network_conf.h | 11 ++++---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 5 ++-- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 13 ++++------ 8 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/cfg.mk b/cfg.mk index d72b039..07a794a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -250,7 +250,6 @@ useless_free_options = \ # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) # y virNetworkObjFree -# n virNetworkObjListFree FIXME # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 24a5f7c..4efad43 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -73,17 +73,33 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script");
-bool -virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint)
I don't see a reason to move this function ...
I just wanted the class initiatiors to be together. I prefer having it that way. Michal
On Tue, Mar 03, 2015 at 13:40:48 +0100, Michal Privoznik wrote:
On 03.03.2015 11:47, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:28 +0100, Michal Privoznik wrote:
Well, one day this will be self-locking object, but not today. But lets prepare the code for that! Moreover, virNetworkObjListFree() is no longer needed, so turn it into virNetworkObjListDispose().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 1 - src/conf/network_conf.c | 53 +++++++++++++++++++++++++++++---------- src/conf/network_conf.h | 11 ++++---- src/libvirt_private.syms | 2 +- src/network/bridge_driver.c | 5 ++-- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_network.c | 2 +- src/test/test_driver.c | 13 ++++------ 8 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/cfg.mk b/cfg.mk index d72b039..07a794a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -250,7 +250,6 @@ useless_free_options = \ # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) # y virNetworkObjFree -# n virNetworkObjListFree FIXME # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 24a5f7c..4efad43 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -73,17 +73,33 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script");
-bool -virNetworkObjTaint(virNetworkObjPtr obj, - virNetworkTaintFlags taint)
I don't see a reason to move this function ...
I just wanted the class initiatiors to be together. I prefer having it that way.
You moved it under the two LookupBy* and there are no class initiator prior to this patch in src/conf/network_conf.c so the statement above doesn't make sense. Anyways, it's not worth bikeshedding about stuff like this.
Michal
ACK with or without touching virNetworkObjTaint
Now that all the code uses accessors, don't expose the structure anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++++ src/conf/network_conf.h | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 4efad43..b3c0ac7 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -51,6 +51,13 @@ /* currently, /sbin/tc implementation allows up to 16 bits for minor class size */ #define CLASS_ID_BITMAP_SIZE (1<<16) +struct _virNetworkObjList { + virObject parent; + + size_t count; + virNetworkObjPtr *objs; +}; + VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route", "bridge", "private", "vepa", "passthrough", "hostdev") diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 5725258..f7ab2cf 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -277,12 +277,6 @@ struct _virNetworkObj { typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; -struct _virNetworkObjList { - virObject parent; - - size_t count; - virNetworkObjPtr *objs; -}; typedef enum { VIR_NETWORK_TAINT_HOOK, /* Hook script was executed over -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:29 +0100, Michal Privoznik wrote:
Now that all the code uses accessors, don't expose the structure anyway.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 7 +++++++ src/conf/network_conf.h | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-)
ACK, Peter
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 126 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++--------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 +++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 125 insertions(+), 125 deletions(-) diff --git a/cfg.mk b/cfg.mk index 07a794a..6885f9e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -160,7 +160,6 @@ useless_free_options = \ --name=virNWFilterRuleDefFree \ --name=virNWFilterRuleInstFree \ --name=virNetworkDefFree \ - --name=virNetworkObjFree \ --name=virNodeDeviceDefFree \ --name=virNodeDeviceObjFree \ --name=virObjectUnref \ @@ -249,7 +248,6 @@ useless_free_options = \ # y virNetworkDefFree # n virNetworkFree (returns int) # n virNetworkFreeName (returns int) -# y virNetworkObjFree # n virNodeDevCapsDefFree FIXME # y virNodeDeviceDefFree # n virNodeDeviceFree (returns int) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b3c0ac7..8f140d2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -80,11 +80,19 @@ VIR_ENUM_IMPL(virNetworkForwardDriverName, VIR_ENUM_IMPL(virNetworkTaint, VIR_NETWORK_TAINT_LAST, "hook-script"); +static virClassPtr virNetworkObjClass; static virClassPtr virNetworkObjListClass; +static void virNetworkObjDispose(void *obj); static void virNetworkObjListDispose(void *obj); static int virNetworkObjOnceInit(void) { + if (!(virNetworkObjClass = virClassNew(virClassForObjectLockable(), + "virNetworkObj", + sizeof(virNetworkObj), + virNetworkObjDispose))) + return -1; + if (!(virNetworkObjListClass = virClassNew(virClassForObject(), "virNetworkObjList", sizeof(virNetworkObjList), @@ -96,6 +104,32 @@ static int virNetworkObjOnceInit(void) VIR_ONCE_GLOBAL_INIT(virNetworkObj) +virNetworkObjPtr +virNetworkObjNew(void) +{ + virNetworkObjPtr net; + + if (virNetworkObjInitialize() < 0) + return NULL; + + if (!(net = virObjectLockableNew(virNetworkObjClass))) + return NULL; + + if (!(net->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) + goto error; + + /* The first three class IDs are already taken */ + ignore_value(virBitmapSetBit(net->class_id, 0)); + ignore_value(virBitmapSetBit(net->class_id, 1)); + ignore_value(virBitmapSetBit(net->class_id, 2)); + + return net; + + error: + virObjectUnref(net); + return NULL; +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -115,10 +149,10 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, size_t i; for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) return nets->objs[i]; - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } return NULL; @@ -130,10 +164,10 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, size_t i; for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) return nets->objs[i]; - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } return NULL; @@ -297,18 +331,14 @@ virNetworkDefFree(virNetworkDefPtr def) VIR_FREE(def); } -void virNetworkObjFree(virNetworkObjPtr net) +static void +virNetworkObjDispose(void *obj) { - if (!net) - return; + virNetworkObjPtr net = obj; virNetworkDefFree(net->def); virNetworkDefFree(net->newDef); virBitmapFree(net->class_id); - - virMutexDestroy(&net->lock); - - VIR_FREE(net); } static void @@ -318,7 +348,7 @@ virNetworkObjListDispose(void *obj) size_t i; for (i = 0; i < nets->count; i++) - virNetworkObjFree(nets->objs[i]); + virObjectUnref(nets->objs[i]); VIR_FREE(nets->objs); } @@ -409,32 +439,20 @@ virNetworkAssignDef(virNetworkObjListPtr nets, return network; } - if (VIR_ALLOC(network) < 0) + if (!(network = virNetworkObjNew())) return NULL; - if (virMutexInit(&network->lock) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot initialize mutex")); - VIR_FREE(network); - return NULL; - } - virNetworkObjLock(network); + virObjectLock(network); - if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0 || - !(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) + if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0) goto error; - /* The first three class IDs are already taken */ - ignore_value(virBitmapSetBit(network->class_id, 0)); - ignore_value(virBitmapSetBit(network->class_id, 1)); - ignore_value(virBitmapSetBit(network->class_id, 2)); - network->def = def; network->persistent = !live; return network; error: - virNetworkObjUnlock(network); - virNetworkObjFree(network); + virObjectUnlock(network); + virObjectUnref(network); return NULL; } @@ -602,17 +620,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i; - virNetworkObjUnlock(net); + virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); + virObjectUnlock(nets->objs[i]); + virObjectUnref(nets->objs[i]); VIR_DELETE_ELEMENT(nets->objs, i, nets->count); break; } - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } } @@ -3014,7 +3032,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, continue; if ((net = virNetworkLoadState(nets, stateDir, entry->d_name))) - virNetworkObjUnlock(net); + virObjectUnlock(net); } closedir(dir); @@ -3055,7 +3073,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, autostartDir, entry->d_name); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); } closedir(dir); @@ -3110,12 +3128,12 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, unsigned int ret = 0; for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i]->def->bridge && STREQ(nets->objs[i]->def->bridge, bridge) && !(skipname && STREQ(nets->objs[i]->def->name, skipname))) ret = 1; - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } return ret; @@ -4223,21 +4241,11 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, cleanup: if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } -void virNetworkObjLock(virNetworkObjPtr obj) -{ - virMutexLock(&obj->lock); -} - -void virNetworkObjUnlock(virNetworkObjPtr obj) -{ - virMutexUnlock(&obj->lock); -} - #define MATCH(FLAG) (flags & (FLAG)) static bool virNetworkMatch(virNetworkObjPtr netobj, @@ -4289,21 +4297,21 @@ virNetworkObjListExport(virConnectPtr conn, for (i = 0; i < netobjs->count; i++) { virNetworkObjPtr netobj = netobjs->objs[i]; - virNetworkObjLock(netobj); + virObjectLock(netobj); if ((!filter || filter(conn, netobj->def)) && virNetworkMatch(netobj, flags)) { if (nets) { if (!(net = virGetNetwork(conn, netobj->def->name, netobj->def->uuid))) { - virNetworkObjUnlock(netobj); + virObjectUnlock(netobj); goto cleanup; } tmp_nets[nnets] = net; } nnets++; } - virNetworkObjUnlock(netobj); + virObjectUnlock(netobj); } if (tmp_nets) { @@ -4360,21 +4368,21 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, for (i = 0; i < nets->count && got < nnames; i++) { virNetworkObjPtr obj = nets->objs[i]; - virNetworkObjLock(obj); + virObjectLock(obj); if (filter && !filter(conn, obj->def)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); continue; } if ((active && virNetworkObjIsActive(obj)) || (!active && !virNetworkObjIsActive(obj))) { if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); goto error; } got++; } - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } return got; @@ -4396,16 +4404,16 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, for (i = 0; i < nets->count; i++) { virNetworkObjPtr obj = nets->objs[i]; - virNetworkObjLock(obj); + virObjectLock(obj); if (filter && !filter(conn, obj->def)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); continue; } if ((active && virNetworkObjIsActive(obj)) || (!active && !virNetworkObjIsActive(obj))) count++; - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } return count; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index f7ab2cf..7f48b9a 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -258,7 +258,7 @@ struct _virNetworkDef { typedef struct _virNetworkObj virNetworkObj; typedef virNetworkObj *virNetworkObjPtr; struct _virNetworkObj { - virMutex lock; + virObjectLockable parent; pid_t dnsmasqPid; pid_t radvdPid; @@ -275,6 +275,8 @@ struct _virNetworkObj { unsigned int taint; }; +virNetworkObjPtr virNetworkObjNew(void); + typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; @@ -305,7 +307,6 @@ bool virNetworkObjTaint(virNetworkObjPtr obj, virNetworkTaintFlags taint); void virNetworkDefFree(virNetworkDefPtr def); -void virNetworkObjFree(virNetworkObjPtr net); typedef bool (*virNetworkObjListFilter)(virConnectPtr conn, virNetworkDefPtr def); @@ -412,9 +413,6 @@ int virNetworkObjIsDuplicate(virNetworkObjListPtr nets, virNetworkDefPtr def, bool check_active); -void virNetworkObjLock(virNetworkObjPtr obj); -void virNetworkObjUnlock(virNetworkObjPtr obj); - VIR_ENUM_DECL(virNetworkForward) # define VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a9ce2cd..660579c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -566,7 +566,6 @@ virNetworkLoadAllState; virNetworkObjAssignDef; virNetworkObjFindByName; virNetworkObjFindByUUID; -virNetworkObjFree; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; @@ -574,11 +573,10 @@ virNetworkObjListForEach; virNetworkObjListGetNames; virNetworkObjListNew; virNetworkObjListNumOfNetworks; -virNetworkObjLock; +virNetworkObjNew; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; virNetworkObjTaint; -virNetworkObjUnlock; virNetworkObjUnsetDefTransient; virNetworkObjUpdate; virNetworkRemoveInactive; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0bde89d..2d4a6c5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -348,9 +348,9 @@ networkUpdateAllState(virNetworkObjPtr obj, { int ret = -1; - virNetworkObjLock(obj); + virObjectLock(obj); if (!virNetworkObjIsActive(obj)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return 0; } @@ -407,7 +407,7 @@ networkUpdateAllState(virNetworkObjPtr obj, if (!obj->persistent && !obj->active) networkRemoveInactive(obj); else - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -417,7 +417,7 @@ networkAutostartConfigs(virNetworkObjPtr net, { int ret = -1; - virNetworkObjLock(net); + virObjectLock(net); if (net->autostart && !virNetworkObjIsActive(net) && networkStartNetwork(net) < 0) @@ -425,7 +425,7 @@ networkAutostartConfigs(virNetworkObjPtr net, ret = 0; cleanup: - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -1746,7 +1746,7 @@ networkRefreshDaemonsHelper(virNetworkObjPtr net, void *opaque ATTRIBUTE_UNUSED) { - virNetworkObjLock(net); + virObjectLock(net); if (virNetworkObjIsActive(net) && ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || @@ -1760,7 +1760,7 @@ networkRefreshDaemonsHelper(virNetworkObjPtr net, networkRefreshDhcpDaemon(net); networkRefreshRadvd(net); } - virNetworkObjUnlock(net); + virObjectUnlock(net); return 0; } @@ -1781,7 +1781,7 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr net, void *opaque ATTRIBUTE_UNUSED) { - virNetworkObjLock(net); + virObjectLock(net); if (virNetworkObjIsActive(net) && ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || @@ -1794,7 +1794,7 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr net, /* failed to add but already logged */ } } - virNetworkObjUnlock(net); + virObjectUnlock(net); return 0; } @@ -2496,7 +2496,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -2522,7 +2522,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -2670,7 +2670,7 @@ static int networkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -2689,7 +2689,7 @@ static int networkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -2959,7 +2959,7 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3016,7 +3016,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3077,7 +3077,7 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3249,7 +3249,7 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3284,7 +3284,7 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3335,7 +3335,7 @@ static int networkDestroy(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3364,7 +3364,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -3389,7 +3389,7 @@ static char *networkGetBridgeName(virNetworkPtr net) { cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return bridge; } @@ -3410,7 +3410,7 @@ static int networkGetAutostart(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -3478,7 +3478,7 @@ static int networkSetAutostart(virNetworkPtr net, VIR_FREE(configFile); VIR_FREE(autostartLink); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3651,7 +3651,7 @@ networkGetDHCPLeases(virNetworkPtr network, virJSONValueFree(leases_array); if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return rv; @@ -4124,7 +4124,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; error: @@ -4327,7 +4327,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; error: @@ -4477,7 +4477,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; @@ -4581,7 +4581,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index e626ff6..3c879e4 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -230,7 +230,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; net->active = 1; net->autostart = 1; - virNetworkObjUnlock(net); + virObjectUnlock(net); return net; cleanup: @@ -265,7 +265,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } net->active = 1; net->autostart = 1; - virNetworkObjUnlock(net); + virObjectUnlock(net); return net; @@ -433,7 +433,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -457,7 +457,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -484,7 +484,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -505,7 +505,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -526,7 +526,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -551,7 +551,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4b3aa24..3703d00 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -787,7 +787,7 @@ testOpenDefault(virConnectPtr conn) goto error; } netobj->active = 1; - virNetworkObjUnlock(netobj); + virObjectUnlock(netobj); if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML))) goto error; @@ -1155,7 +1155,7 @@ testParseNetworks(testConnPtr privconn, } obj->active = 1; - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } ret = 0; @@ -3508,7 +3508,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, cleanup: if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -3532,7 +3532,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, cleanup: if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -3621,7 +3621,7 @@ static int testNetworkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -3642,7 +3642,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -3675,7 +3675,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); testDriverUnlock(privconn); return ret; } @@ -3708,7 +3708,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); testDriverUnlock(privconn); return ret; } @@ -3746,7 +3746,7 @@ static int testNetworkUndefine(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); testDriverUnlock(privconn); return ret; } @@ -3796,7 +3796,7 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); testDriverUnlock(privconn); return ret; } @@ -3833,7 +3833,7 @@ static int testNetworkCreate(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } @@ -3866,7 +3866,7 @@ static int testNetworkDestroy(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); testDriverUnlock(privconn); return ret; } @@ -3893,7 +3893,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } @@ -3922,7 +3922,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return bridge; } @@ -3947,7 +3947,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } @@ -3972,7 +3972,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, cleanup: if (privnet) - virNetworkObjUnlock(privnet); + virObjectUnlock(privnet); return ret; } diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 280db30..6df161f 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -40,7 +40,7 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr if (!(dev = virNetworkDefParseString(inXmlData))) goto fail; - if (VIR_ALLOC(obj) < 0) + if (!(obj = virNetworkObjNew())) goto fail; obj->def = dev; @@ -66,7 +66,7 @@ testCompareXMLToConfFiles(const char *inxml, const char *outconf, dnsmasqCapsPtr VIR_FREE(actual); VIR_FREE(pidfile); virCommandFree(cmd); - virNetworkObjFree(obj); + virObjectUnref(obj); dnsmasqContextFree(dctx); return ret; } diff --git a/tests/objectlocking.ml b/tests/objectlocking.ml index 2476731..2fa33e4 100644 --- a/tests/objectlocking.ml +++ b/tests/objectlocking.ml @@ -84,7 +84,6 @@ let lockedObjMethods = [ *) let objectLockMethods = [ "virDomainObjLock"; - "virNetworkObjLock"; "virStoragePoolObjLock"; "virNodeDevObjLock" ] @@ -99,7 +98,6 @@ let objectLockMethods = [ *) let objectUnlockMethods = [ "virDomainObjUnlock"; - "virNetworkObjUnlock"; "virStoragePoolObjUnlock"; "virNodeDevObjUnlock" ] -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:30 +0100, Michal Privoznik wrote:
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 126 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++--------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 +++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 125 insertions(+), 125 deletions(-)
...
@@ -602,17 +620,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i;
- virNetworkObjUnlock(net); + virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); + virObjectUnlock(nets->objs[i]); + virObjectUnref(nets->objs[i]);
VIR_DELETE_ELEMENT(nets->objs, i, nets->count); break; } - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } }
The function above is very strange. Your changes should not have any functional impact, but I think we should fix it right away. 1) why do we unlock the object that is going to be deleted ? 2) why do we bother locking the objects in between? We are comparing just the pointer to the object so there's no need to wait for the lock. If we keep the original object locked there's no need to do any of that. 3) the network object is freed _before_ it's removed from the list. Although the list is always locked before access, I don't think that's a good practice. I'd like to see the function fixed either separately or as a follow up. I'd also like to see that patch before. ACK to the rest though, this can be fixed in a individual patch. Peter
On 03.03.2015 12:05, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:30 +0100, Michal Privoznik wrote:
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 126 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++--------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 +++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 125 insertions(+), 125 deletions(-)
...
@@ -602,17 +620,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i;
- virNetworkObjUnlock(net); + virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); + virObjectUnlock(nets->objs[i]); + virObjectUnref(nets->objs[i]);
VIR_DELETE_ELEMENT(nets->objs, i, nets->count); break; } - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } }
The function above is very strange. Your changes should not have any functional impact, but I think we should fix it right away.
1) why do we unlock the object that is going to be deleted ?
To get the order of locking right.
2) why do we bother locking the objects in between? We are comparing just the pointer to the object so there's no need to wait for the lock.
Yep, I've saved that for a separate patch, which is not posted yet though.
If we keep the original object locked there's no need to do any of that.
3) the network object is freed _before_ it's removed from the list. Although the list is always locked before access, I don't think that's a good practice.
Not anymore. Any API that enters the remove function already has at least one reference (obtained via virNetworkObjFind*). So this merely decrease the refcount on the object since the list does no longer hold a reference to the object.
I'd like to see the function fixed either separately or as a follow up. I'd also like to see that patch before.
ACK to the rest though, this can be fixed in a individual patch.
Peter
Michal
On Tue, Mar 03, 2015 at 13:47:32 +0100, Michal Privoznik wrote:
On 03.03.2015 12:05, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:30 +0100, Michal Privoznik wrote:
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 126 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++--------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 +++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 125 insertions(+), 125 deletions(-)
...
@@ -602,17 +620,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i;
- virNetworkObjUnlock(net); + virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); + virObjectUnlock(nets->objs[i]); + virObjectUnref(nets->objs[i]);
VIR_DELETE_ELEMENT(nets->objs, i, nets->count); break; } - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } }
The function above is very strange. Your changes should not have any functional impact, but I think we should fix it right away.
1) why do we unlock the object that is going to be deleted ?
To get the order of locking right.
Well, this would make it only simpler to deal with locking in the loop below since the code always grabs the network driver lock for every operation. And since ...
2) why do we bother locking the objects in between? We are comparing just the pointer to the object so there's no need to wait for the lock.
Yep, I've saved that for a separate patch, which is not posted yet though.
... locking of the object doesn't make sense there shouldn't be any issue.
If we keep the original object locked there's no need to do any of that.
3) the network object is freed _before_ it's removed from the list. Although the list is always locked before access, I don't think that's a good practice.
Not anymore. Any API that enters the remove function already has at least one reference (obtained via virNetworkObjFind*). So this merely decrease the refcount on the object since the list does no longer hold a reference to the object.
That is not true at this point. At this point in the series there's only one reference ever in the list so that the object gets deleted in this function. You add reference counting in patch 28/31.
I'd like to see the function fixed either separately or as a follow up. I'd also like to see that patch before.
ACK to the rest though, this can be fixed in a individual patch.
Peter
Michal
On 03.03.2015 14:10, Peter Krempa wrote:
On Tue, Mar 03, 2015 at 13:47:32 +0100, Michal Privoznik wrote:
On 03.03.2015 12:05, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:30 +0100, Michal Privoznik wrote:
So far it's just a structure which happens to have 'Obj' in its name, but otherwise it not related to virObject at all. No reference counting, not virObjectLock(), nothing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- cfg.mk | 2 - src/conf/network_conf.c | 126 ++++++++++++++++++++------------------ src/conf/network_conf.h | 8 +-- src/libvirt_private.syms | 4 +- src/network/bridge_driver.c | 56 ++++++++--------- src/parallels/parallels_network.c | 16 ++--- src/test/test_driver.c | 32 +++++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 2 - 9 files changed, 125 insertions(+), 125 deletions(-)
...
@@ -602,17 +620,17 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i;
- virNetworkObjUnlock(net); + virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); + virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); + virObjectUnlock(nets->objs[i]); + virObjectUnref(nets->objs[i]);
VIR_DELETE_ELEMENT(nets->objs, i, nets->count); break; } - virNetworkObjUnlock(nets->objs[i]); + virObjectUnlock(nets->objs[i]); } }
The function above is very strange. Your changes should not have any functional impact, but I think we should fix it right away.
1) why do we unlock the object that is going to be deleted ?
To get the order of locking right.
Well, this would make it only simpler to deal with locking in the loop below since the code always grabs the network driver lock for every operation. And since ...
2) why do we bother locking the objects in between? We are comparing just the pointer to the object so there's no need to wait for the lock.
Yep, I've saved that for a separate patch, which is not posted yet though.
... locking of the object doesn't make sense there shouldn't be any issue.
If we keep the original object locked there's no need to do any of that.
3) the network object is freed _before_ it's removed from the list. Although the list is always locked before access, I don't think that's a good practice.
Not anymore. Any API that enters the remove function already has at least one reference (obtained via virNetworkObjFind*). So this merely decrease the refcount on the object since the list does no longer hold a reference to the object.
That is not true at this point. At this point in the series there's only one reference ever in the list so that the object gets deleted in this function. You add reference counting in patch 28/31.
Okay, considered this squashed in: diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8f140d2..3ea8975 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -620,17 +620,13 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, { size_t i; - virObjectUnlock(net); for (i = 0; i < nets->count; i++) { - virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { - virObjectUnlock(nets->objs[i]); - virObjectUnref(nets->objs[i]); - VIR_DELETE_ELEMENT(nets->objs, i, nets->count); + virObjectUnlock(net); + virObjectUnref(net); break; } - virObjectUnlock(nets->objs[i]); } } Michal
Later we can turn APIs to lock the object if needed instead of relying on caller to mutually exclude itself (probably done by locking a big lock anyway). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 8f140d2..7e8b867 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -52,7 +52,7 @@ #define CLASS_ID_BITMAP_SIZE (1<<16) struct _virNetworkObjList { - virObject parent; + virObjectLockable parent; size_t count; virNetworkObjPtr *objs; @@ -93,7 +93,7 @@ static int virNetworkObjOnceInit(void) virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObject(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), "virNetworkObjList", sizeof(virNetworkObjList), virNetworkObjListDispose))) @@ -137,7 +137,7 @@ virNetworkObjListPtr virNetworkObjListNew(void) if (virNetworkObjInitialize() < 0) return NULL; - if (!(nets = virObjectNew(virNetworkObjListClass))) + if (!(nets = virObjectLockableNew(virNetworkObjListClass))) return NULL; return nets; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:31 +0100, Michal Privoznik wrote:
Later we can turn APIs to lock the object if needed instead of relying on caller to mutually exclude itself (probably done by locking a big lock anyway).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
This patch is short enough to be squashed into patch 27 that will actually use it. Peter
This is practically copy of qemuDomObjEndAPI. The reason why is it so widely available is to avoid code duplication, since the function is going to be called from our bridge driver, test driver and parallels driver too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 13 +++++++++++-- src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7e8b867..95d8b4d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -130,6 +130,16 @@ virNetworkObjNew(void) return NULL; } +void +virNetworkObjEndAPI(virNetworkObjPtr *net) +{ + if (!*net) + return; + + virObjectUnlock(*net); + *net = NULL; +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -4240,8 +4250,7 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, } cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 7f48b9a..2cedc0a 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -276,6 +276,7 @@ struct _virNetworkObj { }; virNetworkObjPtr virNetworkObjNew(void); +void virNetworkObjEndAPI(virNetworkObjPtr *net); typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 660579c..fb658fa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -564,6 +564,7 @@ virNetworkIpDefPrefix; virNetworkLoadAllConfigs; virNetworkLoadAllState; virNetworkObjAssignDef; +virNetworkObjEndAPI; virNetworkObjFindByName; virNetworkObjFindByUUID; virNetworkObjGetPersistentDef; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:32 +0100, Michal Privoznik wrote:
This is practically copy of qemuDomObjEndAPI. The reason why is it so widely available is to avoid code duplication, since the function is going to be called from our bridge driver, test driver and parallels driver too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 13 +++++++++++-- src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7e8b867..95d8b4d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -130,6 +130,16 @@ virNetworkObjNew(void) return NULL; }
+void +virNetworkObjEndAPI(virNetworkObjPtr *net) +{ + if (!*net) + return; + + virObjectUnlock(*net); + *net = NULL; +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -4240,8 +4250,7 @@ virNetworkObjIsDuplicate(virNetworkObjListPtr nets, }
cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; }
You should also convert the onle place in virNetworkLoadAllConfigs() to this new helper since it will also return a referenced object. I'll point out the problem later. ACK with virNetworkLoadAllConfigs tweaked too. Peter
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 57 +++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 2d4a6c5..c2a992a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2495,8 +2495,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -2521,8 +2520,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -2669,8 +2667,7 @@ static int networkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -2688,8 +2685,7 @@ static int networkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -2958,8 +2954,7 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkDefFree(def); if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3015,8 +3010,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virObjectEventStateQueue(driver->networkEventState, event); if (freeDef) virNetworkDefFree(def); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3076,8 +3070,7 @@ networkUndefine(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3248,8 +3241,7 @@ networkUpdate(virNetworkPtr net, } ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3283,8 +3275,7 @@ static int networkCreate(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3334,8 +3325,7 @@ static int networkDestroy(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3363,8 +3353,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, ret = virNetworkDefFormat(def, flags); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -3388,8 +3377,7 @@ static char *networkGetBridgeName(virNetworkPtr net) { ignore_value(VIR_STRDUP(bridge, network->def->bridge)); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return bridge; } @@ -3409,8 +3397,7 @@ static int networkGetAutostart(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -3477,8 +3464,7 @@ static int networkSetAutostart(virNetworkPtr net, cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3650,8 +3636,7 @@ networkGetDHCPLeases(virNetworkPtr network, VIR_FREE(custom_lease_file); virJSONValueFree(leases_array); - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return rv; @@ -4123,8 +4108,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; error: @@ -4326,8 +4310,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; error: @@ -4476,8 +4459,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, } ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) { virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; @@ -4580,8 +4562,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:33 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 57 +++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-)
In the qemu driver that is the inspiration for this change we always pair the *EndAPI call with a *ObjFrom* ... in this case networkObjFromNetwork. I'd really appreciate if you could first scrub the code base of direct lookup and use the networkObjFromNetwork helper instead so that we can pair every EndAPI call with that. ACK to this patch though. If you don't fancy cleaning the existing places that manually lookup the object I can do it. Please let me know. Peter
On 03.03.2015 14:40, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:33 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 57 +++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-)
In the qemu driver that is the inspiration for this change we always pair the *EndAPI call with a *ObjFrom* ... in this case networkObjFromNetwork.
I'd really appreciate if you could first scrub the code base of direct lookup and use the networkObjFromNetwork helper instead so that we can pair every EndAPI call with that.
ACK to this patch though. If you don't fancy cleaning the existing places that manually lookup the object I can do it. Please let me know.
I've got such patch ready. However, the problem is lock ordering. Currently, networkObjFromNetwork Locks the network driver lock, looks up the network object and unlocks the network driver. However, some APIs (in fact a lot of them) still rely on network driver locked. And I can't just move the locking in those APIs, right? Because that way I'd try to lock network driver with an object already locked. Therefore I saved the cleanup patch that does exactly what you're requesting here for later. Sorry. Michal
On Thu, Feb 26, 2015 at 15:17:33 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 57 +++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 38 deletions(-)
ACK
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3703d00..a7a3848 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3507,8 +3507,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, net->def->name, net->def->uuid); cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; } @@ -3531,8 +3530,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, net->def->name, net->def->uuid); cleanup: - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); return ret; } @@ -3620,8 +3618,7 @@ static int testNetworkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -3641,8 +3638,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -3674,8 +3670,7 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) virNetworkDefFree(def); if (event) testObjectEventQueue(privconn, event); - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); testDriverUnlock(privconn); return ret; } @@ -3707,8 +3702,7 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) virNetworkDefFree(def); if (event) testObjectEventQueue(privconn, event); - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); testDriverUnlock(privconn); return ret; } @@ -3745,8 +3739,7 @@ static int testNetworkUndefine(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); testDriverUnlock(privconn); return ret; } @@ -3795,8 +3788,7 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); testDriverUnlock(privconn); return ret; } @@ -3832,8 +3824,7 @@ static int testNetworkCreate(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } @@ -3865,8 +3856,7 @@ static int testNetworkDestroy(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); testDriverUnlock(privconn); return ret; } @@ -3892,8 +3882,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, ret = virNetworkDefFormat(privnet->def, flags); cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } @@ -3921,8 +3910,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { ignore_value(VIR_STRDUP(bridge, privnet->def->bridge)); cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return bridge; } @@ -3946,8 +3934,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, ret = 0; cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } @@ -3971,8 +3958,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, ret = 0; cleanup: - if (privnet) - virObjectUnlock(privnet); + virNetworkObjEndAPI(&privnet); return ret; } -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:34 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-)
ACK
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 3c879e4..7618890 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -432,8 +432,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -456,8 +455,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -483,8 +481,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, ret = virNetworkDefFormat(network->def, flags); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -504,8 +501,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -525,8 +521,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -550,8 +545,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:35 +0100, Michal Privoznik wrote:
So far, this is pure code replacement. But once we introduce reference counting to virNetworkObj this will be more handy as there'll be only one function to change: virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
ACK, I see that you will fix the two remaining places in parallelsLoadNetwork() and parallelsAddRoutedNetwork() in one of the next patches. Peter
Every API that touches internal structure of the object must lock the object first. Not every API that has the object as an argument needs to do that though. Some APIs just pass the object to lower layers, which, however, must lock the object then. Look at the code, you'll get my meaning soon. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 95d8b4d..5b0f36f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -156,31 +156,41 @@ virNetworkObjListPtr virNetworkObjListNew(void) virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret = NULL; size_t i; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nets->objs[i]; + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); - return NULL; + return ret; } virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { + virNetworkObjPtr ret = NULL; size_t i; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; + if (STREQ(nets->objs[i]->def->name, name)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); - return NULL; + return ret; } bool @@ -453,14 +463,17 @@ virNetworkAssignDef(virNetworkObjListPtr nets, return NULL; virObjectLock(network); + virObjectLock(nets); if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0) goto error; network->def = def; network->persistent = !live; + virObjectUnlock(nets); return network; error: + virObjectUnlock(nets); virObjectUnlock(network); virObjectUnref(network); return NULL; @@ -631,6 +644,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, size_t i; virObjectUnlock(net); + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { @@ -642,6 +656,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); } /* return ips[index], or NULL if there aren't enough ips */ @@ -3137,6 +3152,7 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, size_t i; unsigned int ret = 0; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); if (nets->objs[i]->def->bridge && @@ -3145,6 +3161,7 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, ret = 1; virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); return ret; } @@ -4304,6 +4321,7 @@ virNetworkObjListExport(virConnectPtr conn, if (nets && VIR_ALLOC_N(tmp_nets, netobjs->count + 1) < 0) goto cleanup; + virObjectLock(netobjs); for (i = 0; i < netobjs->count; i++) { virNetworkObjPtr netobj = netobjs->objs[i]; virObjectLock(netobj); @@ -4314,6 +4332,7 @@ virNetworkObjListExport(virConnectPtr conn, netobj->def->name, netobj->def->uuid))) { virObjectUnlock(netobj); + virObjectUnlock(netobjs); goto cleanup; } tmp_nets[nnets] = net; @@ -4322,6 +4341,7 @@ virNetworkObjListExport(virConnectPtr conn, } virObjectUnlock(netobj); } + virObjectUnlock(netobjs); if (tmp_nets) { /* trim the array to the final size */ @@ -4353,6 +4373,7 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, /* Deliberately don't use for-loop here. We want to allow * @callback to remove a network object, in which case * nets->count is decremented. */ + virObjectLock(nets); while (i < nets->count) { oldcount = nets->count; if (callback(nets->objs[i], opaque) < 0) @@ -4360,6 +4381,7 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, if (oldcount == nets->count) i++; } + virObjectUnlock(nets); return ret; } @@ -4375,6 +4397,7 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, int got = 0; size_t i; + virObjectLock(nets); for (i = 0; i < nets->count && got < nnames; i++) { virNetworkObjPtr obj = nets->objs[i]; virObjectLock(obj); @@ -4387,12 +4410,14 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, (!active && !virNetworkObjIsActive(obj))) { if (VIR_STRDUP(names[got], obj->def->name) < 0) { virObjectUnlock(obj); + virObjectUnlock(nets); goto error; } got++; } virObjectUnlock(obj); } + virObjectUnlock(nets); return got; @@ -4411,6 +4436,7 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, int count = 0; size_t i; + virObjectLock(nets); for (i = 0; i < nets->count; i++) { virNetworkObjPtr obj = nets->objs[i]; virObjectLock(obj); @@ -4424,6 +4450,7 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, count++; virObjectUnlock(obj); } + virObjectUnlock(nets); return count; } -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:36 +0100, Michal Privoznik wrote:
Every API that touches internal structure of the object must lock the object first. Not every API that has the object as an argument needs to do that though. Some APIs just pass the object to lower layers, which, however, must lock the object then. Look
Too, many, commas.
at the code, you'll get my meaning soon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
@@ -631,6 +644,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, size_t i;
virObjectUnlock(net); + virObjectLock(nets);
Hmm, okay. My comment to the previous patch is invalid as this would actually introduce a lock ordering problem.
for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); if (nets->objs[i] == net) { @@ -642,6 +656,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets); }
/* return ips[index], or NULL if there aren't enough ips */
ACK
On Thu, Feb 26, 2015 at 15:17:36 +0100, Michal Privoznik wrote:
Every API that touches internal structure of the object must lock the object first. Not every API that has the object as an argument needs to do that though. Some APIs just pass the object to lower layers, which, however, must lock the object then. Look at the code, you'll get my meaning soon.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 95d8b4d..5b0f36f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -156,31 +156,41 @@ virNetworkObjListPtr virNetworkObjListNew(void) virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { + virNetworkObjPtr ret = NULL; size_t i;
+ virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nets->objs[i]; + if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets);
- return NULL; + return ret; }
virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { + virNetworkObjPtr ret = NULL; size_t i;
+ virObjectLock(nets); for (i = 0; i < nets->count; i++) { virObjectLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; + if (STREQ(nets->objs[i]->def->name, name)) { + ret = nets->objs[i]; + break; + } virObjectUnlock(nets->objs[i]); } + virObjectUnlock(nets);
- return NULL; + return ret; }
bool
Later review showed that making the two functions above self locking won't be ideal. Peter
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 11 +++++++---- src/parallels/parallels_network.c | 9 +++++---- src/test/test_driver.c | 4 ++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5b0f36f..c620201 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return; virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; } @@ -164,6 +165,7 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -184,6 +186,7 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -469,13 +472,13 @@ virNetworkAssignDef(virNetworkObjListPtr nets, network->def = def; network->persistent = !live; + virObjectRef(network); virObjectUnlock(nets); return network; error: virObjectUnlock(nets); - virObjectUnlock(network); - virObjectUnref(network); + virNetworkObjEndAPI(&network); return NULL; } @@ -3057,7 +3060,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, continue; if ((net = virNetworkLoadState(nets, stateDir, entry->d_name))) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); } closedir(dir); @@ -3098,7 +3101,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, autostartDir, entry->d_name); if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); } closedir(dir); diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 7618890..da29060 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -230,7 +230,6 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) goto cleanup; net->active = 1; net->autostart = 1; - virObjectUnlock(net); return net; cleanup: @@ -265,7 +264,6 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) } net->active = 1; net->autostart = 1; - virObjectUnlock(net); return net; @@ -277,7 +275,7 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn) static int parallelsLoadNetworks(parallelsConnPtr privconn) { virJSONValuePtr jobj, jobj2; - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; int ret = -1; int count; size_t i; @@ -305,16 +303,19 @@ static int parallelsLoadNetworks(parallelsConnPtr privconn) net = parallelsLoadNetwork(privconn, jobj2); if (!net) goto cleanup; + else + virNetworkObjEndAPI(&net); } - if (!parallelsAddRoutedNetwork(privconn)) + if (!(net = parallelsAddRoutedNetwork(privconn))) goto cleanup; ret = 0; cleanup: virJSONValueFree(jobj); + virNetworkObjEndAPI(&net); return ret; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a7a3848..bfa0e91 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -787,7 +787,7 @@ testOpenDefault(virConnectPtr conn) goto error; } netobj->active = 1; - virObjectUnlock(netobj); + virNetworkObjEndAPI(&netobj); if (!(interfacedef = virInterfaceDefParseString(defaultInterfaceXML))) goto error; @@ -1155,7 +1155,7 @@ testParseNetworks(testConnPtr privconn, } obj->active = 1; - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); } ret = 0; -- 2.0.5
On Thu, Feb 26, 2015 at 15:17:37 +0100, Michal Privoznik wrote:
This patch turns both virNetworkObjFindByUUID() and virNetworkObjFindByName() to return an referenced object so that even if caller unlocks it, it's for sure that object won't disappear meanwhile. Especially if the object (in general) is locked and unlocked during the caller run. Moreover, this commit is nicely small, since the object unrefing can be done in virNetworkObjEndAPI().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 11 +++++++---- src/parallels/parallels_network.c | 9 +++++---- src/test/test_driver.c | 4 ++-- 3 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 5b0f36f..c620201 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -137,6 +137,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return;
virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; }
@@ -164,6 +165,7 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]); @@ -184,6 +186,7 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, virObjectLock(nets->objs[i]); if (STREQ(nets->objs[i]->def->name, name)) { ret = nets->objs[i]; + virObjectRef(ret); break; } virObjectUnlock(nets->objs[i]);
The two hunks above probably won't be a good idea in the light of the review for 29/31. Peter
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 45 +++------------------------------------------ 1 file changed, 3 insertions(+), 42 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c2a992a..56debb3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -129,9 +129,7 @@ networkObjFromNetwork(virNetworkPtr net) virNetworkObjPtr network; char uuidstr[VIR_UUID_STRING_BUFLEN]; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); - networkDriverUnlock(); if (!network) { virUUIDFormat(net->uuid, uuidstr); @@ -264,6 +262,8 @@ networkRemoveInactive(virNetworkObjPtr net) int ret = -1; + networkDriverLock(); + /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) { @@ -315,6 +315,7 @@ networkRemoveInactive(virNetworkObjPtr net) VIR_FREE(radvdpidbase); VIR_FREE(statusfile); dnsmasqContextFree(dctx); + networkDriverUnlock(); return ret; } @@ -701,11 +702,9 @@ networkStateAutoStart(void) if (!driver) return; - networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfigs, NULL); - networkDriverUnlock(); } /** @@ -2479,9 +2478,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, uuid); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching uuid '%s'"), @@ -2505,9 +2502,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), name); @@ -2531,12 +2526,10 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); nactive = virNetworkObjListNumOfNetworks(driver->networks, true, virConnectNumOfNetworksCheckACL, conn); - networkDriverUnlock(); return nactive; } @@ -2547,12 +2540,10 @@ static int networkConnectListNetworks(virConnectPtr conn, char **const names, in if (virConnectListNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); got = virNetworkObjListGetNames(driver->networks, true, names, nnames, virConnectListNetworksCheckACL, conn); - networkDriverUnlock(); return got; } @@ -2564,12 +2555,10 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); ninactive = virNetworkObjListNumOfNetworks(driver->networks, false, virConnectNumOfDefinedNetworksCheckACL, conn); - networkDriverUnlock(); return ninactive; } @@ -2580,12 +2569,10 @@ static int networkConnectListDefinedNetworks(virConnectPtr conn, char **const na if (virConnectListDefinedNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); got = virNetworkObjListGetNames(driver->networks, false, names, nnames, virConnectListDefinedNetworksCheckACL, conn); - networkDriverUnlock(); return got; } @@ -2601,11 +2588,9 @@ networkConnectListAllNetworks(virConnectPtr conn, if (virConnectListAllNetworksEnsureACL(conn) < 0) goto cleanup; - networkDriverLock(); ret = virNetworkObjListExport(conn, driver->networks, nets, virConnectListAllNetworksCheckACL, flags); - networkDriverUnlock(); cleanup: return ret; @@ -2916,8 +2901,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup; @@ -2955,7 +2938,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -2967,8 +2949,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup; @@ -3011,7 +2991,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3023,8 +3002,6 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL; - networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3071,7 +3048,6 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3095,8 +3071,6 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3242,7 +3216,6 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3252,7 +3225,6 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3276,7 +3248,6 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3286,7 +3257,6 @@ static int networkDestroy(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3326,7 +3296,6 @@ static int networkDestroy(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3736,9 +3705,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, virDomainActualNetDefFree(iface->data.network.actual); iface->data.network.actual = NULL; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, iface->data.network.name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), @@ -4144,9 +4111,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) return 0; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, iface->data.network.name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), @@ -4343,9 +4308,7 @@ networkReleaseActualDevice(virDomainDefPtr dom, if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) return 0; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, iface->data.network.name); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), @@ -4501,9 +4464,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) char *dev_name = NULL; *netaddr = NULL; - networkDriverLock(); network = virNetworkObjFindByName(driver->networks, netname); - networkDriverUnlock(); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), -- 2.0.5
On 26.02.2015 15:17, Michal Privoznik wrote:
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 45 +++------------------------------------------ 1 file changed, 3 insertions(+), 42 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c2a992a..56debb3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -129,9 +129,7 @@ networkObjFromNetwork(virNetworkPtr net) virNetworkObjPtr network; char uuidstr[VIR_UUID_STRING_BUFLEN];
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); - networkDriverUnlock();
if (!network) { virUUIDFormat(net->uuid, uuidstr); @@ -264,6 +262,8 @@ networkRemoveInactive(virNetworkObjPtr net)
int ret = -1;
+ networkDriverLock(); + /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, driver->dnsmasqStateDir))) { @@ -315,6 +315,7 @@ networkRemoveInactive(virNetworkObjPtr net) VIR_FREE(radvdpidbase); VIR_FREE(statusfile); dnsmasqContextFree(dctx); + networkDriverUnlock(); return ret; }
This is not gonna fly alone. Consider this squashed in: diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4df18f2..7d3132a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -262,7 +262,10 @@ networkRemoveInactive(virNetworkObjPtr net) int ret = -1; + virObjectRef(net); + virObjectUnlock(net); networkDriverLock(); + virObjectLock(net); /* remove the (possibly) existing dnsmasq and radvd files */ if (!(dctx = dnsmasqContextNew(def->name, @@ -316,6 +319,7 @@ networkRemoveInactive(virNetworkObjPtr net) VIR_FREE(statusfile); dnsmasqContextFree(dctx); networkDriverUnlock(); + virObjectUnref(net); return ret; } The problem is, if there's already another thread doing undefine, it already has driver and the network object locked. And as soon as it calls virNetworkRemoveInactive() (not to be seen in the context though) a deadlock occurs, because virNetworkRemoveInactive() locks the network list and objects in the list, one after another. So, one thread already have locked driver and the network object list and is trying to lock a network object in the list. However, the other thread has the object locked and is waiting on the network driver lock. Therefore the diff, which does the locking in correct order. Michal
On Thu, Feb 26, 2015 at 15:17:38 +0100, Michal Privoznik wrote:
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 45 +++------------------------------------------ 1 file changed, 3 insertions(+), 42 deletions(-)
This patch allows the following race: Threads 1 and 2 want to define a network with same name. Both threads call into: virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, bool live) { virNetworkObjPtr network; // both threads are here now if ((network = virNetworkObjFindByName(nets, def->name))) { // they both serialize on the virNetworkObjFindByName, but neither of // those finds the object before the other one manages to add it virNetworkObjAssignDef(network, def, live); return network; } if (!(network = virNetworkObjNew())) // now both threads allocate the object ... return NULL; virObjectLock(network); virObjectLock(nets); // and serialize here again, but both threads think now that they // shall add the network to the list ... if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0) .. and do so. goto error; virNetworkAssignDef() will need more love. I think that virNetworkObjFindByName and friends should not lock or ref the network object and the callers such as virNetworkAssignDef() or networkObjFromNetwork() should do it. In that way, you'll be able to lock the list before and TEST_AND_SET the object in an atomic fashion. Rest of this patch looks okay. Peter
On 03.03.2015 16:26, Peter Krempa wrote:
On Thu, Feb 26, 2015 at 15:17:38 +0100, Michal Privoznik wrote:
Now that we have fine grained locks, there's no need to lock the whole driver. We can rely on self-locking APIs.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 45 +++------------------------------------------ 1 file changed, 3 insertions(+), 42 deletions(-)
This patch allows the following race:
Threads 1 and 2 want to define a network with same name. Both threads call into:
virNetworkObjPtr virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkDefPtr def, bool live) { virNetworkObjPtr network;
// both threads are here now if ((network = virNetworkObjFindByName(nets, def->name))) { // they both serialize on the virNetworkObjFindByName, but neither of // those finds the object before the other one manages to add it virNetworkObjAssignDef(network, def, live); return network; }
if (!(network = virNetworkObjNew())) // now both threads allocate the object ...
return NULL; virObjectLock(network);
virObjectLock(nets); // and serialize here again, but both threads think now that they // shall add the network to the list ... if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0)
.. and do so. goto error;
virNetworkAssignDef() will need more love. I think that virNetworkObjFindByName and friends should not lock or ref the network object and the callers such as virNetworkAssignDef() or networkObjFromNetwork() should do it.
In that way, you'll be able to lock the list before and TEST_AND_SET the object in an atomic fashion.
Rest of this patch looks okay.
D'oh! You're right. This has slipped my mind. So, I'm pushing first 12 patches (yep, some of them were not explicitly ACKed, but 01/31 is trivial enough to be fixed without v2 :-P), and will respin the rest. Thanks for the review! Michal
Well, if 'everywhere' is defined as that part of the driver code that serves virNetwork* APIs. Again, we lower layers already have their locks, so there's no point doing big lock. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/test/test_driver.c | 56 +------------------------------------------------- 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index bfa0e91..de138c6 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3495,10 +3495,7 @@ static virNetworkPtr testNetworkLookupByUUID(virConnectPtr conn, virNetworkObjPtr net; virNetworkPtr ret = NULL; - testDriverLock(privconn); net = virNetworkObjFindByUUID(privconn->networks, uuid); - testDriverUnlock(privconn); - if (net == NULL) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3518,10 +3515,7 @@ static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, virNetworkObjPtr net; virNetworkPtr ret = NULL; - testDriverLock(privconn); net = virNetworkObjFindByName(privconn->networks, name); - testDriverUnlock(privconn); - if (net == NULL) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3540,11 +3534,8 @@ static int testConnectNumOfNetworks(virConnectPtr conn) testConnPtr privconn = conn->privateData; int numActive; - testDriverLock(privconn); numActive = virNetworkObjListNumOfNetworks(privconn->networks, true, NULL, conn); - testDriverUnlock(privconn); - return numActive; } @@ -3552,11 +3543,8 @@ static int testConnectListNetworks(virConnectPtr conn, char **const names, int n testConnPtr privconn = conn->privateData; int n; - testDriverLock(privconn); n = virNetworkObjListGetNames(privconn->networks, true, names, nnames, NULL, conn); - testDriverUnlock(privconn); - return n; } @@ -3565,11 +3553,8 @@ static int testConnectNumOfDefinedNetworks(virConnectPtr conn) testConnPtr privconn = conn->privateData; int numInactive; - testDriverLock(privconn); numInactive = virNetworkObjListNumOfNetworks(privconn->networks, false, NULL, conn); - testDriverUnlock(privconn); - return numInactive; } @@ -3577,11 +3562,8 @@ static int testConnectListDefinedNetworks(virConnectPtr conn, char **const names testConnPtr privconn = conn->privateData; int n; - testDriverLock(privconn); n = virNetworkObjListGetNames(privconn->networks, false, names, nnames, NULL, conn); - testDriverUnlock(privconn); - return n; } @@ -3591,15 +3573,10 @@ testConnectListAllNetworks(virConnectPtr conn, unsigned int flags) { testConnPtr privconn = conn->privateData; - int ret = -1; virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); - testDriverLock(privconn); - ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); - testDriverUnlock(privconn); - - return ret; + return virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); } static int testNetworkIsActive(virNetworkPtr net) @@ -3608,9 +3585,7 @@ static int testNetworkIsActive(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - testDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3628,9 +3603,7 @@ static int testNetworkIsPersistent(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - testDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - testDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -3651,7 +3624,6 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - testDriverLock(privconn); if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; @@ -3671,7 +3643,6 @@ static virNetworkPtr testNetworkCreateXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&net); - testDriverUnlock(privconn); return ret; } @@ -3684,7 +3655,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - testDriverLock(privconn); if ((def = virNetworkDefParseString(xml)) == NULL) goto cleanup; @@ -3703,7 +3673,6 @@ virNetworkPtr testNetworkDefineXML(virConnectPtr conn, const char *xml) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&net); - testDriverUnlock(privconn); return ret; } @@ -3714,7 +3683,6 @@ static int testNetworkUndefine(virNetworkPtr network) int ret = -1; virObjectEventPtr event = NULL; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); if (privnet == NULL) { @@ -3739,7 +3707,6 @@ static int testNetworkUndefine(virNetworkPtr network) cleanup: if (event) testObjectEventQueue(privconn, event); - virNetworkObjEndAPI(&privnet); testDriverUnlock(privconn); return ret; } @@ -3760,8 +3727,6 @@ testNetworkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - testDriverLock(privconn); - network = virNetworkObjFindByUUID(privconn->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3789,7 +3754,6 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - testDriverUnlock(privconn); return ret; } @@ -3800,10 +3764,7 @@ static int testNetworkCreate(virNetworkPtr network) int ret = -1; virObjectEventPtr event = NULL; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3835,9 +3796,7 @@ static int testNetworkDestroy(virNetworkPtr network) int ret = -1; virObjectEventPtr event = NULL; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3857,7 +3816,6 @@ static int testNetworkDestroy(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); - testDriverUnlock(privconn); return ret; } @@ -3870,10 +3828,7 @@ static char *testNetworkGetXMLDesc(virNetworkPtr network, virCheckFlags(0, NULL); - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3891,10 +3846,7 @@ static char *testNetworkGetBridgeName(virNetworkPtr network) { char *bridge = NULL; virNetworkObjPtr privnet; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3921,10 +3873,7 @@ static int testNetworkGetAutostart(virNetworkPtr network, virNetworkObjPtr privnet; int ret = -1; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; @@ -3945,10 +3894,7 @@ static int testNetworkSetAutostart(virNetworkPtr network, virNetworkObjPtr privnet; int ret = -1; - testDriverLock(privconn); privnet = virNetworkObjFindByName(privconn->networks, network->name); - testDriverUnlock(privconn); - if (privnet == NULL) { virReportError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto cleanup; -- 2.0.5
While in previous commits there were some places that relied on the big lock, in this file there's no such place and the big driver lock can be dropped completely. Yay! Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index da29060..d7bd818 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -337,9 +337,7 @@ parallelsNetworkOpen(virConnectPtr conn, int parallelsNetworkClose(virConnectPtr conn) { parallelsConnPtr privconn = conn->privateData; - parallelsDriverLock(privconn); virObjectUnref(privconn->networks); - parallelsDriverUnlock(privconn); return 0; } @@ -348,11 +346,8 @@ static int parallelsConnectNumOfNetworks(virConnectPtr conn) int nactive; parallelsConnPtr privconn = conn->privateData; - parallelsDriverLock(privconn); nactive = virNetworkObjListNumOfNetworks(privconn->networks, true, NULL, conn); - parallelsDriverUnlock(privconn); - return nactive; } @@ -363,11 +358,8 @@ static int parallelsConnectListNetworks(virConnectPtr conn, parallelsConnPtr privconn = conn->privateData; int got; - parallelsDriverLock(privconn); got = virNetworkObjListGetNames(privconn->networks, true, names, nnames, NULL, conn); - parallelsDriverUnlock(privconn); - return got; } @@ -376,11 +368,8 @@ static int parallelsConnectNumOfDefinedNetworks(virConnectPtr conn) int ninactive; parallelsConnPtr privconn = conn->privateData; - parallelsDriverLock(privconn); ninactive = virNetworkObjListNumOfNetworks(privconn->networks, false, NULL, conn); - parallelsDriverUnlock(privconn); - return ninactive; } @@ -391,10 +380,8 @@ static int parallelsConnectListDefinedNetworks(virConnectPtr conn, parallelsConnPtr privconn = conn->privateData; int got; - parallelsDriverLock(privconn); got = virNetworkObjListGetNames(privconn->networks, false, names, nnames, NULL, conn); - parallelsDriverUnlock(privconn); return got; } @@ -403,15 +390,10 @@ static int parallelsConnectListAllNetworks(virConnectPtr conn, unsigned int flags) { parallelsConnPtr privconn = conn->privateData; - int ret = -1; virCheckFlags(VIR_CONNECT_LIST_NETWORKS_FILTERS_ALL, -1); - parallelsDriverLock(privconn); - ret = virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); - parallelsDriverUnlock(privconn); - - return ret; + return virNetworkObjListExport(conn, privconn->networks, nets, NULL, flags); } static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, @@ -421,9 +403,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - parallelsDriverLock(privconn); network = virNetworkObjFindByUUID(privconn->networks, uuid); - parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -444,9 +424,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - parallelsDriverLock(privconn); network = virNetworkObjFindByName(privconn->networks, name); - parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, _("no network with matching name '%s'"), name); @@ -469,10 +447,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, virCheckFlags(VIR_NETWORK_XML_INACTIVE, NULL); - parallelsDriverLock(privconn); network = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); - if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); @@ -492,9 +467,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - parallelsDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -512,9 +485,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) virNetworkObjPtr obj; int ret = -1; - parallelsDriverLock(privconn); obj = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); if (!obj) { virReportError(VIR_ERR_NO_NETWORK, NULL); goto cleanup; @@ -533,9 +504,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, virNetworkObjPtr network; int ret = -1; - parallelsDriverLock(privconn); network = virNetworkObjFindByUUID(privconn->networks, net->uuid); - parallelsDriverUnlock(privconn); if (!network) { virReportError(VIR_ERR_NO_NETWORK, "%s", _("no network with matching uuid")); -- 2.0.5
On 26.02.2015 15:17, Michal Privoznik wrote:
<snip/>
BTW: I've done some testing locally and it helped me to find a bug in my patches (I've just proposed the diff to be squashed in). Basically, my test program defines 10 dummy networks, and spawns a thread over each one of them. The thread does 1000 iterations of some network API calls, e.g. define, undefine, start, destroy, IsActive, IsPersistent, and so on. And the results are astonishing: Without my patches: $ time ./test_network_lock qemu+tcp://localhost/system Connected. Starting workers ... Waiting for workers to end ... real 0m54.083s user 0m1.317s sys 0m1.302s With my patches: $ time ./test_network_lock qemu+tcp://localhost/system Connected. Starting workers ... Waiting for workers to end ... real 0m42.355s user 0m1.231s sys 0m1.324s So nearly 12 seconds speed up! The test program can be found at [1]. Michal 1: http://fpaste.org/191340/
On Fri, Feb 27, 2015 at 02:37:00PM +0100, Michal Privoznik wrote:
On 26.02.2015 15:17, Michal Privoznik wrote:
<snip/>
BTW: I've done some testing locally and it helped me to find a bug in my patches (I've just proposed the diff to be squashed in). Basically, my test program defines 10 dummy networks, and spawns a thread over each one of them. The thread does 1000 iterations of some network API calls, e.g. define, undefine, start, destroy, IsActive, IsPersistent, and so on. And the results are astonishing:
Without my patches:
$ time ./test_network_lock qemu+tcp://localhost/system Connected. Starting workers ... Waiting for workers to end ...
real 0m54.083s user 0m1.317s sys 0m1.302s
With my patches:
$ time ./test_network_lock qemu+tcp://localhost/system Connected. Starting workers ... Waiting for workers to end ...
real 0m42.355s user 0m1.231s sys 0m1.324s
So nearly 12 seconds speed up! The test program can be found at [1].
Michal
Since paste bin's expire, adding the test program content below: -------------------------------------------------------------------- #include <stdio.h> #include <stdlib.h> #include <pthread.h> #include <libvirt/libvirt.h> #include <libvirt/virterror.h> #define NETS 10 #define ITER 1000 static void * workerfunc(void *opaque) { virNetworkPtr *netptr = opaque; virNetworkPtr net = *netptr; virConnectPtr conn = virNetworkGetConnect(net); size_t i; char *xml = NULL; char *bridgename = NULL; for (i = 0; i < ITER; i++) { int isActive = virNetworkIsActive(net); int isPersistent = virNetworkIsPersistent(net); int autostart; if (virNetworkGetAutostart(net, &autostart) < 0) { fprintf(stderr, "Unable to get autostart\n"); goto cleanup; } if (!(xml = virNetworkGetXMLDesc(net, 0))) { fprintf(stderr, "Unable to get net XML\n"); goto cleanup; } if (!(bridgename = virNetworkGetBridgeName(net))) { fprintf(stderr, "Unable to get net bridge name\n"); goto cleanup; } if (virNetworkCreate(net) < 0) { fprintf(stderr, "Unable to start network\n"); goto cleanup; } if (virNetworkDestroy(net) < 0) { fprintf(stderr, "Unable to destroy network\n"); goto cleanup; } if (virNetworkUndefine(net) < 0) { fprintf(stderr, "Unable to undefine network\n"); goto cleanup; } if (!(net = virNetworkDefineXML(conn, xml))) { fprintf(stderr, "Unable to define network back\n"); goto cleanup; } free(xml); free(bridgename); xml = bridgename = NULL; } cleanup: free(xml); free(bridgename); *netptr = net; return NULL; } static int defineNets(virConnectPtr conn, virNetworkPtr **nets, int *nnets) { int ret = -1; char *defxml = NULL; const char *xml = \ "<network>" \ " <name>testnet%d</name>" \ " <forward mode='bridge'/>" \ " <bridge name='br%d'/>" \ "</network>"; for (*nnets = 0; *nnets < NETS; (*nnets)++) { virNetworkPtr net; if (asprintf(&defxml, xml, *nnets, *nnets) < 0) { fprintf(stderr, "Unable to create network XML\n"); goto cleanup; } if (!(*nets = realloc(*nets, (*nnets + 1) * sizeof((*nets)[0])))) { fprintf(stderr, "Unable to allocate memory\n"); goto cleanup; } if (!(net = virNetworkDefineXML(conn, defxml))) { fprintf(stderr, "Unable to define network\n"); goto cleanup; } (*nets)[*nnets] = net; } ret = 0; cleanup: free(defxml); if (ret < 0) { while (*nnets) { (*nnets)--; virNetworkUndefine((*nets)[*nnets]); virNetworkFree((*nets)[*nnets]); } } return ret; } int main(int argc, char **argv) { int ret = -1; virConnectPtr conn = NULL; virNetworkPtr *nets = NULL; int nnets = 0; pthread_t workers[NETS]; size_t nworkers = 0; if (virInitialize() < 0) { fprintf(stderr, "Failed to initialize libvirt.\n"); return -1; } conn = virConnectOpenAuth(argc > 1 ? argv[1] : NULL, virConnectAuthPtrDefault, 0); if (!conn) { fprintf(stderr, "error opening\n"); return -1; } if (defineNets(conn, &nets, &nnets) < 0) goto cleanup; printf("Connected. Starting workers ...\n"); for (nworkers = 0; nworkers < nnets; nworkers++) { if (pthread_create(&workers[nworkers], NULL, workerfunc, &nets[nworkers]) < 0) goto cleanup; } printf("Waiting for workers to end ...\n"); while (nworkers) pthread_join(workers[--nworkers], NULL); ret = 0; cleanup: while (nnets) { nnets--; virNetworkDestroy(nets[nnets]); virNetworkUndefine(nets[nnets]); virNetworkFree(nets[nnets]); } free(nets); while (nworkers) pthread_join(workers[--nworkers], NULL); virConnectClose(conn); return ret; } -------------------------------------------------------------------- -- /kashyap
participants (4)
-
Kashyap Chamarthy -
Maxim Nestratov -
Michal Privoznik -
Peter Krempa