[Libguestfs] [PATCH nbdkit 2/2] vddk: Drive library loading from libdir parameter.

Thu Feb 13 14:23:32 UTC 2020

On 2/13/20 8:01 AM, Richard W.M. Jones wrote:
> Do not use LD_LIBRARY_PATH to locate the VDDK library.  Setting this
> always causes problems because VDDK comes bundled with broken
> replacements for system libraries, such as libcrypto.so and
> libstdc++.so.  Two problems this causes which we have seen in the real
> world:
> 
> (1) User does ‘export LD_LIBRARY_PATH=vmware-vix-disklib-distrib’ and
> that breaks lots of ordinary utilities on their system.
> 
> (2) nbdkit vddk --run subcommand inherits the LD_LIBRARY_PATH
> environment variable from nbdkit, and common commands such as
> 'qemu-img' break, relying on complex workarounds like saving and
> restoring the original LD_LIBRARY_PATH in the subcommand.
> 
> Instead rely on a relatively undocumented feature of dlopen which is
> that when we pass in a full path it will try to load dependent
> libraries from the same directory.
> 
> Note this may break some callers who are not using libdir and
> expecting LD_LIBRARY_PATH to work, so it's a change in behaviour which
> we will have to highlight prominently in the 1.18 release notes.
> 
> Thanks: Dan Berrange, Ming Xie, Eric Blake.

Do you want to use Dan's preferred UTF-8 spelling?

> ---
>   plugins/vddk/nbdkit-vddk-plugin.pod | 29 ++++++++++++++++++++---------
>   configure.ac                        |  1 +
>   plugins/vddk/vddk.c                 | 15 +++++++++++++--
>   tests/test-vddk-real.sh             | 12 ++----------
>   tests/test-vddk.sh                  | 19 +++++++++++++------
>   5 files changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod b/plugins/vddk/nbdkit-vddk-plugin.pod
> index f34b9fba..5f32988b 100644
> --- a/plugins/vddk/nbdkit-vddk-plugin.pod
> +++ b/plugins/vddk/nbdkit-vddk-plugin.pod
> @@ -230,17 +230,22 @@ This parameter is ignored for backwards compatibility.
>   
>   =head1 LIBRARY AND CONFIG FILE LOCATIONS
>   
> -If the VDDK library (F<libvixDiskLib.so.I<N>>) is located on a
> -non-standard path, you may need to set C<LD_LIBRARY_PATH> or modify
> -F</etc/ld.so.conf> before this plugin will work.  In addition you may
> -want to set the C<libdir> parameter so that the VDDK library can load
> -plugins like Advanced Transport.
> +The VDDK library should not be placed on a system library path such as
> +F</usr/lib>.  The reason for this is that the VDDK library is shipped
> +with recompiled libraries like F<libcrypto.so> and F<libstdc++.so>
> +that conflict with system libraries.

Maybe "that can conflict", since we saw the issues on RHEL8 but not 
RHEL7 (so whether there is a conflict depends on what version VDDK was 
compiled against, rather than an unconditional event)

>   
> -Usually the VDDK distribution directory should be passed as the
> -C<libdir> parameter and set C<LD_LIBRARY_PATH> to the F<lib64>
> -subdirectory:
> +You can two choices: Place VDDK in the default libdir which is

s/can/have/

> +compiled into this plugin, for example:
> +
> + $ nbdkit vddk --dump-plugin | grep ^vddk_default_libdir
> + vddk_default_libdir=/usr/lib64/vmware-vix-disklib
> +
> +But the most common way is to set the C<libdir> parameter to point to
> +F<vmware-vix-disklib-distrib/> (which you can unpack anywhere you
> +like), and this plugin will find the VDDK library from there.  For
> +example:
>   
> - LD_LIBRARY_PATH=/path/to/vmware-vix-disklib-distrib/lib64 \
>    nbdkit vddk \
>        libdir=/path/to/vmware-vix-disklib-distrib \
>        file=file.vmdk
> @@ -250,6 +255,12 @@ configuration file, usually including F</etc/vmware/config> and
>   F<$HOME/.vmware/config>, but you can override this using the C<config>
>   parameter.
>   
> +=head2 No need to set C<LD_LIBRARY_PATH>
> +
> +In nbdkit E<le> 1.16 you had to set the environment variable
> +C<LD_LIBRARY_PATH> when using this plugin.  In nbdkit E<ge> 1.18 this
> +is I<not> recommended.
> +
>   =head1 FILE PARAMETER
>   
>   The C<file> parameter can either be a local file, in which case it
> diff --git a/configure.ac b/configure.ac
> index fa902945..d71f06e4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -130,6 +130,7 @@ AC_PROG_INSTALL
>   AC_PROG_CPP
>   AC_CANONICAL_HOST
>   AC_SYS_LARGEFILE
> +AC_CHECK_SIZEOF([long])

Is this strictly necessary, or...

>   
>   AC_C_PROTOTYPES
>   test "x$U" != "x" && AC_MSG_ERROR([Compiler not ANSI compliant])
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index db61c1d8..c49eebcd 100644
> --- a/plugins/vddk/vddk.c
> +++ b/plugins/vddk/vddk.c
> @@ -256,7 +256,18 @@ load_library (void)
>   
>     /* Load the library. */
>     for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) {
> -    dl = dlopen (sonames[i], RTLD_NOW);
> +    CLEANUP_FREE char *path;
> +
> +    /* Set the full path so that dlopen will preferentially load the
> +     * system libraries from the same directory.
> +     */
> +    if (asprintf (&path, "%s/lib%d/%s",
> +                  libdir ? : VDDK_LIBDIR, 8*SIZEOF_LONG, sonames[i]) == -1) {

could you just spell this sizeof(long)*CHAR_BITS?

I'm guessing that the vddk files always ship with a dir/lib32/xxx.so and 
a dir/lib64/xxx.so convention?

Or, can we just hard-code the name '/lib64/', since...

> +++ b/tests/test-vddk.sh
> @@ -1,6 +1,6 @@
>   #!/usr/bin/env bash
>   # nbdkit
> -# Copyright (C) 2018 Red Hat Inc.
> +# Copyright (C) 2018-2020 Red Hat Inc.
>   #
>   # Redistribution and use in source and binary forms, with or without
>   # modification, are permitted provided that the following conditions are
> @@ -34,12 +34,19 @@ source ./functions.sh
>   set -e
>   set -x
>   
> -rm -f test-vddk.out
> -cleanup_fn rm -f test-vddk.out
> +# Real VDDK only works on x86-64.  While this test doesn't test real
> +# VDDK, we do need to know that we're on a 64 bit architecture
> +# (because of the need for the lib64 path), so we might as well only
> +# test x86-64.
> +requires test $(uname -m) = x86_64

...that's all the more we support?

>   
> -LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \
> -LIBRARY_PATH=.libs:$LIBRARY_PATH \
> -nbdkit vddk --dump-plugin > test-vddk.out
> +rm -rf vmware-vix-disklib-distrib test-vddk.out
> +cleanup_fn rm -rf vmware-vix-disklib-distrib test-vddk.out
> +
> +mkdir -p vmware-vix-disklib-distrib/lib64
> +cp .libs/libvixDiskLib.so* vmware-vix-disklib-distrib/lib64/
> +
> +nbdkit vddk libdir=vmware-vix-disklib-distrib --dump-plugin > test-vddk.out
>   cat test-vddk.out
>   
>   grep ^vddk_default_libdir= test-vddk.out
> 

But the idea looks sane to me. I don't have VDDK libraries installed so 
I can't readily test it, but assume this helps.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org