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

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