This neutral code refactoring just moves the place where we preserve errno out one layer, but should have no other effect. --- server/internal.h | 8 ++++---- server/log-stderr.c | 9 ++------- server/log-syslog.c | 13 ++++--------- server/log.c | 12 ++++++++---- 4 files changed, 18 insertions(+), 24 deletions(-) diff --git a/server/internal.h b/server/internal.h index 0b0507e446..1a783e3fc0 100644 --- a/server/internal.h +++ b/server/internal.h @@ -340,10 +340,10 @@ extern void free_debug_flags (void); extern void log_verror (const char *fs, va_list args); /* log-*.c */ -extern void log_stderr_verror (const char *fs, va_list args) - ATTRIBUTE_FORMAT_PRINTF (1, 0); -extern void log_syslog_verror (const char *fs, va_list args) - ATTRIBUTE_FORMAT_PRINTF (1, 0); +extern void log_stderr_verror (int orig_errno, const char *fs, va_list args) + ATTRIBUTE_FORMAT_PRINTF (2, 0); +extern void log_syslog_verror (int orig_errno, const char *fs, va_list args) + ATTRIBUTE_FORMAT_PRINTF (2, 0); /* vfprintf.c */ #if !HAVE_VFPRINTF_PERCENT_M diff --git a/server/log-stderr.c b/server/log-stderr.c index 8a55f5df22..4d8b09da37 100644 --- a/server/log-stderr.c +++ b/server/log-stderr.c @@ -43,12 +43,9 @@ #include "internal.h" -/* Note: preserves the previous value of errno. */ void -log_stderr_verror (const char *fs, va_list args) +log_stderr_verror (int orig_errno, const char *fs, va_list args) { - int err = errno; /* must be first line of function */ - const char *name = threadlocal_get_name (); size_t instance_num = threadlocal_get_instance_num (); int tty; @@ -69,7 +66,7 @@ log_stderr_verror (const char *fs, va_list args) } fprintf (stderr, "error: "); - errno = err; /* must restore in case fs contains %m */ + errno = orig_errno; /* must restore in case fs contains %m */ vfprintf (stderr, fs, args); fprintf (stderr, "\n"); @@ -78,6 +75,4 @@ log_stderr_verror (const char *fs, va_list args) #ifdef HAVE_FUNLOCKFILE funlockfile (stderr); #endif - - errno = err; /* must be last line of function */ } diff --git a/server/log-syslog.c b/server/log-syslog.c index 76c5035b7d..29a7a8252f 100644 --- a/server/log-syslog.c +++ b/server/log-syslog.c @@ -45,11 +45,9 @@ /* Tempted to use LOG_FTP instead of LOG_DAEMON! */ static const int PRIORITY = LOG_DAEMON|LOG_ERR; -/* Note: preserves the previous value of errno. */ void -log_syslog_verror (const char *fs, va_list args) +log_syslog_verror (int orig_errno, const char *fs, va_list args) { - int err = errno; const char *name = threadlocal_get_name (); size_t instance_num = threadlocal_get_instance_num (); CLEANUP_FREE char *msg = NULL; @@ -59,9 +57,9 @@ log_syslog_verror (const char *fs, va_list args) fp = open_memstream (&msg, &len); if (fp == NULL) { /* Fallback to logging using fs, args directly. */ - errno = err; /* Must restore in case fs contains %m */ + errno = orig_errno; /* must restore in case fs contains %m */ vsyslog (PRIORITY, fs, args); - goto out; + return; } if (name) { @@ -71,12 +69,9 @@ log_syslog_verror (const char *fs, va_list args) fprintf (fp, ": "); } - errno = err; /* Must restore in case fs contains %m */ + errno = orig_errno; /* must restore in case fs contains %m */ vfprintf (fp, fs, args); close_memstream (fp); syslog (PRIORITY, "%s", msg); - - out: - errno = err; } diff --git a/server/log.c b/server/log.c index 464e4f9a72..9c1f667a9b 100644 --- a/server/log.c +++ b/server/log.c @@ -46,23 +46,27 @@ void log_verror (const char *fs, va_list args) { + int orig_errno = errno; + switch (log_to) { case LOG_TO_DEFAULT: if (forked_into_background) - log_syslog_verror (fs, args); + log_syslog_verror (orig_errno, fs, args); else - log_stderr_verror (fs, args); + log_stderr_verror (orig_errno, fs, args); break; case LOG_TO_SYSLOG: - log_syslog_verror (fs, args); + log_syslog_verror (orig_errno, fs, args); break; case LOG_TO_STDERR: - log_stderr_verror (fs, args); + log_stderr_verror (orig_errno, fs, args); break; case LOG_TO_NULL: /* nothing */ break; } + + errno = orig_errno; /* Restore errno before leaving the function. */ } /* Note: preserves the previous value of errno. */ -- 2.44.0

Plus a function to clear the last_error field. --- server/internal.h | 3 +++ server/threadlocal.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/server/internal.h b/server/internal.h index 8102ccdefd..c45384a69c 100644 --- a/server/internal.h +++ b/server/internal.h @@ -571,6 +571,9 @@ extern void threadlocal_set_instance_num (size_t instance_num); extern size_t threadlocal_get_instance_num (void); extern void threadlocal_set_errno (int err); extern int threadlocal_get_errno (void); +extern void threadlocal_set_last_error (char *msg); +extern void threadlocal_clear_last_error (void); +extern const char *threadlocal_get_last_error (void); extern void *threadlocal_buffer (size_t size); extern void threadlocal_set_conn (struct connection *conn); extern struct connection *threadlocal_get_conn (void); diff --git a/server/threadlocal.c b/server/threadlocal.c index 9bb656bc4a..74a3c4e52b 100644 --- a/server/threadlocal.c +++ b/server/threadlocal.c @@ -56,6 +56,7 @@ struct threadlocal { char *name; /* Can be NULL. */ size_t instance_num; /* Can be 0. */ int err; + char *last_error; /* Can be NULL. */ void *buffer; /* Can be NULL. */ size_t buffer_size; struct connection *conn; /* Can be NULL. */ @@ -70,6 +71,7 @@ free_threadlocal (void *threadlocalv) struct threadlocal *threadlocal = threadlocalv; free (threadlocal->name); + free (threadlocal->last_error); free (threadlocal->buffer); free (threadlocal); } @@ -176,6 +178,44 @@ threadlocal_get_errno (void) return threadlocal ? threadlocal->err : 0; } +/* Set the last_error field. The ownership of the 'msg' string is + * passed to the threadlocal and will be freed here. + */ +void +threadlocal_set_last_error (char *msg) +{ + struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key); + + if (threadlocal) { + free (threadlocal->last_error); + threadlocal->last_error = msg; + } + else { + /* ... otherwise throw it away, it's informational. */ + free (msg); + } +} + +void +threadlocal_clear_last_error (void) +{ + threadlocal_set_last_error (NULL); +} + +/* Get the last_error field. If successful, this returns a non-NULL + * string. This is valid until something calls nbdkit_error() in the + * same thread, so it should be used quickly. Returning NULL is not + * necessarily an error. The last_error is informational and may not + * be available. + */ +const char * +threadlocal_get_last_error (void) +{ + struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key); + + return threadlocal ? threadlocal->last_error : NULL; +} + /* Return the single pread/pwrite buffer for this thread. The buffer * size is increased to ‘size’ bytes if required. * -- 2.44.0

This sends the last error saved in the connection handle back to the NBD client. This is informational and best effort. qemu reports the error already, for example: $ nbdkit --log=null \ eval open=' echo EPERM Go Away >&2; exit 1 ' get_size=' echo 100 ' \ --run 'qemu-img info "$uri"' qemu-img: Could not open 'nbd+unix://?socket=/tmp/nbdkitIDl6iy/socket': Requested export not available server reported: /tmp/nbdkitRDAfXH/open: Go Away This goes back to at least qemu 2.12.0 (RHEL 7) and possibly earlier, so we can just assume that qemu does this for the test. libnbd requires a patch to display this information. --- tests/Makefile.am | 2 + server/protocol-handshake-newstyle.c | 22 ++++++++++- tests/test-last-error.sh | 55 ++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.am b/tests/Makefile.am index 8c7d6b8c96..89c5fa9d0f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -292,6 +292,7 @@ TESTS += \ test-read-password-interactive.sh \ test-nbd-client.sh \ test-nbd-client-tls.sh \ + test-last-error.sh \ $(NULL) if !IS_WINDOWS TESTS += \ @@ -324,6 +325,7 @@ EXTRA_DIST += \ test-help-plugin.sh \ test-ipv4-lo.sh \ test-ipv6-lo.sh \ + test-last-error.sh \ test-long-name.sh \ test-nbd-client-tls.sh \ test-nbd-client.sh \ diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c index 6b3bc76f0f..46cd0328b1 100644 --- a/server/protocol-handshake-newstyle.c +++ b/server/protocol-handshake-newstyle.c @@ -57,16 +57,32 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply) { GET_CONN; struct nbd_fixed_new_option_reply fixed_new_option_reply; + const char *last_error = NULL; + uint32_t replylen = 0; + + if (NBD_REP_IS_ERR (reply)) { + last_error = threadlocal_get_last_error (); + /* Note that calling nbdkit_error will invalidate last_error, so + * be careful below. + */ + if (last_error) { + size_t len = strlen (last_error); + if (len <= NBD_MAX_STRING) + replylen = len; + } + } fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC); fixed_new_option_reply.option = htobe32 (option); fixed_new_option_reply.reply = htobe32 (reply); - fixed_new_option_reply.replylen = htobe32 (0); + fixed_new_option_reply.replylen = htobe32 (replylen); debug ("replying to %s with %s", name_of_nbd_opt (option), name_of_nbd_rep (reply)); if (conn->send (&fixed_new_option_reply, - sizeof fixed_new_option_reply, 0) == -1) { + sizeof fixed_new_option_reply, + replylen > 0 ? SEND_MORE : 0) == -1) { + err: /* The protocol document says that the client is allowed to simply * drop the connection after sending NBD_OPT_ABORT, or may read * the reply. @@ -77,6 +93,8 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply) nbdkit_error ("write: %s: %m", name_of_nbd_opt (option)); return -1; } + if (replylen > 0 && conn->send (last_error, replylen, 0) == -1) + goto err; return 0; } diff --git a/tests/test-last-error.sh b/tests/test-last-error.sh new file mode 100755 index 0000000000..fc720606d9 --- /dev/null +++ b/tests/test-last-error.sh @@ -0,0 +1,55 @@ +#!/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. + +source ./functions.sh +set -e +set -x + +# Test informational error messages sent to the NBD client. +# qemu-img supports this since at least 2.12.0. + +requires_run +requires_plugin eval +requires qemu-img --version + +out=last-error.out +rm -f $out +cleanup_fn rm -f $out + +export out + +nbdkit eval \ + open=' echo EPERM Go Away >&2; exit 1 ' get_size=' echo 0 ' \ + --run ' qemu-img info "$uri" > $out 2>&1 ||: ' +cat $out + +grep "Go Away" $out -- 2.44.0

On Wed, Jul 24, 2024 at 04:18:52PM -0500, Eric Blake wrote: Pushed in 7563596fdb..46484ca8e6 (with the error section moved to the end as suggested above). Rich. -- 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