[libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef

[John Ferlan]

On 07/11/2017 11:52 AM, Michal Privoznik wrote:
> On 06/03/2017 03:27 PM, John Ferlan wrote:
>> Move the consumption of @newdef into virSecretObjNew and then handle that
>> in the calling path.  Because on error the calling code expects to free
>> @newdef, unset obj->def for the creation failure error paths.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/conf/virsecretobj.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
>> index c0bcfab..ca13cf8 100644
>> --- a/src/conf/virsecretobj.c
>> +++ b/src/conf/virsecretobj.c
>> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>>  VIR_ONCE_GLOBAL_INIT(virSecretObj)
>>  
>>  static virSecretObjPtr
>> -virSecretObjNew(void)
>> +virSecretObjNew(virSecretDefPtr def)
>>  {
>>      virSecretObjPtr obj;
>>  
>> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>>          return NULL;
>>  
>>      virObjectLock(obj);
>> +    obj->def = def;
>>  
>>      return obj;
>>  }
>> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>>          goto error;
>>      }
>>  
>> -    if (!(obj = virSecretObjNew()))
>> +    if (!(obj = virSecretObjNew(newdef)))
>>          goto cleanup;
>>  
>>      /* Generate the possible configFile and base64File strings
>>       * using the configDir, uuidstr, and appropriate suffix
>>       */
>>      if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
>> -        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
>> +        !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) {
>> +        obj->def = NULL;
>>          goto error;
>> +    }
> 
> I don't quite see the value of this patch, esp. because you have to
> manually unset the ->def in each error path.
> 
> Michal
> 

Well that's part of that "longer term" vision thing where I was having
the @def be consumed in a new object. I've had to scale that back a bit
due to comments related to the object, but this code was already was all
being done in parallel - so that's why it's like that.

I could drop this one, although having @def consumed by vir*ObjNew() is
something that I have been doing throughout the various changes.  So
far, virInterfaceObjNew already has this, but patches for nwfilter and
nodedev also follow the same pattern.

I'm 50/50 right now on it and can drop it if you'd prefer. Yes, the
drawback is "obvious" that on failure, clearing obj->def needs to be
done to avoid the potential double free problem.

John