[Libguestfs] [nbdkit PATCH] nbd: Add new nbd forwarding plugin

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20171113/5d9e66d8/attachment.sig>