[libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver

[Michal Privoznik]

On 07/14/2017 02:01 PM, John Ferlan wrote:
> 
> 
> On 07/14/2017 04:26 AM, Michal Privoznik wrote:
>> On 07/13/2017 06:54 PM, John Ferlan wrote:
>>>
>>>
>>> On 07/11/2017 11:52 AM, Michal Privoznik wrote:
>>>> On 06/03/2017 03:27 PM, John Ferlan wrote:
>>>>> Since the virSecretObjListAdd technically consumes @def on success,
>>>>> the secretDefineXML should fetch the @def from the object afterwards
>>>>> and manage as an @objdef and set @def = NULL immediately.
>>>>
>>>> Really? virSecretObjListAdd sets ->def pointer in the object, but it
>>>> doesn't touch the definition otherwise. So I think a caller is safe to
>>>> continue using the pointer.
>>>>
>>>> Michal
>>>>
>>>
>>> Let's consider the code before my change...
>>>
>>> @def is added to the @obj
>>>
>>> "Something" causes us to jump to the "restore_backup:" label and @backup
>>> == NULL.
>>>
>>> That means virSecretObjListRemove runs and because @obj has @def it ends
>>> up calling virSecretDefFree
>>
>> The only way this can happen is when @obj has refcnt == 1. Because then
>> unref() calls dispose() which calls virSecretDefFree(). However, @obj
>> will have at least 2 references when entering restore_backup label. In
>> virSecretObjListAdd() when creating the new object via virSecretObjNew()
>> obj has refcnt = 1, and then we ref the object again. But wait a second:
>> if the object is already in both of the hash tables we return that
>> reference and don't increase the refcnt! So in the end,
>> virSecretObjListAdd() can return an object with refcnt == 1 and refcnt
>> == 2. This is obviously wrong and root cause of the problem you are
>> seeing. As I describe in the other e-mail, this breaks refcounting and
>> needs to be fixed.
>>
>> Michal
>>
> 
> Ah - I see what's happened - in my mind I'm already at the next patch
> where the else has a virObjectUnref(obj) after the ListRemove call and
> thus falling into cleanup and doing a virSecretDefFree would have been
> bad if @def was not NULL.
> 
> I don't understand the "But wait a second: if the object is already in
> both of the hash tables we return that reference and don't increase the
> refcnt! "
> 
> When we leave ObjListAdd - the refcnt should include 1 for New and 1 for
> the HashTable, so it should be 2. This is where I contend domainobj's
> have it wonky (or wrong) because if the Remove is called each HashRemove
> will decrement the refcnt by 1. But all the callers there "know" this
> and thus "choose" to use just Unlock at times rather than EndAPI. When
> they use EndAPI, they always will Ref the object prior which IMO causes
> too much thinking/knowledge of the consumer.

Oh, you're right. I misread the code. So the virSecretObjListAdd()
should return an object with 3 references. Two are for the two hash
tables object is in, third is for the caller to use and later free by
calling EndAPI.

> 
> I'll go read/respond to your 8/8 reply in a moment - the caffeine is
> starting to work through the morning haze...
> 
> I understand you object to the virSecretObjGetDef call as unnecessary;

I don't care that much. I just find it surprising that introducing new
variable (which I have to remember anyway when reading the code) is
considered as more readable than dereferencing directly. Moreover, the
Levensthein distance between the two is just 2 ;-)

> however, what if I use VIR_STEAL_PTR.

How?

> In the long run it's protection
> against needing to appropriately place the def = NULL much later in this
> code because we know the object owns it, but we wanted to use it and not
> create another temporary. It protects against some future adjustment
> that doesn't account for @def isn't owned by us and jumps to cleanup
> free'ing @def when we don't own it.
> 
> John
> 

Michal