[libvirt] [PATCH] virsh: Correctly initialize libvirt

[Matthias Bolte]

2011/5/9 Jiri Denemark <jdenemar at redhat.com>:
> virsh didn't call virInitialize(), which (among other things)
> initializes virLastErr thread local variable. As a result of that, virsh
> could just segfault in virEventRegisterDefaultImpl() since that is the
> first call that touches (resets) virLastErr.
>
> I have no idea what lucky coincidence made this bug visible but I was
> able to reproduce it in 100% cases but only in one specific environment
> which included building in sandbox.

Well, actually all public API that can be called as the first public
API function ever called in a sound program calls virInitialize first
internally. For example virConnectOpen. The documentation of
virInitialize suggest to call it first, but doesn't require it.

A fix in line with the current behavior would be to make
virEventRegisterDefaultImpl and virEventRunDefaultImpl call
virInitialize first too.

But this doesn't fix another problem, a program could call
virDomainFree(NULL) for some reason as the first public API call ever
made in the process and libvirt would try to report an error but would
segfault because of calling virResetLastError before virInitialize was
called.

So you found a much bigger problem and I'm not sure about the correct
general solution. We could make calling virInitialize first mandatory
but that might break existing applications. Or we make all public API
functions call virInitialize first.

> ---
>  tools/virsh.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 2b16714..08e6905 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -12819,14 +12819,20 @@ main(int argc, char **argv)
>     char *defaultConn;
>     bool ret = true;
>
> +    memset(ctl, 0, sizeof(vshControl));
> +    ctl->imode = true;          /* default is interactive mode */
> +    ctl->log_fd = -1;           /* Initialize log file descriptor */
> +
>     if (!setlocale(LC_ALL, "")) {
>         perror("setlocale");
>         /* failure to setup locale is not fatal */
>     }
> -    if (!bindtextdomain(PACKAGE, LOCALEDIR)) {
> -        perror("bindtextdomain");

Why did you remove the call to bindtextdomain?

Matthias