On Fri, Mar 06, 2015 at 14:42:34 +0100, Michal Privoznik wrote:
> 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@xxxxxxxxxx>
> >> ---
> >> 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.
Since we don't support changing the name nor UUID of a network object
(or any other libvirt externaly visible object),
you could split them out from being part of the definition and use them
in the object itself where you could mark them as immutable.
>
> 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.
They definitely might benefit from them, but refactoring to a rwlock
will be pain as you need to make sure that you mark the parts correctly.
Also saving some work to avoid future refactors creates technical debt.
I'll rather sacrifice a bit of performance in this case than to
introduce a hack.
>
> Michal
>
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list