Eric Blake
2017-Nov-12 02:33 UTC
[Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
This is a minimal implementation of an NBD forwarder; it lets us
convert between old and newstyle connections (great if a client
expects one style but the real server only provides the other),
or add TLS safety on top of a server without having to rewrite
that server. Right now, the real server is expected to live
on a named Unix socket, and the transactions are serialized
rather than interleaved; further enhancements could be made to
also permit TCP servers or more efficient transmission.
I also envision the possibility of enhancing our testsuite to
use NBD forwarding as a great test of our server.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Note: ./nbdkit is a bit unsafe in that it blindly inherits $valgrind
from the environment; but that was nice because it let me do:
valgrind='gdb --args' ./nbdkit nbd ...
for a gdb debug session to work out the kinks in my code. Maybe we
want to support NBDKIT_GDB similarly to NBDKIT_VALGRIND, and to clean
up the script to not foolishly inherit variables outside of the
namespace, but that's a task for another day.
---
configure.ac | 1 +
plugins/Makefile.am | 1 +
plugins/nbd/Makefile.am | 63 +++++
plugins/nbd/nbd.c | 484 ++++++++++++++++++++++++++++++++++++++
plugins/nbd/nbdkit-nbd-plugin.pod | 96 ++++++++
5 files changed, 645 insertions(+)
create mode 100644 plugins/nbd/Makefile.am
create mode 100644 plugins/nbd/nbd.c
create mode 100644 plugins/nbd/nbdkit-nbd-plugin.pod
diff --git a/configure.ac b/configure.ac
index 0856f97..be7c3c9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -453,6 +453,7 @@ AC_CONFIG_FILES([Makefile
plugins/guestfs/Makefile
plugins/gzip/Makefile
plugins/libvirt/Makefile
+ plugins/nbd/Makefile
plugins/ocaml/Makefile
plugins/perl/Makefile
plugins/python/Makefile
diff --git a/plugins/Makefile.am b/plugins/Makefile.am
index f8483e6..790fc65 100644
--- a/plugins/Makefile.am
+++ b/plugins/Makefile.am
@@ -39,6 +39,7 @@ SUBDIRS = \
guestfs \
gzip \
libvirt \
+ nbd \
ocaml \
perl \
python \
diff --git a/plugins/nbd/Makefile.am b/plugins/nbd/Makefile.am
new file mode 100644
index 0000000..6d105c2
--- /dev/null
+++ b/plugins/nbd/Makefile.am
@@ -0,0 +1,63 @@
+# nbdkit
+# Copyright (C) 2017 Red Hat Inc.
+# All rights reserved.
+#
+# 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.
+
+EXTRA_DIST = nbdkit-nbd-plugin.pod
+
+CLEANFILES = *~
+
+plugindir = $(libdir)/nbdkit/plugins
+
+plugin_LTLIBRARIES = nbdkit-nbd-plugin.la
+
+nbdkit_nbd_plugin_la_SOURCES = \
+ nbd.c \
+ $(top_srcdir)/include/nbdkit-plugin.h
+
+nbdkit_nbd_plugin_la_CPPFLAGS = \
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/src
+nbdkit_nbd_plugin_la_CFLAGS = \
+ $(WARNINGS_CFLAGS)
+nbdkit_nbd_plugin_la_LDFLAGS = \
+ -module -avoid-version -shared
+
+if HAVE_POD2MAN
+
+man_MANS = nbdkit-nbd-plugin.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-nbd-plugin.1: nbdkit-nbd-plugin.pod
+ $(POD2MAN) $(POD2MAN_ARGS) --section=1 --name=`basename $@ .1` $< $@.t
&& \
+ if grep 'POD ERROR' $@.t; then rm $@.t; exit 1; fi && \
+ mv $@.t $@
+
+endif
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
new file mode 100644
index 0000000..b7ef91f
--- /dev/null
+++ b/plugins/nbd/nbd.c
@@ -0,0 +1,484 @@
+/* nbdkit
+ * Copyright (C) 2017 Red Hat Inc.
+ * All rights reserved.
+ *
+ * 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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <stdbool.h>
+#include <string.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <errno.h>
+#include <inttypes.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
+#include <nbdkit-plugin.h>
+#include "protocol.h"
+
+static char *filename = NULL;
+static char *export = NULL;
+
+static void
+nbd_unload (void)
+{
+ free (filename);
+ free (export);
+}
+
+/* Called for each key=value passed on the command line. This plugin
+ * accepts socket=<filename> (required for now) and export=<name>
(optional).
+ */
+static int
+nbd_config (const char *key, const char *value)
+{
+ if (strcmp (key, "socket") == 0) {
+ /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
+ free (filename);
+ filename = nbdkit_absolute_path (value);
+ if (!filename)
+ return -1;
+ }
+ else if (strcmp (key, "export") == 0) {
+ free (export);
+ export = strdup (value);
+ if (!export) {
+ nbdkit_error ("memory failure: %m");
+ return -1;
+ }
+ }
+ else {
+ nbdkit_error ("unknown parameter '%s'", key);
+ return -1;
+ }
+
+ return 0;
+}
+
+/* Check the user did pass a socket=<FILENAME> parameter. */
+static int
+nbd_config_complete (void)
+{
+ struct sockaddr_un sock;
+
+ if (filename == NULL) {
+ nbdkit_error ("you must supply the socket=<FILENAME> parameter
after the plugin name on the command line");
+ return -1;
+ }
+ if (strlen (filename) >= sizeof sock.sun_path) {
+ nbdkit_error ("socket file name too large");
+ return -1;
+ }
+ if (!export)
+ export = strdup ("");
+ if (!export) {
+ nbdkit_error ("memory failure: %m");
+ return -1;
+ }
+ return 0;
+}
+
+#define nbd_config_help \
+ "socket=<FILENAME> (required) The Unix socket to connect
to.\n" \
+ "export=<NAME> Export name to connect to (default
"").\n" \
+
+/* The per-connection handle. */
+struct handle {
+ int fd;
+ int flags;
+ int64_t size;
+ uint64_t cookie;
+ bool dead;
+};
+
+/* Read an entire buffer, returning 0 on success or -1 with errno set */
+static int read_full (int fd, void *buf, size_t len)
+{
+ ssize_t r;
+
+ while (len) {
+ r = read (fd, buf, len);
+ if (r < 0) {
+ if (errno == EINTR || errno == EAGAIN)
+ continue;
+ return -1;
+ }
+ if (!r) {
+ /* Unexpected EOF */
+ errno = EBADMSG;
+ return -1;
+ }
+ buf += r;
+ len -= r;
+ }
+ return 0;
+}
+
+/* Write an entire buffer, returning 0 on success or -1 with errno set */
+static int write_full (int fd, const void *buf, size_t len)
+{
+ ssize_t r;
+
+ while (len) {
+ r = write (fd, buf, len);
+ if (r < 0) {
+ if (errno == EINTR || errno == EAGAIN)
+ continue;
+ return -1;
+ }
+ buf += r;
+ len -= r;
+ }
+ return 0;
+}
+
+/* Create the per-connection handle. */
+static void *
+nbd_open (int readonly)
+{
+ struct handle *h;
+ struct sockaddr_un sock = { .sun_family = AF_UNIX };
+ struct old_handshake old;
+ uint64_t version;
+
+ h = calloc (1, sizeof *h);
+ if (h == NULL) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+ h->fd = socket (AF_UNIX, SOCK_STREAM, 0);
+ if (h->fd < 0) {
+ nbdkit_error ("socket: %m");
+ return NULL;
+ }
+ strncpy (sock.sun_path, filename, sizeof (sock.sun_path) - 1);
+ if (connect (h->fd, (const struct sockaddr *) &sock, sizeof sock) <
0) {
+ nbdkit_error ("connect: %m");
+ goto err;
+ }
+
+ /* old and new handshake share first 16 bytes */
+ if (read_full (h->fd, &old, offsetof (struct old_handshake,
exportsize))) {
+ nbdkit_error ("unable to read magic: %m");
+ goto err;
+ }
+ if (strncmp(old.nbdmagic, "NBDMAGIC", sizeof old.nbdmagic)) {
+ nbdkit_error ("wrong magic, %s is not an NBD server", filename);
+ goto err;
+ }
+ version = be64toh (old.version);
+ if (version == OLD_VERSION) {
+ if (read_full (h->fd,
+ (char *) &old + offsetof (struct old_handshake, exportsize),
+ sizeof old - offsetof (struct old_handshake, exportsize))) {
+ nbdkit_error ("unable to read old handshake: %m");
+ goto err;
+ }
+ h->size = be64toh (old.exportsize);
+ h->flags = be16toh (old.eflags);
+ }
+ else if (version == NEW_VERSION) {
+ uint16_t gflags;
+ uint32_t cflags;
+ struct new_option opt;
+ struct new_handshake_finish finish;
+ size_t expect;
+
+ if (read_full (h->fd, &gflags, sizeof gflags)) {
+ nbdkit_error ("unable to read global flags: %m");
+ goto err;
+ }
+ cflags = htobe32(gflags & (NBD_FLAG_FIXED_NEWSTYLE |
NBD_FLAG_NO_ZEROES));
+ if (write_full (h->fd, &cflags, sizeof cflags)) {
+ nbdkit_error ("unable to return global flags: %m");
+ goto err;
+ }
+
+ /* For now, we don't do any option haggling, but go straight into
+ transmission phase */
+ opt.version = htobe64 (NEW_VERSION);
+ opt.option = htobe32 (NBD_OPT_EXPORT_NAME);
+ opt.optlen = htobe32 (strlen (export));
+ if (write_full (h->fd, &opt, sizeof opt) ||
+ write_full (h->fd, export, strlen (export))) {
+ nbdkit_error ("unable to request export '%s': %m",
export);
+ goto err;
+ }
+ expect = sizeof finish;
+ if (gflags & NBD_FLAG_NO_ZEROES)
+ expect -= sizeof finish.zeroes;
+ if (read_full (h->fd, &finish, expect)) {
+ nbdkit_error ("unable to read new handshake: %m");
+ goto err;
+ }
+ h->size = be64toh (finish.exportsize);
+ h->flags = be16toh (finish.eflags);
+ }
+ else {
+ nbdkit_error ("unexpected version %#" PRIx64, version);
+ goto err;
+ }
+
+ return h;
+
+ err:
+ close (h->fd);
+ return NULL;
+}
+
+/* Free up the per-connection handle. */
+static void
+nbd_close (void *handle)
+{
+ struct handle *h = handle;
+
+ close (h->fd);
+ free (h);
+}
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
+
+/* Get the file size. */
+static int64_t
+nbd_get_size (void *handle)
+{
+ struct handle *h = handle;
+
+ return h->size;
+}
+
+static int nbd_can_write (void *handle)
+{
+ struct handle *h = handle;
+
+ return !(h->flags & NBD_FLAG_READ_ONLY);
+}
+
+static int nbd_can_flush (void *handle)
+{
+ struct handle *h = handle;
+
+ return h->flags & NBD_FLAG_SEND_FLUSH;
+}
+
+static int nbd_is_rotational (void *handle)
+{
+ struct handle *h = handle;
+
+ return h->flags & NBD_FLAG_ROTATIONAL;
+}
+
+static int nbd_can_trim (void *handle)
+{
+ struct handle *h = handle;
+
+ return h->flags & NBD_FLAG_SEND_TRIM;
+}
+
+static int nbd_request (struct handle *h, uint32_t type, uint64_t offset,
+ uint32_t count, uint64_t *cookie)
+{
+ struct request req = {
+ .magic = htobe32 (NBD_REQUEST_MAGIC),
+ /* TODO nbdkit should have a way to pass flags, separate from cmd type */
+ .type = htobe32 (type),
+ .handle = htobe64 (h->cookie),
+ .offset = htobe64 (offset),
+ .count = htobe32 (count),
+ };
+ int r;
+
+ /* TODO full parallel support requires tracking cookies for handling
+ out-of-order responses */
+ *cookie = h->cookie++;
+ if (h->dead) {
+ nbdkit_set_error (ESHUTDOWN);
+ return -1;
+ }
+ nbdkit_debug ("sending request with type %d and cookie %" PRIu64,
type,
+ *cookie);
+ r = write_full (h->fd, &req, sizeof req);
+ if (r < 0)
+ h->dead = true;
+ return r;
+}
+
+static int nbd_reply (struct handle *h, uint64_t cookie)
+{
+ struct reply rep;
+
+ if (read_full (h->fd, &rep, sizeof rep) < 0) {
+ h->dead = true;
+ return -1;
+ }
+ nbdkit_debug ("received reply for cookie %" PRIu64, cookie);
+ if (be32toh (rep.magic) != NBD_REPLY_MAGIC ||
+ be64toh (rep.handle) != cookie) {
+ nbdkit_set_error (EIO);
+ h->dead = true;
+ return -1;
+ }
+ switch (be32toh (rep.error)) {
+ case NBD_SUCCESS:
+ return 0;
+ case NBD_EPERM:
+ nbdkit_set_error (EPERM);
+ return -1;
+ case NBD_EIO:
+ nbdkit_set_error (EIO);
+ return -1;
+ case NBD_ENOMEM:
+ nbdkit_set_error (ENOMEM);
+ return -1;
+ default:
+ nbdkit_debug ("unexpected error %d, squashing to EINVAL",
+ be32toh (rep.error));
+ /* fallthrough */
+ case NBD_EINVAL:
+ nbdkit_set_error (EINVAL);
+ return -1;
+ case NBD_ENOSPC:
+ nbdkit_set_error (ENOSPC);
+ return -1;
+ case NBD_ESHUTDOWN:
+ nbdkit_set_error (ESHUTDOWN);
+ return -1;
+ }
+}
+
+/* Read data from the file. */
+static int
+nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
+{
+ struct handle *h = handle;
+ uint64_t cookie;
+
+ if (nbd_request (h, NBD_CMD_READ, offset, count, &cookie) < 0 ||
+ nbd_reply (h, cookie) < 0)
+ return -1;
+ if (read_full (h->fd, buf, count) < 0) {
+ h->dead = true;
+ return -1;
+ }
+ return 0;
+}
+
+/* Write data to the file. */
+static int
+nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset)
+{
+ struct handle *h = handle;
+ uint64_t cookie;
+
+ if (nbd_request (h, NBD_CMD_WRITE, offset, count, &cookie) < 0)
+ return -1;
+ if (write_full (h->fd, buf, count) < 0) {
+ h->dead = true;
+ return -1;
+ }
+ return nbd_reply (h, cookie);
+}
+
+/* Write data to the file. */
+static int
+nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
+{
+ struct handle *h = handle;
+ uint64_t cookie;
+ uint32_t cmd = NBD_CMD_WRITE_ZEROES;
+
+ if (!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES)) {
+ /* Trigger a fall back to writing */
+ errno = EOPNOTSUPP;
+ return -1;
+ }
+
+ if (!may_trim)
+ cmd |= NBD_CMD_FLAG_NO_HOLE;
+ if (nbd_request (h, cmd, offset, count, &cookie) < 0)
+ return -1;
+ return nbd_reply (h, cookie);
+}
+
+/* Trim a portion of the file. */
+static int
+nbd_trim (void *handle, uint32_t count, uint64_t offset)
+{
+ struct handle *h = handle;
+ uint64_t cookie;
+
+ if (nbd_request (h, NBD_CMD_TRIM, offset, count, &cookie) < 0)
+ return -1;
+ return nbd_reply (h, cookie);
+}
+
+/* Flush the file to disk. */
+static int
+nbd_flush (void *handle)
+{
+ struct handle *h = handle;
+ uint64_t cookie;
+
+ if (nbd_request (h, NBD_CMD_FLUSH, 0, 0, &cookie) < 0)
+ return -1;
+ return nbd_reply (h, cookie);
+}
+
+static struct nbdkit_plugin plugin = {
+ .name = "nbd",
+ .longname = "nbdkit nbd plugin",
+ .version = PACKAGE_VERSION,
+ .unload = nbd_unload,
+ .config = nbd_config,
+ .config_complete = nbd_config_complete,
+ .config_help = nbd_config_help,
+ .open = nbd_open,
+ .close = nbd_close,
+ .get_size = nbd_get_size,
+ .can_write = nbd_can_write,
+ .can_flush = nbd_can_flush,
+ .is_rotational = nbd_is_rotational,
+ .can_trim = nbd_can_trim,
+ .pread = nbd_pread,
+ .pwrite = nbd_pwrite,
+ .zero = nbd_zero,
+ .flush = nbd_flush,
+ .trim = nbd_trim,
+ .errno_is_preserved = 1,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod
b/plugins/nbd/nbdkit-nbd-plugin.pod
new file mode 100644
index 0000000..2951a32
--- /dev/null
+++ b/plugins/nbd/nbdkit-nbd-plugin.pod
@@ -0,0 +1,96 @@
+=encoding utf8
+
+=head1 NAME
+
+nbdkit-nbd-plugin - nbdkit nbd plugin
+
+=head1 SYNOPSIS
+
+ nbdkit nbd socket=FILENAME [export=NAME]
+
+=head1 DESCRIPTION
+
+C<nbdkit-nbd-plugin> is an NBD forwarding plugin for L<nbdkit(1)>.
+
+It provides an NBD server that forwards all traffic as a client to
+another existing NBD server. A primary usage of this setup is to
+alter the set of features available to the ultimate end client,
+without having to change the original server (for example, to convert
+between oldstyle and newtyle, or to add TLS support where the original
+server lacks it).
+
+For now, this is limited to connecting to another NBD server over a
+named Unix socket without TLS, although it is feasible that future
+additions will support network sockets and encryption.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<socket=FILENAME>
+
+Connect to the NBD server located at the Unix socket C<FILENAME>. The
+server can speak either new or old style protocol.
+
+This parameter is required.
+
+=item B<export=NAME>
+
+If this parameter is given, and the server speaks new style protocol,
+then connect to the named export instead of the default (empty
+string).
+
+=back
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-plugin(3)>.
+
+=head1 AUTHORS
+
+Eric Blake
+
+=head1 COPYRIGHT
+
+Copyright (C) 2017 Red Hat Inc.
+
+=head1 LICENSE
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are
+met:
+
+=over 4
+
+=item *
+
+Redistributions of source code must retain the above copyright
+notice, this list of conditions and the following disclaimer.
+
+=item *
+
+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.
+
+=item *
+
+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.
+
+=back
+
+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.
--
2.13.6
Eric Blake
2017-Nov-12 03:16 UTC
Re: [Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
On 11/11/2017 08:33 PM, Eric Blake wrote:> This is a minimal implementation of an NBD forwarder; it lets us > convert between old and newstyle connections (great if a client > expects one style but the real server only provides the other), > or add TLS safety on top of a server without having to rewrite > that server. Right now, the real server is expected to live > on a named Unix socket, and the transactions are serialized > rather than interleaved; further enhancements could be made to > also permit TCP servers or more efficient transmission. >> +/* Read an entire buffer, returning 0 on success or -1 with errno set */ > +static int read_full (int fd, void *buf, size_t len) > +{ > + ssize_t r; > + > + while (len) { > + r = read (fd, buf, len); > + if (r < 0) { > + if (errno == EINTR || errno == EAGAIN) > + continue; > + return -1;Hmm, emacs picked TAB indentation, but it looks like we prefer spaces (I certainly prefer it, but forgot to tell emacs my preferences, and the project doesn't have an automatic .dir-locals.el). Do you need me to resubmit with the TABs removed? Should we add a .dir-locals.el? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake
2017-Nov-12 03:26 UTC
Re: [Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
On 11/11/2017 08:33 PM, Eric Blake wrote:> This is a minimal implementation of an NBD forwarder; it lets us > convert between old and newstyle connections (great if a client > expects one style but the real server only provides the other), > or add TLS safety on top of a server without having to rewrite > that server. Right now, the real server is expected to live > on a named Unix socket, and the transactions are serialized > rather than interleaved; further enhancements could be made to > also permit TCP servers or more efficient transmission. > > I also envision the possibility of enhancing our testsuite to > use NBD forwarding as a great test of our server. > > Signed-off-by: Eric Blake <eblake@redhat.com> >> + > +/* Free up the per-connection handle. */ > +static void > +nbd_close (void *handle) > +{ > + struct handle *h = handle; > + > + close (h->fd); > + free (h); > +}Oh, I just realized: prior to dropping the socket, this should do: if (!h->dead) nbd_request (h, NBD_CMD_DISC, 0, 0, &cookie); to let the real server get a cleaner shutdown. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Eric Blake
2017-Nov-13 18:54 UTC
Re: [Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
On 11/11/2017 08:33 PM, Eric Blake wrote:> This is a minimal implementation of an NBD forwarder; it lets us > convert between old and newstyle connections (great if a client > expects one style but the real server only provides the other), > or add TLS safety on top of a server without having to rewrite > that server. Right now, the real server is expected to live > on a named Unix socket, and the transactions are serialized > rather than interleaved; further enhancements could be made to > also permit TCP servers or more efficient transmission. > > I also envision the possibility of enhancing our testsuite to > use NBD forwarding as a great test of our server. > > Signed-off-by: Eric Blake <eblake@redhat.com> >> +/* Read an entire buffer, returning 0 on success or -1 with errno set */ > +static int read_full (int fd, void *buf, size_t len) > +{ > + ssize_t r; > + > + while (len) { > + r = read (fd, buf, len); > + if (r < 0) { > + if (errno == EINTR || errno == EAGAIN) > + continue; > + return -1; > + } > + if (!r) { > + /* Unexpected EOF */ > + errno = EBADMSG;This errno value does not travel over for the wire in NBD during transmission phase, and results in the client seeing EINVAL; however, based on how this function is sometimes called... [1]> +/* Write an entire buffer, returning 0 on success or -1 with errno set */ > +static int write_full (int fd, const void *buf, size_t len) > +{ > + ssize_t r; > + > + while (len) { > + r = write (fd, buf, len); > + if (r < 0) { > + if (errno == EINTR || errno == EAGAIN) > + continue; > + return -1;Likewise when this function fails with EPIPE... [2]> +static int nbd_request (struct handle *h, uint32_t type, uint64_t offset, > + uint32_t count, uint64_t *cookie) > +{ > + struct request req = { > + .magic = htobe32 (NBD_REQUEST_MAGIC), > + /* TODO nbdkit should have a way to pass flags, separate from cmd type */ > + .type = htobe32 (type), > + .handle = htobe64 (h->cookie), > + .offset = htobe64 (offset), > + .count = htobe32 (count), > + }; > + int r; > + > + /* TODO full parallel support requires tracking cookies for handling > + out-of-order responses */ > + *cookie = h->cookie++; > + if (h->dead) { > + nbdkit_set_error (ESHUTDOWN); > + return -1; > + } > + nbdkit_debug ("sending request with type %d and cookie %" PRIu64, type, > + *cookie); > + r = write_full (h->fd, &req, sizeof req); > + if (r < 0) > + h->dead = true;[2] ...failing with ESHUTDOWN instead of EPIPE (which turns into EINVAL)> + return r; > +} > + > +static int nbd_reply (struct handle *h, uint64_t cookie) > +{ > + struct reply rep; > + > + if (read_full (h->fd, &rep, sizeof rep) < 0) { > + h->dead = true; > + return -1;[1] ...and failing with ESHUTDOWN instead of EBADMSG (which turns into EINVAL) make more sense from the client's point of view. I'm going to submit a v2 of this patch; I also have an idea up my sleeve for how to make things interleave (by having a dedicated reader pthread, as well as a pipe() pair per transaction to manage the handoffs between transactions issuing the request and the reader thread handling the reply) that I hope to include in my v2 series if I can get it further in my testing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2017-Nov-14 10:16 UTC
Re: [Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
On Sat, Nov 11, 2017 at 08:33:26PM -0600, Eric Blake wrote:> Note: ./nbdkit is a bit unsafe in that it blindly inherits $valgrind > from the environment; but that was nice because it let me do: > valgrind='gdb --args' ./nbdkit nbd ... > for a gdb debug session to work out the kinks in my code. Maybe we > want to support NBDKIT_GDB similarly to NBDKIT_VALGRIND, and to clean > up the script to not foolishly inherit variables outside of the > namespace, but that's a task for another day.As a fix for the valgrind issue, how about this patch? Of course it will stop you from (ab-)using valgrind!>From 75dd0d10221f59f2c92aa4768675b226d56856ac Mon Sep 17 00:00:00 2001From: "Richard W.M. Jones" <rjones@redhat.com> Date: Tue, 14 Nov 2017 10:15:15 +0000 Subject: [PATCH] ./nbdkit: Don't inherit valgrind from the environment. Thanks: Eric Blake. --- nbdkit.in | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nbdkit.in b/nbdkit.in index 167d512..878a6bd 100644 --- a/nbdkit.in +++ b/nbdkit.in @@ -103,6 +103,8 @@ fi # the program under valgrind. This is used by the tests. if [ "$NBDKIT_VALGRIND" ]; then valgrind="@VALGRIND@ --vgdb=no --leak-check=full --error-exitcode=119 --suppressions=$s/valgrind-suppressions --trace-children=no --child-silent-after-fork=yes --run-libc-freeres=no" +else + unset valgrind fi # Run the final command. -- 2.13.2 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/
Richard W.M. Jones
2017-Nov-14 10:17 UTC
Re: [Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
On Sat, Nov 11, 2017 at 09:16:53PM -0600, Eric Blake wrote:> On 11/11/2017 08:33 PM, Eric Blake wrote: > > This is a minimal implementation of an NBD forwarder; it lets us > > convert between old and newstyle connections (great if a client > > expects one style but the real server only provides the other), > > or add TLS safety on top of a server without having to rewrite > > that server. Right now, the real server is expected to live > > on a named Unix socket, and the transactions are serialized > > rather than interleaved; further enhancements could be made to > > also permit TCP servers or more efficient transmission. > > > > > +/* Read an entire buffer, returning 0 on success or -1 with errno set */ > > +static int read_full (int fd, void *buf, size_t len) > > +{ > > + ssize_t r; > > + > > + while (len) { > > + r = read (fd, buf, len); > > + if (r < 0) { > > + if (errno == EINTR || errno == EAGAIN) > > + continue; > > + return -1; > > Hmm, emacs picked TAB indentation, but it looks like we prefer spaces (I > certainly prefer it, but forgot to tell emacs my preferences, and the > project doesn't have an automatic .dir-locals.el). Do you need me to > resubmit with the TABs removed? Should we add a .dir-locals.el?Don't worry about tabs vs spaces. 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
Richard W.M. Jones
2017-Nov-14 10:19 UTC
Re: [Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin
On Tue, Nov 14, 2017 at 10:16:16AM +0000, Richard W.M. Jones wrote:> On Sat, Nov 11, 2017 at 08:33:26PM -0600, Eric Blake wrote: > > Note: ./nbdkit is a bit unsafe in that it blindly inherits $valgrind > > from the environment; but that was nice because it let me do: > > valgrind='gdb --args' ./nbdkit nbd ... > > for a gdb debug session to work out the kinks in my code. Maybe we > > want to support NBDKIT_GDB similarly to NBDKIT_VALGRIND, and to clean > > up the script to not foolishly inherit variables outside of the > > namespace, but that's a task for another day. > > As a fix for the valgrind issue, how about this patch? Of course it > will stop you from (ab-)using valgrind! > > >From 75dd0d10221f59f2c92aa4768675b226d56856ac Mon Sep 17 00:00:00 2001 > From: "Richard W.M. Jones" <rjones@redhat.com> > Date: Tue, 14 Nov 2017 10:15:15 +0000 > Subject: [PATCH] ./nbdkit: Don't inherit valgrind from the environment. > > Thanks: Eric Blake. > --- > nbdkit.in | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/nbdkit.in b/nbdkit.in > index 167d512..878a6bd 100644 > --- a/nbdkit.in > +++ b/nbdkit.in > @@ -103,6 +103,8 @@ fi > # the program under valgrind. This is used by the tests. > if [ "$NBDKIT_VALGRIND" ]; then > valgrind="@VALGRIND@ --vgdb=no --leak-check=full --error-exitcode=119 --suppressions=$s/valgrind-suppressions --trace-children=no --child-silent-after-fork=yes --run-libc-freeres=no" > +else > + unset valgrind > fi > > # Run the final command.Ignore this one, I've seen your other patch now. 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