8:14 a.m.
On 07/14/2017 08:26 AM, Michal Privoznik wrote:
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.
Coffee is strong this morning or light has dawned on marblehead.
Let me go back to the "existing" code for a moment where @def isn't set
to NULL when we get to restore_backup and call ObjListRemove
After Add, we have R=2,L=1
After Remove, we have R=1
fall into cleanup:
Call virSecretDefFree(def) where @def != NULL, so it free's it
Call virSecretObjEndAPI(&obj) which calls Unref, setting R=0 and calling
virSecretDefFree(obj->def)... But wait, we already did that because we
allowed the DefFree(def) to free something it didn't own.
>>
>> 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.
Object is only in one hash table at this point in time, so the return is
ref = 2, lock = 1. One ref is for the object allocation and one ref for
the table.
IMO, when EndAPI is called, it's an indication that the caller is done
with the obj; however, because the obj is in a hash table, the other ref
remains. I've always considered EndAPI an indication that we're done
with our reference to the object whether that was from an Add or a
Lookup, both of which should return with an incremented refcnt and a
locked object. As an aside, Add should also increment for each hash
table we're in because Remove will decrement for each. That's where I
think the domainobj code is wrong.
>
> 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 ;-)
As long as someone remain vigilant that @def is set to NULL at some
point in the code after Add but before cleanup:, then we're good. But on
the off chance that it doesn't (which I've shown above), then we're not
in a happy state.
> however, what if I use VIR_STEAL_PTR.
How?
Instead of:
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
def = NULL;
objdef = virSecretObjGetDef(obj);
it'd be
if (!(obj = virSecretObjListAdd(driver->secrets, def,
driver->configDir, &backup)))
goto cleanup;
VIR_STEAL_PTR(objdef, def);
John
> 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