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;

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;

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 -- libvir-list mailing list libvir-list(a)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