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