Re: [libvirt] [PATCH v2 05/10] qemu: don't pass virConnectPtr around for secrets

2018-02-15 Thread John Ferlan 


On 02/15/2018 11:50 AM, Daniel P. Berrangé wrote:
> During domain startup there are many places where we need to acquire
> secrets. Currently code passes around a virConnectPtr, except in the
> places where we pass in NULL. So there are a few codepaths where ability
> to start guests using secrets will fail. Change to acquire a handle to
> the secret driver when needed.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/qemu/qemu_domain.c| 111 
> ++
>  src/qemu/qemu_domain.h|  15 +++
>  src/qemu/qemu_driver.c|  18 
>  src/qemu/qemu_hotplug.c   |  64 +++---
>  src/qemu/qemu_hotplug.h   |  15 +++
>  src/qemu/qemu_migration.c |  10 ++---
>  src/qemu/qemu_process.c   |  40 +
>  tests/qemuhotplugtest.c   |   4 +-
>  8 files changed, 114 insertions(+), 163 deletions(-)
> 

[...]
I tried a Coverity build...

> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index f3ec5d8042..62d84a7364 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -836,24 +836,21 @@ bool 
> qemuDomainDiskHasEncryptionSecret(virStorageSourcePtr src)
>  ATTRIBUTE_NONNULL(1);
>  
>  qemuDomainSecretInfoPtr
> -qemuDomainSecretInfoTLSNew(virConnectPtr conn,
> -   qemuDomainObjPrivatePtr priv,
> +qemuDomainSecretInfoTLSNew(qemuDomainObjPrivatePtr priv,
> const char *srcAlias,
> const char *secretUUID);
>  
>  void qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr disk)
>  ATTRIBUTE_NONNULL(1);
>  
> -int qemuDomainSecretHostdevPrepare(virConnectPtr conn,
> -   qemuDomainObjPrivatePtr priv,
> +int qemuDomainSecretHostdevPrepare(qemuDomainObjPrivatePtr priv,
> virDomainHostdevDefPtr hostdev)
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);

Need to adjust ATTRIBUTE_NONNULL

>  
>  void qemuDomainSecretChardevDestroy(virDomainChrSourceDefPtr dev)
>  ATTRIBUTE_NONNULL(1);
>  
> -int qemuDomainSecretChardevPrepare(virConnectPtr conn,
> -   virQEMUDriverConfigPtr cfg,
> +int qemuDomainSecretChardevPrepare(virQEMUDriverConfigPtr cfg,
> qemuDomainObjPrivatePtr priv,
> const char *chrAlias,
> virDomainChrSourceDefPtr dev)

ATTRIBUTE_NONNULL

> @@ -863,8 +860,7 @@ int qemuDomainSecretChardevPrepare(virConnectPtr conn,
>  void qemuDomainSecretDestroy(virDomainObjPtr vm)
>  ATTRIBUTE_NONNULL(1);
>  
> -int qemuDomainSecretPrepare(virConnectPtr conn,
> -virQEMUDriverPtr driver,
> +int qemuDomainSecretPrepare(virQEMUDriverPtr driver,
>  virDomainObjPtr vm)
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);

ATTRIBUTE_NONNULL

>  
> @@ -1000,8 +996,7 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr 
> driver,
>   qemuDomainAsyncJob asyncJob);
>  
>  int
> -qemuDomainPrepareDiskSource(virConnectPtr conn,
> -virDomainDiskDefPtr disk,
> +qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
>  qemuDomainObjPrivatePtr priv,
>  virQEMUDriverConfigPtr cfg);
>  

[...]

Reviewed-by: John Ferlan 

John

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

[libvirt] [PATCH v2 05/10] qemu: don't pass virConnectPtr around for secrets

2018-02-15 Thread Daniel P . Berrangé 
During domain startup there are many places where we need to acquire
secrets. Currently code passes around a virConnectPtr, except in the
places where we pass in NULL. So there are a few codepaths where ability
to start guests using secrets will fail. Change to acquire a handle to
the secret driver when needed.

Signed-off-by: Daniel P. Berrangé 
---
 src/qemu/qemu_domain.c| 111 ++
 src/qemu/qemu_domain.h|  15 +++
 src/qemu/qemu_driver.c|  18 
 src/qemu/qemu_hotplug.c   |  64 +++---
 src/qemu/qemu_hotplug.h   |  15 +++
 src/qemu/qemu_migration.c |  10 ++---
 src/qemu/qemu_process.c   |  40 +
 tests/qemuhotplugtest.c   |   4 +-
 8 files changed, 114 insertions(+), 163 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 178ec24ae7..2182b7927d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1151,7 +1151,6 @@ qemuDomainChrSourcePrivateDispose(void *obj)
 
 
 /* qemuDomainSecretPlainSetup:
- * @conn: Pointer to connection
  * @secinfo: Pointer to secret info
  * @usageType: The virSecretUsageType
  * @username: username to use for authentication (may be NULL)
@@ -1162,24 +1161,33 @@ qemuDomainChrSourcePrivateDispose(void *obj)
  * Returns 0 on success, -1 on failure with error message
  */
 static int
-qemuDomainSecretPlainSetup(virConnectPtr conn,
-   qemuDomainSecretInfoPtr secinfo,
+qemuDomainSecretPlainSetup(qemuDomainSecretInfoPtr secinfo,
virSecretUsageType usageType,
const char *username,
virSecretLookupTypeDefPtr seclookupdef)
 {
+virConnectPtr conn;
+int ret = -1;
+
+conn = virGetConnectSecret();
+if (!conn)
+return -1;
+
 secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN;
 if (VIR_STRDUP(secinfo->s.plain.username, username) < 0)
-return -1;
+goto cleanup;
 
-return virSecretGetSecretString(conn, seclookupdef, usageType,
->s.plain.secret,
->s.plain.secretlen);
+ret = virSecretGetSecretString(conn, seclookupdef, usageType,
+   >s.plain.secret,
+   >s.plain.secretlen);
+
+ cleanup:
+virObjectUnref(conn);
+return ret;
 }
 
 
 /* qemuDomainSecretAESSetup:
- * @conn: Pointer to connection
  * @priv: pointer to domain private object
  * @secinfo: Pointer to secret info
  * @srcalias: Alias of the disk/hostdev used to generate the secret alias
@@ -1193,8 +1201,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
  * Returns 0 on success, -1 on failure with error message
  */
 static int
-qemuDomainSecretAESSetup(virConnectPtr conn,
- qemuDomainObjPrivatePtr priv,
+qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv,
  qemuDomainSecretInfoPtr secinfo,
  const char *srcalias,
  virSecretUsageType usageType,
@@ -1202,6 +1209,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn,
  virSecretLookupTypeDefPtr seclookupdef,
  bool isLuks)
 {
+virConnectPtr conn;
 int ret = -1;
 uint8_t *raw_iv = NULL;
 size_t ivlen = QEMU_DOMAIN_AES_IV_LEN;
@@ -1210,16 +1218,20 @@ qemuDomainSecretAESSetup(virConnectPtr conn,
 uint8_t *ciphertext = NULL;
 size_t ciphertextlen = 0;
 
+conn = virGetConnectSecret();
+if (!conn)
+return -1;
+
 secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_AES;
 if (VIR_STRDUP(secinfo->s.aes.username, username) < 0)
-return -1;
+goto cleanup;
 
 if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, 
isLuks)))
-return -1;
+goto cleanup;
 
 /* Create a random initialization vector */
 if (!(raw_iv = virCryptoGenerateRandom(ivlen)))
-return -1;
+goto cleanup;
 
 /* Encode the IV and save that since qemu will need it */
 if (!(secinfo->s.aes.iv = virStringEncodeBase64(raw_iv, ivlen)))
@@ -1250,13 +1262,12 @@ qemuDomainSecretAESSetup(virConnectPtr conn,
 VIR_DISPOSE_N(raw_iv, ivlen);
 VIR_DISPOSE_N(secret, secretlen);
 VIR_DISPOSE_N(ciphertext, ciphertextlen);
-
+virObjectUnref(conn);
 return ret;
 }
 
 
 /* qemuDomainSecretSetup:
- * @conn: Pointer to connection
  * @priv: pointer to domain private object
  * @secinfo: Pointer to secret info
  * @srcalias: Alias of the disk/hostdev used to generate the secret alias
@@ -1273,8 +1284,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn,
  * Returns 0 on success, -1 on failure
  */
 static int
-qemuDomainSecretSetup(virConnectPtr conn,
-  qemuDomainObjPrivatePtr priv,
+qemuDomainSecretSetup(qemuDomainObjPrivatePtr priv,
   qemuDomainSecretInfoPtr secinfo,