4:33 a.m.
I'm trying to track down:
https://bugzilla.redhat.com/show_bug.cgi?id=810613
I *think* this is a libvirt bug, but I can only reproduce it on one
machine, not on another machine that has a virtually identical setup.
Anyway, something is confusing me about libvirt. In this commit:
----------------------------------------------------------------------
commit 74c75671331d284e1f777f9692b72e9737520bf0
Author: Michal Privoznik <mprivozn(a)redhat.com>
Date: Thu Aug 18 10:44:08 2011 +0200
daemon: initialize GnuTLS
When spice_tls is set but listen_tls is not, we don't initialize
GnuTLS library. So any later gnutls call (e.g. during migration,
where we initialize a certificate) will access uninitialized GnuTLS
internal structs and throws an error.
Although, we might now initialize GnuTLS twice, it is safe according
to the documentation:
This function can be called many times,
but will only do something the first time.
This patch creates 2 functions: virNetTLSInit and virNetTLSDeinit
with respect to written above.
----------------------------------------------------------------------
... a pair of functions called virNetTLSInit & virNetTLSDeinit are
introduced, which would appear to do the right thing, calling first
gnutls_global_init, then gnutls_global_deinit. This looks correct to
me.
However the above commit is later amended by this commit:
----------------------------------------------------------------------
commit eaddec976ef06457fee4a4ce86b8c7ee906183b7
Author: Michal Privoznik <mprivozn(a)redhat.com>
Date: Wed Aug 24 16:16:45 2011 +0200
daemon: Move TLS initialization to virInitialize
My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
introduced a regression by removing TLS initialization from client.
----------------------------------------------------------------------
which removes virNetTLSDeinit. This appears to be a mistake, or at
least I can't see the logical reason for it, and according to the
gnutls docs, it would introduce a memory leak looking exactly like the
one I am chasing down.
The current code mentions virNetTLSDeinit only in a comment, and never
calls gnutls_global_deinit.
Any ideas?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
11:19 a.m.
On 04/07/2012 03:33 AM, Richard W.M. Jones wrote:
However the above commit is later amended by this commit:
----------------------------------------------------------------------
commit eaddec976ef06457fee4a4ce86b8c7ee906183b7
Author: Michal Privoznik <mprivozn(a)redhat.com>
Date: Wed Aug 24 16:16:45 2011 +0200
daemon: Move TLS initialization to virInitialize
My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
introduced a regression by removing TLS initialization from client.
----------------------------------------------------------------------
which removes virNetTLSDeinit. This appears to be a mistake, or at
least I can't see the logical reason for it, and according to the
gnutls docs, it would introduce a memory leak looking exactly like the
one I am chasing down.
I remember asking at the time, and seem to remember this answer:
gnutls_global_init is not thread-safe, and therefore must not be called
in the context of a library that might be in use by a multi-threaded
parent application. Same goes for gnutls_global_deinit. We'd rather
leak the memory (which isn't really a leak unless you dynamically load
and unload libvirt multiple times in your app) than risk thread
unsafety, all while waiting for gnutls to fix their thread-safety issue.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:08 a.m.
On Mon, Apr 09, 2012 at 10:19:38AM -0600, Eric Blake wrote:
On 04/07/2012 03:33 AM, Richard W.M. Jones wrote:
> However the above commit is later amended by this commit:
>
> ----------------------------------------------------------------------
> commit eaddec976ef06457fee4a4ce86b8c7ee906183b7
> Author: Michal Privoznik <mprivozn(a)redhat.com>
> Date: Wed Aug 24 16:16:45 2011 +0200
>
> daemon: Move TLS initialization to virInitialize
>
> My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
> introduced a regression by removing TLS initialization from client.
> ----------------------------------------------------------------------
>
> which removes virNetTLSDeinit. This appears to be a mistake, or at
> least I can't see the logical reason for it, and according to the
> gnutls docs, it would introduce a memory leak looking exactly like the
> one I am chasing down.
I remember asking at the time, and seem to remember this answer:
gnutls_global_init is not thread-safe, and therefore must not be called
in the context of a library that might be in use by a multi-threaded
parent application.
OK, although this function we *do* call :-)
Same goes for gnutls_global_deinit.
There's still a confusing comment in libvirt. Take a look at the very
bottom of 'src/rpc/virnettlscontext.c'.
We'd rather leak the memory (which isn't really a leak unless
you
dynamically load and unload libvirt multiple times in your app) than
risk thread unsafety, all while waiting for gnutls to fix their
thread-safety issue.
I had a look at the initialization code in gnutls, and it is pretty
complex ...
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://et.redhat.com/~rjones/virt-df/
6:14 a.m.
On Tue, Apr 10, 2012 at 12:08:14PM +0100, Richard W.M. Jones wrote:
On Mon, Apr 09, 2012 at 10:19:38AM -0600, Eric Blake wrote:
> On 04/07/2012 03:33 AM, Richard W.M. Jones wrote:
>
> > However the above commit is later amended by this commit:
> >
> > ----------------------------------------------------------------------
> > commit eaddec976ef06457fee4a4ce86b8c7ee906183b7
> > Author: Michal Privoznik <mprivozn(a)redhat.com>
> > Date: Wed Aug 24 16:16:45 2011 +0200
> >
> > daemon: Move TLS initialization to virInitialize
> >
> > My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
> > introduced a regression by removing TLS initialization from client.
> > ----------------------------------------------------------------------
> >
> > which removes virNetTLSDeinit. This appears to be a mistake, or at
> > least I can't see the logical reason for it, and according to the
> > gnutls docs, it would introduce a memory leak looking exactly like the
> > one I am chasing down.
>
> I remember asking at the time, and seem to remember this answer:
>
> gnutls_global_init is not thread-safe, and therefore must not be called
> in the context of a library that might be in use by a multi-threaded
> parent application.
OK, although this function we *do* call :-)
> Same goes for gnutls_global_deinit.
There's still a confusing comment in libvirt. Take a look at the very
bottom of 'src/rpc/virnettlscontext.c'.
Yep that comment is bogus
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 :|
4640
days inactive
4643
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Richard W.M. Jones