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

[Michal Privoznik]

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.

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