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 <jferlan@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 matter since we're freeing def anyway. If you want it - I can put it there. FWIW: I was looking at the ->encryption code as a model... At least in the domain_conf code... John
virStorageSourceBackingStoreClear(def); }
Jan