Re: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk

2014-07-03 Thread Ján Tomko 
On 07/02/2014 05:44 PM, John Ferlan wrote:
 On 07/02/2014 09:10 AM, Ján Tomko wrote:
 On 06/27/2014 05:11 PM, John Ferlan wrote:
 @@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
  VIR_FREE(def-timestamps);
  
  virStorageNetHostDefFree(def-nhosts, def-hosts);
 -virStorageSourceAuthClear(def);
 +virStorageAuthDefFree(def-auth);

 I don't like *Clear functions leaving pointers to freed memory behind, but
 this one is only called right before freeing the StorageSource and it already
 leaves def-hosts.

 
 I think you are asking for a 'def-auth = NULL;' right?

Yes.

 Similarly a 'def-hosts = NULL;' Of course it doesn't matter
 since we're freeing def anyway.  If you want it - I can put
 it there.

I think that's better left for a separate cleanup. I'll make a note on my TODO
list :)

Jan



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk

2014-07-02 Thread Michal Privoznik 

On 27.06.2014 17:11, John Ferlan wrote:

Replace the inline auth struct in virStorageSource with a pointer
to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
and qemu_command sources for finding the auth data for a domain disk

Signed-off-by: John Ferlan jfer...@redhat.com
---
  src/conf/domain_conf.c| 106 +++---
  src/libvirt_private.syms  |   1 -
  src/qemu/qemu_command.c   |  72 +--
  src/qemu/qemu_conf.c  |  26 +++-
  src/util/virstoragefile.c |  14 +-
  src/util/virstoragefile.h |  10 +
  tests/qemuargv2xmltest.c  |   1 -
  7 files changed, 81 insertions(+), 149 deletions(-)




diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 63f322a..6664547 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -45,6 +45,7 @@
  #include domain_conf.h
  #include snapshot_conf.h
  #include storage_conf.h
+#include secret_conf.h
  #include network/bridge_driver.h
  #include virnetdevtap.h
  #include base64.h
@@ -2469,9 +2470,7 @@ static char *
  qemuGetSecretString(virConnectPtr conn,
  const char *scheme,
  bool encoded,
-int diskSecretType,
-char *username,
-unsigned char *uuid, char *usage,
+virStorageAuthDefPtr authdef,
  virSecretUsageType secretUsageType)
  {
  size_t secret_size;
@@ -2480,25 +2479,26 @@ qemuGetSecretString(virConnectPtr conn,
  char uuidStr[VIR_UUID_STRING_BUFLEN];

  /* look up secret */
-switch (diskSecretType) {
+switch (authdef-secretType) {


Ahh, this answers my question in 1/5. Okay, leave the structure public.

Michal

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

Re: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk

2014-07-02 Thread Ján Tomko 
On 06/27/2014 05:11 PM, John Ferlan wrote:
 Replace the inline auth struct in virStorageSource with a pointer
 to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
 and qemu_command sources for finding the auth data for a domain disk
 
 Signed-off-by: John Ferlan jfer...@redhat.com
 ---
  src/conf/domain_conf.c| 106 
 +++---
  src/libvirt_private.syms  |   1 -
  src/qemu/qemu_command.c   |  72 +--
  src/qemu/qemu_conf.c  |  26 +++-
  src/util/virstoragefile.c |  14 +-
  src/util/virstoragefile.h |  10 +
  tests/qemuargv2xmltest.c  |   1 -
  7 files changed, 81 insertions(+), 149 deletions(-)
 

 @@ -2650,6 +2663,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
  
   error:
  VIR_FREE(options);
 +virStorageAuthDefFree(disk-src-auth);

This causes a double free - both callers free disk on failure.

  return -1;
  }
  

 @@ -2738,6 +2767,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, 
 virURIPtr uri,
   error:
  virStorageNetHostDefClear(def-src-hosts);
  VIR_FREE(def-src-hosts);

 +VIR_FREE(def-src-auth);

This should be freed by the callers too. (by StorageAuthDefFree)

  goto cleanup;
  }
  

 @@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
  VIR_FREE(def-timestamps);
  
  virStorageNetHostDefFree(def-nhosts, def-hosts);
 -virStorageSourceAuthClear(def);
 +virStorageAuthDefFree(def-auth);

I don't like *Clear functions leaving pointers to freed memory behind, but
this one is only called right before freeing the StorageSource and it already
leaves def-hosts.

  
  virStorageSourceBackingStoreClear(def);
  }

Jan




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk

2014-07-02 Thread John Ferlan 


On 07/02/2014 09:10 AM, Ján Tomko wrote:
 On 06/27/2014 05:11 PM, John Ferlan wrote:
 Replace the inline auth struct in virStorageSource with a pointer
 to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
 and qemu_command sources for finding the auth data for a domain disk

 Signed-off-by: John Ferlan jfer...@redhat.com
 ---
  src/conf/domain_conf.c| 106 
 +++---
  src/libvirt_private.syms  |   1 -
  src/qemu/qemu_command.c   |  72 +--
  src/qemu/qemu_conf.c  |  26 +++-
  src/util/virstoragefile.c |  14 +-
  src/util/virstoragefile.h |  10 +
  tests/qemuargv2xmltest.c  |   1 -
  7 files changed, 81 insertions(+), 149 deletions(-)

 
 @@ -2650,6 +2663,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
  
   error:
  VIR_FREE(options);
 +virStorageAuthDefFree(disk-src-auth);
 
 This causes a double free - both callers free disk on failure.
 

Oh right - the need to either sent disk-src-auth = NULL or
follow what was done in domain_conf - that is:

@@ -2590,6 +2590,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
 {
 char *options = NULL;
 char *p, *e, *next;
+virStorageAuthDefPtr authdef = NULL;
 
 p = strchr(disk-src-path, ':');
 if (p) {
@@ -2621,15 +2622,17 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
 
 if (STRPREFIX(p, id=)) {
 const char *secrettype;
-/* formulate a disk-src-auth */
-if (VIR_ALLOC(disk-src-auth)  0)
+/* formulate authdef for disk-src-auth */
+if (VIR_ALLOC(authdef)  0)
 goto error;
 
-if (VIR_STRDUP(disk-src-auth-username, p + strlen(id=))  0)
+if (VIR_STRDUP(authdef-username, p + strlen(id=))  0)
 goto error;
 secrettype = virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)
-if (VIR_STRDUP(disk-src-auth-secrettype, secrettype)  0)
+if (VIR_STRDUP(authdef-secrettype, secrettype)  0)
 goto error;
+disk-src-auth = authdef;
+authdef = NULL;
 
 /* Cannot formulate a secretType (eg, usage or uuid) given
  * what is provided.
@@ -2663,7 +2666,7 @@ static int qemuParseRBDString(virDomainDiskDefPtr disk)
 
  error:
 VIR_FREE(options);
-virStorageAuthDefFree(disk-src-auth);
+virStorageAuthDefFree(authdef);
 return -1;
 }


  return -1;
  }
  
 
 @@ -2738,6 +2767,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, 
 virURIPtr uri,
   error:
  virStorageNetHostDefClear(def-src-hosts);
  VIR_FREE(def-src-hosts);
 
 +VIR_FREE(def-src-auth);
 
 This should be freed by the callers too. (by StorageAuthDefFree)
 

Hmm.. what was I thinking - even Coverity didn't catch this one...  
Similar to RBD:

@@ -2676,6 +2679,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr
 char *sock = NULL;
 char *volimg = NULL;
 char *secret = NULL;
+virStorageAuthDefPtr authdef = NULL;
 
 if (VIR_ALLOC(def-src-hosts)  0)
 goto error;
@@ -2734,22 +2738,24 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIP
 
 if (uri-user) {
 const char *secrettype;
-/* formulate a def-src-auth */
-if (VIR_ALLOC(def-src-auth)  0)
+/* formulate authdef for disk-src-auth */
+if (VIR_ALLOC(authdef)  0)
 goto error;
 
 secret = strchr(uri-user, ':');
 if (secret)
 *secret = '\0';
 
-if (VIR_STRDUP(def-src-auth-username, uri-user)  0)
+if (VIR_STRDUP(authdef-username, uri-user)  0)
 goto error;
 if (STREQ(scheme, iscsi)) {
 secrettype =
 virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_ISCSI);
-if (VIR_STRDUP(def-src-auth-secrettype, secrettype)  0)
+if (VIR_STRDUP(authdef-secrettype, secrettype)  0)
 goto error;
 }
+def-src-auth = authdef;
+authdef = NULL;
 
 /* Cannot formulate a secretType (eg, usage or uuid) given
  * what is provided.
@@ -2767,7 +2773,7 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr
  error:
 virStorageNetHostDefClear(def-src-hosts);
 VIR_FREE(def-src-hosts);
-VIR_FREE(def-src-auth);
+virStorageAuthDefFree(authdef);
 goto cleanup;
 }


  goto cleanup;
  }
  
 
 @@ -1802,7 +1790,7 @@ virStorageSourceClear(virStorageSourcePtr def)
  VIR_FREE(def-timestamps);
  
  virStorageNetHostDefFree(def-nhosts, def-hosts);
 -virStorageSourceAuthClear(def);
 +virStorageAuthDefFree(def-auth);
 
 I don't like *Clear functions leaving pointers to freed memory behind, but
 this one is only called right before freeing the StorageSource and it already
 leaves def-hosts.
 

I think you are asking for a 'def-auth = NULL;' right?
Similarly a 'def-hosts = NULL;' Of course it doesn't

[libvirt] [PATCH 3/5] Utilize virDomainDiskAuth for domain disk

2014-06-27 Thread John Ferlan 
Replace the inline auth struct in virStorageSource with a pointer
to a virStorageAuthDefPtr and utilize between the domain_conf, qemu_conf,
and qemu_command sources for finding the auth data for a domain disk

Signed-off-by: John Ferlan jfer...@redhat.com
---
 src/conf/domain_conf.c| 106 +++---
 src/libvirt_private.syms  |   1 -
 src/qemu/qemu_command.c   |  72 +--
 src/qemu/qemu_conf.c  |  26 +++-
 src/util/virstoragefile.c |  14 +-
 src/util/virstoragefile.h |  10 +
 tests/qemuargv2xmltest.c  |   1 -
 7 files changed, 81 insertions(+), 149 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b7aa4f5..93f09a7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5203,7 +5203,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 {
 virDomainDiskDefPtr def;
 xmlNodePtr sourceNode = NULL;
-xmlNodePtr cur, child;
+xmlNodePtr cur;
 xmlNodePtr save_ctxt = ctxt-node;
 char *type = NULL;
 char *device = NULL;
@@ -5227,10 +5227,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 virStorageEncryptionPtr encryption = NULL;
 char *serial = NULL;
 char *startupPolicy = NULL;
-char *authUsername = NULL;
-char *authUsage = NULL;
-char *authUUID = NULL;
-char *usageType = NULL;
+virStorageAuthDefPtr authdef = NULL;
 char *tray = NULL;
 char *removable = NULL;
 char *logical_block_size = NULL;
@@ -5432,65 +5429,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 VIR_FREE(ready);
 }
 } else if (xmlStrEqual(cur-name, BAD_CAST auth)) {
-authUsername = virXMLPropString(cur, username);
-if (authUsername == NULL) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
-   _(missing username for auth));
+if (!(authdef = virStorageAuthDefParse(node-doc, cur)))
+goto error;
+if ((auth_secret_usage =
+ virSecretUsageTypeFromString(authdef-secrettype))  0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _(invalid secret type %s),
+   authdef-secrettype);
 goto error;
-}
-
-def-src-auth.secretType = VIR_STORAGE_SECRET_TYPE_NONE;
-child = cur-children;
-while (child != NULL) {
-if (child-type == XML_ELEMENT_NODE 
-xmlStrEqual(child-name, BAD_CAST secret)) {
-usageType = virXMLPropString(child, type);
-if (usageType == NULL) {
-virReportError(VIR_ERR_XML_ERROR, %s,
-   _(missing type for secret));
-goto error;
-}
-auth_secret_usage =
-virSecretUsageTypeFromString(usageType);
-if (auth_secret_usage  0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _(invalid secret type %s),
-   usageType);
-goto error;
-}
-
-authUUID = virXMLPropString(child, uuid);
-authUsage = virXMLPropString(child, usage);
-
-if (authUUID != NULL  authUsage != NULL) {
-virReportError(VIR_ERR_XML_ERROR, %s,
-   _(only one of uuid and usage can 
be specified));
-goto error;
-}
-
-if (!authUUID  !authUsage) {
-virReportError(VIR_ERR_XML_ERROR, %s,
-   _(either uuid or usage should be 
- specified for a secret));
-goto error;
-}
-
-if (authUUID != NULL) {
-def-src-auth.secretType = 
VIR_STORAGE_SECRET_TYPE_UUID;
-if (virUUIDParse(authUUID,
- def-src-auth.secret.uuid)  0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _(malformed uuid %s),
-   authUUID);
-goto error;
-}
-} else if (authUsage != NULL) {
-def-src-auth.secretType = 
VIR_STORAGE_SECRET_TYPE_USAGE;
-def-src-auth.secret.usage =