[libvirt] [PATCH 0/8] More virsecretobj changes to prepare for common object
A couple I forgot from earlier series, but a few things found since then along with some clarifications to make the ObjAdd processing a bit more like I've found for nwfilter, nodedev, and interface as well as making the ObjListRemove processing a bit clearer and more consistent. Along with the other series and the virObject v2 adjustments - this brings everything close enough to start sending the patches that will tie all this stuff together to create/use a common object model. John Ferlan (8): secret: Whitespace modification for secret_driver secret: Alter FindByUUID to expect the formatted uuidstr secret: Rename variable in virSecretObjListAdd secret: Clean up virSecretObjListAdd processing secret: Remove need for local configFile and base64File in ObjectAdd secret: Have virSecretObjNew consume newdef secret: Properly handle @def after virSecretObjAdd in driver secret: Handle object list removal and deletion properly src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------ src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 50 ++++++++++++------ 3 files changed, 94 insertions(+), 83 deletions(-) -- 2.9.4
Ensure two empty lines between functions. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index fc01e6d..a096fdb 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -72,6 +72,7 @@ secretDriverLock(void) virMutexLock(&driver->lock); } + static void secretDriverUnlock(void) { @@ -79,7 +80,6 @@ secretDriverUnlock(void) } - static virSecretObjPtr secretObjFromSecret(virSecretPtr secret) { @@ -109,6 +109,7 @@ secretConnectNumOfSecrets(virConnectPtr conn) conn); } + static int secretConnectListSecrets(virConnectPtr conn, char **uuids, @@ -279,6 +280,7 @@ secretDefineXML(virConnectPtr conn, return ret; } + static char * secretGetXMLDesc(virSecretPtr secret, unsigned int flags) @@ -304,6 +306,7 @@ secretGetXMLDesc(virSecretPtr secret, return ret; } + static int secretSetValue(virSecretPtr secret, const unsigned char *value, @@ -340,6 +343,7 @@ secretSetValue(virSecretPtr secret, return ret; } + static unsigned char * secretGetValue(virSecretPtr secret, size_t *value_size, @@ -377,6 +381,7 @@ secretGetValue(virSecretPtr secret, return ret; } + static int secretUndefine(virSecretPtr secret) { @@ -415,6 +420,7 @@ secretUndefine(virSecretPtr secret) return ret; } + static int secretStateCleanup(void) { @@ -435,6 +441,7 @@ secretStateCleanup(void) return 0; } + static int secretStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, @@ -486,6 +493,7 @@ secretStateInitialize(bool privileged, return -1; } + static int secretStateReload(void) { @@ -500,6 +508,7 @@ secretStateReload(void) return 0; } + static int secretConnectSecretEventRegisterAny(virConnectPtr conn, virSecretPtr secret, @@ -521,6 +530,7 @@ secretConnectSecretEventRegisterAny(virConnectPtr conn, return callbackID; } + static int secretConnectSecretEventDeregisterAny(virConnectPtr conn, int callbackID) @@ -565,6 +575,7 @@ static virStateDriver stateDriver = { .stateReload = secretStateReload, }; + int secretRegister(void) { -- 2.9.4
On 06/03/2017 03:27 PM, John Ferlan wrote:
Ensure two empty lines between functions.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
ACK Michal
Since we're storing a virUUIDFormat'd string in our Hash Table, let's modify the Lookup API to receive a formatted string as well. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 18 +++++++----------- src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 10 +++++----- 3 files changed, 13 insertions(+), 17 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 0311335..62b86b7 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -171,12 +171,8 @@ virSecretObjListDispose(void *obj) */ static virSecretObjPtr virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, - const unsigned char *uuid) + const char *uuidstr) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(uuid, uuidstr); - return virObjectRef(virHashLookup(secrets->objs, uuidstr)); } @@ -184,7 +180,7 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, /** * virSecretObjFindByUUID: * @secrets: list of secret objects - * @uuid: secret uuid to find + * @uuidstr: secret uuid to find * * This function locks @secrets and finds the secret object which * corresponds to @uuid. @@ -193,12 +189,12 @@ virSecretObjListFindByUUIDLocked(virSecretObjListPtr secrets, */ virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, - const unsigned char *uuid) + const char *uuidstr) { virSecretObjPtr obj; virObjectLock(secrets); - obj = virSecretObjListFindByUUIDLocked(secrets, uuid); + obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr); virObjectUnlock(secrets); if (obj) virObjectLock(obj); @@ -346,13 +342,14 @@ virSecretObjListAdd(virSecretObjListPtr secrets, if (oldDef) *oldDef = NULL; + virUUIDFormat(newdef->uuid, uuidstr); + /* Is there a secret already matching this UUID */ - if ((obj = virSecretObjListFindByUUIDLocked(secrets, newdef->uuid))) { + if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { virObjectLock(obj); def = obj->def; if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { - virUUIDFormat(def->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), @@ -390,7 +387,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, /* Generate the possible configFile and base64File strings * using the configDir, uuidstr, and appropriate suffix */ - virUUIDFormat(newdef->uuid, uuidstr); if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) goto cleanup; diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h index a355da7..0196813 100644 --- a/src/conf/virsecretobj.h +++ b/src/conf/virsecretobj.h @@ -40,7 +40,7 @@ virSecretObjListNew(void); virSecretObjPtr virSecretObjListFindByUUID(virSecretObjListPtr secrets, - const unsigned char *uuid); + const char *uuidstr); virSecretObjPtr virSecretObjListFindByUsage(virSecretObjListPtr secrets, diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index a096fdb..8ddae57 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -86,8 +86,8 @@ secretObjFromSecret(virSecretPtr secret) virSecretObjPtr obj; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(obj = virSecretObjListFindByUUID(driver->secrets, secret->uuid))) { - virUUIDFormat(secret->uuid, uuidstr); + virUUIDFormat(secret->uuid, uuidstr); + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); return NULL; @@ -148,10 +148,10 @@ secretLookupByUUID(virConnectPtr conn, virSecretPtr ret = NULL; virSecretObjPtr obj; virSecretDefPtr def; + char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuid))) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(uuid, uuidstr); + virUUIDFormat(uuid, uuidstr); + if (!(obj = virSecretObjListFindByUUID(driver->secrets, uuidstr))) { virReportError(VIR_ERR_NO_SECRET, _("no secret with matching uuid '%s'"), uuidstr); goto cleanup; -- 2.9.4
On 06/03/2017 03:27 PM, John Ferlan wrote:
Since we're storing a virUUIDFormat'd string in our Hash Table, let's modify the Lookup API to receive a formatted string as well.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 18 +++++++----------- src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 10 +++++----- 3 files changed, 13 insertions(+), 17 deletions(-)
ACK Michal
Rename @def to @objdef - it'll make future patches easier. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 62b86b7..e3bcbe5 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -332,7 +332,7 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virSecretDefPtr *oldDef) { virSecretObjPtr obj; - virSecretDefPtr def; + virSecretDefPtr objdef; virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL; @@ -347,26 +347,26 @@ virSecretObjListAdd(virSecretObjListPtr secrets, /* Is there a secret already matching this UUID */ if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) { virObjectLock(obj); - def = obj->def; + objdef = obj->def; - if (STRNEQ_NULLABLE(def->usage_id, newdef->usage_id)) { + if (STRNEQ_NULLABLE(objdef->usage_id, newdef->usage_id)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s is already defined for " "use with %s"), - uuidstr, def->usage_id); + uuidstr, objdef->usage_id); goto cleanup; } - if (def->isprivate && !newdef->isprivate) { + if (objdef->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); goto cleanup; } if (oldDef) - *oldDef = def; + *oldDef = objdef; else - virSecretDefFree(def); + virSecretDefFree(objdef); obj->def = newdef; } else { /* No existing secret with same UUID, @@ -375,8 +375,8 @@ virSecretObjListAdd(virSecretObjListPtr secrets, newdef->usage_type, newdef->usage_id))) { virObjectLock(obj); - def = obj->def; - virUUIDFormat(def->uuid, uuidstr); + objdef = obj->def; + virUUIDFormat(objdef->uuid, uuidstr); virReportError(VIR_ERR_INTERNAL_ERROR, _("a secret with UUID %s already defined for " "use with %s"), -- 2.9.4
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rename @def to @objdef - it'll make future patches easier.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
ACK Michal
Make use of an error: label to handle the failure and need to call virSecretObjEndAPI for the object to set it to NULL for return. Also rather than an if/else processing - have the initial "if" which is just replacing the @newdef into obj->def goto cleanup, thus allowing the remaining code to be extracted from the else and appear to more inline. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index e3bcbe5..1bafd0b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, { virSecretObjPtr obj; virSecretDefPtr objdef; - virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL; @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets, _("a secret with UUID %s is already defined for " "use with %s"), uuidstr, objdef->usage_id); - goto cleanup; + goto error; } if (objdef->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); - goto cleanup; + goto error; } if (oldDef) @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets, else virSecretDefFree(objdef); obj->def = newdef; - } else { - /* No existing secret with same UUID, - * try look for matching usage instead */ - if ((obj = virSecretObjListFindByUsageLocked(secrets, - newdef->usage_type, - newdef->usage_id))) { - virObjectLock(obj); - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("a secret with UUID %s already defined for " - "use with %s"), - uuidstr, newdef->usage_id); - goto cleanup; - } + goto cleanup; + } - /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix - */ - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) - goto cleanup; + /* No existing secret with same UUID, + * try to look for matching usage instead */ + if ((obj = virSecretObjListFindByUsageLocked(secrets, + newdef->usage_type, + newdef->usage_id))) { + virObjectLock(obj); + objdef = obj->def; + virUUIDFormat(objdef->uuid, uuidstr); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a secret with UUID %s already defined for " + "use with %s"), + uuidstr, newdef->usage_id); + goto error; + } - if (!(obj = virSecretObjNew())) - goto cleanup; + /* Generate the possible configFile and base64File strings + * using the configDir, uuidstr, and appropriate suffix + */ + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + goto cleanup; - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) - goto cleanup; + if (!(obj = virSecretObjNew())) + goto cleanup; - obj->def = newdef; - VIR_STEAL_PTR(obj->configFile, configFile); - VIR_STEAL_PTR(obj->base64File, base64File); - virObjectRef(obj); - } + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + goto error; - ret = obj; - obj = NULL; + obj->def = newdef; + VIR_STEAL_PTR(obj->configFile, configFile); + VIR_STEAL_PTR(obj->base64File, base64File); + virObjectRef(obj); cleanup: - virSecretObjEndAPI(&obj); VIR_FREE(configFile); VIR_FREE(base64File); virObjectUnlock(secrets); - return ret; + return obj; + + error: + virSecretObjEndAPI(&obj); + goto cleanup; } -- 2.9.4
On 06/03/2017 03:27 PM, John Ferlan wrote:
Make use of an error: label to handle the failure and need to call virSecretObjEndAPI for the object to set it to NULL for return.
Also rather than an if/else processing - have the initial "if" which is just replacing the @newdef into obj->def goto cleanup, thus allowing the remaining code to be extracted from the else and appear to more inline.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index e3bcbe5..1bafd0b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, { virSecretObjPtr obj; virSecretDefPtr objdef; - virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL;
@@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets, _("a secret with UUID %s is already defined for " "use with %s"), uuidstr, objdef->usage_id); - goto cleanup; + goto error; }
if (objdef->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); - goto cleanup; + goto error; }
if (oldDef) @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets, else virSecretDefFree(objdef); obj->def = newdef; - } else { - /* No existing secret with same UUID, - * try look for matching usage instead */ - if ((obj = virSecretObjListFindByUsageLocked(secrets, - newdef->usage_type, - newdef->usage_id))) { - virObjectLock(obj); - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("a secret with UUID %s already defined for " - "use with %s"), - uuidstr, newdef->usage_id); - goto cleanup; - } + goto cleanup; + }
- /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix - */ - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) - goto cleanup; + /* No existing secret with same UUID, + * try to look for matching usage instead */ + if ((obj = virSecretObjListFindByUsageLocked(secrets, + newdef->usage_type, + newdef->usage_id))) { + virObjectLock(obj); + objdef = obj->def; + virUUIDFormat(objdef->uuid, uuidstr); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a secret with UUID %s already defined for " + "use with %s"), + uuidstr, newdef->usage_id); + goto error; + }
- if (!(obj = virSecretObjNew())) - goto cleanup; + /* Generate the possible configFile and base64File strings + * using the configDir, uuidstr, and appropriate suffix + */ + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + goto cleanup;
- if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) - goto cleanup; + if (!(obj = virSecretObjNew())) + goto cleanup;
- obj->def = newdef; - VIR_STEAL_PTR(obj->configFile, configFile); - VIR_STEAL_PTR(obj->base64File, base64File); - virObjectRef(obj); - } + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + goto error;
Frankly, I find this very confusing. While reading the commit message, I understand why you sometimes jump to cleanup and other times to error label. But if I were to just look at the code alone, it's completely random to me. I think I'd much rather see the pattern that's here. However, if you really dislike if-else branching (dislike also as in prepare for upcoming patches), I suggest changing just that. Michal
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
Make use of an error: label to handle the failure and need to call virSecretObjEndAPI for the object to set it to NULL for return.
Also rather than an if/else processing - have the initial "if" which is just replacing the @newdef into obj->def goto cleanup, thus allowing the remaining code to be extracted from the else and appear to more inline.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index e3bcbe5..1bafd0b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, { virSecretObjPtr obj; virSecretDefPtr objdef; - virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL;
@@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets, _("a secret with UUID %s is already defined for " "use with %s"), uuidstr, objdef->usage_id); - goto cleanup; + goto error; }
if (objdef->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); - goto cleanup; + goto error; }
if (oldDef) @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets, else virSecretDefFree(objdef); obj->def = newdef; - } else { - /* No existing secret with same UUID, - * try look for matching usage instead */ - if ((obj = virSecretObjListFindByUsageLocked(secrets, - newdef->usage_type, - newdef->usage_id))) { - virObjectLock(obj); - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("a secret with UUID %s already defined for " - "use with %s"), - uuidstr, newdef->usage_id); - goto cleanup; - } + goto cleanup; + }
- /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix - */ - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) - goto cleanup; + /* No existing secret with same UUID, + * try to look for matching usage instead */ + if ((obj = virSecretObjListFindByUsageLocked(secrets, + newdef->usage_type, + newdef->usage_id))) { + virObjectLock(obj); + objdef = obj->def; + virUUIDFormat(objdef->uuid, uuidstr); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a secret with UUID %s already defined for " + "use with %s"), + uuidstr, newdef->usage_id); + goto error; + }
- if (!(obj = virSecretObjNew())) - goto cleanup; + /* Generate the possible configFile and base64File strings + * using the configDir, uuidstr, and appropriate suffix + */ + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + goto cleanup;
- if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) - goto cleanup; + if (!(obj = virSecretObjNew())) + goto cleanup;
- obj->def = newdef; - VIR_STEAL_PTR(obj->configFile, configFile); - VIR_STEAL_PTR(obj->base64File, base64File); - virObjectRef(obj); - } + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + goto error;
Frankly, I find this very confusing. While reading the commit message, I understand why you sometimes jump to cleanup and other times to error label. But if I were to just look at the code alone, it's completely random to me. I think I'd much rather see the pattern that's here. However, if you really dislike if-else branching (dislike also as in prepare for upcoming patches), I suggest changing just that.
Michal
I'll return the if - else - logic... and update the commit message. Would you like to see a version of that? It does affect the next couple of patches. For patch 5 it's just a simple indention adjustment. Since there's questions in 6 I'll address those there. John
On 07/13/2017 06:36 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
Make use of an error: label to handle the failure and need to call virSecretObjEndAPI for the object to set it to NULL for return.
Also rather than an if/else processing - have the initial "if" which is just replacing the @newdef into obj->def goto cleanup, thus allowing the remaining code to be extracted from the else and appear to more inline.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 74 ++++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index e3bcbe5..1bafd0b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, { virSecretObjPtr obj; virSecretDefPtr objdef; - virSecretObjPtr ret = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL, *base64File = NULL;
@@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets, _("a secret with UUID %s is already defined for " "use with %s"), uuidstr, objdef->usage_id); - goto cleanup; + goto error; }
if (objdef->isprivate && !newdef->isprivate) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot change private flag on existing secret")); - goto cleanup; + goto error; }
if (oldDef) @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets, else virSecretDefFree(objdef); obj->def = newdef; - } else { - /* No existing secret with same UUID, - * try look for matching usage instead */ - if ((obj = virSecretObjListFindByUsageLocked(secrets, - newdef->usage_type, - newdef->usage_id))) { - virObjectLock(obj); - objdef = obj->def; - virUUIDFormat(objdef->uuid, uuidstr); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("a secret with UUID %s already defined for " - "use with %s"), - uuidstr, newdef->usage_id); - goto cleanup; - } + goto cleanup; + }
- /* Generate the possible configFile and base64File strings - * using the configDir, uuidstr, and appropriate suffix - */ - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) - goto cleanup; + /* No existing secret with same UUID, + * try to look for matching usage instead */ + if ((obj = virSecretObjListFindByUsageLocked(secrets, + newdef->usage_type, + newdef->usage_id))) { + virObjectLock(obj); + objdef = obj->def; + virUUIDFormat(objdef->uuid, uuidstr); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("a secret with UUID %s already defined for " + "use with %s"), + uuidstr, newdef->usage_id); + goto error; + }
- if (!(obj = virSecretObjNew())) - goto cleanup; + /* Generate the possible configFile and base64File strings + * using the configDir, uuidstr, and appropriate suffix + */ + if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || + !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + goto cleanup;
- if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) - goto cleanup; + if (!(obj = virSecretObjNew())) + goto cleanup;
- obj->def = newdef; - VIR_STEAL_PTR(obj->configFile, configFile); - VIR_STEAL_PTR(obj->base64File, base64File); - virObjectRef(obj); - } + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + goto error;
Frankly, I find this very confusing. While reading the commit message, I understand why you sometimes jump to cleanup and other times to error label. But if I were to just look at the code alone, it's completely random to me. I think I'd much rather see the pattern that's here. However, if you really dislike if-else branching (dislike also as in prepare for upcoming patches), I suggest changing just that.
Michal
I'll return the if - else - logic... and update the commit message.
Would you like to see a version of that?
Yes please.
It does affect the next couple of patches. For patch 5 it's just a simple indention adjustment. Since there's questions in 6 I'll address those there.
That's okay, you can just send another version (and state that some patches were ACKed already). Michal
Rather than assign to a local variable, let's just assign directly to the object using the error path for cleanup. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 1bafd0b..c0bcfab 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -334,7 +334,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets, virSecretObjPtr obj; virSecretDefPtr objdef; char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *configFile = NULL, *base64File = NULL; virObjectLock(secrets); @@ -385,27 +384,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets, goto error; } + if (!(obj = virSecretObjNew())) + goto cleanup; + /* Generate the possible configFile and base64File strings * using the configDir, uuidstr, and appropriate suffix */ - if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || - !(base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) - goto cleanup; - - if (!(obj = virSecretObjNew())) - goto cleanup; + if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) || + !(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64"))) + goto error; if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) goto error; obj->def = newdef; - VIR_STEAL_PTR(obj->configFile, configFile); - VIR_STEAL_PTR(obj->base64File, base64File); virObjectRef(obj); cleanup: - VIR_FREE(configFile); - VIR_FREE(base64File); virObjectUnlock(secrets); return obj; -- 2.9.4
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than assign to a local variable, let's just assign directly to the object using the error path for cleanup.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
ACK Michal
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@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; + } - if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) + if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0) { + obj->def = NULL; goto error; + } - obj->def = newdef; virObjectRef(obj); cleanup: -- 2.9.4
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@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
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@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
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@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 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
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. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/secret/secret_driver.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 8ddae57..32cd8bb 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -210,6 +210,7 @@ secretDefineXML(virConnectPtr conn, { virSecretPtr ret = NULL; virSecretObjPtr obj = NULL; + virSecretDefPtr objdef; virSecretDefPtr backup = NULL; virSecretDefPtr def; virObjectEventPtr event = NULL; @@ -225,8 +226,10 @@ secretDefineXML(virConnectPtr conn, if (!(obj = virSecretObjListAdd(driver->secrets, def, driver->configDir, &backup))) goto cleanup; + def = NULL; + objdef = virSecretObjGetDef(obj); - if (!def->isephemeral) { + if (!objdef->isephemeral) { if (backup && backup->isephemeral) { if (virSecretObjSaveData(obj) < 0) goto restore_backup; @@ -248,17 +251,16 @@ secretDefineXML(virConnectPtr conn, /* Saved successfully - drop old values */ virSecretDefFree(backup); - event = virSecretEventLifecycleNew(def->uuid, - def->usage_type, - def->usage_id, + event = virSecretEventLifecycleNew(objdef->uuid, + objdef->usage_type, + objdef->usage_id, VIR_SECRET_EVENT_DEFINED, 0); ret = virGetSecret(conn, - def->uuid, - def->usage_type, - def->usage_id); - def = NULL; + objdef->uuid, + objdef->usage_type, + objdef->usage_id); goto cleanup; restore_backup: -- 2.9.4
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
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 very next thing we do is call virSecretDefFree on the same @def address. While it is the same thing and I could do a VIR_STEAL_PTR(objdef, def); instead, the patch just follows the pattern I've adopted in other patches where @def = NULL and objdef = vir*ObjGetDef() IDC either way, but to avoid a path where @def could be free'd twice, something needs to be done. John
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
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
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
On 07/14/2017 08:26 AM, Michal Privoznik wrote:
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.
Coffee is strong this morning or light has dawned on marblehead. Let me go back to the "existing" code for a moment where @def isn't set to NULL when we get to restore_backup and call ObjListRemove After Add, we have R=2,L=1 After Remove, we have R=1 fall into cleanup: Call virSecretDefFree(def) where @def != NULL, so it free's it Call virSecretObjEndAPI(&obj) which calls Unref, setting R=0 and calling virSecretDefFree(obj->def)... But wait, we already did that because we allowed the DefFree(def) to free something it didn't own.
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.
Object is only in one hash table at this point in time, so the return is ref = 2, lock = 1. One ref is for the object allocation and one ref for the table. IMO, when EndAPI is called, it's an indication that the caller is done with the obj; however, because the obj is in a hash table, the other ref remains. I've always considered EndAPI an indication that we're done with our reference to the object whether that was from an Add or a Lookup, both of which should return with an incremented refcnt and a locked object. As an aside, Add should also increment for each hash table we're in because Remove will decrement for each. That's where I think the domainobj code is wrong.
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 ;-)
As long as someone remain vigilant that @def is set to NULL at some point in the code after Add but before cleanup:, then we're good. But on the off chance that it doesn't (which I've shown above), then we're not in a happy state.
however, what if I use VIR_STEAL_PTR.
How?
Instead of: if (!(obj = virSecretObjListAdd(driver->secrets, def, driver->configDir, &backup))) goto cleanup; def = NULL; objdef = virSecretObjGetDef(obj); it'd be if (!(obj = virSecretObjListAdd(driver->secrets, def, driver->configDir, &backup))) goto cleanup; VIR_STEAL_PTR(objdef, def); John
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
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref after the call virSecretObjListRemove, be more explicit by calling virObjectUnref and setting @obj to NULL for secretUndefine and in the error path of secretDefineXML. This also fixes a leak during virSecretLoad if the virSecretLoadValue fails the code jumps to cleanup without setting @ret = obj, thus calling virSecretObjListRemove which only accounts for the object reference related to adding the object to the list during virSecretObjListAdd, but does not account for the reference to the object itself as the return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI on the object recently added thus reducing the refcnt to zero. Thus cleaning up the virSecretLoadValue error path to make it clearer what needs to be done on failure. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 14 ++++++-------- src/secret/secret_driver.c | 9 +++++++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index ca13cf8..26646dd 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -918,7 +918,6 @@ virSecretLoad(virSecretObjListPtr secrets, { virSecretDefPtr def = NULL; virSecretObjPtr obj = NULL; - virSecretObjPtr ret = NULL; if (!(def = virSecretDefParseFile(path))) goto cleanup; @@ -930,16 +929,15 @@ virSecretLoad(virSecretObjListPtr secrets, goto cleanup; def = NULL; - if (virSecretLoadValue(obj) < 0) - goto cleanup; - - ret = obj; - obj = NULL; + if (virSecretLoadValue(obj) < 0) { + virSecretObjListRemove(secrets, obj); + virObjectUnref(obj); + obj = NULL; + } cleanup: - virSecretObjListRemove(secrets, obj); virSecretDefFree(def); - return ret; + return obj; } diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 32cd8bb..14483fd 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -268,10 +268,13 @@ secretDefineXML(virConnectPtr conn, * the backup. The current def will be handled below. * Otherwise, this is a new secret, thus remove it. */ - if (backup) + if (backup) { virSecretObjSetDef(obj, backup); - else + } else { virSecretObjListRemove(driver->secrets, obj); + virObjectUnref(obj); + obj = NULL; + } cleanup: virSecretDefFree(def); @@ -411,6 +414,8 @@ secretUndefine(virSecretPtr secret) virSecretObjDeleteData(obj); virSecretObjListRemove(driver->secrets, obj); + virObjectUnref(obj); + obj = NULL; ret = 0; -- 2.9.4
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref after the call virSecretObjListRemove, be more explicit by calling virObjectUnref and setting @obj to NULL for secretUndefine and in the error path of secretDefineXML.
This also fixes a leak during virSecretLoad if the virSecretLoadValue fails the code jumps to cleanup without setting @ret = obj, thus calling virSecretObjListRemove which only accounts for the object reference related to adding the object to the list during virSecretObjListAdd, but does not account for the reference to the object itself as the return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI on the object recently added thus reducing the refcnt to zero. Thus cleaning up the virSecretLoadValue error path to make it clearer what needs to be done on failure.
I think the real reason is that we cannot call virSecretObjEndAPI() because that *unlocks* the secret object. However, the object is already unlocked at that point by virSecretObjListRemove() and thus we would unlock twice while locking just once. Honestly, I'd rather see that explanation in the commit message. But it's your call.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 14 ++++++-------- src/secret/secret_driver.c | 9 +++++++-- 2 files changed, 13 insertions(+), 10 deletions(-)
ACK Michal
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref after the call virSecretObjListRemove, be more explicit by calling virObjectUnref and setting @obj to NULL for secretUndefine and in the error path of secretDefineXML.
This also fixes a leak during virSecretLoad if the virSecretLoadValue fails the code jumps to cleanup without setting @ret = obj, thus calling virSecretObjListRemove which only accounts for the object reference related to adding the object to the list during virSecretObjListAdd, but does not account for the reference to the object itself as the return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI on the object recently added thus reducing the refcnt to zero. Thus cleaning up the virSecretLoadValue error path to make it clearer what needs to be done on failure.
I think the real reason is that we cannot call virSecretObjEndAPI() because that *unlocks* the secret object. However, the object is already unlocked at that point by virSecretObjListRemove() and thus we would unlock twice while locking just once. Honestly, I'd rather see that explanation in the commit message. But it's your call.
Partially true - although calling Unlock on something already Unlocked by the same thread IIRC doesn't do much other than cause an error, but we don't fail on that error. It's ironic this relates to something Erik and I discussed during the virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now has a reference on the object and would always need to Unref it even after removing it from the List. This just happened to be where I had that oh sh*t moment and realized that when calling Remove we were essentially unlocking and thus yes calling EndAPI would unlock and unlocked object. I'll add something in about that.. John As an aside, this is exactly why I started down the path of common objects. Consider how the callers to virDomainObjListAdd go through great lengths to managed the returned object some quite differently based on what they know about how many refs are returned and whether the calling function calls virDomainObjEndAPI. If it does, there's always a virObjectRef done on the object after the Add returns. It's a subtle thing, but confusing nonetheless. The thing is the *Remove API will call virHashRemoveEntry which decrements a refcnt for each table; however, the *Add API only increments the refcnt once for adding into each table. Callers are very careful to understand and manage that. I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj the @obj regardless if Add or Lookup was used. The callers shouldn't have to know they need to either use *EndAPI or ObjUnlock. In any case, I digress and that's a different issue for another day
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 14 ++++++-------- src/secret/secret_driver.c | 9 +++++++-- 2 files changed, 13 insertions(+), 10 deletions(-)
ACK
Michal
On 07/13/2017 07:19 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref after the call virSecretObjListRemove, be more explicit by calling virObjectUnref and setting @obj to NULL for secretUndefine and in the error path of secretDefineXML.
This also fixes a leak during virSecretLoad if the virSecretLoadValue fails the code jumps to cleanup without setting @ret = obj, thus calling virSecretObjListRemove which only accounts for the object reference related to adding the object to the list during virSecretObjListAdd, but does not account for the reference to the object itself as the return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI on the object recently added thus reducing the refcnt to zero. Thus cleaning up the virSecretLoadValue error path to make it clearer what needs to be done on failure.
I think the real reason is that we cannot call virSecretObjEndAPI() because that *unlocks* the secret object. However, the object is already unlocked at that point by virSecretObjListRemove() and thus we would unlock twice while locking just once. Honestly, I'd rather see that explanation in the commit message. But it's your call.
Partially true - although calling Unlock on something already Unlocked by the same thread IIRC doesn't do much other than cause an error, but we don't fail on that error.
I don't think this is true. Just consider the following general example: Thread A | Thread B ------------------------------ 1) lock(X) | 2) | lock(x) 3) unlock(X) | 4) unlock(X) | 5) | work(X) In this scenario, thread B starts working on X thinking it's locked while it isn't. Also, man-page for pthread_mutex_lock says that unlocking an unlocked lock leads to undefined behaviour (we use NORMAL non-robust mutexes. And yes, I had to search for what does it mean non-robust mutex).
It's ironic this relates to something Erik and I discussed during the virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now has a reference on the object and would always need to Unref it even after removing it from the List.
About that. I think that if we do our ref counting right, we don't need to do any special unref(). I mean: undefine(X) { obj = lookup(X); // obj now has refcnt = 2, one beacuse it's in the // list, one for the undefine() listRemove(obj); // obj.refcnt = 1, the list reference is gone endApi(&obj); // undefine() reference is gone, obj is freed } Of course, if list uses two hash tables (one for UUIDs one for names) it holds two references, but then listRemove() also unref()-s twice.
This just happened to be where I had that oh sh*t moment and realized that when calling Remove we were essentially unlocking and thus yes calling EndAPI would unlock and unlocked object. I'll add something in about that..
John
As an aside, this is exactly why I started down the path of common objects. Consider how the callers to virDomainObjListAdd go through great lengths to managed the returned object some quite differently based on what they know about how many refs are returned and whether the calling function calls virDomainObjEndAPI. If it does, there's always a virObjectRef done on the object after the Add returns. It's a subtle thing, but confusing nonetheless.
The thing is the *Remove API will call virHashRemoveEntry which decrements a refcnt for each table; however, the *Add API only increments the refcnt once for adding into each table. Callers are very careful to understand and manage that.
I haven't looked into the code, but if this is true then listAdd() and listRemove() functions are broken because they mangle refcount. It should be able to do the following and having the refcount of an object unchanged at the end: listAdd(obj); listRemove(obj); If the refcount is not the same at the end as it was at the beginning, the list functions are broken and need to be fixed
I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj the @obj regardless if Add or Lookup was used. The callers shouldn't have to know they need to either use *EndAPI or ObjUnlock. In any case, I digress and that's a different issue for another day
I totally agree on this! Michal
On 07/14/2017 04:15 AM, Michal Privoznik wrote:
On 07/13/2017 07:19 PM, John Ferlan wrote:
On 07/11/2017 11:52 AM, Michal Privoznik wrote:
On 06/03/2017 03:27 PM, John Ferlan wrote:
Rather than rely on virSecretObjEndAPI to make the final virObjectUnref after the call virSecretObjListRemove, be more explicit by calling virObjectUnref and setting @obj to NULL for secretUndefine and in the error path of secretDefineXML.
This also fixes a leak during virSecretLoad if the virSecretLoadValue fails the code jumps to cleanup without setting @ret = obj, thus calling virSecretObjListRemove which only accounts for the object reference related to adding the object to the list during virSecretObjListAdd, but does not account for the reference to the object itself as the return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI on the object recently added thus reducing the refcnt to zero. Thus cleaning up the virSecretLoadValue error path to make it clearer what needs to be done on failure.
I think the real reason is that we cannot call virSecretObjEndAPI() because that *unlocks* the secret object. However, the object is already unlocked at that point by virSecretObjListRemove() and thus we would unlock twice while locking just once. Honestly, I'd rather see that explanation in the commit message. But it's your call.
Partially true - although calling Unlock on something already Unlocked by the same thread IIRC doesn't do much other than cause an error, but we don't fail on that error.
I don't think this is true. Just consider the following general example:
Thread A | Thread B ------------------------------ 1) lock(X) | 2) | lock(x) 3) unlock(X) | 4) unlock(X) | 5) | work(X)
In this scenario, thread B starts working on X thinking it's locked while it isn't. Also, man-page for pthread_mutex_lock says that unlocking an unlocked lock leads to undefined behaviour (we use NORMAL non-robust mutexes. And yes, I had to search for what does it mean non-robust mutex).
I assume that step 2 is supposed to be (X) and not (x)? In your scenario above ThreadB gains the lock after step 3 since that's the moment in time when ThreadA gives it up. When ThreadA goes to unlock in step 4, it fails because it doesn't own the lock as only ThreadB can unlock. Without ThreadB in the picture, then the second unlock would be a failure because the thread isn't locked, cannot unlock an unlocked object. IMO: undefined behavior is what I'd call a way to say you could get an error that the lock is already unlocked or you could get an error that you don't own the lock to unlock, but we don't want to explain that so we'll just it's undefined. For some reason I have this recollection about fork'd children that really make things crazy, but I'm not sure I can put it into words.
It's ironic this relates to something Erik and I discussed during the virNodeDev* changes w/r/t the "owner" (driver) after the Add/Append now has a reference on the object and would always need to Unref it even after removing it from the List.
About that. I think that if we do our ref counting right, we don't need to do any special unref(). I mean:
undefine(X) { obj = lookup(X); // obj now has refcnt = 2, one beacuse it's in the // list, one for the undefine() listRemove(obj); // obj.refcnt = 1, the list reference is gone endApi(&obj); // undefine() reference is gone, obj is freed }
Of course, if list uses two hash tables (one for UUIDs one for names) it holds two references, but then listRemove() also unref()-s twice.
When one enters a listRemove function, there will be either 2 or 3 refs on the object (1 or 2 hash tables) and the object is locked. When one leaves a listRemove function, there is 1 reference on the object and the object is unlocked. So is the problem that listRemove shouldn't make that last Unlock? Or that the caller shouldn't make an EndAPI call after calling Remove? The corollary is that the listAdd would leave with 2 or 3 refs (again number of hash tables) and the object locked. The caller is then expected to run EndAPI. The obj is then left with 1 or 2 refs because it's in a table and not because it's being used again (since EndAPI will set *obj = NULL). My "chosen" solution is to keep the Unlock in listRemove, but I can be swayed to have listRemove keep the lock thus having callers use EndAPI instead of Unref. That would be something I would want to propagate to other series so as to keep things similar between all consumers.
This just happened to be where I had that oh sh*t moment and realized that when calling Remove we were essentially unlocking and thus yes calling EndAPI would unlock and unlocked object. I'll add something in about that..
John
As an aside, this is exactly why I started down the path of common objects. Consider how the callers to virDomainObjListAdd go through great lengths to managed the returned object some quite differently based on what they know about how many refs are returned and whether the calling function calls virDomainObjEndAPI. If it does, there's always a virObjectRef done on the object after the Add returns. It's a subtle thing, but confusing nonetheless.
The thing is the *Remove API will call virHashRemoveEntry which decrements a refcnt for each table; however, the *Add API only increments the refcnt once for adding into each table. Callers are very careful to understand and manage that.
I haven't looked into the code, but if this is true then listAdd() and listRemove() functions are broken because they mangle refcount. It should be able to do the following and having the refcount of an object unchanged at the end:
listAdd(obj); listRemove(obj);
If the refcount is not the same at the end as it was at the beginning, the list functions are broken and need to be fixed
Go check callers of virDomainObjListAdd... Every single qemu case will always virObjectRef(vm) after a call... Other consumers will sometimes use EndAPI and other times use Unlock based primarily on whether they called Ref as well. It's at best confusing. The problem in my mind is that the Add was only refcnt'ing once for two lists while the Remove was having two unref's. I think if obj = Add is successful, then it's still up to whatever code does the Remove(obj) code to subsequently indicate it's really done with @obj by performing the Unref. At least that's the goal I've been working towards... John
I'd rather see that mgmt go away. It should be "safe" to call EndAPI obj the @obj regardless if Add or Lookup was used. The callers shouldn't have to know they need to either use *EndAPI or ObjUnlock. In any case, I digress and that's a different issue for another day
I totally agree on this!
Michal
ping? A bit involved, but encompasses a few things I have found or made more common in other vir*obj's Tks, John On 06/03/2017 09:27 AM, John Ferlan wrote:
A couple I forgot from earlier series, but a few things found since then along with some clarifications to make the ObjAdd processing a bit more like I've found for nwfilter, nodedev, and interface as well as making the ObjListRemove processing a bit clearer and more consistent.
Along with the other series and the virObject v2 adjustments - this brings everything close enough to start sending the patches that will tie all this stuff together to create/use a common object model.
John Ferlan (8): secret: Whitespace modification for secret_driver secret: Alter FindByUUID to expect the formatted uuidstr secret: Rename variable in virSecretObjListAdd secret: Clean up virSecretObjListAdd processing secret: Remove need for local configFile and base64File in ObjectAdd secret: Have virSecretObjNew consume newdef secret: Properly handle @def after virSecretObjAdd in driver secret: Handle object list removal and deletion properly
src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------ src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 50 ++++++++++++------ 3 files changed, 94 insertions(+), 83 deletions(-)
ping^2 ? Tks, John Would help especially since I could post more for the recently posted virObject adjustment... On 06/14/2017 09:29 PM, John Ferlan wrote:
ping?
A bit involved, but encompasses a few things I have found or made more common in other vir*obj's
Tks,
John
On 06/03/2017 09:27 AM, John Ferlan wrote:
A couple I forgot from earlier series, but a few things found since then along with some clarifications to make the ObjAdd processing a bit more like I've found for nwfilter, nodedev, and interface as well as making the ObjListRemove processing a bit clearer and more consistent.
Along with the other series and the virObject v2 adjustments - this brings everything close enough to start sending the patches that will tie all this stuff together to create/use a common object model.
John Ferlan (8): secret: Whitespace modification for secret_driver secret: Alter FindByUUID to expect the formatted uuidstr secret: Rename variable in virSecretObjListAdd secret: Clean up virSecretObjListAdd processing secret: Remove need for local configFile and base64File in ObjectAdd secret: Have virSecretObjNew consume newdef secret: Properly handle @def after virSecretObjAdd in driver secret: Handle object list removal and deletion properly
src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------ src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 50 ++++++++++++------ 3 files changed, 94 insertions(+), 83 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
ping^3 ? Tks, John On 06/22/2017 06:26 PM, John Ferlan wrote:
ping^2 ?
Tks,
John
Would help especially since I could post more for the recently posted virObject adjustment...
On 06/14/2017 09:29 PM, John Ferlan wrote:
ping?
A bit involved, but encompasses a few things I have found or made more common in other vir*obj's
Tks,
John
On 06/03/2017 09:27 AM, John Ferlan wrote:
A couple I forgot from earlier series, but a few things found since then along with some clarifications to make the ObjAdd processing a bit more like I've found for nwfilter, nodedev, and interface as well as making the ObjListRemove processing a bit clearer and more consistent.
Along with the other series and the virObject v2 adjustments - this brings everything close enough to start sending the patches that will tie all this stuff together to create/use a common object model.
John Ferlan (8): secret: Whitespace modification for secret_driver secret: Alter FindByUUID to expect the formatted uuidstr secret: Rename variable in virSecretObjListAdd secret: Clean up virSecretObjListAdd processing secret: Remove need for local configFile and base64File in ObjectAdd secret: Have virSecretObjNew consume newdef secret: Properly handle @def after virSecretObjAdd in driver secret: Handle object list removal and deletion properly
src/conf/virsecretobj.c | 125 +++++++++++++++++++++------------------------ src/conf/virsecretobj.h | 2 +- src/secret/secret_driver.c | 50 ++++++++++++------ 3 files changed, 94 insertions(+), 83 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Michal Privoznik