10:44 a.m.
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(a)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