Re: [Libguestfs] virt-v2v valgrind errors in libosinfo
On Tue, Apr 14, 2020 at 05:42:13PM +0200, Pino Toscano wrote:
On Tuesday, 14 April 2020 15:16:49 CEST Richard W.M. Jones wrote:
> On Tue, Apr 14, 2020 at 12:37:07PM +0200, Pino Toscano wrote:
> > > Unfortunately we never free the database.
> >
> > Hm it is never freed? Wouldn't that result in actual leaks, since
> > OsinfoDb_t_finalize (g_object_unref'ing the OsinfoDb) wouldn't be
> > called?
>
> I was thinking because of this:
>
>
https://github.com/libguestfs/virt-v2v/blob/cc294b7735dda467179b93a061d36...
>
> which IIUC will allocate a DB (on first access) but it is never
> released.
Oh this is interesting. I read in the documentation that custom blocks
have tag Custom_tag, which is higher than No_scan_tag, and thus they
aren't scanned by the GC. Indeed, even trying to Gc.finalize on the
object inside the lazy seems to have no effect.
So this is correct about custom blocks, but you can still attach a
C finalizer and it will run if the object is finalized. However the
problem is this object cannot be finalized because of (as you say
below) db being a top-level entry.
As an aside, C-level finalizers and Gc.finalize work quite a bit
differently (I didn't check, but I believe they must use different
paths in the garbage collector).
I guess this is becase db is a top-level variable in
Libosinfo_utils;
is there a way to change that behaviour somehow?
I can't think of a way to make that happen, and to have the db being
created at most once in the program (which is what you're achieving
with the top level / lazy construction). I added a suppression for
the db already:
https://github.com/libguestfs/virt-v2v/commit/8e870da79b5a61513f568b0b81c...
> > > It could be that to express
> > > this properly we'd need to expose (db, os) tuples to the OCaml
garbage
> > > collector.
> >
> > I thought about this, and according to knowledge this would be needed
> > only if we want osinfo_os objects alive even when the osinfo_db gets
> > "out of scope". Considering that the osinfo_db is kept basically
> > statically this should be fine.
>
> Right.
>
> I don't believe the current code is wrong, my concern is more about
> clearing up valgrind errors before the release.
Yup, it makes sense to do that.
I'll add further suppressions for the other valgrind errors.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html