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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
