search for: nbd_open

There are a few other issues that Coverity found, but I believe all can be ignored ... except one: We don't set umask anywhere inside nbdkit. Coverity complains that this is a problem where we create temporary files, since the result of mkstemp depends implicitly on the umask value. I think we might consider setting umask anyway (eg. to 022) just to make plugin behaviour more predictable.

gcc 8 gripes (when using './configure --enable-gcc-warnings'): nbd.c: In function 'nbd_open': nbd.c:470:3: error: 'strncpy' specified bound 108 equals destination size [-Werror=stringop-truncation] strncpy (sock.sun_path, sockname, sizeof (sock.sun_path)); The warning is a false positive, given that we currently reject names >= sizeof(sock.sun_path), and thus we are only e...

Accepting only Unix sockets can be a bit limiting; let's be more flexible. Eric Blake (2): nbd: Refactor Unix socket connection nbd: Support TCP socket plugins/nbd/nbdkit-nbd-plugin.pod | 36 ++++-- plugins/nbd/nbd.c | 175 ++++++++++++++++++++++-------- TODO | 3 - 3 files changed, 161 insertions(+), 53 deletions(-) -- 2.20.1

...name_of_nbd_rep (reply.reply)); + else + nbdkit_error ("server used unexpected reply %s to NBD_OPT_GO", + name_of_nbd_rep (reply.reply)); + free (buffer); + return -1; + } + } +} + /* Create the per-connection handle. */ static void * nbd_open (int readonly) @@ -492,6 +644,7 @@ nbd_open (int readonly) } version = be64toh (old.version); if (version == OLD_VERSION) { + nbdkit_debug ("trying oldstyle connection"); if (read_full (h->fd, (char *) &old + offsetof (struct old_handshake, expo...

...e name of Unix socket */ static char *sockname; @@ -73,9 +104,18 @@ static const char *export; /* Number of retries */ static unsigned long retry; +/* True to share single server connection among all clients */ +static bool shared; +static struct handle *shared_handle; + +static struct handle *nbd_open_handle (int readonly); +static void nbd_close_handle (struct handle *h); + static void nbd_unload (void) { + if (shared) + nbd_close_handle (shared_handle); free (sockname); free (servname); } @@ -89,6 +129,7 @@ static int nbd_config (const char *key, const char *value) { char *e...

Found by Coverity. --- plugins/nbd/nbd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index b9e72bc..2b5569b 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -465,6 +465,7 @@ nbd_open (int readonly) h->fd = socket (AF_UNIX, SOCK_STREAM, 0); if (h->fd < 0) { nbdkit_error ("socket: %m"); + free (h); return NULL; } /* We already validated length during nbd_config_complete */ @@ -559,6 +560,7 @@ nbd_open (int readonly) err: close (h...

I got annoyed by qemu-nbd's default of only allowing a single connection; combine that with nbdkit's nbd plugin, and even 'qemu-nbd --list' of nbdkit counts as the single connection and immediately hangs up. If we introduce a shared mode, then 'qemu-nbd --list' can connect as many times as it wants without killing the original qemu-nbd wrapped by nbdkit. But this in turn

...t;); + return -1; + } + + if (setsockopt (h->fd, IPPROTO_TCP, TCP_NODELAY, &optval, + sizeof(int)) == -1) { + nbdkit_error ("cannot set TCP_NODELAY option: %m"); + return -1; + } + return 0; +} + /* Create the per-connection handle. */ static void * nbd_open (int readonly) @@ -876,7 +955,11 @@ nbd_open (int readonly) } h->fd = -1; - if (nbd_connect_unix (h) == -1) + if (sockname) { + if (nbd_connect_unix (h) == -1) + goto err; + } + else if (nbd_connect_tcp (h) == -1) goto err; /* old and new handshake share same meaning...

I tried reproducing the testsuite failure on test-parallel-*.sh using the same machine Rich posted a log from, but did not quickly hit it on a loop of make -j20 check TESTS=test-parallel-{file,nbd}.sh But I still think this series can't hurt. Eric Blake (2): nbd: Don't advertise writes if nbdkit is readonly tests: Make parallel tests more robust plugins/nbd/nbd.c | 2 ++

I'm hoping to implement .extents for the nbd plugin; this is a prerequisite. I'm not sure about patch 3 - if we like it, I'll squash it to 2, if we don't, I think we are okay just dropping it. I'm also wondering if we have to worry about malicious plugins that don't populate the entire .pread buffer in an effort to get nbdkit to expose portions of the heap; my patch 7 loses

Updated based on other changes that have happened in the meantime: - rely more on cleanup.h (throughout) - split structured read for easier review (patch 2 and 3 were combined in v1) - rely on nbdkit not leaking a server's partial answer (patch 3) - add tests (patch 5) - other bug fixes I found while testing it - drop EOVERFLOW patch for now; it will be separate once upstream NBD protocol

...This check is just for sanity that the reader thread concept + works; it won't work once we allow interleaved requests */ + assert (fd == h->trans.u.fds[0]); + h->trans.u.fds[0] = -1; + nbd_unlock (h); + close (fd); + errno = err; return err ? -1 : 0; } @@ -396,6 +465,17 @@ nbd_open (int readonly) goto err; } + /* Spawn a dedicated reader thread */ + if ((errno = pthread_mutex_init (&h->lock, NULL))) { + nbdkit_error ("failed to initialize mutex"); + goto err; + } + if ((errno = pthread_create (&h->reader, NULL, nbd_reader, h))) { +...

...nbd_reply (struct handle *h, int cookie) +{ + int err; + struct transaction trans; + + err = nbd_reply_raw (h, &trans); + assert (err < 0 || cookie == trans.cookie); + if (err > 0) + errno = err; + return err ? -1 : 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...

With this, I am finally able to get the nbd plugin to do out-of-order responses to the client. Once this series goes in, we should be ready for Rich to cut a release. Eric Blake (4): nbd: Split reading into separate thread nbd: Protect writes with mutex nbd: Enable parallel handling tests: Test parallel nbd behavior plugins/nbd/nbd.c | 217

I'm still working on the interleaving (and Rich reminded me on IRC that we still don't have THREAD_MODEL_PARALLEL working anywhere yet, anyways). Since nbdkit doesn't really have a parallel plugin yet, my testing on that front will have to use qemu-nbd as the original server, as well as qemu-io as the driver (qemu-io's aio_read and aio_write commands can be used to trigger

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

...f export on remote server, default '', ignored for oldstyle */ -static const char *export; - -/* Number of retries */ -static unsigned retry; - -/* True to share single server connection among all clients */ -static bool shared; -static struct handle *shared_handle; - -static struct handle *nbd_open_handle (int readonly); -static void nbd_close_handle (struct handle *h); - -static void -nbd_unload (void) -{ - if (shared) - nbd_close_handle (shared_handle); - free (sockname); - free (servname); -} - -/* Called for each key=value passed on the command line. This plugin - * accepts socket=...

Patch 1 played with an early draft of Rich's Fedora 30 libnbd package: https://bugzilla.redhat.com/show_bug.cgi?id=1713767#c17 Note that comment 21 provides a newer package 0.1.1-1 with a different API; and that libnbd has more unreleased API changes in the pipeline (whether that will be called 0.2 or 0.1.2); so we'll have to tweak things based on what is actually available in distros.

libnbd-0.1.2-1 is now available in Fedora 29/30 updates-testing, although it was not compiled against libxml2 so it lacks uri support (I ended up testing patch 4 with a self-built libnbd). Diffs since v1 - rebase to master, bump from libnbd 0.1 to 0.1.2, add URI support, better timing results Still not done - patch 5 needs associated tests Eric Blake (5): nbd: Check for libnbd nbd:

Hi Jens, this series removes the revalidate_disk() function, which has been a really odd duck in the last years. The prime reason why most people use it is because it propagates a size change from the gendisk to the block_device structure. But it also calls into the rather ill defined ->revalidate_disk method which is rather useless for the callers. So this adds a new helper to just