[libvirt] [PATCH 11/10] secret: Introduce virSecretObjDeleteConfig and virSecretObjDeleteData

[Cole Robinson]

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 at 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;
>