[PATCH nbdkit 0/9] nbd: Implement command= and socket-fd= parameters.
I fixed the deadlock - turned out to be an actual bug in the nbd plugin (see patch 8). I changed the command syntax so it's now: nbdkit nbd command=qemu arg=-f arg=qcow2 arg=/path/to/disk.qcow2 Nir wrote: 18:08 < nsoffer> rwmjones: regarding the nbd proxy patches, did you have specific flow that help us? 18:08 < nsoffer> rwmjones: or this is just a way to support qcow2 in the nbdkit pipeline? A bit of both - it's nice if nbdkit can "open" qcow2 files. Rich.
* Change the title so it's informative and searchable. * Remove references to the non-libnbd plugin. * Headings for examples. * Correct reference to qemu-nbd(8) man page. * General copy-editing to improve readability. * Change style in places so it matches other manual pages. --- plugins/nbd/nbdkit-nbd-plugin.pod | 192 +++++++++++++++++------------- 1 file changed, 109 insertions(+), 83 deletions(-) diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod b/plugins/nbd/nbdkit-nbd-plugin.pod index 618058ac..5653703f 100644 --- a/plugins/nbd/nbdkit-nbd-plugin.pod +++ b/plugins/nbd/nbdkit-nbd-plugin.pod @@ -1,109 +1,119 @@ =head1 NAME -nbdkit-nbd-plugin - nbdkit nbd plugin +nbdkit-nbd-plugin - proxy / forward to another NBD server =head1 SYNOPSIS - nbdkit nbd { socket=SOCKNAME | hostname=HOST [port=PORT] | [uri=]URI } - [export=NAME] [retry=N] [shared=BOOL] [tls=MODE] [tls-certificates=DIR] - [tls-verify=BOOL] [tls-username=NAME] [tls-psk=FILE] + nbdkit nbd { hostname=HOST [port=PORT] | + socket=SOCKNAME | + [uri=]URI } + [export=NAME] [retry=N] [shared=BOOL] + [tls=MODE] [tls-certificates=DIR] [tls-verify=BOOL] + [tls-username=NAME] [tls-psk=FILE] =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 newstyle, or to add TLS support where the -original server lacks it). Use of this plugin along with nbdkit -filters (adding I<--filter> to the nbdkit command line) makes it -possible to apply any nbdkit filter to any other NBD server. - -Remember that when using this plugin as a bridge between an encrypted -and a non-encrypted endpoint, it is best to preserve encryption over -TCP and use plaintext only on a Unix socket. +C<nbdkit-nbd-plugin> is a plugin for L<nbdkit(1)> that lets you +forward the connection to another NBD server. There are several uses +for this plugin: + +=over 4 + +=item * + +Adjust the set of features seen by the ultimate NBD client without +having to change the original server. For example, convert between +the oldstyle and newstyle protocols, or add TLS support if the +original server lacks it. + +=item * + +Apply nbdkit filters to any other NBD server. + +=item * + +With L<qemu-nbd(8)>, read and write qcow2 files with nbdkit. + +=back =head1 PARAMETERS -One of B<socket>, B<hostname> or B<uri> must be provided to designate -the server. The server can speak either new or old style -protocol. C<uri=> is a magic config key and may be omitted in most -cases. See L<nbdkit(1)/Magic parameters>. - -The following parameters are available whether or not the plugin was -compiled against libnbd: +One of B<socket>, B<hostname> (optionally with B<port>), or B<uri> +must be given to specify which NBD server to forward to: =over 4 -=item B<socket=>SOCKNAME - -Connect to the NBD server located at the Unix socket C<SOCKNAME>. - =item B<hostname=>HOST -Connect to the NBD server at the given remote C<HOST> using a TCP socket. +Connect to the NBD server at the remote C<HOST> using a TCP socket. =item B<port=>PORT -When B<hostname> is supplied, use B<PORT> instead of the default port +When C<hostname> is supplied, use C<PORT> instead of the default port 10809. +=item B<socket=>SOCKNAME + +Connect to the NBD server using Unix domain socket C<SOCKNAME>. + +=item [B<uri=>]URI + +When C<uri> is supplied, decode C<URI> to determine the address to +connect to. A URI can specify a TCP connection (such as +C<nbd://localhost:10809/export>) or a Unix socket (such as +C<nbd+unix:///export?socket=/path/to/sock>). Remember you may need to +quote the parameter to protect it from the shell. + +C<uri=> is a magic config key and may be omitted in most +cases. See L<nbdkit(1)/Magic parameters>. + +=back + +Other parameters control the NBD connection: + +=over 4 + =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 export (the -empty string). If C<uri> is supplied, the export name should be +empty string). If C<uri> is supplied, the export name should be embedded in the URI instead. =item B<retry=>N If the initial connection attempt to the server fails, retry up to -B<N> times more after a one-second delay between tries (default 0). +C<N> times more after a one-second delay between tries (default 0). -=item B<shared=>BOOL +=item B<shared=true> -If this parameter is false (default), the plugin will open a distinct -connection to the server for each client making a connection to -nbdkit, and the remote server does not have to be started until -immediately before the first nbdkit client. If this parameter is set -to true, the plugin will open a single connection to the server when -nbdkit is first started (the C<delay> parameter may be necessary to -coordinate timing of the remote server startup), and all clients to -nbdkit will share that single connection. +By default the plugin will open a new connection to the server for +each client making a connection to nbdkit. The remote server does not +have to be started until immediately before the first nbdkit client +connects. -=back +If this parameter is set to C<true>, the plugin will open a single +connection to the server when nbdkit is first started (the C<retry> +parameter may be necessary to coordinate timing of the remote server +startup), and all clients to nbdkit will share that single connection. -The following parameters are only available if the plugin was compiled -against libnbd: +=item B<tls=off> -=over 4 +=item B<tls=on> -=item B<uri=>URI +=item B<tls=require> -When B<uri> is supplied, decode B<URI> to determine the address to -connect to. A URI can specify a TCP connection (such as -C<nbd://localhost:10809/export>) or a Unix socket (such as -C<nbd+unix:///export?socket=/path/to/sock>). Remember to use proper -shell quoting to prevent B<URI> from accidentally being handled as a -shell glob. The B<uri> parameter is only available when the plugin was -compiled against libnbd with URI support; C<nbdkit --dump-plugin nbd> -will contain C<libnbd_uri=1> if this is the case. - -=item B<tls=>MODE - -Selects which TLS mode to use with the server. If no other tls option +Selects which TLS mode to use with the server. If no other tls option is present, this defaults to C<off>, where the client does not attempt -encryption (and may be rejected by a server that requires it). If +encryption (and may be rejected by a server that requires it). If omitted but another tls option is present, this defaults to C<on>, where the client opportunistically attempts a TLS handshake, but will continue running unencrypted if the server does not support -encryption. If set to C<require> or if the C<uri> parameter is used +encryption. If set to C<require> or if the C<uri> parameter is used with a scheme that requires encryption (such as C<nbds://host>), then this requires an encrypted connection to the server. -The B<tls> parameter is only available when the plugin was compiled +The C<tls> parameter is only available when the plugin was compiled against libnbd with TLS support; C<nbdkit --dump-plugin nbd> will contain C<libnbd_tls=1> if this is the case. Note the difference between C<--tls=...> controlling what nbdkit serves, and C<tls=...> @@ -114,11 +124,11 @@ controlling what the nbd plugin connects to as a client. This specifies the directory containing X.509 client certificates to present to the server. -=item B<tls-verify=>BOOL +=item B<tls-verify=false> -This defaults to true; setting it to false disables server name -verification, which opens you to potential Man-in-the-Middle (MITM) -attacks, but allows for a simpler setup for distributing certificates. +Setting this parameter to false disables server name verification, +which opens you to potential Man-in-the-Middle (MITM) attacks, but +allows for a simpler setup for distributing certificates. =item B<tls-username=>NAME @@ -128,16 +138,18 @@ alongside the certificate. =item B<tls-psk=>FILE If provided, this is the filename containing the Pre-Shared Keys (PSK) -to present to the server. While this is easier to set up than X.509, +to present to the server. While this is easier to set up than X.509, it requires that the PSK file be transmitted over a secure channel. =back =head1 EXAMPLES +=head2 Convert oldstyle server to encrypted newstyle + Expose the contents of an export served by an old style server over a Unix socket to TCP network clients that only want to consume encrypted -data. Use I<--exit-with-parent> to clean up nbdkit at the same time +data. Use I<--exit-with-parent> to clean up nbdkit at the same time that the old server exits. ( sock=`mktemp -u` @@ -148,23 +160,31 @@ that the old server exits. │ new client │ ────────▶│ nbdkit │ ───────────▶│ old server │ └────────────┘ TCP └────────┘ Unix └────────────┘ -Combine nbdkit's partition filter with qemu-nbd's ability to visit -qcow2 files (nbdkit does not have a native qcow2 plugin), performing -the same task as the deprecated C<qemu-nbd -P 1 -f qcow2 -/path/to/image.qcow2> command. Allow multiple clients, even though -C<qemu-nbd> without B<-t> normally quits after the first client, and -utilize a 5-second retry to give qemu-nbd time to create the socket: +=head2 Add nbdkit-partition-filter to qemu-nbd + +Combine nbdkit's partition filter with L<qemu-nbd(8)>’s ability to +visit qcow2 files (since nbdkit does not have a native qcow2 plugin). + +This performs the same task as the deprecated qemu-nbd I<-P> option: + + qemu-nbd -P 1 -f qcow2 /path/to/image.qcow2 + +Also this allows multiple clients, even though C<qemu-nbd> without +I<-t> normally quits after the first client, and utilizes a 5-second +retry to give qemu-nbd time to create the socket: ( sock=`mktemp -u` nbdkit --exit-with-parent --filter=partition nbd \ nbd+unix:///\?socket=$sock shared=1 retry=5 partition=1 & exec qemu-nbd -k $sock -f qcow2 /path/to/image.qcow2 ) -Conversely, expose the contents of export I<foo> from a new style -server with encrypted data to a client that can only consume -unencrypted old style. Use I<--run> to clean up nbdkit at the time the -client exits. In general, note that it is best to keep the plaintext -connection limited to a Unix socket on the local machine. +=head2 Convert newstyle server for oldstyle-only client + +Expose the contents of export C<foo> from a newstyle server with +encrypted data to a client that can only consume unencrypted old +style. Use I<--run> to clean up nbdkit at the time the client exits. +In general, note that it is best to keep the plaintext connection +limited to a Unix socket on the local machine. nbdkit -U - -o --tls=off nbd hostname=example.com export=foo tls=require \ --run '/path/to/oldclient --socket=$unixsocket' @@ -173,10 +193,16 @@ connection limited to a Unix socket on the local machine. │ old client │ ───────────▶│ nbdkit │ ────────▶│ new server │ └────────────┘ Unix └────────┘ TCP └────────────┘ -Learn which features are provided by libnbd by inspecting the -C<libnbd_*> lines: +=head1 DUMP PLUGIN OUTPUT - nbdkit --dump-plugin nbd +You can learn which features are provided by libnbd by inspecting the +C<libnbd_*> lines in I<--dump-plugin> output: + + $ nbdkit --dump-plugin nbd + [...] + libnbd_version=1.2.3 + libnbd_tls=1 + libnbd_uri=1 =head1 FILES @@ -202,7 +228,7 @@ L<nbdkit-filter(3)>, L<nbdkit-tls(1)>, L<nbdkit-plugin(3)>, L<libnbd(3)>, -L<qemu-nbd(1)>. +L<qemu-nbd(8)>. =head1 AUTHORS -- 2.25.0
On 7/1/20 2:17 AM, Richard W.M. Jones wrote:
* Change the title so it's informative and searchable.
* Remove references to the non-libnbd plugin.
* Headings for examples.
* Correct reference to qemu-nbd(8) man page.
* General copy-editing to improve readability.
* Change style in places so it matches other manual pages. --- plugins/nbd/nbdkit-nbd-plugin.pod | 192 +++++++++++++++++------------- 1 file changed, 109 insertions(+), 83 deletions(-)
+ +=item [B<uri=>]URI + +When C<uri> is supplied, decode C<URI> to determine the address to +connect to. A URI can specify a TCP connection (such as +C<nbd://localhost:10809/export>) or a Unix socket (such as +C<nbd+unix:///export?socket=/path/to/sock>). Remember you may need to +quote the parameter to protect it from the shell. + +C<uri=> is a magic config key and may be omitted in most +cases. See L<nbdkit(1)/Magic parameters>.
Missing mention that URI support requires a capable libnbd (contrast to how TLS support also requires a capable libnbd...
-The B<tls> parameter is only available when the plugin was compiled +The C<tls> parameter is only available when the plugin was compiled against libnbd with TLS support; C<nbdkit --dump-plugin nbd> will contain C<libnbd_tls=1> if this is the case. Note the difference between C<--tls=...> controlling what nbdkit serves, and C<tls=...>
...here. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Jul 06, 2020 at 02:27:58PM -0500, Eric Blake wrote:
On 7/1/20 2:17 AM, Richard W.M. Jones wrote:
* Change the title so it's informative and searchable.
* Remove references to the non-libnbd plugin.
* Headings for examples.
* Correct reference to qemu-nbd(8) man page.
* General copy-editing to improve readability.
* Change style in places so it matches other manual pages. --- plugins/nbd/nbdkit-nbd-plugin.pod | 192 +++++++++++++++++------------- 1 file changed, 109 insertions(+), 83 deletions(-)
+ +=item [B<uri=>]URI + +When C<uri> is supplied, decode C<URI> to determine the address to +connect to. A URI can specify a TCP connection (such as +C<nbd://localhost:10809/export>) or a Unix socket (such as +C<nbd+unix:///export?socket=/path/to/sock>). Remember you may need to +quote the parameter to protect it from the shell. + +C<uri=> is a magic config key and may be omitted in most +cases. See L<nbdkit(1)/Magic parameters>.
Missing mention that URI support requires a capable libnbd (contrast to how TLS support also requires a capable libnbd...
-The B<tls> parameter is only available when the plugin was compiled +The C<tls> parameter is only available when the plugin was compiled against libnbd with TLS support; C<nbdkit --dump-plugin nbd> will contain C<libnbd_tls=1> if this is the case. Note the difference between C<--tls=...> controlling what nbdkit serves, and C<tls=...>
...here.
Fixed now, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 7/1/20 2:17 AM, Richard W.M. Jones wrote:
* Change the title so it's informative and searchable.
* Remove references to the non-libnbd plugin.
* Headings for examples.
* Correct reference to qemu-nbd(8) man page.
* General copy-editing to improve readability.
* Change style in places so it matches other manual pages. --- plugins/nbd/nbdkit-nbd-plugin.pod | 192 +++++++++++++++++------------- 1 file changed, 109 insertions(+), 83 deletions(-)
diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod b/plugins/nbd/nbdkit-nbd-plugin.pod index 618058ac..5653703f 100644 --- a/plugins/nbd/nbdkit-nbd-plugin.pod +++ b/plugins/nbd/nbdkit-nbd-plugin.pod @@ -1,109 +1,119 @@ =head1 NAME
-nbdkit-nbd-plugin - nbdkit nbd plugin +nbdkit-nbd-plugin - proxy / forward to another NBD server
=head1 SYNOPSIS
- nbdkit nbd { socket=SOCKNAME | hostname=HOST [port=PORT] | [uri=]URI } - [export=NAME] [retry=N] [shared=BOOL] [tls=MODE] [tls-certificates=DIR] - [tls-verify=BOOL] [tls-username=NAME] [tls-psk=FILE] + nbdkit nbd { hostname=HOST [port=PORT] | + socket=SOCKNAME | + [uri=]URI }
Hmm - at one point I had started patches to add vsock_cid= as another connection option; I should rebase those on top of your patches adding command= connections. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
--- tests/test-nbd.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test-nbd.c b/tests/test-nbd.c index a8c4067e..2db7c035 100644 --- a/tests/test-nbd.c +++ b/tests/test-nbd.c @@ -52,11 +52,13 @@ main (int argc, char *argv[]) char *sockarg = NULL; size_t i, size; - /* If wrapping once is good, why not do it twice! Shows that we can - * convert between either style of server options. */ + /* Run an oldstyle nbdkit server. */ if (test_start_nbdkit ("-o", "file", "file-data", NULL) == -1) exit (EXIT_FAILURE); + /* Now run a second (newstyle) server connecting to the oldstyle + * server above. + */ if (asprintf (&sockarg, "socket=%s", sock) < 0) { perror ("asprintf"); exit (EXIT_FAILURE); @@ -65,6 +67,9 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); free (sockarg); + /* And now run another oldstyle server connecting to the newstyle + * server above, for a total of 3 nbdkit instances. + */ if (asprintf (&sockarg, "socket=%s", sock) < 0) { perror ("asprintf"); exit (EXIT_FAILURE); @@ -73,6 +78,7 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); free (sockarg); + /* Now connect to the front (oldstyle) server using libguestfs. */ g = guestfs_create (); if (g == NULL) { perror ("guestfs_create"); -- 2.25.0
This features lack a test, so add one. --- tests/test-nbd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test-nbd.c b/tests/test-nbd.c index 2db7c035..ebc9efa3 100644 --- a/tests/test-nbd.c +++ b/tests/test-nbd.c @@ -57,13 +57,15 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); /* Now run a second (newstyle) server connecting to the oldstyle - * server above. + * server above. Add shared=true on this handle to test that + * feature. */ if (asprintf (&sockarg, "socket=%s", sock) < 0) { perror ("asprintf"); exit (EXIT_FAILURE); } - if (test_start_nbdkit ("-e", "wrap", "nbd", sockarg, NULL) == -1) + if (test_start_nbdkit ("-e", "wrap", "nbd", sockarg, + "shared=true", NULL) == -1) exit (EXIT_FAILURE); free (sockarg); -- 2.25.0
Also: * Reorder if (uri) .. else if (sockname) .. else if (hostname) so that the same ordering is used when we create the connection later. * Moved the comments around to more closely reflect what the function actually does. This should be pure refactoring. --- plugins/nbd/nbd.c | 83 +++++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 38 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index b21590e6..fc2d7d0c 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -77,6 +77,9 @@ struct handle { pthread_t reader; }; +/* Connect to server via URI */ +static const char *uri; + /* Connect to server via absolute name of Unix socket */ static char *sockname; @@ -84,9 +87,6 @@ static char *sockname; static const char *hostname; static const char *port; -/* Connect to server via URI */ -static const char *uri; - /* Name of export on remote server, default '', ignored for oldstyle */ static const char *export; @@ -193,56 +193,60 @@ nbdplug_config (const char *key, const char *value) return 0; } -/* Check the user passed exactly one socket description. */ static int nbdplug_config_complete (void) { - if (sockname) { + int c = !!sockname + !!hostname + !!uri; + + /* Check the user passed exactly one connection parameter. */ + if (c > 1) { + nbdkit_error ("cannot mix Unix ‘socket’, TCP ‘hostname’/‘port’ " + "and ‘uri’ parameters"); + return -1; + } + if (c == 0) { + nbdkit_error ("exactly one of ‘socket’, ‘hostname’ " + "and ‘uri’ parameters must be specified"); + return -1; + } + + /* Port, if present, should only be used with hostname. */ + if (port && !hostname) { + nbdkit_error ("‘port’ parameter should only be used with ‘hostname’"); + return -1; + } + + if (uri) { + struct nbd_handle *nbd = nbd_create (); + + if (!nbd) { + nbdkit_error ("unable to query libnbd details: %s", nbd_get_error ()); + return -1; + } + if (!nbd_supports_uri (nbd)) { + nbdkit_error ("libnbd was compiled without uri support"); + nbd_close (nbd); + return -1; + } + nbd_close (nbd); + } + else if (sockname) { struct sockaddr_un sock; - if (hostname || port) { - nbdkit_error ("cannot mix Unix socket and TCP hostname/port parameters"); - return -1; - } - else if (uri) { - nbdkit_error ("cannot mix Unix socket and URI parameters"); - return -1; - } if (strlen (sockname) > sizeof sock.sun_path) { nbdkit_error ("socket file name too large"); return -1; } } else if (hostname) { - if (uri) { - nbdkit_error ("cannot mix TCP hostname/port and URI parameters"); - return -1; - } if (!port) port = "10809"; } - else if (uri) { - struct nbd_handle *nbd = nbd_create (); - - if (port) { - nbdkit_error ("cannot mix TCP hostname/port and URI parameters"); - return -1; - } - if (!nbd) { - nbdkit_error ("unable to query libnbd details: %s", nbd_get_error ()); - return -1; - } - if (!nbd_supports_uri (nbd)) { - nbdkit_error ("libnbd was compiled without uri support"); - nbd_close (nbd); - return -1; - } - nbd_close (nbd); - } else { - nbdkit_error ("must supply socket=, hostname= or uri= of external NBD server"); - return -1; + else { + abort (); /* can't happen, if checks above were correct */ } + /* Check the other parameters. */ if (!export) export = ""; @@ -264,6 +268,7 @@ nbdplug_config_complete (void) nbd_close (nbd); } + /* Create the shared connection. */ if (shared && (shared_handle = nbdplug_open_handle (false)) == NULL) return -1; return 0; @@ -485,8 +490,10 @@ nbdplug_open_handle (int readonly) r = nbd_connect_uri (h->nbd, uri); else if (sockname) r = nbd_connect_unix (h->nbd, sockname); - else + else if (hostname) r = nbd_connect_tcp (h->nbd, hostname, port); + else + abort (); if (r == -1) { if (retries--) { nbdkit_debug ("connect failed; will try again: %s", nbd_get_error ()); -- 2.25.0
Also fix the debugging message when it exits. --- plugins/nbd/nbd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index fc2d7d0c..20c06e2a 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -312,6 +312,8 @@ nbdplug_reader (void *handle) { struct handle *h = handle; + nbdkit_debug ("nbd: started reader thread"); + while (!nbd_aio_is_dead (h->nbd) && !nbd_aio_is_closed (h->nbd)) { struct pollfd fds[2] = { [0].fd = h->fd, @@ -348,7 +350,7 @@ nbdplug_reader (void *handle) } nbdkit_debug ("state machine changed to %s", nbd_connection_state (h->nbd)); - nbdkit_debug ("exiting state machine thread"); + nbdkit_debug ("exiting reader thread"); return NULL; } -- 2.25.0
It's not necessary to cache this, and the libnbd API doesn't guarantee that it always stays the same. --- plugins/nbd/nbd.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 20c06e2a..8997174e 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -71,7 +71,6 @@ struct transaction { struct handle { /* These fields are read-only once initialized */ struct nbd_handle *nbd; - int fd; /* Cache of nbd_aio_get_fd */ int fds[2]; /* Pipe for kicking the reader thread */ bool readonly; pthread_t reader; @@ -316,7 +315,7 @@ nbdplug_reader (void *handle) while (!nbd_aio_is_dead (h->nbd) && !nbd_aio_is_closed (h->nbd)) { struct pollfd fds[2] = { - [0].fd = h->fd, + [0].fd = nbd_aio_get_fd (h->nbd), [1].fd = h->fds[0], [1].events = POLLIN, }; @@ -469,7 +468,6 @@ nbdplug_open_handle (int readonly) #endif retry: - h->fd = -1; h->nbd = nbd_create (); if (!h->nbd) goto err; @@ -505,9 +503,6 @@ nbdplug_open_handle (int readonly) } goto err; } - h->fd = nbd_aio_get_fd (h->nbd); - if (h->fd == -1) - goto err; if (readonly) h->readonly = true; -- 2.25.0
On 7/1/20 2:17 AM, Richard W.M. Jones wrote:
It's not necessary to cache this, and the libnbd API doesn't guarantee that it always stays the same.
But should the libnbd API guarantee it? Then again, looking at the various examples in libnbd itself, I do see that they all check nbd_aio_get_fd() frequently, rather than caching it; and glib-main-loop.c even mentions: /* The poll file descriptor can change or become invalid at any * time. Okay, this change makes sense. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
As per the libnbd documentation, you must check nbd_aio_get_direction after poll returns. http://libguestfs.org/libnbd.3.html#Notifying-libnbd-when-an-event-happens I also added parentheses around & operators and added error checking. --- plugins/nbd/nbd.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 8997174e..05f78777 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -314,6 +314,7 @@ nbdplug_reader (void *handle) nbdkit_debug ("nbd: started reader thread"); while (!nbd_aio_is_dead (h->nbd) && !nbd_aio_is_closed (h->nbd)) { + int r; struct pollfd fds[2] = { [0].fd = nbd_aio_get_fd (h->nbd), [1].fd = h->fds[0], @@ -332,10 +333,16 @@ nbdplug_reader (void *handle) break; } - if (dir & LIBNBD_AIO_DIRECTION_READ && fds[0].revents & POLLIN) - nbd_aio_notify_read (h->nbd); - else if (dir & LIBNBD_AIO_DIRECTION_WRITE && fds[0].revents & POLLOUT) - nbd_aio_notify_write (h->nbd); + dir = nbd_aio_get_direction (h->nbd); + + if ((dir & LIBNBD_AIO_DIRECTION_READ) && (fds[0].revents & POLLIN)) + r = nbd_aio_notify_read (h->nbd); + else if ((dir & LIBNBD_AIO_DIRECTION_WRITE) && (fds[0].revents & POLLOUT)) + r = nbd_aio_notify_write (h->nbd); + if (r == -1) { + nbdkit_error ("%s", nbd_get_error ()); + break; + } /* Check if we were kicked because a command was started */ if (fds[1].revents & POLLIN) { -- 2.25.0
This option didn't work except when using --foreground (or using another option that implies this). This is because it creates a background reader thread, but that thread is invalidated if nbdkit forks, resulting in deadlocks because the worker threads are fruitlessly sending notifications to a background thread that no longer exists. The fix is to move this work to .after_fork. --- plugins/nbd/nbd.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 05f78777..91ed91e0 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -109,7 +109,7 @@ static void nbdplug_close_handle (struct handle *h); static void nbdplug_unload (void) { - if (shared) + if (shared && shared_handle) nbdplug_close_handle (shared_handle); free (sockname); free (tls_certificates); @@ -266,8 +266,15 @@ nbdplug_config_complete (void) } nbd_close (nbd); } + return 0; +} - /* Create the shared connection. */ +/* Create the shared connection. Because this may create a background + * thread it must be done after we fork. + */ +static int +nbdplug_after_fork (void) +{ if (shared && (shared_handle = nbdplug_open_handle (false)) == NULL) return -1; return 0; @@ -858,6 +865,7 @@ static struct nbdkit_plugin plugin = { .config_complete = nbdplug_config_complete, .config_help = nbdplug_config_help, .magic_config_key = "uri", + .after_fork = nbdplug_after_fork, .dump_plugin = nbdplug_dump_plugin, .open = nbdplug_open, .close = nbdplug_close, -- 2.25.0
On 7/1/20 2:17 AM, Richard W.M. Jones wrote:
This option didn't work except when using --foreground (or using another option that implies this). This is because it creates a background reader thread, but that thread is invalidated if nbdkit forks, resulting in deadlocks because the worker threads are fruitlessly sending notifications to a background thread that no longer exists.
The fix is to move this work to .after_fork. --- plugins/nbd/nbd.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
Good catch.
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 05f78777..91ed91e0 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -109,7 +109,7 @@ static void nbdplug_close_handle (struct handle *h); static void nbdplug_unload (void) { - if (shared) + if (shared && shared_handle) nbdplug_close_handle (shared_handle); free (sockname); free (tls_certificates); @@ -266,8 +266,15 @@ nbdplug_config_complete (void) } nbd_close (nbd); } + return 0; +}
- /* Create the shared connection. */ +/* Create the shared connection. Because this may create a background + * thread it must be done after we fork. + */ +static int +nbdplug_after_fork (void) +{ if (shared && (shared_handle = nbdplug_open_handle (false)) == NULL) return -1; return 0; @@ -858,6 +865,7 @@ static struct nbdkit_plugin plugin = { .config_complete = nbdplug_config_complete, .config_help = nbdplug_config_help, .magic_config_key = "uri", + .after_fork = nbdplug_after_fork, .dump_plugin = nbdplug_dump_plugin, .open = nbdplug_open, .close = nbdplug_close,
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
* command=qemu-nbd arg=-f arg=qcow2 arg=/path/to/qcow2 runs qemu-nbd (using systemd socket activation) as a subprocess. * socket-fd=5 allows the parent process to set up the NBD socket and pass the file descriptor down to nbdkit. These correspond closely to the libnbd functions nbd_connect_systemd_socket_activation and nbd_connect_socket. There is another function, nbd_connect_command, for connecting to a subprocess using stdin/stdout, but it didn't seem worth wiring that up at the moment as it can only be used to run a child nbdkit process, and it's unclear why that would be useful for end users (perhaps more useful for testing however). --- plugins/nbd/nbdkit-nbd-plugin.pod | 73 ++++++++++++++++++++++++------- tests/Makefile.am | 2 + plugins/nbd/nbd.c | 51 +++++++++++++++++++-- tests/test-nbd-qcow2.sh | 67 ++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 21 deletions(-) diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod b/plugins/nbd/nbdkit-nbd-plugin.pod index 5653703f..59cb8f32 100644 --- a/plugins/nbd/nbdkit-nbd-plugin.pod +++ b/plugins/nbd/nbdkit-nbd-plugin.pod @@ -4,8 +4,10 @@ nbdkit-nbd-plugin - proxy / forward to another NBD server =head1 SYNOPSIS - nbdkit nbd { hostname=HOST [port=PORT] | + nbdkit nbd { command=COMMAND [arg=ARG [...]] | + hostname=HOST [port=PORT] | socket=SOCKNAME | + socket-fd=FD | [uri=]URI } [export=NAME] [retry=N] [shared=BOOL] [tls=MODE] [tls-certificates=DIR] [tls-verify=BOOL] @@ -38,11 +40,32 @@ With L<qemu-nbd(8)>, read and write qcow2 files with nbdkit. =head1 PARAMETERS -One of B<socket>, B<hostname> (optionally with B<port>), or B<uri> -must be given to specify which NBD server to forward to: +One of B<socket>, B<hostname> (optionally with B<port>), B<uri>, +B<socket-fd> or B<command> must be given to specify which NBD server +to forward to: =over 4 +=item B<command=>COMMAND + +=item B<arg=>ARG + +Run an NBD server, usually L<qemu-nbd(8)>, as an external command. +See L</EXAMPLES> below. + +C<COMMAND> is the program to run, followed by zero or more C<arg=ARG> +parameters for each argument. For example: + + nbdkit nbd command=qemu-nbd arg=-f arg=qcow2 arg=$PWD/disk.qcow2 + +would run the command C<qemu-nbd -f qcow2 $PWD/disk.qcow2>. Because +nbdkit may change directory before running the command, you may need +to ensure that all file paths used in parameters (like the disk name +above) are absolute paths. + +This uses the libnbd API L<nbd_connect_systemd_socket_activation(3)>. +This option implies C<shared=true>. + =item B<hostname=>HOST Connect to the NBD server at the remote C<HOST> using a TCP socket. @@ -56,6 +79,12 @@ When C<hostname> is supplied, use C<PORT> instead of the default port Connect to the NBD server using Unix domain socket C<SOCKNAME>. +=item B<socket-fd=>FD + +Connect to the NBD server over a socket file descriptor inherited by +nbdkit. This uses the libnbd API L<nbd_connect_socket(3)>. This +option implies C<shared=true>. + =item [B<uri=>]URI When C<uri> is supplied, decode C<URI> to determine the address to @@ -85,11 +114,16 @@ embedded in the URI instead. If the initial connection attempt to the server fails, retry up to C<N> times more after a one-second delay between tries (default 0). +=item B<shared=false> + =item B<shared=true> -By default the plugin will open a new connection to the server for -each client making a connection to nbdkit. The remote server does not -have to be started until immediately before the first nbdkit client +If using C<command> or C<socket-fd> modes then this defaults to true, +otherwise false. + +If false the plugin will open a new connection to the server for each +client making a connection to nbdkit. The remote server does not have +to be started until immediately before the first nbdkit client connects. If this parameter is set to C<true>, the plugin will open a single @@ -160,24 +194,29 @@ that the old server exits. │ new client │ ────────▶│ nbdkit │ ───────────▶│ old server │ └────────────┘ TCP └────────┘ Unix └────────────┘ +=head2 Use qemu-nbd to open a qcow2 file + +Run qemu-nbd as the server, allowing you to read and write qcow2 files +(since nbdkit does not have a native qcow2 plugin). This allows you +to use nbdkit filters on top, see the next example. + + nbdkit nbd command=qemu-nbd arg=-f arg=qcow2 arg=/path/to/image.qcow2 + +qemu-nbd is cleaned up when nbdkit exits. + =head2 Add nbdkit-partition-filter to qemu-nbd -Combine nbdkit's partition filter with L<qemu-nbd(8)>’s ability to -visit qcow2 files (since nbdkit does not have a native qcow2 plugin). +Combine L<nbdkit-partition-filter(1)> with L<qemu-nbd(8)>’s ability to +visit qcow2 files: + + nbdkit --filter=partition nbd \ + command=qemu-nbd arg=-f arg=qcow2 arg=/path/to/image.qcow2 \ + partition=1 This performs the same task as the deprecated qemu-nbd I<-P> option: qemu-nbd -P 1 -f qcow2 /path/to/image.qcow2 -Also this allows multiple clients, even though C<qemu-nbd> without -I<-t> normally quits after the first client, and utilizes a 5-second -retry to give qemu-nbd time to create the socket: - - ( sock=`mktemp -u` - nbdkit --exit-with-parent --filter=partition nbd \ - nbd+unix:///\?socket=$sock shared=1 retry=5 partition=1 & - exec qemu-nbd -k $sock -f qcow2 /path/to/image.qcow2 ) - =head2 Convert newstyle server for oldstyle-only client Expose the contents of export C<foo> from a newstyle server with diff --git a/tests/Makefile.am b/tests/Makefile.am index 9ab6a7af..ea3a5b6e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -681,11 +681,13 @@ if HAVE_LIBNBD LIBGUESTFS_TESTS += test-nbd TESTS += \ test-nbd-extents.sh \ + test-nbd-qcow2.sh \ test-nbd-tls.sh \ test-nbd-tls-psk.sh \ $(NULL) EXTRA_DIST += \ test-nbd-extents.sh \ + test-nbd-qcow2.sh \ test-nbd-tls.sh \ test-nbd-tls-psk.sh \ $(NULL) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 91ed91e0..e446d52b 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -57,6 +57,9 @@ #include "byte-swapping.h" #include "cleanup.h" #include "utils.h" +#include "vector.h" + +DEFINE_VECTOR_TYPE(string_vector, const char *); /* The per-transaction details */ struct transaction { @@ -86,6 +89,12 @@ static char *sockname; static const char *hostname; static const char *port; +/* Connect to a command. */ +static string_vector command = empty_vector; + +/* Connect to a socket file descriptor. */ +static int socket_fd = -1; + /* Name of export on remote server, default '', ignored for oldstyle */ static const char *export; @@ -114,6 +123,7 @@ nbdplug_unload (void) free (sockname); free (tls_certificates); free (tls_psk); + free (command.ptr); /* the strings are statically allocated */ } /* Called for each key=value passed on the command line. This plugin @@ -140,6 +150,20 @@ nbdplug_config (const char *key, const char *value) port = value; else if (strcmp (key, "uri") == 0) uri = value; + else if (strcmp (key, "command") == 0 || strcmp (key, "arg") == 0) { + if (string_vector_append (&command, value) == -1) { + nbdkit_error ("realloc: %m"); + return -1; + } + } + else if (strcmp (key, "socket-fd") == 0) { + if (nbdkit_parse_int ("socket-fd", value, &socket_fd) == -1) + return -1; + if (socket_fd < 0) { + nbdkit_error ("socket-fd must be >= 0"); + return -1; + } + } else if (strcmp (key, "export") == 0) export = value; else if (strcmp (key, "retry") == 0) { @@ -195,16 +219,17 @@ nbdplug_config (const char *key, const char *value) static int nbdplug_config_complete (void) { - int c = !!sockname + !!hostname + !!uri; + int c = !!sockname + !!hostname + !!uri + + (command.size > 0) + (socket_fd >= 0); /* Check the user passed exactly one connection parameter. */ if (c > 1) { - nbdkit_error ("cannot mix Unix ‘socket’, TCP ‘hostname’/‘port’ " - "and ‘uri’ parameters"); + nbdkit_error ("cannot mix Unix ‘socket’, TCP ‘hostname’/‘port’, " + "‘command’, ‘socket-fd’ and ‘uri’ parameters"); return -1; } if (c == 0) { - nbdkit_error ("exactly one of ‘socket’, ‘hostname’ " + nbdkit_error ("exactly one of ‘socket’, ‘hostname’, ‘command’, ‘socket-fd’ " "and ‘uri’ parameters must be specified"); return -1; } @@ -241,6 +266,17 @@ nbdplug_config_complete (void) if (!port) port = "10809"; } + else if (command.size > 0) { + /* Add NULL sentinel to the command. */ + if (string_vector_append (&command, NULL) == -1) { + nbdkit_error ("realloc: %m"); + return -1; + } + shared = true; + } + else if (socket_fd >= 0) { + shared = true; + } else { abort (); /* can't happen, if checks above were correct */ } @@ -285,6 +321,9 @@ nbdplug_after_fork (void) "socket=<SOCKNAME> The Unix socket to connect to.\n" \ "hostname=<HOST> The hostname for the TCP socket to connect to.\n" \ "port=<PORT> TCP port or service name to use (default 10809).\n" \ + "command=<COMMAND> Command to run.\n" \ + "arg=<ARG> Parameters for command.\n" \ + "socket-fd=<FD> Socket file descriptor to connect to.\n" \ "export=<NAME> Export name to connect to (default \"\").\n" \ "retry=<N> Retry connection up to N seconds (default 0).\n" \ "shared=<BOOL> True to share one server connection among all clients,\n" \ @@ -506,6 +545,10 @@ nbdplug_open_handle (int readonly) r = nbd_connect_unix (h->nbd, sockname); else if (hostname) r = nbd_connect_tcp (h->nbd, hostname, port); + else if (command.size > 0) + r = nbd_connect_systemd_socket_activation (h->nbd, (char **) command.ptr); + else if (socket_fd >= 0) + r = nbd_connect_socket (h->nbd, socket_fd); else abort (); if (r == -1) { diff --git a/tests/test-nbd-qcow2.sh b/tests/test-nbd-qcow2.sh new file mode 100755 index 00000000..72aac544 --- /dev/null +++ b/tests/test-nbd-qcow2.sh @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2018-2020 Red Hat Inc. +# +# 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. + +source ./functions.sh +set -e +set -x + +requires test -f disk +requires guestfish --version +requires qemu-img --version +requires qemu-nbd --version + +disk=nbd-qcow2-disk.qcow2 +pid=nbd-qcow2.pid +sock=`mktemp -u` +files="$disk $pid $sock" +rm -f $files +cleanup_fn rm -f $files + +# Create a qcow2 disk for testing. +qemu-img convert -f raw disk -O qcow2 $disk + +# Run qemu-nbd via nbdkit with the partition filter. +start_nbdkit -P $pid -U $sock \ + --filter=partition \ + nbd command=qemu-nbd arg=-f arg=qcow2 arg="$PWD/$disk" \ + partition=1 + +qemu-img info "nbd:unix:$sock" + +# See if we can open the disk which should be unpartitioned. +guestfish -v -x --format=raw -a "nbd://?socket=$sock" -m /dev/sda <<EOF + cat /hello.txt + + # Write something. + write /test.txt "hello" + cat /test.txt +EOF -- 2.25.0
On Wed, Jul 1, 2020 at 10:17 AM Richard W.M. Jones <rjones@redhat.com> wrote:
I fixed the deadlock - turned out to be an actual bug in the nbd plugin (see patch 8).
I changed the command syntax so it's now:
nbdkit nbd command=qemu arg=-f arg=qcow2 arg=/path/to/disk.qcow2
Nir wrote:
18:08 < nsoffer> rwmjones: regarding the nbd proxy patches, did you have specific flow that help us? 18:08 < nsoffer> rwmjones: or this is just a way to support qcow2 in the nbdkit pipeline?
A bit of both - it's nice if nbdkit can "open" qcow2 files.
Having libqcow2 would be even better, but since we don't have it this seems like a very useful feature.
On Thu, Jul 02, 2020 at 02:51:48PM +0300, Nir Soffer wrote:
On Wed, Jul 1, 2020 at 10:17 AM Richard W.M. Jones <rjones@redhat.com> wrote:
I fixed the deadlock - turned out to be an actual bug in the nbd plugin (see patch 8).
I changed the command syntax so it's now:
nbdkit nbd command=qemu arg=-f arg=qcow2 arg=/path/to/disk.qcow2
Nir wrote:
18:08 < nsoffer> rwmjones: regarding the nbd proxy patches, did you have specific flow that help us? 18:08 < nsoffer> rwmjones: or this is just a way to support qcow2 in the nbdkit pipeline?
A bit of both - it's nice if nbdkit can "open" qcow2 files.
Having libqcow2 would be even better, but since we don't have it this seems like a very useful feature.
I guess that "libqcow2" is unlikely to happen because the existing code depends on internals of qemu (things like QMP, QOM and coroutines), and hopefully no one would be foolhardy enough to write a second qcow2 implementation. Maybe that's changed since the qemu-storage-daemon was implemented? Anyway, this does seem like the best we can do in nbdkit. 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
participants (3)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones