Re: [libvirt] [PATCH v2 23/24] virNetworkObjUnsetDefTransient: Lock object list if needed

On 06.03.2015 14:31, Peter Krempa wrote: > On Thu, Mar 05, 2015 at 12:05:24 +0100, Michal Privoznik wrote: >> This patch alone does not make much sense, I know. But it >> prepares ground for next patch which when looking up a network in >> the object list will not lock each network separately when >> accessing its definition. Therefore we must have all the places >> changing network definition lock the list. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/conf/network_conf.c | 9 ++++++++- >> src/conf/network_conf.h | 3 ++- >> src/network/bridge_driver.c | 4 ++-- >> 3 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index 3d318ce..007cebb 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -537,12 +537,19 @@ virNetworkObjSetDefTransient(virNetworkObjPtr network, bool live) >> * This *undoes* what virNetworkObjSetDefTransient did. >> */ >> void > > I've looked through the next patch and you are basically trying to make > the name and UUID pointers for domain immutable or at leas write locked > ... > >> -virNetworkObjUnsetDefTransient(virNetworkObjPtr network) >> +virNetworkObjUnsetDefTransient(virNetworkObjListPtr nets, >> + virNetworkObjPtr network) >> { >> if (network->newDef) { >> + virObjectRef(network); >> + virObjectUnlock(network); >> + virObjectLock(nets); >> + virObjectLock(network); >> + virObjectUnref(network); > > But I don't really like pulling in the complexity into this helper. > > >> virNetworkDefFree(network->def); >> network->def = network->newDef; >> network->newDef = NULL; >> + virObjectUnlock(nets); >> } >> } > > While I like the idea, I'd rather see a conversion to R/W locks or > making of the name and UUID pointers immutable than this hack. Well: 1) We don't have an virObjectRWLockable or something similar. I can add it, but that would postpone merging this patchset for yet another version. 2) Nor UUID nor name can be made immutable, as we are storing just a pointers to network objects in the array. Not UUID or name. It's not a hash table like in virDomainObjList* [1]. And when looking up an object, we access each object's definition directly. Therefore all other places changing definition must lock the object list.

I agree, that one day we can change this to RW locks, but then again - that'd require more rework which can be saved for a follow up series. Moreover, if I introduce new RWLockable object, other drivers might benefit from it too.

Michal