On 03/08/2016 12:35 PM, John Ferlan wrote:
Move and rename secretDeleteSaved from secret_driver into secret_conf and split it up into two parts since there is error path code that looks to just delete the secret data file
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/secret_conf.c | 21 +++++++++++++++++++++ src/conf/secret_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ src/secret/secret_driver.c | 22 ++++++---------------- 4 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index f6eee6f..52f78bd 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -685,6 +685,27 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets, }
+int +virSecretObjDeleteConfig(virSecretObjPtr secret) +{ + /* When the XML is missing, we'll still allow the attempt to + * delete the secret data. Without a configFile, the secret + won't be loaded again, so we have succeeded already. */
This comment seems weirdly placed now. If it's kept at all it should be placed at the ObjDeleteData call sites. Or rework it as a comment in ObjDeleteData to explain why we don't care about failure in this case.
+ if (!secret->def->ephemeral && + unlink(secret->configFile) < 0 && errno != ENOENT) + return -1; +
This should report have a virReportSystemError call. The original code doesn't have one, but that's a bug Minor stuff though so ACK in general, I don't care if you fix before pushing but not sure if there's a formal policy on that - Cole
+ return 0; +} + + +void +virSecretObjDeleteData(virSecretObjPtr secret) +{ + (void)unlink(secret->base64File); +} + + void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index d3bd10c..e2f69b5 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -114,6 +114,10 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets, virSecretObjListACLFilter filter, virConnectPtr conn);
+int virSecretObjDeleteConfig(virSecretObjPtr secret); + +void virSecretObjDeleteData(virSecretObjPtr secret); + void virSecretDefFree(virSecretDefPtr def); virSecretDefPtr virSecretDefParseString(const char *xml); virSecretDefPtr virSecretDefParseFile(const char *filename); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cbc36de..2437b0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -787,6 +787,8 @@ virSecretDefFree; virSecretDefParseFile; virSecretDefParseString; virSecretLoadAllConfigs; +virSecretObjDeleteConfig; +virSecretObjDeleteData; virSecretObjEndAPI; virSecretObjListAdd; virSecretObjListExport; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index b8d9ecc..e4315f3 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -175,19 +175,6 @@ secretSaveValue(const virSecretObj *secret) return ret; }
-static int -secretDeleteSaved(const virSecretObj *secret) -{ - if (unlink(secret->configFile) < 0 && errno != ENOENT) - return -1; - - /* When the XML is missing, the rest may waste disk space, but the secret - won't be loaded again, so we have succeeded already. */ - (void)unlink(secret->base64File); - - return 0; -} - /* Driver functions */
static int @@ -325,8 +312,10 @@ secretDefineXML(virConnectPtr conn, goto restore_backup; } } else if (backup && !backup->ephemeral) { - if (secretDeleteSaved(secret) < 0) + if (virSecretObjDeleteConfig(secret) < 0) goto restore_backup; + + virSecretObjDeleteData(secret); } /* Saved successfully - drop old values */ new_attrs = NULL; @@ -489,10 +478,11 @@ secretUndefine(virSecretPtr obj) if (virSecretUndefineEnsureACL(obj->conn, secret->def) < 0) goto cleanup;
- if (!secret->def->ephemeral && - secretDeleteSaved(secret) < 0) + if (virSecretObjDeleteConfig(secret) < 0) goto cleanup;
+ virSecretObjDeleteData(secret); + virSecretObjListRemove(driver->secrets, secret);
ret = 0;