Libguestfs - Jul 2020 - Re: [nbdkit PATCH] nbd: Add vsock-cid= transport option

With new enough libnbd, we already support vsock by virtue of uri=;
however, it's also nice to allow direct exposure of the
nbd_connect_vsock() api.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/nbd/nbdkit-nbd-plugin.pod | 29 +++++++++++---
 plugins/nbd/nbd.c                 | 63 ++++++++++++++++++++++++-------
 2 files changed, 73 insertions(+), 19 deletions(-)

diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod
b/plugins/nbd/nbdkit-nbd-plugin.pod
index e4ac40d1..bd980209 100644
--- a/plugins/nbd/nbdkit-nbd-plugin.pod
+++ b/plugins/nbd/nbdkit-nbd-plugin.pod
@@ -6,6 +6,7 @@ nbdkit-nbd-plugin - proxy / forward to another NBD server

  nbdkit nbd { command=COMMAND [arg=ARG [...]] |
               hostname=HOST [port=PORT] |
+              vhost-cid=CID [port=PORT] |
               socket=SOCKNAME |
               socket-fd=FD |
               [uri=]URI }
@@ -40,9 +41,9 @@ With L<qemu-nbd(8)>, read and write qcow2 files with
nbdkit.

 =head1 PARAMETERS

-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:
+One of B<socket>, B<hostname> (optionally with B<port>),
B<vsock-cid>
+(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

@@ -71,7 +72,22 @@ This option implies C<shared=true>.
 =item B<port=>PORT

 Connect to the NBD server at the remote C<HOST> using a TCP socket.
-The optional port parameter overrides the default port (10809).
+The optional port parameter overrides the default port (10809), and
+may be a 16-bit number or a non-numeric string to look up the
+well-known port for a service name.
+
+=item B<vhost-cid=>CID
+
+=item B<port=>PORT
+
+Connect to the NBD server at the given vsock C<CID> (for example, in a
+guest VM, using the cid 2 will connect to a server in the host).  The
+optional port parameter overrides the default port (10809), and must
+be a 32-bit number.  This only works for platforms with the
+C<AF_VSOCK> family of sockets and libnbd new enough to use it;
+C<nbdkit --dump-plugin nbd> will contain C<libnbd_vsock=1> if this
is
+the case.  For more details on AF_VSOCK, see
+L<nbdkit-service(1)/AF_VSOCK>.

 =item B<socket=>SOCKNAME

@@ -87,8 +103,9 @@ option implies C<shared=true>.

 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
+C<nbd://localhost:10809/export>), a Unix socket (such as
+C<nbd+unix:///export?socket=/path/to/sock>), or a vsock connection
+(such as C<nbd+vsock:///2:10809/export>).  Remember you may need to
 quote the parameter to protect it from the shell.

 The C<uri> parameter is only available when the plugin was compiled
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index e446d52b..3f84375b 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -46,6 +46,7 @@
 #include <semaphore.h>
 #include <poll.h>
 #include <fcntl.h>
+#include <sys/socket.h>

 #include <libnbd.h>

@@ -61,6 +62,12 @@

 DEFINE_VECTOR_TYPE(string_vector, const char *);

+#if !defined AF_VSOCK || !LIBNBD_HAVE_NBD_CONNECT_VSOCK
+#define USE_VSOCK 0
+#else
+#define USE_VSOCK 1
+#endif
+
 /* The per-transaction details */
 struct transaction {
   int64_t cookie;
@@ -87,8 +94,15 @@ static char *sockname;

 /* Connect to server via TCP socket */
 static const char *hostname;
+
+/* Valid with TCP or VSOCK */
 static const char *port;

+/* Connect to server via AF_VSOCK socket */
+static const char *raw_cid;
+static uint32_t cid;
+static uint32_t vport;
+
 /* Connect to a command. */
 static string_vector command = empty_vector;

@@ -126,11 +140,8 @@ nbdplug_unload (void)
   free (command.ptr); /* the strings are statically allocated */
 }

-/* Called for each key=value passed on the command line.  This plugin
- * accepts socket=<sockname>,
hostname=<hostname>/port=<port>, or
- * [uri=]<uri> (exactly one connection required), and optional
- * parameters export=<name>, retry=<n>, shared=<bool> and
various
- * tls settings.
+/* Called for each key=value passed on the command line.  See
+ * nbdplug_config_help for the various keys recognized.
  */
 static int
 nbdplug_config (const char *key, const char *value)
@@ -148,6 +159,10 @@ nbdplug_config (const char *key, const char *value)
     hostname = value;
   else if (strcmp (key, "port") == 0)
     port = value;
+  else if (strcmp (key, "vsock_cid") == 0 ||
+           strcmp (key, "vsock-cid") == 0 ||
+           strcmp (key, "cid") == 0)
+    raw_cid = value;
   else if (strcmp (key, "uri") == 0)
     uri = value;
   else if (strcmp (key, "command") == 0 || strcmp (key,
"arg") == 0) {
@@ -220,23 +235,25 @@ static int
 nbdplug_config_complete (void)
 {
   int c = !!sockname + !!hostname + !!uri +
-    (command.size > 0) + (socket_fd >= 0);
+    (command.size > 0) + (socket_fd >= 0) + !!raw_cid;

   /* Check the user passed exactly one connection parameter. */
   if (c > 1) {
     nbdkit_error ("cannot mix Unix ‘socket’, TCP ‘hostname’/‘port’, "
-                  "‘command’, ‘socket-fd’ and ‘uri’ parameters");
+                  "‘vhost-cid’, ‘command’, ‘socket-fd’ and ‘uri’
parameters");
     return -1;
   }
   if (c == 0) {
-    nbdkit_error ("exactly one of ‘socket’, ‘hostname’, ‘command’,
‘socket-fd’ "
-                  "and ‘uri’ parameters must be specified");
+    nbdkit_error ("exactly one of ‘socket’, ‘hostname’, ‘vhost-cid’,
"
+                  "‘command’, ‘socket-fd’ 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’");
+  /* Port, if present, should only be used with hostname or vsock-cid. */
+  if (port && !(hostname || raw_cid)) {
+    nbdkit_error ("‘port’ parameter should only be used with ‘hostname’ or
"
+                  "‘vsock-cid’");
     return -1;
   }

@@ -266,6 +283,18 @@ nbdplug_config_complete (void)
     if (!port)
       port = "10809";
   }
+  else if (raw_cid) {
+#if !USE_VSOCK
+    nbdkit_error ("libnbd was compiled without vsock support");
+    return -1;
+#else
+    if (!port)
+      port = "10809";
+    if (nbdkit_parse_uint32_t ("vsock_cid", raw_cid, &cid) == -1
||
+        nbdkit_parse_uint32_t ("port", port, &vport) == -1)
+      return -1;
+#endif
+  }
   else if (command.size > 0) {
     /* Add NULL sentinel to the command. */
     if (string_vector_append (&command, NULL) == -1) {
@@ -320,7 +349,8 @@ nbdplug_after_fork (void)
   "[uri=]<URI>            URI of an NBD socket to connect to (if
supported).\n" \
   "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" \
+  "port=<PORT>            TCP/VHOST port or service name to use
(default 10809).\n" \
+  "vhost_cid=<CID>        The cid for the VSOCK socket to connect
to.\n" \
   "command=<COMMAND>      Command to run.\n" \
   "arg=<ARG>              Parameters for command.\n" \
   "socket-fd=<FD>         Socket file descriptor to connect
to.\n" \
@@ -346,6 +376,7 @@ nbdplug_dump_plugin (void)
   printf ("libnbd_version=%s\n", nbd_get_version (nbd));
   printf ("libnbd_tls=%d\n", nbd_supports_tls (nbd));
   printf ("libnbd_uri=%d\n", nbd_supports_uri (nbd));
+  printf ("libnbd_vsock=%d\n", USE_VSOCK);
   nbd_close (nbd);
 }

@@ -545,6 +576,12 @@ 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 (raw_cid)
+#if !USE_VSOCK
+    abort ();
+#else
+    r = nbd_connect_vsock (h->nbd, cid, vport);
+#endif
   else if (command.size > 0)
     r = nbd_connect_systemd_socket_activation (h->nbd, (char **)
command.ptr);
   else if (socket_fd >= 0)
-- 
2.27.0

On Tue, Jul 07, 2020 at 08:17:31AM -0500, Eric Blake
wrote:
> With new enough libnbd, we already support vsock by virtue of uri=;
> however, it's also nice to allow direct exposure of the
> nbd_connect_vsock() api.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/nbd/nbdkit-nbd-plugin.pod | 29 +++++++++++---
>  plugins/nbd/nbd.c                 | 63 ++++++++++++++++++++++++-------
>  2 files changed, 73 insertions(+), 19 deletions(-)
> 
> diff --git a/plugins/nbd/nbdkit-nbd-plugin.pod
b/plugins/nbd/nbdkit-nbd-plugin.pod
> index e4ac40d1..bd980209 100644
> --- a/plugins/nbd/nbdkit-nbd-plugin.pod
> +++ b/plugins/nbd/nbdkit-nbd-plugin.pod
> @@ -6,6 +6,7 @@ nbdkit-nbd-plugin - proxy / forward to another NBD server
> 
>   nbdkit nbd { command=COMMAND [arg=ARG [...]] |
>                hostname=HOST [port=PORT] |
> +              vhost-cid=CID [port=PORT] |
>                socket=SOCKNAME |
>                socket-fd=FD |
>                [uri=]URI }
> @@ -40,9 +41,9 @@ With L<qemu-nbd(8)>, read and write qcow2 files
with nbdkit.
> 
>  =head1 PARAMETERS
> 
> -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:
> +One of B<socket>, B<hostname> (optionally with B<port>),
B<vsock-cid>
> +(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
> 
> @@ -71,7 +72,22 @@ This option implies C<shared=true>.
>  =item B<port=>PORT
> 
>  Connect to the NBD server at the remote C<HOST> using a TCP socket.
> -The optional port parameter overrides the default port (10809).
> +The optional port parameter overrides the default port (10809), and
> +may be a 16-bit number or a non-numeric string to look up the
> +well-known port for a service name.
> +
> +=item B<vhost-cid=>CID
> +
> +=item B<port=>PORT
> +
> +Connect to the NBD server at the given vsock C<CID> (for example, in
a
> +guest VM, using the cid 2 will connect to a server in the host).  The
> +optional port parameter overrides the default port (10809), and must
> +be a 32-bit number.  This only works for platforms with the
> +C<AF_VSOCK> family of sockets and libnbd new enough to use it;
> +C<nbdkit --dump-plugin nbd> will contain C<libnbd_vsock=1> if
this is
> +the case.  For more details on AF_VSOCK, see
> +L<nbdkit-service(1)/AF_VSOCK>.
> 
>  =item B<socket=>SOCKNAME
> 
> @@ -87,8 +103,9 @@ option implies C<shared=true>.
> 
>  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
> +C<nbd://localhost:10809/export>), a Unix socket (such as
> +C<nbd+unix:///export?socket=/path/to/sock>), or a vsock connection
> +(such as C<nbd+vsock:///2:10809/export>).  Remember you may need to
>  quote the parameter to protect it from the shell.
> 
>  The C<uri> parameter is only available when the plugin was compiled
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index e446d52b..3f84375b 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -46,6 +46,7 @@
>  #include <semaphore.h>
>  #include <poll.h>
>  #include <fcntl.h>
> +#include <sys/socket.h>
> 
>  #include <libnbd.h>
> 
> @@ -61,6 +62,12 @@
> 
>  DEFINE_VECTOR_TYPE(string_vector, const char *);
> 
> +#if !defined AF_VSOCK || !LIBNBD_HAVE_NBD_CONNECT_VSOCK
> +#define USE_VSOCK 0
> +#else
> +#define USE_VSOCK 1
> +#endif
> +
>  /* The per-transaction details */
>  struct transaction {
>    int64_t cookie;
> @@ -87,8 +94,15 @@ static char *sockname;
> 
>  /* Connect to server via TCP socket */
>  static const char *hostname;
> +
> +/* Valid with TCP or VSOCK */
>  static const char *port;
> 
> +/* Connect to server via AF_VSOCK socket */
> +static const char *raw_cid;
> +static uint32_t cid;
> +static uint32_t vport;
> +
>  /* Connect to a command. */
>  static string_vector command = empty_vector;
> 
> @@ -126,11 +140,8 @@ nbdplug_unload (void)
>    free (command.ptr); /* the strings are statically allocated */
>  }
> 
> -/* Called for each key=value passed on the command line.  This plugin
> - * accepts socket=<sockname>,
hostname=<hostname>/port=<port>, or
> - * [uri=]<uri> (exactly one connection required), and optional
> - * parameters export=<name>, retry=<n>, shared=<bool>
and various
> - * tls settings.
> +/* Called for each key=value passed on the command line.  See
> + * nbdplug_config_help for the various keys recognized.
>   */
>  static int
>  nbdplug_config (const char *key, const char *value)
> @@ -148,6 +159,10 @@ nbdplug_config (const char *key, const char *value)
>      hostname = value;
>    else if (strcmp (key, "port") == 0)
>      port = value;
> +  else if (strcmp (key, "vsock_cid") == 0 ||
> +           strcmp (key, "vsock-cid") == 0 ||
> +           strcmp (key, "cid") == 0)
> +    raw_cid = value;
>    else if (strcmp (key, "uri") == 0)
>      uri = value;
>    else if (strcmp (key, "command") == 0 || strcmp (key,
"arg") == 0) {
> @@ -220,23 +235,25 @@ static int
>  nbdplug_config_complete (void)
>  {
>    int c = !!sockname + !!hostname + !!uri +
> -    (command.size > 0) + (socket_fd >= 0);
> +    (command.size > 0) + (socket_fd >= 0) + !!raw_cid;
> 
>    /* Check the user passed exactly one connection parameter. */
>    if (c > 1) {
>      nbdkit_error ("cannot mix Unix ‘socket’, TCP ‘hostname’/‘port’,
"
> -                  "‘command’, ‘socket-fd’ and ‘uri’
parameters");
> +                  "‘vhost-cid’, ‘command’, ‘socket-fd’ and ‘uri’
parameters");
>      return -1;
>    }
>    if (c == 0) {
> -    nbdkit_error ("exactly one of ‘socket’, ‘hostname’, ‘command’,
‘socket-fd’ "
> -                  "and ‘uri’ parameters must be specified");
> +    nbdkit_error ("exactly one of ‘socket’, ‘hostname’, ‘vhost-cid’,
"
> +                  "‘command’, ‘socket-fd’ 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’");
> +  /* Port, if present, should only be used with hostname or vsock-cid. */
> +  if (port && !(hostname || raw_cid)) {
> +    nbdkit_error ("‘port’ parameter should only be used with
‘hostname’ or "
> +                  "‘vsock-cid’");
>      return -1;
>    }
> 
> @@ -266,6 +283,18 @@ nbdplug_config_complete (void)
>      if (!port)
>        port = "10809";
>    }
> +  else if (raw_cid) {
> +#if !USE_VSOCK
> +    nbdkit_error ("libnbd was compiled without vsock support");
> +    return -1;
> +#else
> +    if (!port)
> +      port = "10809";
> +    if (nbdkit_parse_uint32_t ("vsock_cid", raw_cid, &cid)
== -1 ||
> +        nbdkit_parse_uint32_t ("port", port, &vport) == -1)
> +      return -1;
> +#endif
> +  }
>    else if (command.size > 0) {
>      /* Add NULL sentinel to the command. */
>      if (string_vector_append (&command, NULL) == -1) {
> @@ -320,7 +349,8 @@ nbdplug_after_fork (void)
>    "[uri=]<URI>            URI of an NBD socket to connect to
(if supported).\n" \
>    "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" \
> +  "port=<PORT>            TCP/VHOST port or service name to use
(default 10809).\n" \
> +  "vhost_cid=<CID>        The cid for the VSOCK socket to
connect to.\n" \
>    "command=<COMMAND>      Command to run.\n" \
>    "arg=<ARG>              Parameters for command.\n" \
>    "socket-fd=<FD>         Socket file descriptor to connect
to.\n" \
> @@ -346,6 +376,7 @@ nbdplug_dump_plugin (void)
>    printf ("libnbd_version=%s\n", nbd_get_version (nbd));
>    printf ("libnbd_tls=%d\n", nbd_supports_tls (nbd));
>    printf ("libnbd_uri=%d\n", nbd_supports_uri (nbd));
> +  printf ("libnbd_vsock=%d\n", USE_VSOCK);
>    nbd_close (nbd);
>  }
> 
> @@ -545,6 +576,12 @@ 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 (raw_cid)
> +#if !USE_VSOCK
> +    abort ();
> +#else
> +    r = nbd_connect_vsock (h->nbd, cid, vport);
> +#endif
>    else if (command.size > 0)
>      r = nbd_connect_systemd_socket_activation (h->nbd, (char **)
command.ptr);
>    else if (socket_fd >= 0)
I wonder if we should just call the flag "vsock=..."?

However patch looks good, ACK.

It may be possible to add a test.  See this example, especially the
line requires_linux_kernel_version.

https://github.com/libguestfs/nbdkit/blob/master/tests/test-vsock.sh

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/7/20 11:26 AM, Richard W.M. Jones wrote:
> On Tue, Jul 07, 2020 at 08:17:31AM -0500, Eric Blake wrote:
>> With new enough libnbd, we already support vsock by virtue of uri=;
>> however, it's also nice to allow direct exposure of the
>> nbd_connect_vsock() api.
>>
>> @@ -320,7 +349,8 @@ nbdplug_after_fork (void)
>>     "[uri=]<URI>            URI of an NBD socket to connect
to (if supported).\n" \
>>     "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" \
>> +  "port=<PORT>            TCP/VHOST port or service name to
use (default 10809).\n" \
>> +  "vhost_cid=<CID>        The cid for the VSOCK socket to
connect to.\n" \
>> +  else if (raw_cid)
>> +#if !USE_VSOCK
>> +    abort ();
>> +#else
>> +    r = nbd_connect_vsock (h->nbd, cid, vport);
>> +#endif
>>     else if (command.size > 0)
>>       r = nbd_connect_systemd_socket_activation (h->nbd, (char **)
command.ptr);
>>     else if (socket_fd >= 0)
> 
> I wonder if we should just call the flag "vsock=..."?
Sure, that makes sense.  Doing that for nbdkit as client over vsock 
matches 'nbdkit --vsock' in the other direction for offering server over
vsock.
> 
> However patch looks good, ACK.
> 
> It may be possible to add a test.  See this example, especially the
> line requires_linux_kernel_version.
> 
> https://github.com/libguestfs/nbdkit/blob/master/tests/test-vsock.sh
Thanks; the test will be a separate commit, but I'll play with the idea.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org