Re: [PATCH] Fix memory leak in virDomainSnapshotDiskDefClear()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>> ---
>>  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.
>
> 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;
 }

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

-- 
Nehal J Wani

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]