[PATCH nbdkit 0/4] server: Improve errors from failed dlopen()
Improve errors from failed dlopen(), especially when the dlopen filename contains non-printable characters. Rich.
This function added recently in commit 7df09064a4 ("server: Explain what to do if plugin/filter loading fails") was rather hard to follow. Document and simplify the parameters. This is just refactoring, it should have no effect on what the code prints. Updates: commit 7df09064a4e993a74d2d4421b70b272e7121c929 --- server/main.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/server/main.c b/server/main.c index c354f3bf36..7c9dc27626 100644 --- a/server/main.c +++ b/server/main.c @@ -982,19 +982,32 @@ make_uri (void) return r; } +/* Common error handling for dlopen failures in open_plugin_so or + * open_filter_so. + * + * The error message is held implicitly in dlerror(). + * + * 'what' is the string "plugin" or "filter". + * + * 'filename' is the file we were trying to dlopen. + * + * 'short_name' is the original short name that was passed to nbdkit, + * or NULL if no short name was used. + */ static void failed_to_load_error (const char *what, - const char *orig_name, const char *name, int short_name) + const char *filename, + const char *short_name) { fprintf (stderr, "%s: error: cannot open %s '%s': %s\n", - program_name, what, name, dlerror ()); + program_name, what, filename, dlerror ()); if (short_name) fprintf (stderr, "\n" "To add this functionality you might need to install a separate\n" "%s package such as nbdkit-%s-%s (Fedora) or\n" "nbdkit-%s-%s (Debian).\n", - what, orig_name, what, what, orig_name); + what, short_name, what, what, short_name); fprintf (stderr, "\n" "Use '%s --help' or " @@ -1026,7 +1039,7 @@ open_plugin_so (size_t i, const char *name, int short_name) dl = dlopen (filename, RTLD_NOW|RTLD_GLOBAL); if (dl == NULL) - failed_to_load_error ("plugin", orig_name, name, short_name); + failed_to_load_error ("plugin", filename, short_name ? orig_name : NULL); /* Initialize the plugin. See dlopen(3) to understand C weirdness. */ dlerror (); @@ -1073,7 +1086,7 @@ open_filter_so (struct backend *next, size_t i, dl = dlopen (filename, RTLD_NOW|RTLD_GLOBAL); if (dl == NULL) - failed_to_load_error ("filter", orig_name, name, short_name); + failed_to_load_error ("filter", filename, short_name ? orig_name : NULL); /* Initialize the filter. See dlopen(3) to understand C weirdness. */ dlerror (); -- 2.44.0
Commit 7df09064a4 ("server: Explain what to do if plugin/filter loading fails") added a useful tip for how to install the plugin or filter package from common Linux distros. However it was not very selective and might suggest impossible package names. Fix this by only printing the tip if it's a plausible package name. Added { ... } for clarity in the code. Updates: commit 7df09064a4e993a74d2d4421b70b272e7121c929 --- server/main.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/server/main.c b/server/main.c index 7c9dc27626..f3cb604cbd 100644 --- a/server/main.c +++ b/server/main.c @@ -63,6 +63,7 @@ #include <dlfcn.h> +#include "ascii-ctype.h" #include "ascii-string.h" #include "exit-with-parent.h" #include "nbd-protocol.h" @@ -982,6 +983,27 @@ make_uri (void) return r; } +/* Is it the name of a possible nbdkit-<name>-plugin or + * nbdkit-<name>-filter that might be a Fedora or Debian package? + * This matches more than all possible names, but it's just a guide + * for help output. + */ +static int +is_possible_package_name (const char *name) +{ + size_t i, n = strlen (name); + + for (i = 0; i < n; ++i) { + char c = name[i]; + if (ascii_isalnum (c)) + continue; + if (c == '-' || c == '_' || c == '.') + continue; + return 0; + } + return 1; +} + /* Common error handling for dlopen failures in open_plugin_so or * open_filter_so. * @@ -1001,13 +1023,14 @@ failed_to_load_error (const char *what, { fprintf (stderr, "%s: error: cannot open %s '%s': %s\n", program_name, what, filename, dlerror ()); - if (short_name) + if (short_name && is_possible_package_name (short_name)) { fprintf (stderr, "\n" "To add this functionality you might need to install a separate\n" "%s package such as nbdkit-%s-%s (Fedora) or\n" "nbdkit-%s-%s (Debian).\n", what, short_name, what, what, short_name); + } fprintf (stderr, "\n" "Use '%s --help' or " -- 2.44.0
If the filename contains strange or non-printable characters, then it's better to quote it rather than mess up the output. We had a report of a case where DOS line endings were used in a shebang plugin, like this: #!/usr/sbin/nbdkit python\r\n What happens here is that Linux ran nbdkit with the parameter "python\r", resulting in the peculiar error message: $ /path/to/plugin.py -plugin.so: cannot open shared object file: No such file or directory The complete error message contained the literal '\r' character, resulting in the first part of the error being overwritten when printed. After this change, the error message becomes: $ ./nbdkit /path/to/plugin.py nbdkit: error: cannot open plugin "/home/rjones/d/nbdkit/plugins/python\r/.libs/nbdkit-python\r-plugin.so": /home/rjones/d/nbdkit/plugins/python\r/.libs/nbdkit-python\r-plugin.so: cannot open shared object file: No such file or directory which may be clearer. Reported-by: Mykola Ivanets Thanks: Eric Blake --- server/main.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/server/main.c b/server/main.c index f3cb604cbd..baea095680 100644 --- a/server/main.c +++ b/server/main.c @@ -1021,8 +1021,11 @@ failed_to_load_error (const char *what, const char *filename, const char *short_name) { - fprintf (stderr, "%s: error: cannot open %s '%s': %s\n", - program_name, what, filename, dlerror ()); + fprintf (stderr, "%s: error: cannot open %s \"", program_name, what); + c_string_quote (filename, stderr); + fprintf (stderr, "\": "); + c_string_quote (dlerror (), stderr); + fprintf (stderr, "\n"); if (short_name && is_possible_package_name (short_name)) { fprintf (stderr, "\n" -- 2.44.0
--- tests/Makefile.am | 3 ++ tests/test-shebang-crlf.sh | 59 ++++++++++++++++++++++++++++++++++++++ tests/shebang-crlf.py | 2 ++ 3 files changed, 64 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index f03ed2cb9a..16f7d408b0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1287,6 +1287,7 @@ TESTS += \ test-python-peer.sh \ test-python-thread-model.sh \ test-shebang-python.sh \ + test-shebang-crlf.sh \ $(NULL) EXTRA_DIST += \ python-error.py \ @@ -1298,6 +1299,7 @@ EXTRA_DIST += \ python-peer.py \ python-thread-model.py \ shebang.py \ + shebang-crlf.py \ test-python-error.sh \ test-python-exception.sh \ test-python-export-name.sh \ @@ -1309,6 +1311,7 @@ EXTRA_DIST += \ test-python-thread-model.sh \ test-python.sh \ test-shebang-python.sh \ + test-shebang-crlf.sh \ test_python.py \ $(NULL) diff --git a/tests/test-shebang-crlf.sh b/tests/test-shebang-crlf.sh new file mode 100755 index 0000000000..c11d5c0b89 --- /dev/null +++ b/tests/test-shebang-crlf.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright Red Hat +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# See also: shebang-crlf.py +# +# CRLF shebang line used to cause a confusing error message: +# -plugin.so: cannot open shared object file: No such file or directoryon +# (In fact what is happening is that the first part of the message +# is hidden before a CR character.) +# +# We still expect this script to fail, but for the error not to be +# confusing. + +source ./functions.sh +set -e +set -x + +script=$abs_top_srcdir/tests/shebang-crlf.py +if test ! -x "$script"; then + echo "$0: could not locate $script or not executable" + exit 1 +fi + +out="test-shebang-crlf.out" +rm -f $out +cleanup_fn rm -f $out + +$script --verbose --dump-plugin 2> $out ||: +cat $out +grep -E 'python\\r' $out diff --git a/tests/shebang-crlf.py b/tests/shebang-crlf.py new file mode 100755 index 0000000000..2713f2c3cb --- /dev/null +++ b/tests/shebang-crlf.py @@ -0,0 +1,2 @@ +#!../nbdkit python +# See also: test-shebang-crlf.sh -- 2.44.0
On Thu, Apr 18, 2024 at 08:38:02PM +0100, Richard W.M. Jones wrote:
--- tests/Makefile.am | 3 ++ tests/test-shebang-crlf.sh | 59 ++++++++++++++++++++++++++++++++++++++ tests/shebang-crlf.py | 2 ++ 3 files changed, 64 insertions(+)
--- /dev/null +++ b/tests/shebang-crlf.py @@ -0,0 +1,2 @@ +#!../nbdkit python +# See also: test-shebang-crlf.sh --
Is checking in a bare CR to git going to cause problems to someone who checks out the repository on a platform where they choose to have git do CR munging? This seems like the sort of plugin where we are better off generating it on the fly rather than relying on the file surviving the user's environment. Otherwise, this series looks fine. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
On Thu, Apr 18, 2024 at 03:32:33PM -0500, Eric Blake wrote:
On Thu, Apr 18, 2024 at 08:38:02PM +0100, Richard W.M. Jones wrote:
--- tests/Makefile.am | 3 ++ tests/test-shebang-crlf.sh | 59 ++++++++++++++++++++++++++++++++++++++ tests/shebang-crlf.py | 2 ++ 3 files changed, 64 insertions(+)
--- /dev/null +++ b/tests/shebang-crlf.py @@ -0,0 +1,2 @@ +#!../nbdkit python +# See also: test-shebang-crlf.sh --
Is checking in a bare CR to git going to cause problems to someone who checks out the repository on a platform where they choose to have git do CR munging? This seems like the sort of plugin where we are better off generating it on the fly rather than relying on the file surviving the user's environment.
Yes I think that's a better idea. I'll respin this (probably not this week) with the suggested change.
Otherwise, this series looks fine.
-- Eric Blake, Principal Software Engineer
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Thu, Apr 18, 2024 at 10:55:27PM +0100, Richard W.M. Jones wrote:
On Thu, Apr 18, 2024 at 03:32:33PM -0500, Eric Blake wrote:
On Thu, Apr 18, 2024 at 08:38:02PM +0100, Richard W.M. Jones wrote:
--- tests/Makefile.am | 3 ++ tests/test-shebang-crlf.sh | 59 ++++++++++++++++++++++++++++++++++++++ tests/shebang-crlf.py | 2 ++ 3 files changed, 64 insertions(+)
--- /dev/null +++ b/tests/shebang-crlf.py @@ -0,0 +1,2 @@ +#!../nbdkit python +# See also: test-shebang-crlf.sh --
Is checking in a bare CR to git going to cause problems to someone who checks out the repository on a platform where they choose to have git do CR munging? This seems like the sort of plugin where we are better off generating it on the fly rather than relying on the file surviving the user's environment.
Yes I think that's a better idea. I'll respin this (probably not this week) with the suggested change.
Upstream with your suggested fix hopefully in: https://gitlab.com/nbdkit/nbdkit/-/commit/6327c8dbe90b21804cd938f399410c201f... https://gitlab.com/nbdkit/nbdkit/-/commit/f2ace4a1450b881953de4743f271622778... https://gitlab.com/nbdkit/nbdkit/-/commit/1e483c15b22e93bb85b194a8dc279bbf81... https://gitlab.com/nbdkit/nbdkit/-/commit/5d01b47d7a2b1fc92fa516c074bd4deadf... and the last commit had a mistake, fixed later by: https://gitlab.com/nbdkit/nbdkit/-/commit/aade881287fbf5c7ef3539d2f01e0d6eea... Rich.
Otherwise, this series looks fine.
-- Eric Blake, Principal Software Engineer
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW _______________________________________________ Libguestfs mailing list -- guestfs@lists.libguestfs.org To unsubscribe send an email to guestfs-leave@lists.libguestfs.org
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
participants (2)
-
Eric Blake -
Richard W.M. Jones