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