On 03/04/14 10:44, Ján Tomko wrote:
> [cc-ing Peter as he invented the function]
>
> On 03/03/2014 09:04 PM, Nehal J Wani wrote:
>>>> ---
>>>> src/conf/snapshot_conf.c | 1 +
>>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>>>> index 12b0930..475525f 100644
>>>> --- a/src/conf/snapshot_conf.c
>>>> +++ b/src/conf/snapshot_conf.c
>>>> @@ -82,6 +82,7 @@ virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
>>>> {
>>>> VIR_FREE(disk->name);
>>>> VIR_FREE(disk->file);
>>>> + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
>>>
>>> This leaves nhosts and hosts at their original values, which seems to be OK
>>> everywhere this function is called, but it might be a problem if someone tries
>>> to reuse the disk definition instead of freeing it.
Maybe we should just note that this is happening in the function and let
our future selves worry about that in case somebody will need to use the
clear function in a different context.
>>>
>>> I'd rather write this as:
>>>
>>> while (disk->nhosts)
>>> virDomainDiskHostDefClear(&disk->hosts[--def->nhosts])
>>> VIR_FREE(disk->hosts)
>>>
>>> Jan
>>>
>>
>> 1. Since virDomainDiskHostDefFree() already calls VIR_FREE(hosts),
>> shouldn't the modified patch just be:
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 12b0930..a233e8e 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -82,6 +82,8 @@
>> virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
>> {
>> VIR_FREE(disk->name);
>> VIR_FREE(disk->file);
>> + virDomainDiskHostDefFree(disk->nhosts, disk->hosts);
>> + disk->nhosts = 0;
>> }
>
> This leaves the pointer to the freed memory in disk->hosts.
>
>>
>> void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
>>
>> 2. I have a question. Why don't we have disk->nhosts = 0; each time
>> someone calls virDomainDiskHostDefFree(disk->nhosts, disk->hosts) in
>> ./src/conf/domain_conf.c, ./src/storage/storage_driver.c and
>> ./src/qemu/qemu_conf.c?
>
> In most cases either the whole def gets freed, or hosts and nhosts are
> overwritten. I'm not sure if we overwrite it in all cases in
> qemuTranslateDiskSourcePool or it needs to be added there too.
>
>> Or we can change the definition of virDomainDiskHostDefFree to
>> virDomainDiskHostDefFree(size_t *nhosts, virDomainDiskHostDefPtr hosts)
>> and make *nhosts = 0 inside the function itself and make the necessary
>> changes wherever this function is called?
>>
>
> That looks more awkward than the original function to me.
Seconded. Additionally it would require a macro as calling the function
with reference operators would be even more ugly.
>
> I think it should be called virDomainDiskHostDefsFree instead, since it's
> different from all the other DefFree functions, which only take one DefPtr
> argument. The callers should make sure that the leftover values in
> nhosts/hosts aren't reused.
>
Hmmm I must agree that my choice of name for this function wasn't ideal.
It does indeed free more entries, so that the plural form would be a
better choice.
> Jan
>
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list