Libguestfs - Apr 2020 - Re: virt-v2v valgrind errors in libosinfo

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/cc294b7735dda467179b93a061d3631ac3547f26/v2v/libosinfo_utils.ml#L24

which IIUC will allocate a DB (on first access) but it is never
released.  (Note: I'm not saying this code is wrong)
> > 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.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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/cc294b7735dda467179b93a061d3631ac3547f26/v2v/libosinfo_utils.ml#L24
> 
> 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.

I guess this is becase db is a top-level variable in Libosinfo_utils;
is there a way to change that behaviour somehow?
> > > 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.

-- 
Pino Toscano

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/cc294b7735dda467179b93a061d3631ac3547f26/v2v/libosinfo_utils.ml#L24
> > 
> > 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/8e870da79b5a61513f568b0b81c773084b8d7997
> > > > 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