Libguestfs - Nov 2017 - Re: [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

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

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

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

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.
-- 
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/

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

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