Re: [PATCH libnbd 1/2] generator: Print full error in handle_reply_error
by Richard W.M. Jones
On Wed, Jul 24, 2024 at 04:04:58PM -0500, Eric Blake wrote:
> On Tue, Jul 23, 2024 at 05:30:12PM GMT, Richard W.M. Jones wrote:
> > Print the full error from the server during handshaking. This
> > modifies the contract of handle_reply_error so it calls set_error,
> > which can be overridden by callers or ignored completely.
> > ---
> > generator/states-newstyle-opt-go.c | 32 +---------
> > generator/states-newstyle-opt-list.c | 5 +-
> > generator/states-newstyle-opt-meta-context.c | 8 +--
> > generator/states-newstyle.c | 62 ++++++++++++++++++--
> > 4 files changed, 63 insertions(+), 44 deletions(-)
>
> This refactoring makes sense to get at the server's message (qemu
> already sends such messages, and I see your other thread on teaching
> nbdkit to do the same). ACK series.
Thanks, I pushed this as 9d1a65d0e..474a4ae6c.
One problem with this is that we take the server's (ie. untrusted)
string and create an error message from it. Most libnbd callers write
these errors to stderr directly. Thus server output can be
interpreted by the terminal.
This is also a problem in current qemu actually:
$ nbdkit --log=null eval open=' printf "EPERM x\\r mess up the output mess up the output mess up the output" >&2; exit 1 ' get_size=' echo 0 ' --run 'qemu-img info "$uri"'
qemu-img: Could not open 'nbd+unix://?socket=/tmp/nbdkitUO2nYZ/socket': Requested export not available
mess up the output mess up the output mess up the output
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
4 months
[PATCH nbdkit 0/3] Send informational error messages to NBD client
by Richard W.M. Jones
Currently nbdkit only sends an error code (like NBD_REP_ERR_*) and
possibly an errno (NBD_E*) back to the NBD client. This loses a lot
of useful information, meaning callers can only find out what really
went wrong by looking at server-side logs, which of course might not
be available or easy to access.
This change sends informational error messages back to the NBD client ...
in most cases. There are some caveats, see patch 3/3.
Rich.
4 months
[PATCH libnbd 1/2] generator: Print full error in handle_reply_error
by Richard W.M. Jones
Print the full error from the server during handshaking. This
modifies the contract of handle_reply_error so it calls set_error,
which can be overridden by callers or ignored completely.
---
generator/states-newstyle-opt-go.c | 32 +---------
generator/states-newstyle-opt-list.c | 5 +-
generator/states-newstyle-opt-meta-context.c | 8 +--
generator/states-newstyle.c | 62 ++++++++++++++++++--
4 files changed, 63 insertions(+), 44 deletions(-)
diff --git a/generator/states-newstyle-opt-go.c b/generator/states-newstyle-opt-go.c
index 5bc9a9aec..f6eb8afc6 100644
--- a/generator/states-newstyle-opt-go.c
+++ b/generator/states-newstyle-opt-go.c
@@ -247,37 +247,9 @@ NEWSTYLE.OPT_GO.CHECK_REPLY:
SET_NEXT_STATE (%.DEAD);
return 0;
}
- /* Decode expected known errors into a nicer string */
- switch (reply) {
- case NBD_REP_ERR_UNSUP:
+ if (reply == NBD_REP_ERR_UNSUP)
assert (h->opt_current == NBD_OPT_INFO);
- set_error (ENOTSUP, "handshake: server lacks NBD_OPT_INFO support");
- break;
- case NBD_REP_ERR_POLICY:
- case NBD_REP_ERR_PLATFORM:
- set_error (0, "handshake: server policy prevents NBD_OPT_GO");
- break;
- case NBD_REP_ERR_INVALID:
- case NBD_REP_ERR_TOO_BIG:
- set_error (EINVAL, "handshake: server rejected NBD_OPT_GO as invalid");
- break;
- case NBD_REP_ERR_TLS_REQD:
- set_error (ENOTSUP, "handshake: server requires TLS encryption first");
- break;
- case NBD_REP_ERR_UNKNOWN:
- set_error (ENOENT, "handshake: server has no export named '%s'",
- h->export_name);
- break;
- case NBD_REP_ERR_SHUTDOWN:
- set_error (ESHUTDOWN, "handshake: server is shutting down");
- break;
- case NBD_REP_ERR_BLOCK_SIZE_REQD:
- set_error (EINVAL, "handshake: server requires specific block sizes");
- break;
- default:
- set_error (0, "handshake: unknown reply from NBD_OPT_GO: 0x%" PRIx32,
- reply);
- }
+
nbd_internal_reset_size_and_flags (h);
h->meta_valid = false;
err = nbd_get_errno () ? : ENOTSUP;
diff --git a/generator/states-newstyle-opt-list.c b/generator/states-newstyle-opt-list.c
index cdd4676ea..6605ee0a8 100644
--- a/generator/states-newstyle-opt-list.c
+++ b/generator/states-newstyle-opt-list.c
@@ -127,9 +127,8 @@ NEWSTYLE.OPT_LIST.CHECK_REPLY:
SET_NEXT_STATE (%.DEAD);
return 0;
}
- err = ENOTSUP;
- set_error (err, "unexpected response, possibly the server does not "
- "support listing exports");
+ debug (h, "unexpected response, possibly the server does not "
+ "support listing exports");
break;
}
diff --git a/generator/states-newstyle-opt-meta-context.c b/generator/states-newstyle-opt-meta-context.c
index 6f016e66b..3945411e4 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -270,12 +270,8 @@ NEWSTYLE.OPT_META_CONTEXT.CHECK_REPLY:
}
if (opt == h->opt_current) {
- /* XXX Should we decode specific expected errors, like
- * REP_ERR_UNKNOWN to ENOENT or REP_ERR_TOO_BIG to ERANGE?
- */
- err = ENOTSUP;
- set_error (err, "unexpected response, possibly the server does not "
- "support meta contexts");
+ debug (h, "unexpected response, possibly the server does not "
+ "support meta contexts");
CALL_CALLBACK (h->opt_cb.completion, &err);
nbd_internal_free_option (h);
SET_NEXT_STATE (%.NEGOTIATING);
diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index 45893a8bd..6bcba3fbd 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -79,14 +79,18 @@ prepare_for_reply_payload (struct nbd_handle *h, uint32_t opt)
return 0;
}
-/* Check an unexpected server reply. If it is an error, log any
- * message from the server and return 0; otherwise, return -1.
+/* Check an unexpected server reply error.
+ *
+ * This calls set_error with a descriptive error message and returns
+ * 0. Unless there is a further unexpected error while processing
+ * this error, in which case it calls set_error and returns -1.
*/
static int
handle_reply_error (struct nbd_handle *h)
{
uint32_t len;
uint32_t reply;
+ char *msg = NULL;
len = be32toh (h->sbuf.or.option_reply.replylen);
reply = be32toh (h->sbuf.or.option_reply.reply);
@@ -101,9 +105,57 @@ handle_reply_error (struct nbd_handle *h)
return -1;
}
- if (len > 0)
- debug (h, "handshake: server error message: %.*s", (int)len,
- h->sbuf.or.payload.err_msg);
+ /* Decode expected errors into a nicer string */
+ if (len > 0) {
+ if (asprintf (&msg, ": %.*s",
+ (int)len, h->sbuf.or.payload.err_msg) == -1) {
+ set_error (errno, "asprintf");
+ return -1;
+ }
+ }
+
+ switch (reply) {
+ case NBD_REP_ERR_UNSUP:
+ set_error (ENOTSUP, "the operation is not supported by the server%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_POLICY:
+ set_error (0, "server policy prevents the operation%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_PLATFORM:
+ set_error (0, "the operation is not supported by the server platform%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_INVALID:
+ set_error (EINVAL, "the server rejected this operation as invalid%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_TOO_BIG:
+ set_error (EINVAL, "the operation is too large to process%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_TLS_REQD:
+ set_error (ENOTSUP, "the server requires TLS encryption first%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_UNKNOWN:
+ set_error (ENOENT, "the server has no export named '%s'%s",
+ h->export_name, msg ? : "");
+ break;
+ case NBD_REP_ERR_SHUTDOWN:
+ set_error (ESHUTDOWN, "the server is shutting down%s",
+ msg ? : "");
+ break;
+ case NBD_REP_ERR_BLOCK_SIZE_REQD:
+ set_error (EINVAL, "the server requires specific block sizes%s",
+ msg ? : "");
+ break;
+ default:
+ set_error (0, "handshake: unknown reply from the server: 0x%" PRIx32 "%s",
+ reply, msg ? : "");
+ }
+ free (msg);
return 0;
}
--
2.44.0
4 months
ANNOUNCE: nbdkit 1.40 released
by Richard W.M. Jones
I'm pleased to announce the new stable releases of nbdkit 1.40.
nbdkit is a Network Block Device (NBD) server with a stable plugin ABI
and a permissive license.
nbdkit 1.40.0 can be downloaded from here:
https://download.libguestfs.org/nbdkit/1.40-stable/
git repository:
https://gitlab.com/nbdkit/nbdkit
The release notes are attached below or can be read online at:
https://libguestfs.org/nbdkit-release-notes-1.40.1.html
Rich.
These are the release notes for nbdkit stable release 1.40. This
describes the major changes since 1.38.
nbdkit 1.40.0 was released on 22 July 2024.
Security
The server is now more careful about quoting user-provided filenames
before printing them in error messages (thanks Mykola Ivanets).
Short plugin and filter names ("file" is the short name of
nbdkit-file-plugin(1)) are now more restrictive. This change should
not be visible to users, but tightens up corner cases with possible
security implications. See:
https://gitlab.com/nbdkit/nbdkit/-/commit/f4d5e7d39e3d37a498821a87234127d...
Previous documentation in nbdkit-tls(1) incorrectly asserted that when
using X.509 certificates, nbdkit checks the client's CN. This is not
true. nbdkit only checks that the client presents a certificate issued
by the Certificate Authority specified by the --tls-certificates
directory. The documentation has been corrected. (Thanks Jon
Szymaniak, Daniel P. Berrangé).
nbdkit-ip-filter(1) incorrectly parsed "security:" rules, which might
subtly change the semantics of access lists. This has been fixed in
this release.
nbdkit-ip-filter(1) previously allowed unknown [not IPv4/v6, Unix or
vsock] socket families implicitly, so having a "deny=all" rule would
not necessarily deny every connection. This has been changed in this
release so all unknown socket families are denied.
All past security issues and information about how to report new ones
can be found in nbdkit-security(1).
Plugins
nbdkit-file-plugin(1) now exposes minimum and preferred I/O size and
the rotational property of block devices.
nbdkit-curl-plugin(1) prints the version of libcurl and other useful
information in --dump-plugin output.
nbdkit-vddk-plugin(1) has been tested with VMware VDDK 8.0.3.
Filters
New nbdkit-bzip2-filter(1) supporting bzip2-compressed images (Georg
Pfuetzenreuter).
New nbdkit-rotational-filter(1) which can be used to change the
rotational property of a plugin (whether it advertises that it behaves
like a spinning hard disk, or RAM / flash storage).
New nbdkit-spinning-filter(1) can be used to add seek delays to
simulate a spinning hard disk.
nbdkit-ip-filter(1) has new rule types for checking the client's X.509
Distinguished Name (DN) and Issuer's DN.
Language bindings
Ruby language support has been removed. This did not work because of a
fundamental problem in Ruby's garbage collection. See:
https://gitlab.com/nbdkit/nbdkit/-/commit/7364cbaae809b5ffb6b4dd847cbdd0b...
Server
New --print-uri option which prints the URI of the server to help users
find the NBD endpoint.
Add a common function to find the size of a file or block device which
should work properly across Linux and all the BSDs, and use this in
several places where we need to know the size of a file or block device
(thanks Eric Blake).
When generating an NBD URI with TLS enabled, append
"?tls-certificates=DIR" or "?tls-psk-key=FILE" parameter. For libnbd-
based NBD clients this allows the client to find the corresponding TLS
credentials.
API
New nbdkit_parse_delay(3) function which can be used to parse short
delays and sleeps, like "100ms" or "1.2μs". It is used by
nbdkit-delay-filter(1), nbdkit-retry-filter(1),
nbdkit-retry-request-filter(1) and nbdkit-spinning-filter(1). There
are also bindings in OCaml and Python.
New nbdkit_peer_tls_dn(3) and nbdkit_peer_tls_issuer_dn(3) to read the
client's X.509 certificate Distinguished Name (DN) and Issuer's DN.
Documentation
Each nbdkit API function now has a separate manual page, eg.
nbdkit_parse_size(3) and nbdkit_debug(3).
Fix references to external nbd-server(1) and nbd-client(8) man pages
(Vera Wu).
Revise the main README.md file in the sources.
Tests
CI updates and fixes (Daniel Berrangé, Eric Blake).
Build
The minimum version of gnutls is now ≥ 3.5.18.
Internals
Make error checking of ioctl(2) calls consistent by always checking if
the return value "== -1".
SEE ALSO
nbdkit(1).
AUTHORS
Authors of nbdkit 1.40:
Daniel P. Berrangé
Eric Blake
Georg Pfuetzenreuter
Richard W.M. Jones
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
4 months
Passing block size preferences from NBD -> qemu -> Linux
by Richard W.M. Jones
This is expanding on the commit message I wrote here:
https://gitlab.com/nbdkit/nbdkit/-/commit/780599d2e77c7cc4c1a7e99d0a93328...
A simple "one-liner" to test if NBD block size preferences are passed
correctly through qemu and into a Linux guest is this:
$ nbdkit memory 1G --filter=blocksize-policy \
blocksize-minimum=4096 \
blocksize-preferred=65536 \
blocksize-maximum=8M \
--run '
LIBGUESTFS_HV=/path/to/qemu-system-x86_64 \
LIBGUESTFS_BACKEND=direct \
guestfish --format=raw -a "$uri" \
run : \
debug sh "head -1 /sys/block/*/queue/*_io_size" : \
debug sh "for d in /dev/sd? ; do sg_inq -p 0xb0 \$d ; done" \
'
Current qemu (9.0.0) does not pass the block size preferences
correctly. It's a problem in qemu, not in Linux.
qemu's NBD client requests the block size preferences from nbdkit and
reads them correctly. I verified this by adding some print statements
into nbd/client.c. The sizes are stored in BDRVNBDState 'info' field.
qemu's virtio-scsi driver *can* present a block limits VPD page (0xb0)
containing these limits (see hw/scsi/scsi-disk.c), and Linux is able
to see the contents of this page using tools like 'sg_inq'. Linux
appears to translate the information faithfully into
/sys/block/sdX/queue/{minimum,optimal}_io_size files.
However the virtio-scsi driver in qemu populates this information from
the qemu command line (-device [...]min_io_size=512,opt_io_size=4096).
It doesn't pass the information through from the NBD source backing
the drive.
Fixing this seems like a non-trivial amount of work.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
4 months, 1 week
