[...]
>>>
>>> @@ -391,8 +449,11 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>>> nwfilters,
>>> {
>>> virNWFilterObjPtr obj;
>>> virNWFilterDefPtr objdef;
>>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>>>
>>> - if ((obj = virNWFilterObjListFindByUUID(nwfilters, def->uuid))) {
>>> + virUUIDFormat(def->uuid, uuidstr);
>>> +
>>> + if ((obj = virNWFilterObjListFindByUUID(nwfilters, uuidstr))) {
>>> objdef = obj->def;
>>>
>>> if (STRNEQ(def->name, objdef->name)) {
>>> @@ -406,10 +467,7 @@ virNWFilterObjListAssignDef(virNWFilterObjListPtr
>>> nwfilters,
>>> virNWFilterObjEndAPI(&obj);
>>> } else {
>>> if ((obj = virNWFilterObjListFindByName(nwfilters,
>>> def->name))) {
>>> - char uuidstr[VIR_UUID_STRING_BUFLEN];
>>> -
>>> objdef = obj->def;
>>> - virUUIDFormat(objdef->uuid, uuidstr);
>>> virReportError(VIR_ERR_OPERATION_FAILED,
>>> _("filter '%s' already exists with uuid
>>> %s"),
>>> def->name, uuidstr);
>>> @@ -424,11 +482,13 @@
>>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>>> return NULL;
>>> }
>>>
>>> -
>>> - /* Get a READ lock and immediately promote to WRITE while we adjust
>>> - * data within. */
>>> - if ((obj = virNWFilterObjListFindByName(nwfilters, def->name))) {
>>> - virNWFilterObjPromoteToWrite(obj);
>>> + /* We're about to make some changes to objects on the list - so get
>>> + * the list READ lock in order to Find the object and WRITE lock it
>>> + * while we adjust data within. */
>>> + virObjectRWLockRead(nwfilters);
>>> + if ((obj = virNWFilterObjListFindByNameLocked(nwfilters,
>>> def->name))) {
>>> + virObjectRWUnlock(nwfilters);
>>> + virObjectRWLockWrite(obj);
>>>
>>> objdef = obj->def;
>>> if (virNWFilterDefEqual(def, objdef)) {
>>
>> I think you should have left the above PromoteToWrite(obj) rather than
>> doing a virObjectRWLockWrite(obj) now and would then adapt the code here
>> as mentioned in review of 2/4.
>>
>
> Hmm... true... I think that's because originally I had done the list
> patch before the object patch... So it's probably one of those merge
> things...
>
> Good catch.
>
Revisiting this particular place after some testing I've just done using
the libvirt-tck test... and lots of debug code ;-)...
When we return from the FindByNameLocked, all we have is a refcnt
incremented object. So, we have to Lock it while we still hold the
nwfilters read lock, thus this particular hunk was "mostly" correct.
... Should have taken the lock while we had nwfilters lock
... Using a Write lock avoids the immediate promote in either path
[...]
I'm going to post a v5 soon with the adjustments. I also added one more
place to Demote in the new series. That'd be in the end of this
AssignDef where we get a new filter to be consistent we should return
with the filter read locked only.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
