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

Michal Privoznik mprivozn at redhat.com
Fri Mar 6 14:05:29 UTC 2015


On 06.03.2015 14:47, Peter Krempa wrote:
> 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 at redhat.com>
>>>> ---
>>>>  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.

This would be even uglier hack than what I'm introducing IMO.

> 
>>
>> 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.

Well, it's not a hack rather than something we need to do. Even our
domain code is wrong in this respect. I mean:

virDomainObjListFindByName() accesses domain's definitions directly. We
are just lucky, that this function and some portion of qemuProcessStop()
hasn't been scheduled to run at the same time. That portion which
switches @def and @newDef pointers in the @vm object. We ought to lock
the domain object list there as well.

Michal




More information about the libvir-list mailing list