[libvirt] [PATCH] virt-admin: Call virInitialize to fix startup crash
Similar to what virsh and virt-login-shell do https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing... tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; } + if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + virFileActivateDirOverride(argv[0]); if (!vshInit(ctl, cmdGroups, NULL)) -- 2.7.4
On Mon, Jun 27, 2016 at 02:28:39PM -0400, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
I think we should try finding out what the cause for that is. It might be as simple as adding similar fix to yours and asking the user what error does he see with that fix in. The idea behind that is that scripts shouldn't need to call that initialization as that should be part of any first call they make to the library.
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) {
If this is the right thing to do, it should be virAdmInitialize(). By using virInitialize() we're giving bad example to others as it only works in virt-admin thanks to internal.h being included, I guess.
+ vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + virFileActivateDirOverride(argv[0]);
if (!vshInit(ctl, cmdGroups, NULL)) -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 28/06/16 10:12, Martin Kletzander wrote:
On Mon, Jun 27, 2016 at 02:28:39PM -0400, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
I think we should try finding out what the cause for that is. It might be as simple as adding similar fix to yours and asking the user what error does he see with that fix in. The idea behind that is that scripts shouldn't need to call that initialization as that should be part of any first call they make to the library.
I agree with Martin. The weird thing is, that although the BZ says 100% reproducible, I couldn't reproduce the issue yet and AFAIK neither has Martin, so we definitely need more details. Erik
On Tue, Jun 28, 2016 at 10:12:01AM +0200, Martin Kletzander wrote:
On Mon, Jun 27, 2016 at 02:28:39PM -0400, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
I think we should try finding out what the cause for that is. It might be as simple as adding similar fix to yours and asking the user what error does he see with that fix in. The idea behind that is that scripts shouldn't need to call that initialization as that should be part of any first call they make to the library.
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) {
If this is the right thing to do, it should be virAdmInitialize(). By using virInitialize() we're giving bad example to others as it only works in virt-admin thanks to internal.h being included, I guess.
I tried this instead, what do you think about it? diff --git i/src/util/virerror.c w/src/util/virerror.c index 1177570ef0d5..300b0b90252f 100644 --- i/src/util/virerror.c +++ w/src/util/virerror.c @@ -37,7 +37,7 @@ VIR_LOG_INIT("util.error"); -virThreadLocal virLastErr; +static virThreadLocal virLastErr; virErrorFunc virErrorHandler = NULL; /* global error handler */ void *virUserData = NULL; /* associated data */ diff --git i/src/util/virevent.c w/src/util/virevent.c index e0fd35e41644..8f9d15e46454 100644 --- i/src/util/virevent.c +++ w/src/util/virevent.c @@ -266,6 +266,9 @@ int virEventRegisterDefaultImpl(void) { VIR_DEBUG("registering default event implementation"); + if (virErrorInitialize() < 0) + return -1; + virResetLastError(); if (virEventPollInit() < 0) { diff --git i/src/util/virthread.c w/src/util/virthread.c index 6c495158f566..4b69451dd355 100644 --- i/src/util/virthread.c +++ w/src/util/virthread.c @@ -308,7 +308,8 @@ int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) { int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { + if (!l->initialized && + (ret = pthread_key_create(&l->key, c)) != 0) { errno = ret; return -1; } diff --git i/src/util/virthread.h w/src/util/virthread.h index e466d9bf0184..1ba8a0fe46bb 100644 --- i/src/util/virthread.h +++ w/src/util/virthread.h @@ -54,6 +54,7 @@ typedef virThreadLocal *virThreadLocalPtr; struct virThreadLocal { pthread_key_t key; + bool initialized; }; typedef struct virThread virThread; -- Martin
On Tue, Jun 28, 2016 at 10:49:33AM +0200, Martin Kletzander wrote:
On Tue, Jun 28, 2016 at 10:12:01AM +0200, Martin Kletzander wrote:
On Mon, Jun 27, 2016 at 02:28:39PM -0400, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
I think we should try finding out what the cause for that is. It might be as simple as adding similar fix to yours and asking the user what error does he see with that fix in. The idea behind that is that scripts shouldn't need to call that initialization as that should be part of any first call they make to the library.
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) {
If this is the right thing to do, it should be virAdmInitialize(). By using virInitialize() we're giving bad example to others as it only works in virt-admin thanks to internal.h being included, I guess.
I tried this instead, what do you think about it?
diff --git i/src/util/virerror.c w/src/util/virerror.c index 1177570ef0d5..300b0b90252f 100644 --- i/src/util/virerror.c +++ w/src/util/virerror.c @@ -37,7 +37,7 @@
VIR_LOG_INIT("util.error");
-virThreadLocal virLastErr; +static virThreadLocal virLastErr;
virErrorFunc virErrorHandler = NULL; /* global error handler */ void *virUserData = NULL; /* associated data */
Has no functional effect.
diff --git i/src/util/virevent.c w/src/util/virevent.c index e0fd35e41644..8f9d15e46454 100644 --- i/src/util/virevent.c +++ w/src/util/virevent.c @@ -266,6 +266,9 @@ int virEventRegisterDefaultImpl(void) { VIR_DEBUG("registering default event implementation");
+ if (virErrorInitialize() < 0) + return -1; + virResetLastError();
if (virEventPollInit() < 0) {
This is just hacking around fact that we didn't call virInitialize/virAdmInitialize().
diff --git i/src/util/virthread.c w/src/util/virthread.c index 6c495158f566..4b69451dd355 100644 --- i/src/util/virthread.c +++ w/src/util/virthread.c @@ -308,7 +308,8 @@ int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) { int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { + if (!l->initialized && + (ret = pthread_key_create(&l->key, c)) != 0) { errno = ret; return -1; }
Has no functional effect, since !l->initialized will always be true.
diff --git i/src/util/virthread.h w/src/util/virthread.h index e466d9bf0184..1ba8a0fe46bb 100644 --- i/src/util/virthread.h +++ w/src/util/virthread.h @@ -54,6 +54,7 @@ typedef virThreadLocal *virThreadLocalPtr;
struct virThreadLocal { pthread_key_t key; + bool initialized; };
typedef struct virThread virThread;
Regards, 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 :|
On Tue, Jun 28, 2016 at 09:57:20AM +0100, Daniel P. Berrange wrote:
On Tue, Jun 28, 2016 at 10:49:33AM +0200, Martin Kletzander wrote:
On Tue, Jun 28, 2016 at 10:12:01AM +0200, Martin Kletzander wrote:
On Mon, Jun 27, 2016 at 02:28:39PM -0400, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
I think we should try finding out what the cause for that is. It might be as simple as adding similar fix to yours and asking the user what error does he see with that fix in. The idea behind that is that scripts shouldn't need to call that initialization as that should be part of any first call they make to the library.
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) {
If this is the right thing to do, it should be virAdmInitialize(). By using virInitialize() we're giving bad example to others as it only works in virt-admin thanks to internal.h being included, I guess.
I tried this instead, what do you think about it?
diff --git i/src/util/virerror.c w/src/util/virerror.c index 1177570ef0d5..300b0b90252f 100644 --- i/src/util/virerror.c +++ w/src/util/virerror.c @@ -37,7 +37,7 @@
VIR_LOG_INIT("util.error");
-virThreadLocal virLastErr; +static virThreadLocal virLastErr;
virErrorFunc virErrorHandler = NULL; /* global error handler */ void *virUserData = NULL; /* associated data */
Has no functional effect.
I was under the impression that only static globals were guaranteed to be initialized to zeros.
diff --git i/src/util/virevent.c w/src/util/virevent.c index e0fd35e41644..8f9d15e46454 100644 --- i/src/util/virevent.c +++ w/src/util/virevent.c @@ -266,6 +266,9 @@ int virEventRegisterDefaultImpl(void) { VIR_DEBUG("registering default event implementation");
+ if (virErrorInitialize() < 0) + return -1; + virResetLastError();
if (virEventPollInit() < 0) {
This is just hacking around fact that we didn't call virInitialize/virAdmInitialize().
Which is what we have in other function so that users don't have to call these specifically. Each virAdmConnect and virConnect etc. are calling respective initializers.
diff --git i/src/util/virthread.c w/src/util/virthread.c index 6c495158f566..4b69451dd355 100644 --- i/src/util/virthread.c +++ w/src/util/virthread.c @@ -308,7 +308,8 @@ int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) { int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { + if (!l->initialized && + (ret = pthread_key_create(&l->key, c)) != 0) { errno = ret; return -1; }
Has no functional effect, since !l->initialized will always be true.
Yeah there should've been 'l->initialized = true;' here of course.
diff --git i/src/util/virthread.h w/src/util/virthread.h index e466d9bf0184..1ba8a0fe46bb 100644 --- i/src/util/virthread.h +++ w/src/util/virthread.h @@ -54,6 +54,7 @@ typedef virThreadLocal *virThreadLocalPtr;
struct virThreadLocal { pthread_key_t key; + bool initialized; };
typedef struct virThread virThread;
Regards, 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 :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jun 28, 2016 at 01:01:17PM +0200, Martin Kletzander wrote:
On Tue, Jun 28, 2016 at 09:57:20AM +0100, Daniel P. Berrange wrote:
On Tue, Jun 28, 2016 at 10:49:33AM +0200, Martin Kletzander wrote:
On Tue, Jun 28, 2016 at 10:12:01AM +0200, Martin Kletzander wrote:
On Mon, Jun 27, 2016 at 02:28:39PM -0400, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
I think we should try finding out what the cause for that is. It might be as simple as adding similar fix to yours and asking the user what error does he see with that fix in. The idea behind that is that scripts shouldn't need to call that initialization as that should be part of any first call they make to the library.
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) {
If this is the right thing to do, it should be virAdmInitialize(). By using virInitialize() we're giving bad example to others as it only works in virt-admin thanks to internal.h being included, I guess.
I tried this instead, what do you think about it?
diff --git i/src/util/virerror.c w/src/util/virerror.c index 1177570ef0d5..300b0b90252f 100644 --- i/src/util/virerror.c +++ w/src/util/virerror.c @@ -37,7 +37,7 @@
VIR_LOG_INIT("util.error");
-virThreadLocal virLastErr; +static virThreadLocal virLastErr;
virErrorFunc virErrorHandler = NULL; /* global error handler */ void *virUserData = NULL; /* associated data */
Has no functional effect.
I was under the impression that only static globals were guaranteed to be initialized to zeros.
diff --git i/src/util/virevent.c w/src/util/virevent.c index e0fd35e41644..8f9d15e46454 100644 --- i/src/util/virevent.c +++ w/src/util/virevent.c @@ -266,6 +266,9 @@ int virEventRegisterDefaultImpl(void) { VIR_DEBUG("registering default event implementation");
+ if (virErrorInitialize() < 0) + return -1; + virResetLastError();
if (virEventPollInit() < 0) {
This is just hacking around fact that we didn't call virInitialize/virAdmInitialize().
Which is what we have in other function so that users don't have to call these specifically. Each virAdmConnect and virConnect etc. are calling respective initializers.
This is the currently documented semantics for the virInitialize call. It is mandatory except when calling virConnect as the first API call. It specifically notes you must call virInitialize() if any other libvirt API call is to be used first. The difference is that your call to virErrorInitialize is not thread safe and when virInitialize() is later invoked, you'll get a second call to virErrorInitialize, which you're then hacking around by adding the 'bool initialized' attribute to the thread local. If we want to make virInitialize() optional, then we should make sure that virInitialize() itself is invoked at all the right places - not call virErrorInitialize() directly. Amuzingly virThreadInitialize() is a no-op since we deleted the Win32 custom threads impl, so we can just kill that one off entirely.
diff --git i/src/util/virthread.c w/src/util/virthread.c index 6c495158f566..4b69451dd355 100644 --- i/src/util/virthread.c +++ w/src/util/virthread.c @@ -308,7 +308,8 @@ int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) { int ret; - if ((ret = pthread_key_create(&l->key, c)) != 0) { + if (!l->initialized && + (ret = pthread_key_create(&l->key, c)) != 0) { errno = ret; return -1; }
Has no functional effect, since !l->initialized will always be true.
Yeah there should've been 'l->initialized = true;' here of course.
diff --git i/src/util/virthread.h w/src/util/virthread.h index e466d9bf0184..1ba8a0fe46bb 100644 --- i/src/util/virthread.h +++ w/src/util/virthread.h @@ -54,6 +54,7 @@ typedef virThreadLocal *virThreadLocalPtr;
struct virThreadLocal { pthread_key_t key; + bool initialized; };
typedef struct virThread virThread;
Regards, 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 :|
On 27/06/16 20:28, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + virFileActivateDirOverride(argv[0]);
if (!vshInit(ctl, cmdGroups, NULL))
So, based on Luyao's feedback on the modified patch I attached to https://bugzilla.redhat.com/show_bug.cgi?id=1350315, ACK if you replace virInitialize call with virAdmInitialize (do not forget to export the symbol through public syms :)...). I keep wondering though, why none of us (you, me, and Martin) was able to reproduce it even once. Erik
On Wed, Jun 29, 2016 at 09:34:21AM +0200, Erik Skultety wrote:
On 27/06/16 20:28, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + virFileActivateDirOverride(argv[0]);
if (!vshInit(ctl, cmdGroups, NULL))
So, based on Luyao's feedback on the modified patch I attached to https://bugzilla.redhat.com/show_bug.cgi?id=1350315, ACK if you replace virInitialize call with virAdmInitialize (do not forget to export the symbol through public syms :)...). I keep wondering though, why none of us (you, me, and Martin) was able to reproduce it even once.
I would still rather not require users to need to call that initialization function, but many other things in admin API went differently than I planned them to, so it's just going to be another todo in my user-friendliness branch (that I only have in my head), I guess. Let's at least fix it for now. ACK.
Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/29/2016 03:34 AM, Erik Skultety wrote:
On 27/06/16 20:28, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + virFileActivateDirOverride(argv[0]);
if (!vshInit(ctl, cmdGroups, NULL))
So, based on Luyao's feedback on the modified patch I attached to https://bugzilla.redhat.com/show_bug.cgi?id=1350315, ACK if you replace virInitialize call with virAdmInitialize (do not forget to export the symbol through public syms :)...). I keep wondering though, why none of
I'll just give my ACK to your patch instead :) Please push that
us (you, me, and Martin) was able to reproduce it even once.
Yeah there's definitely some weirdness... see the commit message for the similar virt-login-shell change, which one day just randomly started breaking - Cole
On 29/06/16 14:22, Cole Robinson wrote:
On 06/29/2016 03:34 AM, Erik Skultety wrote:
On 27/06/16 20:28, Cole Robinson wrote:
Similar to what virsh and virt-login-shell do
https://bugzilla.redhat.com/show_bug.cgi?id=1350315 --- I can't actually reproduce the bug, the backtrace is similar to 97973ebb7 which added the same fix for virt-login-shell, and that commit also mentions the randomness of reproducing...
tools/virt-admin.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4acac65..7bff5c3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1371,6 +1371,11 @@ main(int argc, char **argv) return EXIT_FAILURE; }
+ if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + virFileActivateDirOverride(argv[0]);
if (!vshInit(ctl, cmdGroups, NULL))
So, based on Luyao's feedback on the modified patch I attached to https://bugzilla.redhat.com/show_bug.cgi?id=1350315, ACK if you replace virInitialize call with virAdmInitialize (do not forget to export the symbol through public syms :)...). I keep wondering though, why none of
I'll just give my ACK to your patch instead :) Please push that
So I pushed the patch, thank you. Erik
us (you, me, and Martin) was able to reproduce it even once.
Yeah there's definitely some weirdness... see the commit message for the similar virt-login-shell change, which one day just randomly started breaking
- Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Erik Skultety -
Martin Kletzander