diff mbox series

[18/30] storagefile: Add externalDataStore member

Message ID cabbdc4446082cadf83c990995a93f986b1a3985.1570482718.git.crobinso@redhat.com
State Accepted
Commit 884cc9d615f2ab9294e19c047a35fe1775dad955
Headers show
Series storagefile, security: qcow2 data_file support | expand

Commit Message

Cole Robinson Oct. 7, 2019, 9:49 p.m. UTC 
Add the plumbing to track a externalDataStoreRaw as a virStorageSource

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/util/virstoragefile.c | 9 +++++++++
 src/util/virstoragefile.h | 3 +++
 2 files changed, 12 insertions(+)

-- 
2.23.0

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

Comments

Daniel Henrique Barboza Oct. 10, 2019, 3:09 p.m. UTC | #1 
On 10/7/19 6:49 PM, Cole Robinson wrote:
> Add the plumbing to track a externalDataStoreRaw as a virStorageSource

>

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>   src/util/virstoragefile.c | 9 +++++++++

>   src/util/virstoragefile.h | 3 +++

>   2 files changed, 12 insertions(+)

>

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c

> index 7ae6719dd6..ce669b6e0b 100644

> --- a/src/util/virstoragefile.c

> +++ b/src/util/virstoragefile.c

> @@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,

>               return NULL;

>       }

>   

> +    if (src->externalDataStore) {

> +        if (!(def->externalDataStore = virStorageSourceCopy(src->externalDataStore,

> +                                                            true)))

> +            return NULL;

> +    }

> +

>       VIR_STEAL_PTR(ret, def);

>       return ret;

>   }

> @@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)

>       VIR_FREE(def->timestamps);

>       VIR_FREE(def->externalDataStoreRaw);

>   

> +    virObjectUnref(def->externalDataStore);

> +    def->externalDataStore = NULL;


Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
def->externalDataStore if there is no more references to it (which will
assign it to NULL in the end).


LGTM otherwise


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>





> +

>       virStorageNetHostDefFree(def->nhosts, def->hosts);

>       virStorageAuthDefFree(def->auth);

>       virObjectUnref(def->privateData);

> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h

> index bbff511657..d84dad052d 100644

> --- a/src/util/virstoragefile.h

> +++ b/src/util/virstoragefile.h

> @@ -292,6 +292,9 @@ struct _virStorageSource {

>       /* backing chain of the storage source */

>       virStorageSourcePtr backingStore;

>   

> +    /* external data store storage source */

> +    virStorageSourcePtr externalDataStore;

> +

>       /* metadata for storage driver access to remote and local volumes */

>       virStorageDriverDataPtr drv;

>   


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník Oct. 11, 2019, 1:04 p.m. UTC | #2 
On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 10/7/19 6:49 PM, Cole Robinson wrote:
>> Add the plumbing to track a externalDataStoreRaw as a virStorageSource
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>   src/util/virstoragefile.c | 9 +++++++++
>>   src/util/virstoragefile.h | 3 +++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 7ae6719dd6..ce669b6e0b 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,
>>               return NULL;
>>       }
>> +    if (src->externalDataStore) {
>> +        if (!(def->externalDataStore = 
>> virStorageSourceCopy(src->externalDataStore,
>> +                                                            true)))
>> +            return NULL;
>> +    }
>> +
>>       VIR_STEAL_PTR(ret, def);
>>       return ret;
>>   }
>> @@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
>>       VIR_FREE(def->timestamps);
>>       VIR_FREE(def->externalDataStoreRaw);
>> +    virObjectUnref(def->externalDataStore);
>> +    def->externalDataStore = NULL;
> 
> Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
> def->externalDataStore if there is no more references to it (which will
> assign it to NULL in the end).

It is. Point of virXXXClear() functions is to clear passed struct. That 
is, in theory (I'm not saying it's the case of virStorageSourceDef and 
also I'm not saying it isn't), it should be possible to re-use a 
structure after it's been cleared. But for that to happen, all poitners 
must be reset to NULL regardless of refcounters and stuff.

However, there's a memset() called at the end of this function which 
sets all members of this struct to 0, so in the end I agree that the 
line is redundant, but for a different reason :-D

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Daniel Henrique Barboza Oct. 11, 2019, 2:27 p.m. UTC | #3 
On 10/11/19 10:04 AM, Michal Privoznik wrote:
> On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 10/7/19 6:49 PM, Cole Robinson wrote:
>>> Add the plumbing to track a externalDataStoreRaw as a virStorageSource
>>>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>>   src/util/virstoragefile.c | 9 +++++++++
>>>   src/util/virstoragefile.h | 3 +++
>>>   2 files changed, 12 insertions(+)
>>>
>>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>>> index 7ae6719dd6..ce669b6e0b 100644
>>> --- a/src/util/virstoragefile.c
>>> +++ b/src/util/virstoragefile.c
>>> @@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource 
>>> *src,
>>>               return NULL;
>>>       }
>>> +    if (src->externalDataStore) {
>>> +        if (!(def->externalDataStore = 
>>> virStorageSourceCopy(src->externalDataStore,
>>> + true)))
>>> +            return NULL;
>>> +    }
>>> +
>>>       VIR_STEAL_PTR(ret, def);
>>>       return ret;
>>>   }
>>> @@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
>>>       VIR_FREE(def->timestamps);
>>>       VIR_FREE(def->externalDataStoreRaw);
>>> +    virObjectUnref(def->externalDataStore);
>>> +    def->externalDataStore = NULL;
>>
>> Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
>> def->externalDataStore if there is no more references to it (which will
>> assign it to NULL in the end).
>
> It is. Point of virXXXClear() functions is to clear passed struct. 
> That is, in theory (I'm not saying it's the case of 
> virStorageSourceDef and also I'm not saying it isn't), it should be 
> possible to re-use a structure after it's been cleared. But for that 
> to happen, all poitners must be reset to NULL regardless of 
> refcounters and stuff.
>

Got it. I didn't get across an example of this re-use yet, didn't know 
it is a
valid possibility.

> However, there's a memset() called at the end of this function which 
> sets all members of this struct to 0, so in the end I agree that the 
> line is redundant, but for a different reason :-D
>


I was right in the end! That all I'm going to remember ;)


DHB


> Michal

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

Patch

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@  virStorageSourceCopy(const virStorageSource *src,
             return NULL;
     }
 
+    if (src->externalDataStore) {
+        if (!(def->externalDataStore = virStorageSourceCopy(src->externalDataStore,
+                                                            true)))
+            return NULL;
+    }
+
     VIR_STEAL_PTR(ret, def);
     return ret;
 }
@@ -2560,6 +2566,9 @@  virStorageSourceClear(virStorageSourcePtr def)
     VIR_FREE(def->timestamps);
     VIR_FREE(def->externalDataStoreRaw);
 
+    virObjectUnref(def->externalDataStore);
+    def->externalDataStore = NULL;
+
     virStorageNetHostDefFree(def->nhosts, def->hosts);
     virStorageAuthDefFree(def->auth);
     virObjectUnref(def->privateData);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index bbff511657..d84dad052d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -292,6 +292,9 @@  struct _virStorageSource {
     /* backing chain of the storage source */
     virStorageSourcePtr backingStore;
 
+    /* external data store storage source */
+    virStorageSourcePtr externalDataStore;
+
     /* metadata for storage driver access to remote and local volumes */
     virStorageDriverDataPtr drv;