On 07/13/2017 06:43 PM, John Ferlan wrote:
>
>
> 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@xxxxxxxxxx>
>>> ---
>>> 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 know that you're doing it in other patches, but I don't think we need
to do that. It's not like we will make obj->def private. But maybe I'm
missing big picture here. What is your reasoning why should vir*ObjNew
take def? Moreover, other object members are initialized "old way" too
(e.g. obj->base64File). So mixing approaches might be confusing IMO.
>
> 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.
Yeah, I'd prefer it to be dropped, but then again - maybe I'm missing
big picture. So I'm not gonna tell you to do that.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
