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

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(a)redhat.com&gt; >> --- >> 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.