9:19 a.m.
My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
introduced a regression by removing TLS initialization from client.
---
daemon/libvirtd.c | 1 -
src/libvirt.c | 3 +++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 5969a82..8f04a99 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1516,7 +1516,6 @@ int main(int argc, char **argv) {
virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START,
0, "start", NULL);
- virNetTLSInit();
if (daemonSetupNetworking(srv, config,
sock_file, sock_file_ro,
ipsock, privileged) < 0) {
diff --git a/src/libvirt.c b/src/libvirt.c
index c8af3e1..fdf3923 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -40,6 +40,7 @@
#include "memory.h"
#include "configmake.h"
#include "intprops.h"
+#include "rpc/virnettlscontext.h"
#ifndef WITH_DRIVER_MODULES
# ifdef WITH_TEST
@@ -409,6 +410,8 @@ virInitialize(void)
virLogSetFromEnv();
+ virNetTLSInit();
+
VIR_DEBUG("register drivers");
#if HAVE_WINSOCK2_H
--
1.7.3.4
9:45 a.m.
On 08/24/2011 08:19 AM, Michal Privoznik wrote:
My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
introduced a regression by removing TLS initialization from client.
---
daemon/libvirtd.c | 1 -
src/libvirt.c | 3 +++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 5969a82..8f04a99 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1516,7 +1516,6 @@ int main(int argc, char **argv) {
virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START,
0, "start", NULL);
- virNetTLSInit();
This looks odd - having the tls init in a 3rd party call via
virInitialize, but the tls de-init is still directly in libvirtd.
Either we need a virDeinitialize which does the virNetTLSDeinit, and
libvirtd calls virDeinitialize; or you can just drop all calls to
virNetTLSDeinit.
ACK that this fixes the bug, but I think we could do with a v2 that
avoids the asymmetry introduced by this v1 patch.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:58 a.m.
On Wed, Aug 24, 2011 at 08:45:37AM -0600, Eric Blake wrote:
On 08/24/2011 08:19 AM, Michal Privoznik wrote:
>My previous patch 74c75671331d284e1f777f9692b72e9737520bf0
>introduced a regression by removing TLS initialization from client.
>---
> daemon/libvirtd.c | 1 -
> src/libvirt.c | 3 +++
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
>diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>index 5969a82..8f04a99 100644
>--- a/daemon/libvirtd.c
>+++ b/daemon/libvirtd.c
>@@ -1516,7 +1516,6 @@ int main(int argc, char **argv) {
> virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START,
> 0, "start", NULL);
>
>- virNetTLSInit();
This looks odd - having the tls init in a 3rd party call via
virInitialize, but the tls de-init is still directly in libvirtd.
Either we need a virDeinitialize which does the virNetTLSDeinit, and
libvirtd calls virDeinitialize; or you can just drop all calls to
virNetTLSDeinit.
deinitialize is really a waste of time, or even wrong. Some
other libraries libvirt links to might also use TLS, so we
can't ever be sure it is safe to deinitialize. Even in the
daemon i think it is pretty pointless.
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 :|
10:05 a.m.
On 08/24/2011 08:58 AM, Daniel P. Berrange wrote:
> Either we need a virDeinitialize which does the virNetTLSDeinit,
and
> libvirtd calls virDeinitialize; or you can just drop all calls to
> virNetTLSDeinit.
deinitialize is really a waste of time, or even wrong. Some
other libraries libvirt links to might also use TLS, so we
can't ever be sure it is safe to deinitialize. Even in the
daemon i think it is pretty pointless.
If init and deinit are reference counted, then deinit makes sense -
reduce the reference count when our library is done using it without
unloading it from any other library, and if our library was the last
client, then reclaim the resources. But if this is the case, then the
client that is using us as a library has to have symmetric access points
- if virInitialize added a reference count to tls, then virDeinitialize
needs to reduce it.
But I don't know if tls deinit is reference counted - if it is not
counted in a thread-safe manner, then I agree that the only safe course
of action is to never deinit tls. And even if tls deinit is safe, it is
a waste of time to deinit in libvirtd, when we know we are about to
exit(), except in the case where we are trying to silence valgrind.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:09 a.m.
On Wed, Aug 24, 2011 at 09:05:48AM -0600, Eric Blake wrote:
On 08/24/2011 08:58 AM, Daniel P. Berrange wrote:
>>Either we need a virDeinitialize which does the virNetTLSDeinit, and
>>libvirtd calls virDeinitialize; or you can just drop all calls to
>>virNetTLSDeinit.
>
>deinitialize is really a waste of time, or even wrong. Some
>other libraries libvirt links to might also use TLS, so we
>can't ever be sure it is safe to deinitialize. Even in the
>daemon i think it is pretty pointless.
If init and deinit are reference counted, then deinit makes sense -
reduce the reference count when our library is done using it without
unloading it from any other library, and if our library was the last
client, then reclaim the resources. But if this is the case, then
the client that is using us as a library has to have symmetric
access points - if virInitialize added a reference count to tls,
then virDeinitialize needs to reduce it.
But I don't know if tls deinit is reference counted - if it is not
counted in a thread-safe manner, then I agree that the only safe
course of action is to never deinit tls. And even if tls deinit is
safe, it is a waste of time to deinit in libvirtd, when we know we
are about to exit(), except in the case where we are trying to
silence valgrind.
It is reference counted, but they don't protect it with any
mutex, so you can't rely on that being safe :-(
The API docs recommend that users of gnutls_global_init acquire
a mutex before calling it, but that advice is useless if the
callers are spread across different shared libraries linked
into one application :-(
So, IMHO, gnutls_global_deinit() can never be safely used.
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 :|
4873
days inactive
4873
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik