12:56 a.m.
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
---
src/libvirt-domain.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 04545fd..7f8a7ce 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)
for (i = 0; i < info->ndevAlias; i++)
VIR_FREE(info->devAlias[i]);
VIR_FREE(info->devAlias);
+
+ VIR_FREE(info);
}
--
1.9.3
4:32 a.m.
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
---
src/libvirt-domain.c | 2 ++
1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's
documented in virDomainGetFSInfo and virDomainFSInfoFree:
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already
work around this function by freeing the info, this change would
introduce a double free.
I would NACK this if the documentation for both APIs didn't say that's
how this function should behave.
I'd like to hear a second opinion.
Jan
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 04545fd..7f8a7ce 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)
for (i = 0; i < info->ndevAlias; i++)
VIR_FREE(info->devAlias[i]);
VIR_FREE(info->devAlias);
+
+ VIR_FREE(info);
}
--
1.9.3
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:43 a.m.
On 03/10/2015 05:32 PM, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
> in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
>
> ==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
> ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
> ==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
> ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
> ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
>
> Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
> ---
> src/libvirt-domain.c | 2 ++
> 1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's
documented in virDomainGetFSInfo and virDomainFSInfoFree:
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already
work around this function by freeing the info, this change would
introduce a double free.
I would NACK this if the documentation for both APIs didn't say that's
how this function should behave.
I'd like to hear a second opinion.
I don't think this documentation make
any confusable.
for using the function virDomainGetFSInfo(), user also need
to call virDomainFSInfoFree() on each array element, and call free() info.
example:
virDomainFSInfoPtr *info;
ndata = virDomainGetFSInfo(dom, &info, 0);
for (i = 0; i < ndata; i++)
virDomainFSInfoFree(info[i]);
VIR_FREE(info);
Thanks,
Chen
Jan
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 04545fd..7f8a7ce 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)
> for (i = 0; i < info->ndevAlias; i++)
> VIR_FREE(info->devAlias[i]);
> VIR_FREE(info->devAlias);
> +
> + VIR_FREE(info);
> }
> --
> 1.9.3
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
5:38 a.m.
On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
> in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
>
> ==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
> ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
> ==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
> ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
> ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
>
> Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
> ---
> src/libvirt-domain.c | 2 ++
> 1 file changed, 2 insertions(+)
This does fix the memory leak and makes the function behave like it's
documented in virDomainGetFSInfo and virDomainFSInfoFree:
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
But it changes the public API - if there are applications that already
work around this function by freeing the info, this change would
introduce a double free.
Well, this is a good point that we might break somebody's application, but the
virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on
passed argument. In the documentation the @info is an array and we specifically
says that calling virDomainFSInfoFree on each element and then just free the
@info is sufficient, but it isn't and there will be a memory leak. It changes
behavior of public API to correctly free the allocated memory as documentation
says. Let's hear another opinion, but I'm for fixing it.
Pavel
I would NACK this if the documentation for both APIs didn't say that's
how this function should behave.
I'd like to hear a second opinion.
Jan
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 04545fd..7f8a7ce 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11337,4 +11337,6 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)
> for (i = 0; i < info->ndevAlias; i++)
> VIR_FREE(info->devAlias[i]);
> VIR_FREE(info->devAlias);
> +
> + VIR_FREE(info);
> }
> --
> 1.9.3
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:46 a.m.
On Tue, Mar 10, 2015 at 11:38:52AM +0100, Pavel Hrdina wrote:
On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
> On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
> > in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
> >
> > ==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of 793
> > ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
> > ==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
> > ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
> > ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
> >
> > Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
> > ---
> > src/libvirt-domain.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> This does fix the memory leak and makes the function behave like it's
> documented in virDomainGetFSInfo and virDomainFSInfoFree:
> http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo
> http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
>
> But it changes the public API - if there are applications that already
> work around this function by freeing the info, this change would
> introduce a double free.
Well, this is a good point that we might break somebody's application, but the
virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on
passed argument. In the documentation the @info is an array and we specifically
says that calling virDomainFSInfoFree on each element and then just free the
@info is sufficient, but it isn't and there will be a memory leak. It changes
behavior of public API to correctly free the allocated memory as documentation
says. Let's hear another opinion, but I'm for fixing it.
I agree, in the normal way of usage of this API, this will always be wrong.
virsh is leaking this, python bindings are leaking this, perl bindings are
leaking this. Only applications written in C have the opportunity to do a
workaround, and most of our apps are not written in C. I think we have to
fix this really.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:37 a.m.
On Tue, Mar 10, 2015 at 10:46:45AM +0000, Daniel P. Berrange wrote:
On Tue, Mar 10, 2015 at 11:38:52AM +0100, Pavel Hrdina wrote:
> On Tue, Mar 10, 2015 at 10:32:04AM +0100, Ján Tomko wrote:
> > On Tue, Mar 10, 2015 at 01:56:11PM +0800, Chen Fan wrote:
> > > in virDomainFSInfoFree(), don't free the virDomainFSInfo data.
> > >
> > > ==10670== 80 bytes in 2 blocks are definitely lost in loss record 576 of
793
> > > ==10670== at 0x4A06BC3: calloc (vg_replace_malloc.c:618)
> > > ==10670== by 0x509DEBD: virAlloc (viralloc.c:144)
> > > ==10670== by 0x19FBD558: qemuAgentGetFSInfo (qemu_agent.c:1837)
> > > ==10670== by 0x1A03CF91: qemuDomainGetFSInfo (qemu_driver.c:19238)
> > >
> > > Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com>
> > > ---
> > > src/libvirt-domain.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> >
> > This does fix the memory leak and makes the function behave like it's
> > documented in virDomainGetFSInfo and virDomainFSInfoFree:
> > http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetFSInfo
> > http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainFSInfoFree
> >
> > But it changes the public API - if there are applications that already
> > work around this function by freeing the info, this change would
> > introduce a double free.
>
> Well, this is a good point that we might break somebody's application, but the
> virDomainFSInfoFree is the only one *Free API that doesn't call free/unref on
> passed argument. In the documentation the @info is an array and we specifically
> says that calling virDomainFSInfoFree on each element and then just free the
> @info is sufficient, but it isn't and there will be a memory leak. It changes
> behavior of public API to correctly free the allocated memory as documentation
> says. Let's hear another opinion, but I'm for fixing it.
I agree, in the normal way of usage of this API, this will always be wrong.
virsh is leaking this, python bindings are leaking this, perl bindings are
leaking this. Only applications written in C have the opportunity to do a
workaround, and most of our apps are not written in C. I think we have to
fix this really.
Now pushed.
Jan
3551
days inactive
3551
days old
5 comments
4 participants
participants (4)
-
Chen Fan -
Daniel P. Berrange -
Ján Tomko -
Pavel Hrdina