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

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.

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;

however, what if I use VIR_STEAL_PTR.

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