[libvirt] [PATCH v3 00/15] Drop network driver lock
Well, this is interesting. I've turned the virNetworkObjList into using a virHash. And even though the performance in my testing (using the very same test program as in previous versions of the patch set) increased. Without these patches, the test program took 56s to finish. With these patches (that is with virHash table) it takes 46s, which is better. But previously, with network objects stored in flat array I was able to get somewhere around 23s! I wonder what may be the cause. NB, nearly all patches in the series have been ACKed already, but since I'm switching the virNetworkObjList internals, I'm posting them again. Michal Privoznik (15): virNetworkObjListPtr: Turn list into a hash table network_conf: Make virNetworkObj actually virObject network_conf: Introduce virNetworkObjEndAPI bridge_driver: Use virNetworkObjEndAPI test_driver: Use virNetworkObjEndAPI parallels_network: Use virNetworkObjEndAPI virNetworkObjList: Derive from virObjectLockableClass network_conf: Introduce locked versions of lookup functions virNetworkObjListPtr: Make APIs self-locking virNetworkObjFindBy*: Return an reference to found object struct _virNetworkDriverState: Annotate items bridge_driver: Drop networkDriverLock() from everywhere test_driver: Drop testDriverLock() from almost everywhere parallels_network: Drop parallelsDriverLock() from everywhere. bridge_driver: Use more of networkObjFromNetwork cfg.mk | 2 - src/conf/network_conf.c | 545 +++++++++++++++++++++++------------ src/conf/network_conf.h | 13 +- src/libvirt_private.syms | 7 +- src/network/bridge_driver.c | 197 +++---------- src/network/bridge_driver_platform.h | 5 +- src/parallels/parallels_network.c | 62 +--- src/test/test_driver.c | 108 ++----- tests/networkxml2conftest.c | 4 +- tests/objectlocking.ml | 4 - 10 files changed, 441 insertions(+), 506 deletions(-) -- 2.0.5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 382 ++++++++++++++++++++++++++++++------------------ 1 file changed, 237 insertions(+), 145 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9c1d578..ae29c47 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -54,8 +54,7 @@ struct _virNetworkObjList { virObject parent; - size_t count; - virNetworkObjPtr *objs; + virHashTablePtr objs; }; VIR_ENUM_IMPL(virNetworkForward, @@ -96,6 +95,13 @@ static int virNetworkObjOnceInit(void) VIR_ONCE_GLOBAL_INIT(virNetworkObj) +static void +virNetworkObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) +{ + virNetworkObjPtr obj = payload; + virNetworkObjFree(obj); +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -106,37 +112,52 @@ virNetworkObjListPtr virNetworkObjListNew(void) if (!(nets = virObjectNew(virNetworkObjListClass))) return NULL; + if (!(nets->objs = virHashCreate(50, virNetworkObjListDataFree))) { + virObjectUnref(nets); + return NULL; + } + return nets; } virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid) { - size_t i; + virNetworkObjPtr ret = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; - for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); - if (!memcmp(nets->objs[i]->def->uuid, uuid, VIR_UUID_BUFLEN)) - return nets->objs[i]; - virNetworkObjUnlock(nets->objs[i]); - } + virUUIDFormat(uuid, uuidstr); - return NULL; + ret = virHashLookup(nets->objs, uuidstr); + if (ret) + virNetworkObjLock(ret); + return ret; +} + +static int +virNetworkObjSearchName(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *data) +{ + virNetworkObjPtr net = (virNetworkObjPtr) payload; + int want = 0; + + virNetworkObjLock(net); + if (STREQ(net->def->name, (const char *)data)) + want = 1; + virNetworkObjUnlock(net); + return want; } virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name) { - size_t i; + virNetworkObjPtr ret = NULL; - for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); - if (STREQ(nets->objs[i]->def->name, name)) - return nets->objs[i]; - virNetworkObjUnlock(nets->objs[i]); - } - - return NULL; + ret = virHashSearch(nets->objs, virNetworkObjSearchName, name); + if (ret) + virNetworkObjLock(ret); + return ret; } bool @@ -315,12 +336,8 @@ static void virNetworkObjListDispose(void *obj) { virNetworkObjListPtr nets = obj; - size_t i; - for (i = 0; i < nets->count; i++) - virNetworkObjFree(nets->objs[i]); - - VIR_FREE(nets->objs); + virHashFree(nets->objs); } /* @@ -403,6 +420,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, bool live) { virNetworkObjPtr network; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if ((network = virNetworkObjFindByName(nets, def->name))) { virNetworkObjAssignDef(network, def, live); @@ -419,8 +437,11 @@ virNetworkAssignDef(virNetworkObjListPtr nets, } virNetworkObjLock(network); - if (VIR_APPEND_ELEMENT_COPY(nets->objs, nets->count, network) < 0 || - !(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) + if (!(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) + goto error; + + virUUIDFormat(def->uuid, uuidstr); + if (virHashAddEntry(nets->objs, uuidstr, network) < 0) goto error; /* The first three class IDs are already taken */ @@ -600,20 +621,11 @@ virNetworkConfigChangeSetup(virNetworkObjPtr network, unsigned int flags) void virNetworkRemoveInactive(virNetworkObjListPtr nets, virNetworkObjPtr net) { - size_t i; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(net->def->uuid, uuidstr); virNetworkObjUnlock(net); - for (i = 0; i < nets->count; i++) { - virNetworkObjLock(nets->objs[i]); - if (nets->objs[i] == net) { - virNetworkObjUnlock(nets->objs[i]); - virNetworkObjFree(nets->objs[i]); - - VIR_DELETE_ELEMENT(nets->objs, i, nets->count); - break; - } - virNetworkObjUnlock(nets->objs[i]); - } + virHashRemoveEntry(nets->objs, uuidstr); } /* return ips[index], or NULL if there aren't enough ips */ @@ -3102,23 +3114,39 @@ char *virNetworkConfigFile(const char *dir, return ret; } +struct virNetworkBridgeInUseHelperData { + const char *bridge; + const char *skipname; +}; + +static int +virNetworkBridgeInUseHelper(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + int ret = 0; + virNetworkObjPtr net = (virNetworkObjPtr) payload; + const struct virNetworkBridgeInUseHelperData *data = opaque; + + virNetworkObjLock(net); + if (net->def->bridge && + STREQ(net->def->bridge, data->bridge) && + !(data->skipname && STREQ(net->def->name, data->skipname))) + ret = 1; + virNetworkObjUnlock(net); + return ret; +} + int virNetworkBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) { - size_t i; - unsigned int ret = 0; + virNetworkObjPtr obj; + struct virNetworkBridgeInUseHelperData data = {bridge, skipname}; - for (i = 0; i < nets->count; i++) { - virNetworkObjLock(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]); - } + obj = virHashSearch(nets->objs, virNetworkBridgeInUseHelper, &data); - return ret; + return obj != NULL; } char *virNetworkAllocateBridge(virNetworkObjListPtr nets, @@ -4271,6 +4299,52 @@ virNetworkMatch(virNetworkObjPtr netobj, } #undef MATCH +struct virNetworkObjListData { + virConnectPtr conn; + virNetworkPtr *nets; + virNetworkObjListFilter filter; + unsigned int flags; + int nnets; + bool error; +}; + +static void +virNetworkObjListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNetworkObjListData *data = opaque; + virNetworkObjPtr obj = payload; + virNetworkPtr net = NULL; + + if (data->error) + return; + + virNetworkObjLock(obj); + + if (data->filter && + !data->filter(data->conn, obj->def)) + goto cleanup; + + if (!virNetworkMatch(obj, data->flags)) + goto cleanup; + + if (!data->nets) { + data->nnets++; + goto cleanup; + } + + if (!(net = virGetNetwork(data->conn, obj->def->name, obj->def->uuid))) { + data->error = true; + goto cleanup; + } + + data->nets[data->nnets++] = net; + + cleanup: + virNetworkObjUnlock(obj); +} + int virNetworkObjListExport(virConnectPtr conn, virNetworkObjListPtr netobjs, @@ -4278,53 +4352,50 @@ virNetworkObjListExport(virConnectPtr conn, virNetworkObjListFilter filter, unsigned int flags) { - virNetworkPtr *tmp_nets = NULL; - virNetworkPtr net = NULL; - int nnets = 0; int ret = -1; - size_t i; + struct virNetworkObjListData data = { conn, NULL, filter, flags, 0, false}; - if (nets && VIR_ALLOC_N(tmp_nets, netobjs->count + 1) < 0) + if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0) goto cleanup; - for (i = 0; i < netobjs->count; i++) { - virNetworkObjPtr netobj = netobjs->objs[i]; - virNetworkObjLock(netobj); - if ((!filter || filter(conn, netobj->def)) && - virNetworkMatch(netobj, flags)) { - if (nets) { - if (!(net = virGetNetwork(conn, - netobj->def->name, - netobj->def->uuid))) { - virNetworkObjUnlock(netobj); - goto cleanup; - } - tmp_nets[nnets] = net; - } - nnets++; - } - virNetworkObjUnlock(netobj); - } + virHashForEach(netobjs->objs, virNetworkObjListPopulate, &data); - if (tmp_nets) { + if (data.error) + goto cleanup; + + if (data.nets) { /* trim the array to the final size */ - ignore_value(VIR_REALLOC_N(tmp_nets, nnets + 1)); - *nets = tmp_nets; - tmp_nets = NULL; + ignore_value(VIR_REALLOC_N(data.nets, data.nnets + 1)); + *nets = data.nets; + data.nets = NULL; } - ret = nnets; - + ret = data.nnets; cleanup: - if (tmp_nets) { - for (i = 0; i < nnets; i++) - virObjectUnref(tmp_nets[i]); - } + while (data.nets && data.nnets) + virObjectUnref(data.nets[--data.nnets]); - VIR_FREE(tmp_nets); + VIR_FREE(data.nets); return ret; } +struct virNetworkObjListForEachHelperData { + virNetworkObjListIterator callback; + void *opaque; + int ret; +}; + +static void +virNetworkObjListForEachHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNetworkObjListForEachHelperData *data = opaque; + + if (data->callback(payload, data->opaque) < 0) + data->ret = -1; +} + /** * virNetworkObjListForEach: * @nets: a list of network objects @@ -4341,15 +4412,54 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, virNetworkObjListIterator callback, void *opaque) { - int ret = 0; - size_t i = 0; + struct virNetworkObjListForEachHelperData data = {callback, opaque, 0}; + virHashForEach(nets->objs, virNetworkObjListForEachHelper, &data); + return data.ret; +} - for (i = 0; i < nets->count; i++) { - if (callback(nets->objs[i], opaque) < 0) - ret = -1; +struct virNetworkObjListGetHelperData { + virConnectPtr conn; + virNetworkObjListFilter filter; + char **names; + int nnames; + bool active; + int got; + bool error; +}; + +static void +virNetworkObjListGetHelper(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNetworkObjListGetHelperData *data = opaque; + virNetworkObjPtr obj = payload; + + if (data->error) + return; + + if (data->nnames >= 0 && + data->got == data->nnames) + return; + + virNetworkObjLock(obj); + + if (data->filter && + !data->filter(data->conn, obj->def)) + goto cleanup; + + if ((data->active && virNetworkObjIsActive(obj)) || + (!data->active && !virNetworkObjIsActive(obj))) { + if (data->names && + VIR_STRDUP(data->names[data->got], obj->def->name) < 0) { + data->error = true; + goto cleanup; + } + data->got++; } - return ret; + cleanup: + virNetworkObjUnlock(obj); } int @@ -4360,34 +4470,23 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn) { - int got = 0; - size_t i; + int ret = -1; - 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; - } + struct virNetworkObjListGetHelperData data = { + conn, filter, names, nnames, active, 0, false}; - if ((active && virNetworkObjIsActive(obj)) || - (!active && !virNetworkObjIsActive(obj))) { - if (VIR_STRDUP(names[got], obj->def->name) < 0) { - virNetworkObjUnlock(obj); - goto error; - } - got++; - } - virNetworkObjUnlock(obj); + virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); + + if (data.error) + goto cleanup; + + ret = data.got; + cleanup: + if (ret < 0) { + while (data.got) + VIR_FREE(data.names[--data.got]); } - - return got; - - error: - for (i = 0; i < got; i++) - VIR_FREE(names[i]); - return -1; + return ret; } int @@ -4396,24 +4495,31 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, virNetworkObjListFilter filter, virConnectPtr conn) { - int count = 0; - size_t i; + struct virNetworkObjListGetHelperData data = { + conn, filter, NULL, -1, active, 0, false}; - for (i = 0; i < nets->count; i++) { - virNetworkObjPtr obj = nets->objs[i]; - virNetworkObjLock(obj); - if (filter && !filter(conn, obj->def)) { - virNetworkObjUnlock(obj); - continue; - } + virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); - if ((active && virNetworkObjIsActive(obj)) || - (!active && !virNetworkObjIsActive(obj))) - count++; - virNetworkObjUnlock(obj); - } + return data.got; +} - return count; +struct virNetworkObjListPruneHelperData { + unsigned int flags; +}; + +static int +virNetworkObjListPruneHelper(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + const struct virNetworkObjListPruneHelperData *data = opaque; + virNetworkObjPtr obj = (virNetworkObjPtr) payload; + int want = 0; + + virNetworkObjLock(obj); + want = virNetworkMatch(obj, data->flags); + virNetworkObjUnlock(obj); + return want; } /** @@ -4428,21 +4534,7 @@ void virNetworkObjListPrune(virNetworkObjListPtr nets, unsigned int flags) { - size_t i = 0; + struct virNetworkObjListPruneHelperData data = {flags}; - while (i < nets->count) { - virNetworkObjPtr obj = nets->objs[i]; - - virNetworkObjLock(obj); - - if (virNetworkMatch(obj, flags)) { - virNetworkObjUnlock(obj); - virNetworkObjFree(obj); - - VIR_DELETE_ELEMENT(nets->objs, i, nets->count); - } else { - virNetworkObjUnlock(obj); - i++; - } - } + virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); } -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:07 +0100, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 382 ++++++++++++++++++++++++++++++------------------ 1 file changed, 237 insertions(+), 145 deletions(-)
...
int virNetworkBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) { - size_t i; - unsigned int ret = 0; + virNetworkObjPtr obj; + struct virNetworkBridgeInUseHelperData data = {bridge, skipname};
- for (i = 0; i < nets->count; i++) { - virNetworkObjLock(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]); - } + obj = virHashSearch(nets->objs, virNetworkBridgeInUseHelper, &data);
- return ret; + return obj != NULL; }
char *virNetworkAllocateBridge(virNetworkObjListPtr nets, @@ -4271,6 +4299,52 @@ virNetworkMatch(virNetworkObjPtr netobj, } #undef MATCH
+struct virNetworkObjListData { + virConnectPtr conn; + virNetworkPtr *nets; + virNetworkObjListFilter filter; + unsigned int flags; + int nnets; + bool error; +}; + +static void +virNetworkObjListPopulate(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + struct virNetworkObjListData *data = opaque; + virNetworkObjPtr obj = payload; + virNetworkPtr net = NULL;
Similarly to the domain objects this will become problematic once you drop the driver lock. I'm planing to fix the domain version soon, so I'll need to update this one too as we don't have any prior art.
+ + if (data->error) + return; + + virNetworkObjLock(obj); + + if (data->filter && + !data->filter(data->conn, obj->def)) + goto cleanup; + + if (!virNetworkMatch(obj, data->flags)) + goto cleanup; + + if (!data->nets) { + data->nnets++; + goto cleanup; + } + + if (!(net = virGetNetwork(data->conn, obj->def->name, obj->def->uuid))) { + data->error = true; + goto cleanup; + } + + data->nets[data->nnets++] = net; + + cleanup: + virNetworkObjUnlock(obj); +} +
...
+static int +virNetworkObjListPruneHelper(const void *payload, + const void *name ATTRIBUTE_UNUSED, + const void *opaque) +{ + const struct virNetworkObjListPruneHelperData *data = opaque; + virNetworkObjPtr obj = (virNetworkObjPtr) payload; + int want = 0; + + virNetworkObjLock(obj); + want = virNetworkMatch(obj, data->flags); + virNetworkObjUnlock(obj); + return want; }
/** @@ -4428,21 +4534,7 @@ void virNetworkObjListPrune(virNetworkObjListPtr nets, unsigned int flags) { - size_t i = 0; + struct virNetworkObjListPruneHelperData data = {flags};
- while (i < nets->count) { - virNetworkObjPtr obj = nets->objs[i]; - - virNetworkObjLock(obj); - - if (virNetworkMatch(obj, flags)) { - virNetworkObjUnlock(obj); - virNetworkObjFree(obj); - - VIR_DELETE_ELEMENT(nets->objs, i, nets->count); - } else { - virNetworkObjUnlock(obj); - i++; - } - } + virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); }
Now that I've persusaded you to add a function like this you converted the code to the hash table that supports deletion. Oh well :) 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 | 110 ++++++++++++++++++++------------------ 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, 116 insertions(+), 118 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 ae29c47..609990c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -79,11 +79,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), @@ -99,7 +107,33 @@ static void virNetworkObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { virNetworkObjPtr obj = payload; - virNetworkObjFree(obj); + virObjectUnref(obj); +} + +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) @@ -130,7 +164,7 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, ret = virHashLookup(nets->objs, uuidstr); if (ret) - virNetworkObjLock(ret); + virObjectLock(ret); return ret; } @@ -142,10 +176,10 @@ virNetworkObjSearchName(const void *payload, virNetworkObjPtr net = (virNetworkObjPtr) payload; int want = 0; - virNetworkObjLock(net); + virObjectLock(net); if (STREQ(net->def->name, (const char *)data)) want = 1; - virNetworkObjUnlock(net); + virObjectUnlock(net); return want; } @@ -156,7 +190,7 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, ret = virHashSearch(nets->objs, virNetworkObjSearchName, name); if (ret) - virNetworkObjLock(ret); + virObjectLock(ret); return ret; } @@ -318,18 +352,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 @@ -427,35 +457,21 @@ 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); - - if (!(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) - goto error; + virObjectLock(network); virUUIDFormat(def->uuid, uuidstr); if (virHashAddEntry(nets->objs, uuidstr, 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; } @@ -624,7 +640,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(net->def->uuid, uuidstr); - virNetworkObjUnlock(net); + virObjectUnlock(net); virHashRemoveEntry(nets->objs, uuidstr); } @@ -3026,7 +3042,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, continue; if ((net = virNetworkLoadState(nets, stateDir, entry->d_name))) - virNetworkObjUnlock(net); + virObjectUnlock(net); } closedir(dir); @@ -3067,7 +3083,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, autostartDir, entry->d_name); if (net) - virNetworkObjUnlock(net); + virObjectUnlock(net); } closedir(dir); @@ -3128,12 +3144,12 @@ virNetworkBridgeInUseHelper(const void *payload, virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; - virNetworkObjLock(net); + virObjectLock(net); if (net->def->bridge && STREQ(net->def->bridge, data->bridge) && !(data->skipname && STREQ(net->def->name, data->skipname))) ret = 1; - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -4251,21 +4267,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, @@ -4320,7 +4326,7 @@ virNetworkObjListPopulate(void *payload, if (data->error) return; - virNetworkObjLock(obj); + virObjectLock(obj); if (data->filter && !data->filter(data->conn, obj->def)) @@ -4342,7 +4348,7 @@ virNetworkObjListPopulate(void *payload, data->nets[data->nnets++] = net; cleanup: - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } int @@ -4442,7 +4448,7 @@ virNetworkObjListGetHelper(void *payload, data->got == data->nnames) return; - virNetworkObjLock(obj); + virObjectLock(obj); if (data->filter && !data->filter(data->conn, obj->def)) @@ -4459,7 +4465,7 @@ virNetworkObjListGetHelper(void *payload, } cleanup: - virNetworkObjUnlock(obj); + virObjectUnlock(obj); } int @@ -4516,9 +4522,9 @@ virNetworkObjListPruneHelper(const void *payload, virNetworkObjPtr obj = (virNetworkObjPtr) payload; int want = 0; - virNetworkObjLock(obj); + virObjectLock(obj); want = virNetworkMatch(obj, data->flags); - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return want; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3575659..1423676 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 e0d5459..5e78cf7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -565,7 +565,6 @@ virNetworkLoadAllState; virNetworkObjAssignDef; virNetworkObjFindByName; virNetworkObjFindByUUID; -virNetworkObjFree; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; @@ -574,11 +573,10 @@ virNetworkObjListGetNames; virNetworkObjListNew; virNetworkObjListNumOfNetworks; virNetworkObjListPrune; -virNetworkObjLock; +virNetworkObjNew; virNetworkObjReplacePersistentDef; virNetworkObjSetDefTransient; virNetworkObjTaint; -virNetworkObjUnlock; virNetworkObjUnsetDefTransient; virNetworkObjUpdate; virNetworkRemoveInactive; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5752acb..70467f6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -348,9 +348,9 @@ networkUpdateState(virNetworkObjPtr obj, { int ret = -1; - virNetworkObjLock(obj); + virObjectLock(obj); if (!virNetworkObjIsActive(obj)) { - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return 0; } @@ -403,7 +403,7 @@ networkUpdateState(virNetworkObjPtr obj, ret = 0; cleanup: - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -413,7 +413,7 @@ networkAutostartConfig(virNetworkObjPtr net, { int ret = -1; - virNetworkObjLock(net); + virObjectLock(net); if (net->autostart && !virNetworkObjIsActive(net) && networkStartNetwork(net) < 0) @@ -421,7 +421,7 @@ networkAutostartConfig(virNetworkObjPtr net, ret = 0; cleanup: - virNetworkObjUnlock(net); + virObjectUnlock(net); return ret; } @@ -1745,7 +1745,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) || @@ -1759,7 +1759,7 @@ networkRefreshDaemonsHelper(virNetworkObjPtr net, networkRefreshDhcpDaemon(net); networkRefreshRadvd(net); } - virNetworkObjUnlock(net); + virObjectUnlock(net); return 0; } @@ -1780,7 +1780,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) || @@ -1793,7 +1793,7 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr net, /* failed to add but already logged */ } } - virNetworkObjUnlock(net); + virObjectUnlock(net); return 0; } @@ -2497,7 +2497,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -2523,7 +2523,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -2671,7 +2671,7 @@ static int networkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -2690,7 +2690,7 @@ static int networkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -2960,7 +2960,7 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3017,7 +3017,7 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3078,7 +3078,7 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3250,7 +3250,7 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3285,7 +3285,7 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3336,7 +3336,7 @@ static int networkDestroy(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3365,7 +3365,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -3390,7 +3390,7 @@ static char *networkGetBridgeName(virNetworkPtr net) { cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return bridge; } @@ -3411,7 +3411,7 @@ static int networkGetAutostart(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -3479,7 +3479,7 @@ static int networkSetAutostart(virNetworkPtr net, VIR_FREE(configFile); VIR_FREE(autostartLink); if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); networkDriverUnlock(); return ret; } @@ -3652,7 +3652,7 @@ networkGetDHCPLeases(virNetworkPtr network, virJSONValueFree(leases_array); if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return rv; @@ -4125,7 +4125,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; error: @@ -4328,7 +4328,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, ret = 0; cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; error: @@ -4478,7 +4478,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; @@ -4582,7 +4582,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 62ed268..86038bf 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; @@ -445,7 +445,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -469,7 +469,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -496,7 +496,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, cleanup: if (network) - virNetworkObjUnlock(network); + virObjectUnlock(network); return ret; } @@ -517,7 +517,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -538,7 +538,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) cleanup: if (obj) - virNetworkObjUnlock(obj); + virObjectUnlock(obj); return ret; } @@ -563,7 +563,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 693b930..20c77de 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 7826148..fd83d6d 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 Tue, Mar 10, 2015 at 17:45:08 +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 | 110 ++++++++++++++++++++------------------ 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, 116 insertions(+), 118 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 ae29c47..609990c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -79,11 +79,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), @@ -99,7 +107,33 @@ static void virNetworkObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
This function can now be replaced with virObjectFreeHashData.
{ virNetworkObjPtr obj = payload; - virNetworkObjFree(obj); + virObjectUnref(obj); +}
Since I've already reviewed this previously and the conversion to virHash is ok here. ACK if you use the existing free helper. 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 | 20 ++++++++++++++------ src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 609990c..f677e3c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -136,6 +136,16 @@ virNetworkObjNew(void) return NULL; } +void +virNetworkObjEndAPI(virNetworkObjPtr *net) +{ + if (!*net) + return; + + virObjectUnlock(*net); + *net = NULL; +} + virNetworkObjListPtr virNetworkObjListNew(void) { virNetworkObjListPtr nets; @@ -3041,8 +3051,8 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, if (!virFileStripSuffix(entry->d_name, ".xml")) continue; - if ((net = virNetworkLoadState(nets, stateDir, entry->d_name))) - virObjectUnlock(net); + net = virNetworkLoadState(nets, stateDir, entry->d_name); + virNetworkObjEndAPI(&net); } closedir(dir); @@ -3082,8 +3092,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, configDir, autostartDir, entry->d_name); - if (net) - virObjectUnlock(net); + virNetworkObjEndAPI(&net); } closedir(dir); @@ -4266,8 +4275,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 1423676..e0ed714 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 5e78cf7..5fbe094 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -563,6 +563,7 @@ virNetworkIpDefPrefix; virNetworkLoadAllConfigs; virNetworkLoadAllState; virNetworkObjAssignDef; +virNetworkObjEndAPI; virNetworkObjFindByName; virNetworkObjFindByUUID; virNetworkObjGetPersistentDef; -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:09 +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 | 20 ++++++++++++++------ src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 16 insertions(+), 6 deletions(-)
ACK from previous version stands, this had no changes. 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 70467f6..97fda3f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2496,8 +2496,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -2522,8 +2521,7 @@ static virNetworkPtr networkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -2670,8 +2668,7 @@ static int networkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -2689,8 +2686,7 @@ static int networkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -2959,8 +2955,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; } @@ -3016,8 +3011,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; } @@ -3077,8 +3071,7 @@ networkUndefine(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3249,8 +3242,7 @@ networkUpdate(virNetworkPtr net, } ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3284,8 +3276,7 @@ static int networkCreate(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3335,8 +3326,7 @@ static int networkDestroy(virNetworkPtr net) cleanup: if (event) virObjectEventStateQueue(driver->networkEventState, event); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3364,8 +3354,7 @@ static char *networkGetXMLDesc(virNetworkPtr net, ret = virNetworkDefFormat(def, flags); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -3389,8 +3378,7 @@ static char *networkGetBridgeName(virNetworkPtr net) { ignore_value(VIR_STRDUP(bridge, network->def->bridge)); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return bridge; } @@ -3410,8 +3398,7 @@ static int networkGetAutostart(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -3478,8 +3465,7 @@ static int networkSetAutostart(virNetworkPtr net, cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); networkDriverUnlock(); return ret; } @@ -3651,8 +3637,7 @@ networkGetDHCPLeases(virNetworkPtr network, VIR_FREE(custom_lease_file); virJSONValueFree(leases_array); - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return rv; @@ -4124,8 +4109,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; error: @@ -4327,8 +4311,7 @@ networkNotifyActualDevice(virDomainDefPtr dom, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; error: @@ -4477,8 +4460,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; @@ -4581,8 +4563,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:10 +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, I don't see any change. 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/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 20c77de..72f40ed 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 Tue, Mar 10, 2015 at 17:45:11 +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, I don't see any change. 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/parallels/parallels_network.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 86038bf..1bcd2d3 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: @@ -241,7 +240,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) static virNetworkObjPtr parallelsAddRoutedNetwork(parallelsConnPtr privconn) { - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; virNetworkDefPtr def; if (VIR_ALLOC(def) < 0) @@ -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; } @@ -444,8 +445,7 @@ static virNetworkPtr parallelsNetworkLookupByUUID(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -468,8 +468,7 @@ static virNetworkPtr parallelsNetworkLookupByName(virConnectPtr conn, ret = virGetNetwork(conn, network->def->name, network->def->uuid); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -495,8 +494,7 @@ static char *parallelsNetworkGetXMLDesc(virNetworkPtr net, ret = virNetworkDefFormat(network->def, flags); cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } @@ -516,8 +514,7 @@ static int parallelsNetworkIsActive(virNetworkPtr net) ret = virNetworkObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -537,8 +534,7 @@ static int parallelsNetworkIsPersistent(virNetworkPtr net) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virNetworkObjEndAPI(&obj); return ret; } @@ -562,8 +558,7 @@ static int parallelsNetworkGetAutostart(virNetworkPtr net, ret = 0; cleanup: - if (network) - virObjectUnlock(network); + virNetworkObjEndAPI(&network); return ret; } -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:12 +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().
Not entirely true ...
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/parallels/parallels_network.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c index 86038bf..1bcd2d3 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);
... this ...
return net;
cleanup: @@ -241,7 +240,7 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj) static virNetworkObjPtr parallelsAddRoutedNetwork(parallelsConnPtr privconn) { - virNetworkObjPtr net; + virNetworkObjPtr net = NULL; virNetworkDefPtr def;
if (VIR_ALLOC(def) < 0) @@ -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);
... is a somewhat semantic change.
}
- if (!parallelsAddRoutedNetwork(privconn)) + if (!(net = parallelsAddRoutedNetwork(privconn))) goto cleanup;
ret = 0;
cleanup: virJSONValueFree(jobj); + virNetworkObjEndAPI(&net); return ret; }
Anyways. ACK. Peter
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 f677e3c..399c372 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; virHashTablePtr objs; }; @@ -92,7 +92,7 @@ static int virNetworkObjOnceInit(void) virNetworkObjDispose))) return -1; - if (!(virNetworkObjListClass = virClassNew(virClassForObject(), + if (!(virNetworkObjListClass = virClassNew(virClassForObjectLockable(), "virNetworkObjList", sizeof(virNetworkObjList), virNetworkObjListDispose))) @@ -153,7 +153,7 @@ virNetworkObjListPtr virNetworkObjListNew(void) if (virNetworkObjInitialize() < 0) return NULL; - if (!(nets = virObjectNew(virNetworkObjListClass))) + if (!(nets = virObjectLockableNew(virNetworkObjListClass))) return NULL; if (!(nets->objs = virHashCreate(50, virNetworkObjListDataFree))) { -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:13 +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(-)
ACK
This is going to be needed later, when some functions already have the virNetworkObjList object already locked and need to lookup a object to work on. As an example of such function is virNetworkAssignDef(). The other use case might be in virNetworkObjListForEach() callback. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 34 ++++++++++++++++++++++++++++++---- src/conf/network_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 399c372..be5cf0e 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -164,8 +164,9 @@ virNetworkObjListPtr virNetworkObjListNew(void) return nets; } -virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, - const unsigned char *uuid) +virNetworkObjPtr +virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, + const unsigned char *uuid) { virNetworkObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -178,6 +179,18 @@ virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, return ret; } +virNetworkObjPtr +virNetworkObjFindByUUID(virNetworkObjListPtr nets, + const unsigned char *uuid) +{ + virNetworkObjPtr ret; + + virObjectLock(nets); + ret = virNetworkObjFindByUUIDLocked(nets, uuid); + virObjectUnlock(nets); + return ret; +} + static int virNetworkObjSearchName(const void *payload, const void *name ATTRIBUTE_UNUSED, @@ -193,8 +206,9 @@ virNetworkObjSearchName(const void *payload, return want; } -virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, - const char *name) +virNetworkObjPtr +virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, + const char *name) { virNetworkObjPtr ret = NULL; @@ -204,6 +218,18 @@ virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, return ret; } +virNetworkObjPtr +virNetworkObjFindByName(virNetworkObjListPtr nets, + const char *name) +{ + virNetworkObjPtr ret; + + virObjectLock(nets); + ret = virNetworkObjFindByNameLocked(nets, name); + virObjectUnlock(nets); + return ret; +} + bool virNetworkObjTaint(virNetworkObjPtr obj, virNetworkTaintFlags taint) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e0ed714..3e926f7 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -300,8 +300,12 @@ virNetworkObjIsActive(const virNetworkObj *net) virNetworkObjListPtr virNetworkObjListNew(void); +virNetworkObjPtr virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, + const unsigned char *uuid); virNetworkObjPtr virNetworkObjFindByUUID(virNetworkObjListPtr nets, const unsigned char *uuid); +virNetworkObjPtr virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, + const char *name); virNetworkObjPtr virNetworkObjFindByName(virNetworkObjListPtr nets, const char *name); bool virNetworkObjTaint(virNetworkObjPtr obj, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5fbe094..64808ce 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -565,7 +565,9 @@ virNetworkLoadAllState; virNetworkObjAssignDef; virNetworkObjEndAPI; virNetworkObjFindByName; +virNetworkObjFindByNameLocked; virNetworkObjFindByUUID; +virNetworkObjFindByUUIDLocked; virNetworkObjGetPersistentDef; virNetworkObjIsDuplicate; virNetworkObjListExport; -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:14 +0100, Michal Privoznik wrote:
This is going to be needed later, when some functions already have the virNetworkObjList object already locked and need to lookup a object to work on. As an example of such function is virNetworkAssignDef(). The other use case might be in virNetworkObjListForEach() callback.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/network_conf.c | 34 ++++++++++++++++++++++++++++++---- src/conf/network_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 36 insertions(+), 4 deletions(-)
ACK, 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 | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index be5cf0e..98342f9 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -488,13 +488,17 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjPtr network; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if ((network = virNetworkObjFindByName(nets, def->name))) { + virObjectLock(nets); + if ((network = virNetworkObjFindByNameLocked(nets, def->name))) { + virObjectUnlock(nets); virNetworkObjAssignDef(network, def, live); return network; } - if (!(network = virNetworkObjNew())) + if (!(network = virNetworkObjNew())) { + virObjectUnlock(nets); return NULL; + } virObjectLock(network); virUUIDFormat(def->uuid, uuidstr); @@ -503,9 +507,11 @@ virNetworkAssignDef(virNetworkObjListPtr nets, network->def = def; network->persistent = !live; + virObjectUnlock(nets); return network; error: + virObjectUnlock(nets); virObjectUnlock(network); virObjectUnref(network); return NULL; @@ -676,8 +682,14 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(net->def->uuid, uuidstr); + virObjectRef(net); virObjectUnlock(net); + virObjectLock(nets); + virObjectLock(net); virHashRemoveEntry(nets->objs, uuidstr); + virObjectUnlock(net); + virObjectUnlock(nets); + virObjectUnref(net); } /* return ips[index], or NULL if there aren't enough ips */ @@ -3195,7 +3207,9 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, virNetworkObjPtr obj; struct virNetworkBridgeInUseHelperData data = {bridge, skipname}; + virObjectLock(nets); obj = virHashSearch(nets->objs, virNetworkBridgeInUseHelper, &data); + virObjectUnlock(nets); return obj != NULL; } @@ -4395,6 +4409,7 @@ virNetworkObjListExport(virConnectPtr conn, int ret = -1; struct virNetworkObjListData data = { conn, NULL, filter, flags, 0, false}; + virObjectLock(netobjs); if (nets && VIR_ALLOC_N(data.nets, virHashSize(netobjs->objs) + 1) < 0) goto cleanup; @@ -4412,6 +4427,7 @@ virNetworkObjListExport(virConnectPtr conn, ret = data.nnets; cleanup: + virObjectUnlock(netobjs); while (data.nets && data.nnets) virObjectUnref(data.nets[--data.nnets]); @@ -4443,7 +4459,9 @@ virNetworkObjListForEachHelper(void *payload, * @opaque: pointer to pass to the @callback * * Function iterates over the list of network objects and calls - * passed callback over each one of them. + * passed callback over each one of them. You should avoid + * calling those virNetworkObjList APIs, which lock the list + * again in favor of their virNetworkObj*Locked variants. * * Returns: 0 on success, -1 otherwise. */ @@ -4453,7 +4471,9 @@ virNetworkObjListForEach(virNetworkObjListPtr nets, void *opaque) { struct virNetworkObjListForEachHelperData data = {callback, opaque, 0}; + virObjectLock(nets); virHashForEach(nets->objs, virNetworkObjListForEachHelper, &data); + virObjectUnlock(nets); return data.ret; } @@ -4515,7 +4535,9 @@ virNetworkObjListGetNames(virNetworkObjListPtr nets, struct virNetworkObjListGetHelperData data = { conn, filter, names, nnames, active, 0, false}; + virObjectLock(nets); virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); + virObjectUnlock(nets); if (data.error) goto cleanup; @@ -4538,7 +4560,9 @@ virNetworkObjListNumOfNetworks(virNetworkObjListPtr nets, struct virNetworkObjListGetHelperData data = { conn, filter, NULL, -1, active, 0, false}; + virObjectLock(nets); virHashForEach(nets->objs, virNetworkObjListGetHelper, &data); + virObjectUnlock(nets); return data.got; } @@ -4576,5 +4600,7 @@ virNetworkObjListPrune(virNetworkObjListPtr nets, { struct virNetworkObjListPruneHelperData data = {flags}; + virObjectLock(nets); virHashRemoveSet(nets->objs, virNetworkObjListPruneHelper, &data); + virObjectUnlock(nets); } -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:15 +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 | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)
ACK, 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 | 15 ++++++++++----- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 98342f9..a2aa4dc 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -143,6 +143,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return; virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; } @@ -175,7 +176,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets, ret = virHashLookup(nets->objs, uuidstr); if (ret) - virObjectLock(ret); + virObjectRef(ret); return ret; } @@ -188,6 +189,8 @@ virNetworkObjFindByUUID(virNetworkObjListPtr nets, virObjectLock(nets); ret = virNetworkObjFindByUUIDLocked(nets, uuid); virObjectUnlock(nets); + if (ret) + virObjectLock(ret); return ret; } @@ -214,7 +217,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets, ret = virHashSearch(nets->objs, virNetworkObjSearchName, name); if (ret) - virObjectLock(ret); + virObjectRef(ret); return ret; } @@ -227,6 +230,8 @@ virNetworkObjFindByName(virNetworkObjListPtr nets, virObjectLock(nets); ret = virNetworkObjFindByNameLocked(nets, name); virObjectUnlock(nets); + if (ret) + virObjectLock(ret); return ret; } @@ -491,6 +496,7 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virObjectLock(nets); if ((network = virNetworkObjFindByNameLocked(nets, def->name))) { virObjectUnlock(nets); + virObjectLock(network); virNetworkObjAssignDef(network, def, live); return network; } @@ -507,13 +513,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; } @@ -687,7 +693,6 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, virObjectLock(nets); virObjectLock(net); virHashRemoveEntry(nets->objs, uuidstr); - virObjectUnlock(net); virObjectUnlock(nets); virObjectUnref(net); } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 97fda3f..5ef9910 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2939,7 +2939,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (networkStartNetwork(network) < 0) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; } @@ -2988,7 +2987,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (virNetworkSaveConfig(driver->networkConfigDir, def) < 0) { if (!virNetworkObjIsActive(network)) { virNetworkRemoveInactive(driver->networks, network); - network = NULL; goto cleanup; } /* if network was active already, just undo new persistent @@ -3053,11 +3051,8 @@ networkUndefine(virNetworkPtr net) VIR_INFO("Undefining network '%s'", network->def->name); if (!active) { - if (networkRemoveInactive(network) < 0) { - network = NULL; + if (networkRemoveInactive(network) < 0) goto cleanup; - } - network = NULL; } else { /* if the network still exists, it was active, and we need to make @@ -3314,13 +3309,10 @@ static int networkDestroy(virNetworkPtr net) VIR_NETWORK_EVENT_STOPPED, 0); - if (!network->persistent) { - if (networkRemoveInactive(network) < 0) { - network = NULL; - ret = -1; - goto cleanup; - } - network = NULL; + if (!network->persistent && + networkRemoveInactive(network) < 0) { + ret = -1; + goto cleanup; } cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 72f40ed..90af80a 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; @@ -3733,7 +3733,6 @@ static int testNetworkUndefine(virNetworkPtr network) 0); virNetworkRemoveInactive(privconn->networks, privnet); - privnet = NULL; ret = 0; cleanup: @@ -3847,10 +3846,9 @@ static int testNetworkDestroy(virNetworkPtr network) event = virNetworkEventLifecycleNew(privnet->def->name, privnet->def->uuid, VIR_NETWORK_EVENT_STOPPED, 0); - if (!privnet->persistent) { + if (!privnet->persistent) virNetworkRemoveInactive(privconn->networks, privnet); - privnet = NULL; - } + ret = 0; cleanup: -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:16 +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 | 15 ++++++++++----- src/network/bridge_driver.c | 18 +++++------------- src/test/test_driver.c | 10 ++++------ 3 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 98342f9..a2aa4dc 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -143,6 +143,7 @@ virNetworkObjEndAPI(virNetworkObjPtr *net) return;
virObjectUnlock(*net); + virObjectUnref(*net); *net = NULL; }
@@ -175,7 +176,7 @@ virNetworkObjFindByUUIDLocked(virNetworkObjListPtr nets,
ret = virHashLookup(nets->objs, uuidstr); if (ret) - virObjectLock(ret); + virObjectRef(ret); return ret;
Since this ...
}
@@ -188,6 +189,8 @@ virNetworkObjFindByUUID(virNetworkObjListPtr nets, virObjectLock(nets); ret = virNetworkObjFindByUUIDLocked(nets, uuid); virObjectUnlock(nets); + if (ret) + virObjectLock(ret);
and this function now differ not only in the fact that they require the @nets object to be locked or not but in the state of the returned object they deserve a comment telling the difference.
return ret; }
@@ -214,7 +217,7 @@ virNetworkObjFindByNameLocked(virNetworkObjListPtr nets,
ret = virHashSearch(nets->objs, virNetworkObjSearchName, name); if (ret) - virObjectLock(ret); + virObjectRef(ret); return ret; }
@@ -227,6 +230,8 @@ virNetworkObjFindByName(virNetworkObjListPtr nets, virObjectLock(nets); ret = virNetworkObjFindByNameLocked(nets, name); virObjectUnlock(nets); + if (ret) + virObjectLock(ret);
Same here.
return ret; }
ACK, Peter
In order to drop network driver lock, lets annotate which structure items are immutable, which have self-locking APIs and so on. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver_platform.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index b7492e6..d9cf6a8 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -34,8 +34,10 @@ struct _virNetworkDriverState { virMutex lock; + /* Immutable pointer, self-locking APIs */ virNetworkObjListPtr networks; + /* Immutable pointer, Immutable objects */ char *networkConfigDir; char *networkAutostartDir; char *stateDir; @@ -44,6 +46,7 @@ struct _virNetworkDriverState { char *radvdStateDir; dnsmasqCapsPtr dnsmasqCaps; + /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr networkEventState; }; -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:17 +0100, Michal Privoznik wrote:
In order to drop network driver lock, lets annotate which structure items are immutable, which have self-locking APIs and so on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver_platform.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index b7492e6..d9cf6a8 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -34,8 +34,10 @@ struct _virNetworkDriverState { virMutex lock;
+ /* Immutable pointer, self-locking APIs */ virNetworkObjListPtr networks;
+ /* Immutable pointer, Immutable objects */ char *networkConfigDir; char *networkAutostartDir; char *stateDir; @@ -44,6 +46,7 @@ struct _virNetworkDriverState { char *radvdStateDir; dnsmasqCapsPtr dnsmasqCaps;
Unfortunately dnsmasqCaps is not immutable. In networkStartDhcpDaemon it's re-allocated or updated when the network is started.
+ /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr networkEventState; };
ACK if you notify that @dnsmasqCaps is not immutable. 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 | 71 ------------------------------------ src/network/bridge_driver_platform.h | 2 - tests/objectlocking.ml | 2 - 3 files changed, 75 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5ef9910..c6957c3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -88,16 +88,6 @@ VIR_LOG_INIT("network.bridge_driver"); static virNetworkDriverStatePtr driver; - -static void networkDriverLock(void) -{ - virMutexLock(&driver->lock); -} -static void networkDriverUnlock(void) -{ - virMutexUnlock(&driver->lock); -} - static int networkStateCleanup(void); static int networkStartNetwork(virNetworkObjPtr network); @@ -129,9 +119,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); @@ -557,12 +545,6 @@ networkStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) goto error; - if (virMutexInit(&driver->lock) < 0) { - VIR_FREE(driver); - goto error; - } - networkDriverLock(); - /* configuration/state paths are one of * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). @@ -648,8 +630,6 @@ networkStateInitialize(bool privileged, driver->networkEventState = virObjectEventStateNew(); - networkDriverUnlock(); - #ifdef HAVE_FIREWALLD if (!(sysbus = virDBusGetSystemBus())) { virErrorPtr err = virGetLastError(); @@ -683,8 +663,6 @@ networkStateInitialize(bool privileged, return ret; error: - if (driver) - networkDriverUnlock(); networkStateCleanup(); goto cleanup; } @@ -700,11 +678,9 @@ networkStateAutoStart(void) if (!driver) return; - networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfig, NULL); - networkDriverUnlock(); } /** @@ -719,7 +695,6 @@ networkStateReload(void) if (!driver) return 0; - networkDriverLock(); virNetworkLoadAllState(driver->networks, driver->stateDir); virNetworkLoadAllConfigs(driver->networks, @@ -730,7 +705,6 @@ networkStateReload(void) virNetworkObjListForEach(driver->networks, networkAutostartConfig, NULL); - networkDriverUnlock(); return 0; } @@ -746,8 +720,6 @@ networkStateCleanup(void) if (!driver) return -1; - networkDriverLock(); - virObjectEventStateFree(driver->networkEventState); /* free inactive networks */ @@ -762,9 +734,6 @@ networkStateCleanup(void) virObjectUnref(driver->dnsmasqCaps); - networkDriverUnlock(); - virMutexDestroy(&driver->lock); - VIR_FREE(driver); return 0; @@ -2478,9 +2447,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, uuid); - networkDriverUnlock(); if (!network) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); @@ -2506,9 +2473,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); @@ -2532,12 +2497,10 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); nactive = virNetworkObjListNumOfNetworks(driver->networks, true, virConnectNumOfNetworksCheckACL, conn); - networkDriverUnlock(); return nactive; } @@ -2548,12 +2511,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; } @@ -2565,12 +2526,10 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1; - networkDriverLock(); ninactive = virNetworkObjListNumOfNetworks(driver->networks, false, virConnectNumOfDefinedNetworksCheckACL, conn); - networkDriverUnlock(); return ninactive; } @@ -2581,12 +2540,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; } @@ -2602,11 +2559,9 @@ networkConnectListAllNetworks(virConnectPtr conn, if (virConnectListAllNetworksEnsureACL(conn) < 0) goto cleanup; - networkDriverLock(); ret = virNetworkObjListExport(conn, driver->networks, nets, virConnectListAllNetworksCheckACL, flags); - networkDriverUnlock(); cleanup: return ret; @@ -2917,8 +2872,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup; @@ -2955,7 +2908,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -2967,8 +2919,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL; - networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup; @@ -3010,7 +2960,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3022,8 +2971,6 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL; - networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3067,7 +3014,6 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3091,8 +3037,6 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3238,7 +3182,6 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3248,7 +3191,6 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3272,7 +3214,6 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3282,7 +3223,6 @@ static int networkDestroy(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3319,7 +3259,6 @@ static int networkDestroy(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3401,7 +3340,6 @@ static int networkSetAutostart(virNetworkPtr net, char *configFile = NULL, *autostartLink = NULL; int ret = -1; - networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { @@ -3458,7 +3396,6 @@ static int networkSetAutostart(virNetworkPtr net, VIR_FREE(configFile); VIR_FREE(autostartLink); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; } @@ -3729,9 +3666,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'"), @@ -4137,9 +4072,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'"), @@ -4336,9 +4269,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'"), @@ -4494,9 +4425,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'"), diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index d9cf6a8..df1b7a8 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -32,8 +32,6 @@ /* Main driver state */ struct _virNetworkDriverState { - virMutex lock; - /* Immutable pointer, self-locking APIs */ virNetworkObjListPtr networks; diff --git a/tests/objectlocking.ml b/tests/objectlocking.ml index fd83d6d..4fdf95b 100644 --- a/tests/objectlocking.ml +++ b/tests/objectlocking.ml @@ -125,7 +125,6 @@ let driverLockMethods = [ "lxcDriverLock"; "umlDriverLock"; "nodedevDriverLock"; - "networkDriverLock"; "storageDriverLock"; "oneDriverLock" ] @@ -140,7 +139,6 @@ let driverUnlockMethods = [ "lxcDriverUnlock"; "umlDriverUnlock"; "nodedevDriverUnlock"; - "networkDriverUnlock"; "storageDriverUnlock"; "oneDriverUnlock" ] -- 2.0.5
On Tue, Mar 10, 2015 at 17:45:18 +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 | 71 ------------------------------------ src/network/bridge_driver_platform.h | 2 - tests/objectlocking.ml | 2 - 3 files changed, 75 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 5ef9910..c6957c3 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -88,16 +88,6 @@ VIR_LOG_INIT("network.bridge_driver");
static virNetworkDriverStatePtr driver;
- -static void networkDriverLock(void) -{ - virMutexLock(&driver->lock); -} -static void networkDriverUnlock(void) -{ - virMutexUnlock(&driver->lock); -} - static int networkStateCleanup(void);
static int networkStartNetwork(virNetworkObjPtr network); @@ -129,9 +119,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); @@ -557,12 +545,6 @@ networkStateInitialize(bool privileged, if (VIR_ALLOC(driver) < 0) goto error;
- if (virMutexInit(&driver->lock) < 0) { - VIR_FREE(driver); - goto error; - } - networkDriverLock(); - /* configuration/state paths are one of * ~/.config/libvirt/... (session/unprivileged) * /etc/libvirt/... && /var/(run|lib)/libvirt/... (system/privileged). @@ -648,8 +630,6 @@ networkStateInitialize(bool privileged,
driver->networkEventState = virObjectEventStateNew();
- networkDriverUnlock(); - #ifdef HAVE_FIREWALLD if (!(sysbus = virDBusGetSystemBus())) { virErrorPtr err = virGetLastError(); @@ -683,8 +663,6 @@ networkStateInitialize(bool privileged, return ret;
error: - if (driver) - networkDriverUnlock(); networkStateCleanup(); goto cleanup; } @@ -700,11 +678,9 @@ networkStateAutoStart(void) if (!driver) return;
- networkDriverLock(); virNetworkObjListForEach(driver->networks, networkAutostartConfig,
Although it's not obvious as the @driver isn't passed explicitly this internally calls networkStartDhcpDaemon which accesses @driver->dnsmasqStateDir which isn't immutable. Having the access to @driver hidden by the access to a global variable is really sneaky. I'd prefer if we'd pass it explicitly in this case.
NULL); - networkDriverUnlock(); }
/** @@ -719,7 +695,6 @@ networkStateReload(void) if (!driver) return 0;
- networkDriverLock(); virNetworkLoadAllState(driver->networks, driver->stateDir); virNetworkLoadAllConfigs(driver->networks, @@ -730,7 +705,6 @@ networkStateReload(void) virNetworkObjListForEach(driver->networks, networkAutostartConfig,
same problem as above
NULL); - networkDriverUnlock(); return 0; }
@@ -746,8 +720,6 @@ networkStateCleanup(void) if (!driver) return -1;
- networkDriverLock(); - virObjectEventStateFree(driver->networkEventState);
/* free inactive networks */ @@ -762,9 +734,6 @@ networkStateCleanup(void)
virObjectUnref(driver->dnsmasqCaps);
- networkDriverUnlock(); - virMutexDestroy(&driver->lock); - VIR_FREE(driver);
return 0; @@ -2478,9 +2447,7 @@ static virNetworkPtr networkLookupByUUID(virConnectPtr conn, virNetworkObjPtr network; virNetworkPtr ret = NULL;
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, uuid); - networkDriverUnlock(); if (!network) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(uuid, uuidstr); @@ -2506,9 +2473,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); @@ -2532,12 +2497,10 @@ static int networkConnectNumOfNetworks(virConnectPtr conn) if (virConnectNumOfNetworksEnsureACL(conn) < 0) return -1;
- networkDriverLock(); nactive = virNetworkObjListNumOfNetworks(driver->networks, true, virConnectNumOfNetworksCheckACL, conn); - networkDriverUnlock();
return nactive; } @@ -2548,12 +2511,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; } @@ -2565,12 +2526,10 @@ static int networkConnectNumOfDefinedNetworks(virConnectPtr conn) if (virConnectNumOfDefinedNetworksEnsureACL(conn) < 0) return -1;
- networkDriverLock(); ninactive = virNetworkObjListNumOfNetworks(driver->networks, false, virConnectNumOfDefinedNetworksCheckACL, conn); - networkDriverUnlock();
return ninactive; } @@ -2581,12 +2540,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; }
@@ -2602,11 +2559,9 @@ networkConnectListAllNetworks(virConnectPtr conn, if (virConnectListAllNetworksEnsureACL(conn) < 0) goto cleanup;
- networkDriverLock(); ret = virNetworkObjListExport(conn, driver->networks, nets, virConnectListAllNetworksCheckACL, flags); - networkDriverUnlock();
cleanup: return ret; @@ -2917,8 +2872,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL;
- networkDriverLock();
This function will have the same problem as it starts the network.
- if (!(def = virNetworkDefParseString(xml))) goto cleanup;
@@ -2955,7 +2908,6 @@ static virNetworkPtr networkCreateXML(virConnectPtr conn, const char *xml) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -2967,8 +2919,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) virNetworkPtr ret = NULL; virObjectEventPtr event = NULL;
- networkDriverLock(); - if (!(def = virNetworkDefParseString(xml))) goto cleanup;
@@ -3010,7 +2960,6 @@ static virNetworkPtr networkDefineXML(virConnectPtr conn, const char *xml) if (freeDef) virNetworkDefFree(def); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -3022,8 +2971,6 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL;
- networkDriverLock(); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3067,7 +3014,6 @@ networkUndefine(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -3091,8 +3037,6 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1);
- networkDriverLock(); -
This API is restarting the DHCP daemon in some cases so it will share the problem above.
network = virNetworkObjFindByUUID(driver->networks, net->uuid); if (!network) { virReportError(VIR_ERR_NO_NETWORK, @@ -3238,7 +3182,6 @@ networkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
@@ -3248,7 +3191,6 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL;
- networkDriverLock(); network = virNetworkObjFindByUUID(driver->networks, net->uuid);
here too
if (!network) { @@ -3272,7 +3214,6 @@ static int networkCreate(virNetworkPtr net) if (event) virObjectEventStateQueue(driver->networkEventState, event); virNetworkObjEndAPI(&network); - networkDriverUnlock(); return ret; }
The driver lock can't be dropped in that case. I think we can employ a similar mechanism as in qemu driver, where data that changes is stored in a reference counted object and the object pointer is replaced under a lock in case it's required to change the data. The rest of the driver lock dropping looks okay to me though. Peter
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 90af80a..187bd3d 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) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); - testDriverUnlock(privconn); return ret; } @@ -3759,8 +3726,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, @@ -3788,7 +3753,6 @@ testNetworkUpdate(virNetworkPtr net, ret = 0; cleanup: virNetworkObjEndAPI(&network); - testDriverUnlock(privconn); return ret; } @@ -3799,10 +3763,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; @@ -3834,9 +3795,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; @@ -3855,7 +3814,6 @@ static int testNetworkDestroy(virNetworkPtr network) if (event) testObjectEventQueue(privconn, event); virNetworkObjEndAPI(&privnet); - testDriverUnlock(privconn); return ret; } @@ -3868,10 +3826,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; @@ -3889,10 +3844,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; @@ -3919,10 +3871,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; @@ -3943,10 +3892,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
On Tue, Mar 10, 2015 at 17:45:19 +0100, Michal Privoznik wrote:
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(-)
ACK, Peter
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 1bcd2d3..1d3b694 100644 --- a/src/parallels/parallels_network.c +++ b/src/parallels/parallels_network.c @@ -349,9 +349,7 @@ int parallelsNetworkClose(virConnectPtr conn) if (!privconn) return 0; - parallelsDriverLock(privconn); virObjectUnref(privconn->networks); - parallelsDriverUnlock(privconn); return 0; } @@ -360,11 +358,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; } @@ -375,11 +370,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; } @@ -388,11 +380,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; } @@ -403,10 +392,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; } @@ -415,15 +402,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, @@ -433,9 +415,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")); @@ -456,9 +436,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); @@ -481,10 +459,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")); @@ -504,9 +479,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; @@ -524,9 +497,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; @@ -545,9 +516,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 Tue, Mar 10, 2015 at 17:45:20 +0100, Michal Privoznik wrote:
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(-)
ACK, Peter
Now that the network driver lock is ash heap of history, we can use more of networkObjFromNetwork(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c6957c3..ec6948b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -120,7 +120,6 @@ networkObjFromNetwork(virNetworkPtr net) char uuidstr[VIR_UUID_STRING_BUFLEN]; network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { virUUIDFormat(net->uuid, uuidstr); virReportError(VIR_ERR_NO_NETWORK, @@ -2971,12 +2970,8 @@ networkUndefine(virNetworkPtr net) bool active = false; virObjectEventPtr event = NULL; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkUndefineEnsureACL(net->conn, network->def) < 0) goto cleanup; @@ -3037,12 +3032,8 @@ networkUpdate(virNetworkPtr net, VIR_NETWORK_UPDATE_AFFECT_CONFIG, -1); - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkUpdateEnsureACL(net->conn, network->def, flags) < 0) goto cleanup; @@ -3191,13 +3182,8 @@ static int networkCreate(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkCreateEnsureACL(net->conn, network->def) < 0) goto cleanup; @@ -3223,13 +3209,8 @@ static int networkDestroy(virNetworkPtr net) int ret = -1; virObjectEventPtr event = NULL; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkDestroyEnsureACL(net->conn, network->def) < 0) goto cleanup; @@ -3340,13 +3321,9 @@ static int networkSetAutostart(virNetworkPtr net, char *configFile = NULL, *autostartLink = NULL; int ret = -1; - network = virNetworkObjFindByUUID(driver->networks, net->uuid); - if (!network) { - virReportError(VIR_ERR_NO_NETWORK, - "%s", _("no network with matching uuid")); + if (!(network = networkObjFromNetwork(net))) goto cleanup; - } if (virNetworkSetAutostartEnsureACL(net->conn, network->def) < 0) goto cleanup; -- 2.0.5
participants (2)
-
Michal Privoznik -
Peter Krempa