8:01 a.m.
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
8:01 a.m.
New subject: [PATCH nbdkit 2/2] vddk: Drive library loading from libdir parameter.
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
8:23 a.m.
New subject: [PATCH nbdkit 2/2] vddk: Drive library loading from libdir parameter.
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
8:38 a.m.
New subject: [PATCH nbdkit 2/2] vddk: Drive library loading from libdir parameter.
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/
1778
days inactive
1778
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones