[PATCH nbdkit 1/2] vddk: Delay loading VDDK until config_complete.
We were previously dlopen-ing it in the load() method. This is very early and in particular means that the only possible way to configure where we find the library is through environment variables and not through config parameters. Also it's not necessary as we don't call any functions from the library (such as VixDiskLib_InitEx) until config_complete. This change is neutral refactoring as currently we _do_ configure the location through an environment variable (LD_LIBRARY_PATH). --- plugins/vddk/vddk.c | 100 ++++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 49 deletions(-) diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index 5d3764d6..db61c1d8 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -143,54 +143,7 @@ error_function (const char *fs, va_list args) nbdkit_error ("%s", str); } -/* Load and unload the plugin. */ -static void -vddk_load (void) -{ - static const char *sonames[] = { - /* Prefer the newest library in case multiple exist. */ - "libvixDiskLib.so.6", - "libvixDiskLib.so.5", - }; - size_t i; - CLEANUP_FREE char *orig_error = NULL; - - /* Load the library. */ - for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) { - dl = dlopen (sonames[i], RTLD_NOW); - if (dl != NULL) - break; - if (i == 0) { - orig_error = dlerror (); - if (orig_error) - orig_error = strdup (orig_error); - } - } - if (dl == NULL) { - nbdkit_error ("%s\n\n" - "If '%s' is located on a non-standard path you may need to\n" - "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n" - "See the nbdkit-vddk-plugin(1) man page for details.", - orig_error ? : "(unknown error)", sonames[0]); - exit (EXIT_FAILURE); - } - - /* Load symbols. */ -#define STUB(fn,ret,args) \ - do { \ - fn = dlsym (dl, #fn); \ - if (fn == NULL) { \ - nbdkit_error ("required VDDK symbol \"%s\" is missing: %s", \ - #fn, dlerror ()); \ - exit (EXIT_FAILURE); \ - } \ - } while (0) -#define OPTIONAL_STUB(fn,ret,args) fn = dlsym (dl, #fn) -#include "vddk-stubs.h" -#undef STUB -#undef OPTIONAL_STUB -} - +/* Unload the plugin. */ static void vddk_unload (void) { @@ -289,6 +242,54 @@ vddk_config (const char *key, const char *value) return 0; } +/* Load the VDDK library. */ +static void +load_library (void) +{ + static const char *sonames[] = { + /* Prefer the newest library in case multiple exist. */ + "libvixDiskLib.so.6", + "libvixDiskLib.so.5", + }; + size_t i; + CLEANUP_FREE char *orig_error = NULL; + + /* Load the library. */ + for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) { + dl = dlopen (sonames[i], RTLD_NOW); + if (dl != NULL) + break; + if (i == 0) { + orig_error = dlerror (); + if (orig_error) + orig_error = strdup (orig_error); + } + } + if (dl == NULL) { + nbdkit_error ("%s\n\n" + "If '%s' is located on a non-standard path you may need to\n" + "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n" + "See the nbdkit-vddk-plugin(1) man page for details.", + orig_error ? : "(unknown error)", sonames[0]); + exit (EXIT_FAILURE); + } + + /* Load symbols. */ +#define STUB(fn,ret,args) \ + do { \ + fn = dlsym (dl, #fn); \ + if (fn == NULL) { \ + nbdkit_error ("required VDDK symbol \"%s\" is missing: %s", \ + #fn, dlerror ()); \ + exit (EXIT_FAILURE); \ + } \ + } while (0) +#define OPTIONAL_STUB(fn,ret,args) fn = dlsym (dl, #fn) +#include "vddk-stubs.h" +#undef STUB +#undef OPTIONAL_STUB +} + static int vddk_config_complete (void) { @@ -330,6 +331,8 @@ vddk_config_complete (void) #undef missing } + load_library (); + /* Initialize VDDK library. */ DEBUG_CALL ("VixDiskLib_InitEx", "%d, %d, &debug_fn, &error_fn, &error_fn, %s, %s", @@ -831,7 +834,6 @@ static struct nbdkit_plugin plugin = { .name = "vddk", .longname = "VMware VDDK plugin", .version = PACKAGE_VERSION, - .load = vddk_load, .unload = vddk_unload, .config = vddk_config, .config_complete = vddk_config_complete, -- 2.25.0
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. --- 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. -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 +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]) 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) { + nbdkit_error ("asprintf: %m"); + exit (EXIT_FAILURE); + } + + dl = dlopen (path, RTLD_NOW); if (dl != NULL) break; if (i == 0) { @@ -268,7 +279,7 @@ load_library (void) if (dl == NULL) { nbdkit_error ("%s\n\n" "If '%s' is located on a non-standard path you may need to\n" - "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n" + "set libdir=/path/to/vmware-vix-disklib-distrib.\n\n" "See the nbdkit-vddk-plugin(1) man page for details.", orig_error ? : "(unknown error)", sonames[0]); exit (EXIT_FAILURE); diff --git a/tests/test-vddk-real.sh b/tests/test-vddk-real.sh index 52c91232..d9cf3319 100755 --- a/tests/test-vddk-real.sh +++ b/tests/test-vddk-real.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # nbdkit -# Copyright (C) 2018-2019 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 @@ -51,15 +51,7 @@ cleanup_fn rm -f $files qemu-img create -f vmdk test-vddk-real.vmdk 100M -export old_ld_library_path="$LD_LIBRARY_PATH" -export LD_LIBRARY_PATH="$vddkdir/lib64:$LD_LIBRARY_PATH" - nbdkit -f -v -U - \ --filter=readahead \ vddk libdir="$vddkdir" file=test-vddk-real.vmdk \ - --run ' - # VDDK library path breaks qemu-img, we must restore the - # original path here. - export LD_LIBRARY_PATH="$old_ld_library_path" - qemu-img convert $nbd -O raw test-vddk-real.out -' + --run 'qemu-img convert $nbd -O raw test-vddk-real.out' diff --git a/tests/test-vddk.sh b/tests/test-vddk.sh index 19b946b6..c2df4d69 100755 --- a/tests/test-vddk.sh +++ 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 -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 -- 2.25.0
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
On Thu, Feb 13, 2020 at 08:23:32AM -0600, Eric Blake wrote:
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?
I'll fix this. Hope mxie doesn't mind being "romanized" :-)
+ 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...
It uses lib32/ or lib64/. i386 support was dropped in VDDK > 5.1.1, but in theory we support it.
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.
There's a larger problem with this patch as we discussed on IRC, so I'll try to spin a second version. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (2)
-
Eric Blake -
Richard W.M. Jones