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@xxxxxxxxxx>
> ---
> 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;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list