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

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

[John Ferlan]

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 
---
 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. */
+if (!secret->def->ephemeral &&
+unlink(secret->configFile) < 0 && errno != ENOENT)
+return -1;
+
+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;
-- 
2.5.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list