[libvirt] [PATCH 18/30] storagefile: Add externalDataStore member

[Daniel Henrique Barboza]

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