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

Hi Pino:

I've suppressed some OCaml and libosinfo valgrind errors in virt-v2v.

The remaining valgrind errors are here:

  http://oirase.annexia.org/tmp/v2vvg/

They all seem to be basically the same.  But I couldn't work out if
these are expected leaks in the libosinfo code (in which case we
should suppress them), or if they are actual bugs because we are
missing a true destructor here:

 
https://github.com/libguestfs/virt-v2v/blob/8e870da79b5a61513f568b0b81c773084b8d7997/v2v/libosinfo-c.c#L91

Perhaps there's a reason why we cannot have a destructor, for example
that the C database is supposed to hold references to the OS objects?
Unfortunately we never free the database.  It could be that to express
this properly we'd need to expose (db, os) tuples to the OCaml garbage
collector.

If it's all too hard to fix correctly, then adding suppressions is
fine, but it'd be nice to add a comment about what the problems are so
we can work on them in future.

BTW the informational string given here seems to be wrong - copy and
paste error?

 
https://github.com/libguestfs/virt-v2v/blob/8e870da79b5a61513f568b0b81c773084b8d7997/v2v/libosinfo-c.c#L90

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Tuesday, 14 April 2020 11:53:30 CEST Richard W.M. Jones
wrote:
> I've suppressed some OCaml and libosinfo valgrind errors in virt-v2v.
> 
> The remaining valgrind errors are here:
> 
>   http://oirase.annexia.org/tmp/v2vvg/
> 
> They all seem to be basically the same.  But I couldn't work out if
> these are expected leaks in the libosinfo code (in which case we
> should suppress them),
I think this might have to do with the glib allocator (libosinfo is
based on glib, so it allocate using it), that allocates in chunks by
default to avoid fragmentation, IIRC. Do you still get the same issues
if you export G_SLICE=always-malloc when running valgrind?
> or if they are actual bugs because we are
> missing a true destructor here:
> 
>  
https://github.com/libguestfs/virt-v2v/blob/8e870da79b5a61513f568b0b81c773084b8d7997/v2v/libosinfo-c.c#L91
> 
> Perhaps there's a reason why we cannot have a destructor, for example
> that the C database is supposed to hold references to the OS objects?
This is correct according to the way we use OsinfoOs so far, i.e. only
by using what OsinfoDb has, because OsinfoDb holds references on the
OsinfoOs objects. I talked with Fabiano (main libosinfo developer)
about this, and sadly OsinfoDb has also references to other objects
(like the devices) inside each OsinfoOs, so it is not possible to get
the ownership of all the OsinfoOs (g_object_ref) and then get rid of
the OsinfoDb (g_object_unref).
> 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?
> 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.
> BTW the informational string given here seems to be wrong - copy and
> paste error?
> 
>  
https://github.com/libguestfs/virt-v2v/blob/8e870da79b5a61513f568b0b81c773084b8d7997/v2v/libosinfo-c.c#L90
Oops yes. "Db" and "Os" sadly look too similar...

-- 
Pino Toscano

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