[libvirt] [PATCH] virt-admin: Call virInitialize to fix startup crash

[Martin Kletzander]

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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160628/d5f2d12e/attachment-0001.sig>