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

[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 <jferlan at 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
> 
>