Skip to contentSkip to conversation

Implement TLS support to QEMU NBD server & client

PATCH Applied Daniel P. Berrange 4 participants 23 messages Feb 10, 2016 – Feb 16, 2016

Subsystems (2)

v6 of 7 Applied Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v6 cover Implement TLS support to QEMU NBD server & client

This is an update of the series previously posted:

v1: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06126.html

    v2: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01580.html
      v3: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03440.html
        v4: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04160.html

This series of patches implements support for TLS in the QEMU NBD server and client code.

It is implementing the NBD_OPT_STARTTLS option that was previously discussed here:

https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html

And is also described in the NBD spec here:

https://github.com/yoe/nbd/blob/master/doc/proto.md

To ensure that clients always get a suitable error message from the NBD server when it is configured with TLS, a client speaking the new style protocol will always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response. This should all be backwards & forwards compatible with previous QEMU impls of NBD

Usage of TLS is described in the commit messages for each patch, but for sake of people who don't want to explore the series, here's the summary

Starting QEMU system emulator with a disk backed by an TLS encrypted NBD export

  $ qemu-system-x86_64 \
     -object tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls \
     -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

Starting a standalone NBD server providing a TLS encrypted NBD export

  $ qemu-nbd \
     --object tls-creds-x509,id=tls0,endpoint=server,dir=/home/berrange/security/qemutls
     --tls-creds tls0 \
     --export-name default \
     $IMAGEFILE

The --export-name is optional, if omitted, the default "" will be used.

Starting a QEMU system emulator built-in NBD server

  $ qemu-system-x86_64 \
     -qmp unix:/tmp/qmp,server \
     -hda /home/berrange/Fedora-Server-netinst-x86_64-23.iso \
     -object tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,endpoint=server
  $ qmp-shell /tmp/qmp
     (qmp) nbd-server-start addr={"host":"localhost","port":"9000"}
     tls-creds=tls0
     (qmp) nbd-server-add device=ide0-hd0

The first 2 patches are taken from this other pending patch series in order to facilitate merge:

https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00296.html

The first 4 patches are the conversion to the I/O channels framework.

The next 6 patches are general tweaks to QEMU's impl of the NBD protocol for better compliance and/or future proofing.

The next patch provides the NBD protocol TLS implementation.

The final 3 patches allow TLS to be enabled in the QEMU NBD client and servers.

Changed in v6:

- Rebase to resolve conflicts with recent qapi changes (Eric) and recent nbd changes (Paolo) which merged
- Add MODULE_INIT_QOM to qemu-io & qemu-img to ensure they can load the QIOChannel object types

Changed in v5:

- Pulled in https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00297.html
and applied fixes for issues Eric mentioned in that review
- Pulled in https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00302.html
- Rebased to latest git master

Changed in v4:

- Don't pick the first export name in the list if no export name is provided (Paolo)
- Set client requested export name to "" if none is provided by the user (Paolo)
- Set server advertized export name to "" if TLS is enabled and none is provided by the user (Paolo)
- Rename qemu-nbd --exportname to --export-name (Paolo)
- Use iov_discard_front() to simplify iov handling (Paolo)

Changed in v3:

- Rebase to resolve conflicts with recently merged NBD patches

Changed in v2:

- Fix error codes used during NBD TLS option negotiate
- Update patch with helpers for UserCreatable object types

Daniel P. Berrange (16):
qom: add helpers for UserCreatable object types
qemu-nbd: add support for --object command line arg
nbd: convert block client to use I/O channels for connection setup
nbd: convert qemu-nbd server to use I/O channels for connection setup
nbd: convert blockdev NBD server to use I/O channels for connection
setup
nbd: convert to using I/O channels for actual socket I/O
nbd: invert client logic for negotiating protocol version
nbd: make server compliant with fixed newstyle spec
nbd: make client request fixed new style if advertized
nbd: allow setting of an export name for qemu-nbd server
nbd: always query export list in fixed new style protocol
nbd: use "" as a default export name if none provided
nbd: implement TLS support in the protocol negotiation
nbd: enable use of TLS with NBD block driver
nbd: enable use of TLS with qemu-nbd server
nbd: enable use of TLS with nbd-server-start command

 Makefile                        |   6 +-
 block/nbd-client.c              |  91 ++++++---
 block/nbd-client.h              |  10 +-
 block/nbd.c                     | 105 ++++++++--
 blockdev-nbd.c                  | 131 ++++++++++--
 hmp.c                           |  54 ++---
 include/block/nbd.h             |  28 ++-
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  92 +++++++++
 nbd/client.c                    | 440 +++++++++++++++++++++++++++++++++++-----
 nbd/common.c                    |  83 +++++---
 nbd/nbd-internal.h              |  32 ++-
 nbd/server.c                    | 334 +++++++++++++++++++++---------
 qapi/block.json                 |   4 +-
 qemu-img.c                      |   1 +
 qemu-io.c                       |   1 +
 qemu-nbd.c                      | 195 ++++++++++++++----
 qemu-nbd.texi                   |  13 ++
 qmp-commands.hx                 |   2 +-
 qmp.c                           |  76 +------
 qom/object_interfaces.c         | 174 ++++++++++++++++
 tests/Makefile                  |   2 +-
 tests/qemu-iotests/140.out      |   2 +-
 tests/qemu-iotests/143.out      |   2 +-
 vl.c                            |  68 +------
 25 files changed, 1459 insertions(+), 490 deletions(-)
Signature · 1 line
2.5.0
#1 lore raw

PATCH v6 1/16 qom: add helpers for UserCreatable object types

The QMP monitor code has two helper methods object_add and qmp_object_del that are called from several places in the code (QMP, HMP and main emulator startup).

The HMP and main emulator startup code also share further logic that extracts the qom-type & id values from a qdict.

We soon need to use this logic from qemu-img, qemu-io and qemu-nbd too, but don't want those to depend on the monitor, nor do we want to duplicate the code.

To avoid this, move some code out of qmp.c and hmp.c adding new methods to qom/object_interfaces.c

- user_creatable_add - takes a QDict holding a full object definition & instantiates it
- user_creatable_add_type - takes an ID, type name, and QDict holding object properties & instantiates it
- user_creatable_add_opts - takes a QemuOpts holding a full object definition & instantiates it
- user_creatable_add_opts_foreach - variant on user_creatable_add_opts which can be directly used in conjunction with qemu_opts_foreach.
- user_creatable_del - takes an ID and deletes the corresponding object

The existing code is updated to use these new methods.

---
 hmp.c                           |  52 +++---------
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  92 +++++++++++++++++++++
 qmp.c                           |  76 ++----------------
 qom/object_interfaces.c         | 174 ++++++++++++++++++++++++++++++++++++++++
 vl.c                            |  68 ++--------------
 6 files changed, 290 insertions(+), 175 deletions(-)
hmp.c +11 -41
index c6419da..41fb9ca 100644
@@ -30,6 +30,7 @@
 #include "qapi/string-output-visitor.h"
 #include "qapi/util.h"
 #include "qapi-visit.h"
+#include "qom/object_interfaces.h"
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
@@ -1655,58 +1656,27 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    Error *err_end = NULL;
     QemuOpts *opts;
-    char *type = NULL;
-    char *id = NULL;
     OptsVisitor *ov;
-    QDict *pdict;
-    Visitor *v;
+    Object *obj = NULL;
 
     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
     if (err) {
-        goto out;
+        hmp_handle_error(mon, &err);
+        return;
     }
 
     ov = opts_visitor_new(opts);
-    pdict = qdict_clone_shallow(qdict);
-    v = opts_get_visitor(ov);
-
-    visit_start_struct(v, NULL, NULL, 0, &err);
-    if (err) {
-        goto out_clean;
-    }
-
-    qdict_del(pdict, "qom-type");
-    visit_type_str(v, "qom-type", &type, &err);
-    if (err) {
-        goto out_end;
-    }
+    obj = user_creatable_add(qdict, opts_get_visitor(ov), &err);
+    opts_visitor_cleanup(ov);
+    qemu_opts_del(opts);
 
-    qdict_del(pdict, "id");
-    visit_type_str(v, "id", &id, &err);
     if (err) {
-        goto out_end;
+        hmp_handle_error(mon, &err);
     }
-
-    object_add(type, id, pdict, v, &err);
-
-out_end:
-    visit_end_struct(v, &err_end);
-    if (!err && err_end) {
-        qmp_object_del(id, NULL);
+    if (obj) {
+        object_unref(obj);
     }
-    error_propagate(&err, err_end);
-out_clean:
-    opts_visitor_cleanup(ov);
-
-    QDECREF(pdict);
-    qemu_opts_del(opts);
-    g_free(id);
-    g_free(type);
-
-out:
-    hmp_handle_error(mon, &err);
 }
 
 void hmp_getfd(Monitor *mon, const QDict *qdict)
@@ -1934,7 +1904,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
     const char *id = qdict_get_str(qdict, "id");
     Error *err = NULL;
 
-    qmp_object_del(id, &err);
+    user_creatable_del(id, &err);
     hmp_handle_error(mon, &err);
 }
 
include/monitor/monitor.h +0 -3
index 91b95ae..aa0f373 100644
@@ -43,9 +43,6 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque);
 
-void object_add(const char *type, const char *id, const QDict *qdict,
-                Visitor *v, Error **errp);
-
 AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
                                 bool has_opaque, const char *opaque,
                                 Error **errp);
include/qom/object_interfaces.h +92 -0

102 lines not on this page · raw message

qmp.c +7 -69

99 lines not on this page · raw message

qom/object_interfaces.c +174 -0

186 lines not on this page · raw message

vl.c +6 -62

86 lines not on this page · raw message

Signature · 1 line
2.5.0
Reviewed-by on v6 1/16
#2 lore raw

Re: [Qemu-devel] [PATCH v6 01/16] qom: add helpers for UserCreatable object types

Quoted 31 lines · Daniel P. Berrange

The QMP monitor code has two helper methods object_add and qmp_object_del that are called from several places in the code (QMP, HMP and main emulator startup).

The HMP and main emulator startup code also share further logic that extracts the qom-type & id values from a qdict.

We soon need to use this logic from qemu-img, qemu-io and qemu-nbd too, but don't want those to depend on the monitor, nor do we want to duplicate the code.

To avoid this, move some code out of qmp.c and hmp.c adding new methods to qom/object_interfaces.c

- user_creatable_add - takes a QDict holding a full object definition & instantiates it
- user_creatable_add_type - takes an ID, type name, and QDict holding object properties & instantiates it
- user_creatable_add_opts - takes a QemuOpts holding a full object definition & instantiates it
- user_creatable_add_opts_foreach - variant on user_creatable_add_opts which can be directly used in conjunction with qemu_opts_foreach.
- user_creatable_del - takes an ID and deletes the corresponding object

The existing code is updated to use these new methods.

Quoted 9 lines
---
 hmp.c                           |  52 +++---------
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  92 +++++++++++++++++++++
 qmp.c                           |  76 ++----------------
 qom/object_interfaces.c         | 174 ++++++++++++++++++++++++++++++++++++++++
 vl.c                            |  68 ++--------------
 6 files changed, 290 insertions(+), 175 deletions(-)
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#3 lore raw

PATCH v6 2/16 qemu-nbd: add support for --object command line arg

In reply to Daniel P. Berrange

Allow creation of user creatable object types with qemu-nbd via a new --object command line arg. This will be used to supply passwords and/or encryption keys to the various block driver backends via the recently added 'secret' object type.

 # printf letmein > mypasswd.txt
 # qemu-nbd --object secret,id=sec0,file=mypasswd.txt \
      ...other nbd args...
---
 qemu-nbd.c    | 34 ++++++++++++++++++++++++++++++++++
 qemu-nbd.texi |  6 ++++++
 2 files changed, 40 insertions(+)
qemu-nbd.c +34 -0
index d374015..130c306 100644
@@ -24,9 +24,11 @@
 #include "qemu/main-loop.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
+#include "qemu/config-file.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "qom/object_interfaces.h"
 
 #include <getopt.h>
 #include <sys/socket.h>
@@ -41,6 +43,7 @@
 #define QEMU_NBD_OPT_AIO           2
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
+#define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
 static int verbose;
@@ -74,6 +77,9 @@ static void usage(const char *name)
 "  -o, --offset=OFFSET       offset into the image\n"
 "  -P, --partition=NUM       only expose partition NUM\n"
 "\n"
+"General purpose options:\n"
+"  --object type,id=ID,...   define an object such as 'secret' for providing\n"
+"                            passwords and/or encryption keys\n"
 #ifdef __linux__
 "Kernel NBD client support:\n"
 "  -c, --connect=DEV         connect FILE to the local NBD device DEV\n"
@@ -371,6 +377,16 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 }
 
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
+
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -408,6 +424,7 @@ int main(int argc, char **argv)
         { "format", 1, NULL, 'f' },
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
+        { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -433,6 +450,8 @@ int main(int argc, char **argv)
     memset(&sa_sigterm, 0, sizeof(sa_sigterm));
     sa_sigterm.sa_handler = termsig_handler;
     sigaction(SIGTERM, &sa_sigterm, NULL);
+    module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_object_opts);
     qemu_init_exec_dir(argv[0]);
 
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
@@ -588,6 +607,14 @@ int main(int argc, char **argv)
         case '?':
             error_report("Try `%s --help' for more information.", argv[0]);
             exit(EXIT_FAILURE);
+        case QEMU_NBD_OPT_OBJECT: {
+            QemuOpts *opts;
+            opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                           optarg, true);
+            if (!opts) {
+                exit(EXIT_FAILURE);
+            }
+        }   break;
         }
     }
 
@@ -597,6 +624,13 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
+    if (qemu_opts_foreach(&qemu_object_opts,
+                          user_creatable_add_opts_foreach,
+                          NULL, &local_err)) {
+        error_report_err(local_err);
+        exit(EXIT_FAILURE);
+    }
+
     if (disconnect) {
         fd = open(argv[optind], O_RDWR);
         if (fd < 0) {
qemu-nbd.texi +6 -0
index 0027841..a56ebc3 100644
@@ -18,6 +18,12 @@ Export a QEMU disk image using the NBD protocol.
 @var{dev} is an NBD device.
 
 @table @option
+@item --object type,id=@var{id},...props...
+Define a new instance of the @var{type} object class identified by @var{id}.
+See the @code{qemu(1)} manual page for full details of the properties
+supported. The common object type that it makes sense to define is the
+@code{secret} object, which is used to supply passwords and/or encryption
+keys.
 @item -p, --port=@var{port}
 The TCP port to listen on (default @samp{10809})
 @item -o, --offset=@var{offset}
Signature · 1 line
2.5.0
#4 lore raw

PATCH v6 3/16 nbd: convert block client to use I/O channels for connection setup

In reply to Daniel P. Berrange

This converts the NBD block driver client to use the QIOChannelSocket class for initial connection setup. The NbdClientSession struct has two pointers, one to the master QIOChannelSocket providing the raw data channel, and one to a QIOChannel which is the current channel used for I/O. Initially the two point to the same object, but when TLS support is added, they will point to different objects.

The qemu-img & qemu-io tools now need to use MODULE_INIT_QOM to ensure the QIOChannel object classes are registered. The qemu-nbd tool already did this.

In this initial conversion though, all I/O is still actually done using the raw POSIX sockets APIs.

---
 Makefile           |  6 ++---
 block/nbd-client.c | 76 +++++++++++++++++++++++++++++++++---------------------
 block/nbd-client.h |  8 ++++--
 block/nbd.c        | 39 ++++++++++++++--------------
 qemu-img.c         |  1 +
 qemu-io.c          |  1 +
 tests/Makefile     |  2 +-
 7 files changed, 79 insertions(+), 54 deletions(-)
Makefile +3 -3
index 30b1b2d..ce3d64d 100644
@@ -234,9 +234,9 @@ util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
 
 qemu-img.o: qemu-img-cmds.h
 
-qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
-qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
-qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
+qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
 
 qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
 
block/nbd-client.c +47 -29

173 lines not on this page · raw message

block/nbd-client.h +6 -2
index e841340..e8b3283 100644
@@ -4,6 +4,7 @@
 #include "qemu-common.h"
 #include "block/nbd.h"
 #include "block/block_int.h"
+#include "io/channel-socket.h"
 
 /* #define DEBUG_NBD */
 
@@ -17,7 +18,8 @@
 #define MAX_NBD_REQUESTS    16
 
 typedef struct NbdClientSession {
-    int sock;
+    QIOChannelSocket *sioc; /* The master data channel */
+    QIOChannel *ioc; /* The current I/O channel which may differ (eg TLS) */
     uint32_t nbdflags;
     off_t size;
 
@@ -34,7 +36,9 @@ typedef struct NbdClientSession {
 
 NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
 
-int nbd_client_init(BlockDriverState *bs, int sock, const char *export_name,
+int nbd_client_init(BlockDriverState *bs,
+                    QIOChannelSocket *sock,
+                    const char *export_name,
                     Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
block/nbd.c +20 -19
index 1a90bc7..d7116e2 100644
@@ -31,7 +31,6 @@
 #include "qemu/uri.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
-#include "qemu/sockets.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qint.h"
@@ -238,25 +237,25 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
     return &s->client;
 }
 
-static int nbd_establish_connection(BlockDriverState *bs,
-                                    SocketAddress *saddr,
-                                    Error **errp)
+static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+                                                  Error **errp)
 {
-    BDRVNBDState *s = bs->opaque;
-    int sock;
+    QIOChannelSocket *sioc;
+    Error *local_err = NULL;
 
-    sock = socket_connect(saddr, errp, NULL, NULL);
+    sioc = qio_channel_socket_new();
 
-    if (sock < 0) {
-        logout("Failed to establish connection to NBD server\n");
-        return -EIO;
+    qio_channel_socket_connect_sync(sioc,
+                                    saddr,
+                                    &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
     }
 
-    if (!s->client.is_unix) {
-        socket_set_nodelay(sock);
-    }
+    qio_channel_set_delay(QIO_CHANNEL(sioc), false);
 
-    return sock;
+    return sioc;
 }
 
 static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
@@ -264,7 +263,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVNBDState *s = bs->opaque;
     char *export = NULL;
-    int result, sock;
+    int result;
+    QIOChannelSocket *sioc;
     SocketAddress *saddr;
 
     /* Pop the config into our state object. Exit if invalid. */
@@ -276,15 +276,16 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     /* establish TCP connection, return error if it fails
      * TODO: Configurable retry-until-timeout behaviour.
      */
-    sock = nbd_establish_connection(bs, saddr, errp);
+    sioc = nbd_establish_connection(saddr, errp);
     qapi_free_SocketAddress(saddr);
-    if (sock < 0) {
+    if (!sioc) {
         g_free(export);
-        return sock;
+        return -ECONNREFUSED;
     }
 
     /* NBD handshake */
-    result = nbd_client_init(bs, sock, export, errp);
+    result = nbd_client_init(bs, sioc, export, errp);
+    object_unref(OBJECT(sioc));
     g_free(export);
     return result;
 }
qemu-img.c +1 -0
index 163d8c1..7030107 100644
@@ -3104,6 +3104,7 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
+    module_call_init(MODULE_INIT_QOM);
     bdrv_init();
     if (argc < 2) {
         error_exit("Not enough arguments");
qemu-io.c +1 -0
index 83c48f4..6c0c028 100644
@@ -394,6 +394,7 @@ int main(int argc, char **argv)
     progname = basename(argv[0]);
     qemu_init_exec_dir(argv[0]);
 
+    module_call_init(MODULE_INIT_QOM);
     bdrv_init();
 
     while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
tests/Makefile +1 -1
index 650e654..c1c605f 100644
@@ -390,8 +390,8 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 	tests/test-qapi-event.o tests/test-qmp-introspect.o \
 	$(test-qom-obj-y)
 test-crypto-obj-y = $(crypto-obj-y) $(test-qom-obj-y)
-test-block-obj-y = $(block-obj-y) $(test-crypto-obj-y)
 test-io-obj-y = $(io-obj-y) $(test-crypto-obj-y)
+test-block-obj-y = $(block-obj-y) $(test-io-obj-y)
 
 tests/check-qint$(EXESUF): tests/check-qint.o $(test-util-obj-y)
 tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
Signature · 1 line
2.5.0
#5 lore raw

PATCH v6 4/16 nbd: convert qemu-nbd server to use I/O channels for connection setup

In reply to Daniel P. Berrange

This converts the qemu-nbd server to use the QIOChannelSocket class for initial listener socket setup and accepting of client connections. Actual I/O is still being performed against the socket file descriptor using the POSIX socket APIs.

In this initial conversion though, all I/O is still actually done using the raw POSIX sockets APIs.

---
 qemu-nbd.c | 91 ++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 38 deletions(-)
qemu-nbd.c +53 -38
index 130c306..bc309e0 100644
@@ -22,19 +22,17 @@
 #include "block/block_int.h"
 #include "block/nbd.h"
 #include "qemu/main-loop.h"
-#include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "block/snapshot.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
 #include "qom/object_interfaces.h"
+#include "io/channel-socket.h"
 
 #include <getopt.h>
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <netinet/tcp.h>
-#include <arpa/inet.h>
+#include <sys/types.h>
+#include <signal.h>
 #include <libgen.h>
 #include <pthread.h>
 
@@ -53,7 +51,8 @@ static int persistent = 0;
 static enum { RUNNING, TERMINATE, TERMINATING, TERMINATED } state;
 static int shared = 1;
 static int nb_fds;
-static int server_fd;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
 
 static void usage(const char *name)
 {
@@ -236,19 +235,21 @@ static void *nbd_client_thread(void *arg)
     char *device = arg;
     off_t size;
     uint32_t nbdflags;
-    int fd, sock;
+    QIOChannelSocket *sioc;
+    int fd;
     int ret;
     pthread_t show_parts_thread;
     Error *local_error = NULL;
 
-
-    sock = socket_connect(saddr, &local_error, NULL, NULL);
-    if (sock < 0) {
+    sioc = qio_channel_socket_new();
+    if (qio_channel_socket_connect_sync(sioc,
+                                        saddr,
+                                        &local_error) < 0) {
         error_report_err(local_error);
         goto out;
     }
 
-    ret = nbd_receive_negotiate(sock, NULL, &nbdflags,
+    ret = nbd_receive_negotiate(sioc->fd, NULL, &nbdflags,
                                 &size, &local_error);
     if (ret < 0) {
         if (local_error) {
@@ -264,7 +265,7 @@ static void *nbd_client_thread(void *arg)
         goto out_socket;
     }
 
-    ret = nbd_init(fd, sock, nbdflags, size);
+    ret = nbd_init(fd, sioc->fd, nbdflags, size);
     if (ret < 0) {
         goto out_fd;
     }
@@ -285,13 +286,14 @@ static void *nbd_client_thread(void *arg)
         goto out_fd;
     }
     close(fd);
+    object_unref(OBJECT(sioc));
     kill(getpid(), SIGTERM);
     return (void *) EXIT_SUCCESS;
 
 out_fd:
     close(fd);
 out_socket:
-    closesocket(sock);
+    object_unref(OBJECT(sioc));
 out:
     kill(getpid(), SIGTERM);
     return (void *) EXIT_FAILURE;
@@ -308,7 +310,7 @@ static void nbd_export_closed(NBDExport *exp)
     state = TERMINATED;
 }
 
-static void nbd_update_server_fd_handler(int fd);
+static void nbd_update_server_watch(void);
 
 static void nbd_client_closed(NBDClient *client)
 {
@@ -316,37 +318,51 @@ static void nbd_client_closed(NBDClient *client)
     if (nb_fds == 0 && !persistent && state == RUNNING) {
         state = TERMINATE;
     }
-    nbd_update_server_fd_handler(server_fd);
+    nbd_update_server_watch();
     nbd_client_put(client);
 }
 
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 {
-    struct sockaddr_in addr;
-    socklen_t addr_len = sizeof(addr);
+    QIOChannelSocket *cioc;
+    int fd;
 
-    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
-    if (fd < 0) {
-        perror("accept");
-        return;
+    cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+                                     NULL);
+    if (!cioc) {
+        return TRUE;
     }
 
     if (state >= TERMINATE) {
-        close(fd);
-        return;
+        object_unref(OBJECT(cioc));
+        return TRUE;
     }
 
     nb_fds++;
-    nbd_update_server_fd_handler(server_fd);
-    nbd_client_new(exp, fd, nbd_client_closed);
+    nbd_update_server_watch();
+    fd = dup(cioc->fd);
+    if (fd >= 0) {
+        nbd_client_new(exp, fd, nbd_client_closed);
+    }
+    object_unref(OBJECT(cioc));
+
+    return TRUE;
 }
 
-static void nbd_update_server_fd_handler(int fd)
+static void nbd_update_server_watch(void)
 {
     if (nbd_can_accept()) {
-        qemu_set_fd_handler(fd, nbd_accept, NULL, (void *)(uintptr_t)fd);
+        if (server_watch == -1) {
+            server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+                                                 G_IO_IN,
+                                                 nbd_accept,
+                                                 NULL, NULL);
+        }
     } else {
-        qemu_set_fd_handler(fd, NULL, NULL, NULL);
+        if (server_watch != -1) {
+            g_source_remove(server_watch);
+            server_watch = -1;
+        }
     }
 }
 
@@ -433,7 +449,6 @@ int main(int argc, char **argv)
     int flags = BDRV_O_RDWR;
     int partition = -1;
     int ret = 0;
-    int fd;
     bool seen_cache = false;
     bool seen_discard = false;
     bool seen_aio = false;
@@ -632,15 +647,15 @@ int main(int argc, char **argv)
     }
 
     if (disconnect) {
-        fd = open(argv[optind], O_RDWR);
-        if (fd < 0) {
+        int nbdfd = open(argv[optind], O_RDWR);
+        if (nbdfd < 0) {
             error_report("Cannot open %s: %s", argv[optind],
                          strerror(errno));
             exit(EXIT_FAILURE);
         }
-        nbd_disconnect(fd);
+        nbd_disconnect(nbdfd);
 
-        close(fd);
+        close(nbdfd);
 
         printf("%s disconnected\n", argv[optind]);
 
@@ -773,8 +788,9 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
-    fd = socket_listen(saddr, &local_err);
-    if (fd < 0) {
+    server_ioc = qio_channel_socket_new();
+    if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) {
+        object_unref(OBJECT(server_ioc));
         error_report_err(local_err);
         return 1;
     }
@@ -792,8 +808,7 @@ int main(int argc, char **argv)
         memset(&client_thread, 0, sizeof(client_thread));
     }
 
-    server_fd = fd;
-    nbd_update_server_fd_handler(fd);
+    nbd_update_server_watch();
 
     /* now when the initialization is (almost) complete, chdir("/")
      * to free any busy filesystems */
Signature · 1 line
2.5.0
#6 lore raw

PATCH v6 5/16 nbd: convert blockdev NBD server to use I/O channels for connection setup

In reply to Daniel P. Berrange

This converts the blockdev NBD server to use the QIOChannelSocket class for initial listener socket setup and accepting of client connections. Actual I/O is still being performed against the socket file descriptor using the POSIX socket APIs.

---
 blockdev-nbd.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)
blockdev-nbd.c +34 -15
index efc31a4..9baf883 100644
@@ -18,32 +18,48 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "block/nbd.h"
-#include "qemu/sockets.h"
+#include "io/channel-socket.h"
 
-static int server_fd = -1;
+static QIOChannelSocket *server_ioc;
+static int server_watch = -1;
 
-static void nbd_accept(void *opaque)
+static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
+                           gpointer opaque)
 {
-    struct sockaddr_in addr;
-    socklen_t addr_len = sizeof(addr);
+    QIOChannelSocket *cioc;
+    int fd;
 
-    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
+    cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
+                                     NULL);
+    if (!cioc) {
+        return TRUE;
+    }
+
+    fd = dup(cioc->fd);
     if (fd >= 0) {
         nbd_client_new(NULL, fd, nbd_client_put);
     }
+    object_unref(OBJECT(cioc));
+    return TRUE;
 }
 
 void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
 {
-    if (server_fd != -1) {
+    if (server_ioc) {
         error_setg(errp, "NBD server already running");
         return;
     }
 
-    server_fd = socket_listen(addr, errp);
-    if (server_fd != -1) {
-        qemu_set_fd_handler(server_fd, nbd_accept, NULL, NULL);
+    server_ioc = qio_channel_socket_new();
+    if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+        return;
     }
+
+    server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
+                                         G_IO_IN,
+                                         nbd_accept,
+                                         NULL,
+                                         NULL);
 }
 
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
@@ -52,7 +68,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     BlockBackend *blk;
     NBDExport *exp;
 
-    if (server_fd == -1) {
+    if (!server_ioc) {
         error_setg(errp, "NBD server not running");
         return;
     }
@@ -98,9 +114,12 @@ void qmp_nbd_server_stop(Error **errp)
 {
     nbd_export_close_all();
 
-    if (server_fd != -1) {
-        qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
-        close(server_fd);
-        server_fd = -1;
+    if (server_watch != -1) {
+        g_source_remove(server_watch);
+        server_watch = -1;
+    }
+    if (server_ioc) {
+        object_unref(OBJECT(server_ioc));
+        server_ioc = NULL;
     }
 }
Signature · 1 line
2.5.0
#7 lore raw

PATCH v6 6/16 nbd: convert to using I/O channels for actual socket I/O

In reply to Daniel P. Berrange

Now that all callers are converted to use I/O channels for initial connection setup, it is possible to switch the core NBD protocol handling core over to use QIOChannel APIs for actual sockets I/O.

---
 block/nbd-client.c  |  19 +++----
 blockdev-nbd.c      |   6 +--
 include/block/nbd.h |  20 ++++---
 nbd/client.c        |  40 +++++++-------
 nbd/common.c        |  68 ++++++++++++++----------
 nbd/nbd-internal.h  |  18 +++----
 nbd/server.c        | 150 +++++++++++++++++++++++++++++-----------------------
 qemu-nbd.c          |  10 ++--
 8 files changed, 180 insertions(+), 151 deletions(-)
block/nbd-client.c +10 -9
index c4379a9..75bb2d9 100644
@@ -80,7 +80,7 @@ static void nbd_reply_ready(void *opaque)
          * that another thread has done the same thing in parallel, so
          * the socket is not readable anymore.
          */
-        ret = nbd_receive_reply(s->sioc->fd, &s->reply);
+        ret = nbd_receive_reply(s->ioc, &s->reply);
         if (ret == -EAGAIN) {
             return;
         }
@@ -131,6 +131,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
         }
     }
 
+    g_assert(qemu_in_coroutine());
     assert(i < MAX_NBD_REQUESTS);
     request->handle = INDEX_TO_HANDLE(s, i);
 
@@ -146,17 +147,17 @@ static int nbd_co_send_request(BlockDriverState *bs,
                        nbd_reply_ready, nbd_restart_write, bs);
     if (qiov) {
         qio_channel_set_cork(s->ioc, true);
-        rc = nbd_send_request(s->sioc->fd, request);
+        rc = nbd_send_request(s->ioc, request);
         if (rc >= 0) {
-            ret = qemu_co_sendv(s->sioc->fd, qiov->iov, qiov->niov,
-                                offset, request->len);
+            ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+                               offset, request->len, 0);
             if (ret != request->len) {
                 rc = -EIO;
             }
         }
         qio_channel_set_cork(s->ioc, false);
     } else {
-        rc = nbd_send_request(s->sioc->fd, request);
+        rc = nbd_send_request(s->ioc, request);
     }
     aio_set_fd_handler(aio_context, s->sioc->fd, false,
                        nbd_reply_ready, NULL, bs);
@@ -180,8 +181,8 @@ static void nbd_co_receive_reply(NbdClientSession *s,
         reply->error = EIO;
     } else {
         if (qiov && reply->error == 0) {
-            ret = qemu_co_recvv(s->sioc->fd, qiov->iov, qiov->niov,
-                                offset, request->len);
+            ret = nbd_wr_syncv(s->ioc, qiov->iov, qiov->niov,
+                               offset, request->len, 1);
             if (ret != request->len) {
                 reply->error = EIO;
             }
@@ -388,7 +389,7 @@ void nbd_client_close(BlockDriverState *bs)
         return;
     }
 
-    nbd_send_request(client->sioc->fd, &request);
+    nbd_send_request(client->ioc, &request);
 
     nbd_teardown_connection(bs);
 }
@@ -403,7 +404,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
     logout("session init %s\n", export);
     qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
-    ret = nbd_receive_negotiate(sioc->fd, export,
+    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
                                 &client->nbdflags, &client->size, errp);
     if (ret < 0) {
         logout("Failed to negotiate with the NBD server\n");
blockdev-nbd.c +1 -5
index 9baf883..2148753 100644
@@ -27,7 +27,6 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
                            gpointer opaque)
 {
     QIOChannelSocket *cioc;
-    int fd;
 
     cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      NULL);
@@ -35,10 +34,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
         return TRUE;
     }
 
-    fd = dup(cioc->fd);
-    if (fd >= 0) {
-        nbd_client_new(NULL, fd, nbd_client_put);
-    }
+    nbd_client_new(NULL, cioc, nbd_client_put);
     object_unref(OBJECT(cioc));
     return TRUE;
 }
include/block/nbd.h +14 -6
index 7eccb41..1080ef8 100644
@@ -23,6 +23,7 @@
 
 #include "qemu-common.h"
 #include "qemu/option.h"
+#include "io/channel-socket.h"
 
 struct nbd_request {
     uint32_t magic;
@@ -73,12 +74,17 @@ enum {
 /* Maximum size of a single READ/WRITE data buffer */
 #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
 
-ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read);
-int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
+ssize_t nbd_wr_syncv(QIOChannel *ioc,
+                     struct iovec *iov,
+                     size_t niov,
+                     size_t offset,
+                     size_t length,
+                     bool do_read);
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
                           off_t *size, Error **errp);
-int nbd_init(int fd, int csock, uint32_t flags, off_t size);
-ssize_t nbd_send_request(int csock, struct nbd_request *request);
-ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply);
+int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
+ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
+ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply);
 int nbd_client(int fd);
 int nbd_disconnect(int fd);
 
@@ -98,7 +104,9 @@ NBDExport *nbd_export_find(const char *name);
 void nbd_export_set_name(NBDExport *exp, const char *name);
 void nbd_export_close_all(void);
 
-void nbd_client_new(NBDExport *exp, int csock, void (*close_fn)(NBDClient *));
+void nbd_client_new(NBDExport *exp,
+                    QIOChannelSocket *sioc,
+                    void (*close)(NBDClient *));
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
 
nbd/client.c +20 -20
index f07cb48..f6dd0a3 100644
@@ -71,7 +71,7 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
 
 */
 
-int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
+int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
                           off_t *size, Error **errp)
 {
     char buf[256];
@@ -83,7 +83,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
 
     rc = -EINVAL;
 
-    if (read_sync(csock, buf, 8) != 8) {
+    if (read_sync(ioc, buf, 8) != 8) {
         error_setg(errp, "Failed to read data");
         goto fail;
     }
@@ -109,7 +109,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
         goto fail;
     }
 
-    if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+    if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
         error_setg(errp, "Failed to read magic");
         goto fail;
     }
@@ -130,35 +130,35 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
             }
             goto fail;
         }
-        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
         }
         *flags = be16_to_cpu(tmp) << 16;
         /* reserved for future use */
-        if (write_sync(csock, &reserved, sizeof(reserved)) !=
+        if (write_sync(ioc, &reserved, sizeof(reserved)) !=
             sizeof(reserved)) {
             error_setg(errp, "Failed to read reserved field");
             goto fail;
         }
         /* write the export name */
         magic = cpu_to_be64(magic);
-        if (write_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+        if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
             error_setg(errp, "Failed to send export name magic");
             goto fail;
         }
         opt = cpu_to_be32(NBD_OPT_EXPORT_NAME);
-        if (write_sync(csock, &opt, sizeof(opt)) != sizeof(opt)) {
+        if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
             error_setg(errp, "Failed to send export name option number");
             goto fail;
         }
         namesize = cpu_to_be32(strlen(name));
-        if (write_sync(csock, &namesize, sizeof(namesize)) !=
+        if (write_sync(ioc, &namesize, sizeof(namesize)) !=
             sizeof(namesize)) {
             error_setg(errp, "Failed to send export name length");
             goto fail;
         }
-        if (write_sync(csock, (char*)name, strlen(name)) != strlen(name)) {
+        if (write_sync(ioc, (char *)name, strlen(name)) != strlen(name)) {
             error_setg(errp, "Failed to send export name");
             goto fail;
         }
@@ -175,7 +175,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
         }
     }
 
-    if (read_sync(csock, &s, sizeof(s)) != sizeof(s)) {
+    if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
         error_setg(errp, "Failed to read export length");
         goto fail;
     }
@@ -183,19 +183,19 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
     TRACE("Size is %" PRIu64, *size);
 
     if (!name) {
-        if (read_sync(csock, flags, sizeof(*flags)) != sizeof(*flags)) {
+        if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
         *flags = be32_to_cpup(flags);
     } else {
-        if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
         *flags |= be16_to_cpu(tmp);
     }
-    if (read_sync(csock, &buf, 124) != 124) {
+    if (read_sync(ioc, &buf, 124) != 124) {
         error_setg(errp, "Failed to read reserved block");
         goto fail;
     }
@@ -206,11 +206,11 @@ fail:
 }
 
 #ifdef __linux__
-int nbd_init(int fd, int csock, uint32_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size)
 {
     TRACE("Setting NBD socket");
 
-    if (ioctl(fd, NBD_SET_SOCK, csock) < 0) {
+    if (ioctl(fd, NBD_SET_SOCK, sioc->fd) < 0) {
         int serrno = errno;
         LOG("Failed to set NBD socket");
         return -serrno;
@@ -283,7 +283,7 @@ int nbd_client(int fd)
     return ret;
 }
 #else
-int nbd_init(int fd, int csock, uint32_t flags, off_t size)
+int nbd_init(int fd, QIOChannelSocket *ioc, uint32_t flags, off_t size)
 {
     return -ENOTSUP;
 }
@@ -294,7 +294,7 @@ int nbd_client(int fd)
 }
 #endif
 
-ssize_t nbd_send_request(int csock, struct nbd_request *request)
+ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request)
 {
     uint8_t buf[NBD_REQUEST_SIZE];
     ssize_t ret;
@@ -309,7 +309,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
           "{ .from = %" PRIu64", .len = %u, .handle = %" PRIu64", .type=%i}",
           request->from, request->len, request->handle, request->type);
 
-    ret = write_sync(csock, buf, sizeof(buf));
+    ret = write_sync(ioc, buf, sizeof(buf));
     if (ret < 0) {
         return ret;
     }
@@ -321,13 +321,13 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
     return 0;
 }
 
-ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
+ssize_t nbd_receive_reply(QIOChannel *ioc, struct nbd_reply *reply)
 {
     uint8_t buf[NBD_REPLY_SIZE];
     uint32_t magic;
     ssize_t ret;
 
-    ret = read_sync(csock, buf, sizeof(buf));
+    ret = read_sync(ioc, buf, sizeof(buf));
     if (ret < 0) {
         return ret;
     }
nbd/common.c +40 -28

87 lines not on this page · raw message

nbd/nbd-internal.h +8 -10
index c0a6575..9960034 100644
@@ -13,6 +13,7 @@
 #include "sysemu/block-backend.h"
 
 #include "qemu/coroutine.h"
+#include "qemu/iov.h"
 
 #include <errno.h>
 #include <string.h>
@@ -29,7 +30,6 @@
 #include <linux/fs.h>
 #endif
 
-#include "qemu/sockets.h"
 #include "qemu/queue.h"
 #include "qemu/main-loop.h"
 
@@ -90,24 +90,22 @@
 #define NBD_EINVAL     22
 #define NBD_ENOSPC     28
 
-static inline ssize_t read_sync(int fd, void *buffer, size_t size)
+static inline ssize_t read_sync(QIOChannel *ioc, void *buffer, size_t size)
 {
+    struct iovec iov = { .iov_base = buffer, .iov_len = size };
     /* Sockets are kept in blocking mode in the negotiation phase.  After
      * that, a non-readable socket simply means that another thread stole
      * our request/reply.  Synchronization is done with recv_coroutine, so
      * that this is coroutine-safe.
      */
-    return nbd_wr_sync(fd, buffer, size, true);
+    return nbd_wr_syncv(ioc, &iov, 1, 0, size, true);
 }
 
-static inline ssize_t write_sync(int fd, void *buffer, size_t size)
+static inline ssize_t write_sync(QIOChannel *ioc, void *buffer, size_t size)
 {
-    int ret;
-    do {
-        /* For writes, we do expect the socket to be writable.  */
-        ret = nbd_wr_sync(fd, buffer, size, false);
-    } while (ret == -EAGAIN);
-    return ret;
+    struct iovec iov = { .iov_base = buffer, .iov_len = size };
+
+    return nbd_wr_syncv(ioc, &iov, 1, 0, size, false);
 }
 
 #endif
nbd/server.c +84 -66

429 lines not on this page · raw message

qemu-nbd.c +3 -7
index bc309e0..5e54290 100644
@@ -249,7 +249,7 @@ static void *nbd_client_thread(void *arg)
         goto out;
     }
 
-    ret = nbd_receive_negotiate(sioc->fd, NULL, &nbdflags,
+    ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
                                 &size, &local_error);
     if (ret < 0) {
         if (local_error) {
@@ -265,7 +265,7 @@ static void *nbd_client_thread(void *arg)
         goto out_socket;
     }
 
-    ret = nbd_init(fd, sioc->fd, nbdflags, size);
+    ret = nbd_init(fd, sioc, nbdflags, size);
     if (ret < 0) {
         goto out_fd;
     }
@@ -325,7 +325,6 @@ static void nbd_client_closed(NBDClient *client)
 static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 {
     QIOChannelSocket *cioc;
-    int fd;
 
     cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      NULL);
@@ -340,10 +339,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 
     nb_fds++;
     nbd_update_server_watch();
-    fd = dup(cioc->fd);
-    if (fd >= 0) {
-        nbd_client_new(exp, fd, nbd_client_closed);
-    }
+    nbd_client_new(exp, cioc, nbd_client_closed);
     object_unref(OBJECT(cioc));
 
     return TRUE;
Signature · 1 line
2.5.0
#8 lore raw

PATCH v6 7/16 nbd: invert client logic for negotiating protocol version

In reply to Daniel P. Berrange

The nbd_receive_negotiate() method takes different code paths based on whether 'name == NULL', and then checks the expected protocol version in each branch.

This patch inverts the logic, so that it takes different code paths based on what protocol version it receives and then checks if name is NULL or not as needed.

This facilitates later code which allows the client to be capable of using the new style protocol regardless of whether an export name is listed or not.

---
 nbd/client.c | 60 +++++++++++++++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)
nbd/client.c +29 -31
index f6dd0a3..5064788 100644
@@ -116,20 +116,11 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
     magic = be64_to_cpu(magic);
     TRACE("Magic is 0x%" PRIx64, magic);
 
-    if (name) {
+    if (magic == NBD_OPTS_MAGIC) {
         uint32_t reserved = 0;
         uint32_t opt;
         uint32_t namesize;
 
-        TRACE("Checking magic (opts_magic)");
-        if (magic != NBD_OPTS_MAGIC) {
-            if (magic == NBD_CLIENT_MAGIC) {
-                error_setg(errp, "Server does not support export names");
-            } else {
-                error_setg(errp, "Bad magic received");
-            }
-            goto fail;
-        }
         if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
@@ -142,6 +133,10 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             goto fail;
         }
         /* write the export name */
+        if (!name) {
+            error_setg(errp, "Server requires an export name");
+            goto fail;
+        }
         magic = cpu_to_be64(magic);
         if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
             error_setg(errp, "Failed to send export name magic");
@@ -162,39 +157,42 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             error_setg(errp, "Failed to send export name");
             goto fail;
         }
-    } else {
-        TRACE("Checking magic (cli_magic)");
 
-        if (magic != NBD_CLIENT_MAGIC) {
-            if (magic == NBD_OPTS_MAGIC) {
-                error_setg(errp, "Server requires an export name");
-            } else {
-                error_setg(errp, "Bad magic received");
-            }
+        if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+            error_setg(errp, "Failed to read export length");
             goto fail;
         }
-    }
+        *size = be64_to_cpu(s);
+        TRACE("Size is %" PRIu64, *size);
 
-    if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
-        error_setg(errp, "Failed to read export length");
-        goto fail;
-    }
-    *size = be64_to_cpu(s);
-    TRACE("Size is %" PRIu64, *size);
+        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+            error_setg(errp, "Failed to read export flags");
+            goto fail;
+        }
+        *flags |= be16_to_cpu(tmp);
+    } else if (magic == NBD_CLIENT_MAGIC) {
+        if (name) {
+            error_setg(errp, "Server does not support export names");
+            goto fail;
+        }
+
+        if (read_sync(ioc, &s, sizeof(s)) != sizeof(s)) {
+            error_setg(errp, "Failed to read export length");
+            goto fail;
+        }
+        *size = be64_to_cpu(s);
+        TRACE("Size is %" PRIu64, *size);
 
-    if (!name) {
         if (read_sync(ioc, flags, sizeof(*flags)) != sizeof(*flags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
         *flags = be32_to_cpup(flags);
     } else {
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
-            error_setg(errp, "Failed to read export flags");
-            goto fail;
-        }
-        *flags |= be16_to_cpu(tmp);
+        error_setg(errp, "Bad magic received");
+        goto fail;
     }
+
     if (read_sync(ioc, &buf, 124) != 124) {
         error_setg(errp, "Failed to read reserved block");
         goto fail;
Signature · 1 line
2.5.0
#9 lore raw

PATCH v6 8/16 nbd: make server compliant with fixed newstyle spec

In reply to Daniel P. Berrange

If the client does not request the fixed new style protocol, then we should only accept NBD_OPT_EXPORT_NAME. All other options are only valid when fixed new style has been activated.

The qemu-nbd client doesn't currently request fixed new style protocol, but this change won't break qemu-nbd, because it fortunately only ever uses NBD_OPT_EXPORT_NAME, so was never triggering the non-compliant server behaviour.

---
 nbd/server.c | 69 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 23 deletions(-)
nbd/server.c +46 -23
index 15aa03d..074a1e6 100644
@@ -310,6 +310,7 @@ fail:
 static int nbd_negotiate_options(NBDClient *client)
 {
     uint32_t flags;
+    bool fixedNewstyle = false;
 
     /* Client sends:
         [ 0 ..   3]   client flags
@@ -332,14 +333,19 @@ static int nbd_negotiate_options(NBDClient *client)
     }
     TRACE("Checking client flags");
     be32_to_cpus(&flags);
-    if (flags != 0 && flags != NBD_FLAG_C_FIXED_NEWSTYLE) {
-        LOG("Bad client flags received");
+    if (flags & NBD_FLAG_C_FIXED_NEWSTYLE) {
+        TRACE("Support supports fixed newstyle handshake");
+        fixedNewstyle = true;
+        flags &= ~NBD_FLAG_C_FIXED_NEWSTYLE;
+    }
+    if (flags != 0) {
+        TRACE("Unknown client flags 0x%x received", flags);
         return -EIO;
     }
 
     while (1) {
         int ret;
-        uint32_t tmp, length;
+        uint32_t clientflags, length;
         uint64_t magic;
 
         if (nbd_negotiate_read(client->ioc, &magic, sizeof(magic)) !=
@@ -353,10 +359,12 @@ static int nbd_negotiate_options(NBDClient *client)
             return -EINVAL;
         }
 
-        if (nbd_negotiate_read(client->ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (nbd_negotiate_read(client->ioc, &clientflags,
+                               sizeof(clientflags)) != sizeof(clientflags)) {
             LOG("read failed");
             return -EINVAL;
         }
+        clientflags = be32_to_cpu(clientflags);
 
         if (nbd_negotiate_read(client->ioc, &length, sizeof(length)) !=
             sizeof(length)) {
@@ -365,26 +373,41 @@ static int nbd_negotiate_options(NBDClient *client)
         }
         length = be32_to_cpu(length);
 
-        TRACE("Checking option");
-        switch (be32_to_cpu(tmp)) {
-        case NBD_OPT_LIST:
-            ret = nbd_negotiate_handle_list(client, length);
-            if (ret < 0) {
-                return ret;
+        TRACE("Checking option 0x%x", clientflags);
+        if (fixedNewstyle) {
+            switch (clientflags) {
+            case NBD_OPT_LIST:
+                ret = nbd_negotiate_handle_list(client, length);
+                if (ret < 0) {
+                    return ret;
+                }
+                break;
+
+            case NBD_OPT_ABORT:
+                return -EINVAL;
+
+            case NBD_OPT_EXPORT_NAME:
+                return nbd_negotiate_handle_export_name(client, length);
+
+            default:
+                TRACE("Unsupported option 0x%x", clientflags);
+                nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP,
+                                       clientflags);
+                return -EINVAL;
+            }
+        } else {
+            /*
+             * If broken new-style we should drop the connection
+             * for anything except NBD_OPT_EXPORT_NAME
+             */
+            switch (clientflags) {
+            case NBD_OPT_EXPORT_NAME:
+                return nbd_negotiate_handle_export_name(client, length);
+
+            default:
+                TRACE("Unsupported option 0x%x", clientflags);
+                return -EINVAL;
             }
-            break;
-
-        case NBD_OPT_ABORT:
-            return -EINVAL;
-
-        case NBD_OPT_EXPORT_NAME:
-            return nbd_negotiate_handle_export_name(client, length);
-
-        default:
-            tmp = be32_to_cpu(tmp);
-            LOG("Unsupported option 0x%x", tmp);
-            nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_UNSUP, tmp);
-            return -EINVAL;
         }
     }
 }
Signature · 1 line
2.5.0
#10 lore raw

PATCH v6 9/16 nbd: make client request fixed new style if advertized

In reply to Daniel P. Berrange

If the server advertizes support for the fixed new style negotiation, the client should in turn enable new style. This will allow the client to negotiate further NBD options besides the export name.

---
 nbd/client.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)
nbd/client.c +17 -10
index 5064788..88f2ada 100644
@@ -76,7 +76,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
 {
     char buf[256];
     uint64_t magic, s;
-    uint16_t tmp;
     int rc;
 
     TRACE("Receiving negotiation.");
@@ -117,19 +116,26 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
     TRACE("Magic is 0x%" PRIx64, magic);
 
     if (magic == NBD_OPTS_MAGIC) {
-        uint32_t reserved = 0;
+        uint32_t clientflags = 0;
         uint32_t opt;
         uint32_t namesize;
+        uint16_t globalflags;
+        uint16_t exportflags;
 
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
+            sizeof(globalflags)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
         }
-        *flags = be16_to_cpu(tmp) << 16;
-        /* reserved for future use */
-        if (write_sync(ioc, &reserved, sizeof(reserved)) !=
-            sizeof(reserved)) {
-            error_setg(errp, "Failed to read reserved field");
+        *flags = be16_to_cpu(globalflags) << 16;
+        if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+            TRACE("Server supports fixed new style");
+            clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
+        }
+        /* client requested flags */
+        if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
+            sizeof(clientflags)) {
+            error_setg(errp, "Failed to send clientflags field");
             goto fail;
         }
         /* write the export name */
@@ -165,11 +171,12 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         *size = be64_to_cpu(s);
         TRACE("Size is %" PRIu64, *size);
 
-        if (read_sync(ioc, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        if (read_sync(ioc, &exportflags, sizeof(exportflags)) !=
+            sizeof(exportflags)) {
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        *flags |= be16_to_cpu(tmp);
+        *flags |= be16_to_cpu(exportflags);
     } else if (magic == NBD_CLIENT_MAGIC) {
         if (name) {
             error_setg(errp, "Server does not support export names");
Signature · 1 line
2.5.0
#11 lore raw

PATCH v6 10/16 nbd: allow setting of an export name for qemu-nbd server

In reply to Daniel P. Berrange

The qemu-nbd server currently always uses the old style protocol since it never sets any export name. This is a problem because future TLS support will require use of the new style protocol negotiation.

This adds "--exportname NAME" / "-x NAME" arguments to qemu-nbd which allow the user to set an explicit export name. When an export name is set the server will always use the new style NBD protocol.

---
 qemu-nbd.c    | 14 ++++++++++++--
 qemu-nbd.texi |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)
qemu-nbd.c +12 -2
index 5e54290..9710a26 100644
@@ -44,6 +44,7 @@
 #define QEMU_NBD_OPT_OBJECT        5
 
 static NBDExport *exp;
+static bool newproto;
 static int verbose;
 static char *srcpath;
 static SocketAddress *saddr;
@@ -339,7 +340,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 
     nb_fds++;
     nbd_update_server_watch();
-    nbd_client_new(exp, cioc, nbd_client_closed);
+    nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed);
     object_unref(OBJECT(cioc));
 
     return TRUE;
@@ -413,7 +414,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -437,6 +438,7 @@ int main(int argc, char **argv)
         { "persistent", 0, NULL, 't' },
         { "verbose", 0, NULL, 'v' },
         { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
+        { "export-name", 1, NULL, 'x' },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -453,6 +455,7 @@ int main(int argc, char **argv)
     Error *local_err = NULL;
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
+    const char *export_name = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -604,6 +607,9 @@ int main(int argc, char **argv)
         case 't':
             persistent = 1;
             break;
+        case 'x':
+            export_name = optarg;
+            break;
         case 'v':
             verbose = 1;
             break;
@@ -783,6 +789,10 @@ int main(int argc, char **argv)
         error_report_err(local_err);
         exit(EXIT_FAILURE);
     }
+    if (export_name) {
+        nbd_export_set_name(exp, export_name);
+        newproto = true;
+    }
 
     server_ioc = qio_channel_socket_new();
     if (qio_channel_socket_listen_sync(server_ioc, saddr, &local_err) < 0) {
qemu-nbd.texi +3 -0
index a56ebc3..2516963 100644
@@ -73,6 +73,9 @@ Disconnect the device @var{dev}
 Allow up to @var{num} clients to share the device (default @samp{1})
 @item -t, --persistent
 Don't exit on the last connection
+@item -x NAME, --export-name=NAME
+Set the NDB volume export name. This switches the server to use
+the new style NBD protocol negotiation
 @item -v, --verbose
 Display extra debugging information
 @item -h, --help
Signature · 1 line
2.5.0
#12 lore raw

PATCH v6 11/16 nbd: always query export list in fixed new style protocol

In reply to Daniel P. Berrange

With the new style protocol, the NBD client will currenetly send NBD_OPT_EXPORT_NAME as the first (and indeed only) option it wants. The problem is that the NBD protocol spec does not allow for returning an error message with the NBD_OPT_EXPORT_NAME option. So if the server mandates use of TLS, the client will simply see an immediate connection close after issuing NBD_OPT_EXPORT_NAME which is not user friendly.

To improve this situation, if we have the fixed new style protocol, we can sent NBD_OPT_LIST as the first option to query the list of server exports. We can check for our named export in this list and raise an error if it is not found, instead of going ahead and sending NBD_OPT_EXPORT_NAME with a name that we know will be rejected.

This improves the error reporting both in the case that the server required TLS, and in the case that the client requested export name does not exist on the server.

If the server does not support NBD_OPT_LIST, we just ignore that and carry on with NBD_OPT_EXPORT_NAME as before.

---
 nbd/client.c               | 195 ++++++++++++++++++++++++++++++++++++++++++++-
 nbd/server.c               |   2 +
 tests/qemu-iotests/140.out |   2 +-
 tests/qemu-iotests/143.out |   2 +-
 4 files changed, 196 insertions(+), 5 deletions(-)
nbd/client.c +192 -3
index 88f2ada..be5f08d 100644
@@ -71,6 +71,177 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
 
 */
 
+
+static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp)
+{
+    if (!(type & (1 << 31))) {
+        return 0;
+    }
+
+    switch (type) {
+    case NBD_REP_ERR_UNSUP:
+        error_setg(errp, "Unsupported option type %x", opt);
+        break;
+
+    case NBD_REP_ERR_INVALID:
+        error_setg(errp, "Invalid data length for option %x", opt);
+        break;
+
+    default:
+        error_setg(errp, "Unknown error code when asking for option %x", opt);
+        break;
+    }
+
+    return -1;
+}
+
+static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
+{
+    uint64_t magic;
+    uint32_t opt;
+    uint32_t type;
+    uint32_t len;
+    uint32_t namelen;
+
+    *name = NULL;
+    if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+        error_setg(errp, "failed to read list option magic");
+        return -1;
+    }
+    magic = be64_to_cpu(magic);
+    if (magic != NBD_REP_MAGIC) {
+        error_setg(errp, "Unexpected option list magic");
+        return -1;
+    }
+    if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+        error_setg(errp, "failed to read list option");
+        return -1;
+    }
+    opt = be32_to_cpu(opt);
+    if (opt != NBD_OPT_LIST) {
+        error_setg(errp, "Unexpected option type %x expected %x",
+                   opt, NBD_OPT_LIST);
+        return -1;
+    }
+
+    if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) {
+        error_setg(errp, "failed to read list option type");
+        return -1;
+    }
+    type = be32_to_cpu(type);
+    if (type == NBD_REP_ERR_UNSUP) {
+        return 0;
+    }
+    if (nbd_handle_reply_err(opt, type, errp) < 0) {
+        return -1;
+    }
+
+    if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) {
+        error_setg(errp, "failed to read option length");
+        return -1;
+    }
+    len = be32_to_cpu(len);
+
+    if (type == NBD_REP_ACK) {
+        if (len != 0) {
+            error_setg(errp, "length too long for option end");
+            return -1;
+        }
+    } else if (type == NBD_REP_SERVER) {
+        if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
+            error_setg(errp, "failed to read option name length");
+            return -1;
+        }
+        namelen = be32_to_cpu(namelen);
+        if (len != (namelen + sizeof(namelen))) {
+            error_setg(errp, "incorrect option mame length");
+            return -1;
+        }
+        if (namelen > 255) {
+            error_setg(errp, "export name length too long %d", namelen);
+            return -1;
+        }
+
+        *name = g_new0(char, namelen + 1);
+        if (read_sync(ioc, *name, namelen) != namelen) {
+            error_setg(errp, "failed to read export name");
+            g_free(*name);
+            *name = NULL;
+            return -1;
+        }
+        (*name)[namelen] = '\0';
+    } else {
+        error_setg(errp, "Unexpected reply type %x expected %x",
+                   type, NBD_REP_SERVER);
+        return -1;
+    }
+    return 1;
+}
+
+
+static int nbd_receive_query_exports(QIOChannel *ioc,
+                                     const char *wantname,
+                                     Error **errp)
+{
+    uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC);
+    uint32_t opt = cpu_to_be32(NBD_OPT_LIST);
+    uint32_t length = 0;
+    bool foundExport = false;
+
+    TRACE("Querying export list");
+    if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
+        error_setg(errp, "Failed to send list option magic");
+        return -1;
+    }
+
+    if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) {
+        error_setg(errp, "Failed to send list option number");
+        return -1;
+    }
+
+    if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) {
+        error_setg(errp, "Failed to send list option length");
+        return -1;
+    }
+
+    TRACE("Reading available export names");
+    while (1) {
+        char *name = NULL;
+        int ret = nbd_receive_list(ioc, &name, errp);
+
+        if (ret < 0) {
+            g_free(name);
+            name = NULL;
+            return -1;
+        }
+        if (ret == 0) {
+            /* Server doesn't support export listing, so
+             * we will just assume an export with our
+             * wanted name exists */
+            foundExport = true;
+            break;
+        }
+        if (name == NULL) {
+            TRACE("End of export name list");
+            break;
+        }
+        if (g_str_equal(name, wantname)) {
+            foundExport = true;
+            TRACE("Found desired export name '%s'", name);
+        } else {
+            TRACE("Ignored export name '%s'", name);
+        }
+        g_free(name);
+    }
+
+    if (!foundExport) {
+        error_setg(errp, "No export with name '%s' available", wantname);
+        return -1;
+    }
+
+    return 0;
+}
+
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
                           off_t *size, Error **errp)
 {
@@ -121,28 +292,44 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
         uint32_t namesize;
         uint16_t globalflags;
         uint16_t exportflags;
+        bool fixedNewStyle = false;
 
         if (read_sync(ioc, &globalflags, sizeof(globalflags)) !=
             sizeof(globalflags)) {
             error_setg(errp, "Failed to read server flags");
             goto fail;
         }
-        *flags = be16_to_cpu(globalflags) << 16;
+        globalflags = be16_to_cpu(globalflags);
+        *flags = globalflags << 16;
+        TRACE("Global flags are %x", globalflags);
         if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) {
+            fixedNewStyle = true;
             TRACE("Server supports fixed new style");
             clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE;
         }
         /* client requested flags */
+        clientflags = cpu_to_be32(clientflags);
         if (write_sync(ioc, &clientflags, sizeof(clientflags)) !=
             sizeof(clientflags)) {
             error_setg(errp, "Failed to send clientflags field");
             goto fail;
         }
-        /* write the export name */
         if (!name) {
             error_setg(errp, "Server requires an export name");
             goto fail;
         }
+        if (fixedNewStyle) {
+            /* Check our desired export is present in the
+             * server export list. Since NBD_OPT_EXPORT_NAME
+             * cannot return an error message, running this
+             * query gives us good error reporting if the
+             * server required TLS
+             */
+            if (nbd_receive_query_exports(ioc, name, errp) < 0) {
+                goto fail;
+            }
+        }
+        /* write the export name */
         magic = cpu_to_be64(magic);
         if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
             error_setg(errp, "Failed to send export name magic");
@@ -176,7 +363,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             error_setg(errp, "Failed to read export flags");
             goto fail;
         }
-        *flags |= be16_to_cpu(exportflags);
+        exportflags = be16_to_cpu(exportflags);
+        *flags |= exportflags;
+        TRACE("Export flags are %x", exportflags);
     } else if (magic == NBD_CLIENT_MAGIC) {
         if (name) {
             error_setg(errp, "Server does not support export names");
nbd/server.c +2 -0
index 074a1e6..3d2fb10 100644
@@ -294,6 +294,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length)
     }
     name[length] = '\0';
 
+    TRACE("Client requested export '%s'", name);
+
     client->exp = nbd_export_find(name);
     if (!client->exp) {
         LOG("export not found");
tests/qemu-iotests/140.out +1 -1
index fdedeb3..72f1b4c 100644
@@ -9,7 +9,7 @@ read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "drv", "tray-open": true}}
 {"return": {}}
-can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: Failed to read export length
+can't open device nbd+unix:///drv?socket=TEST_DIR/nbd: No export with name 'drv' available
 no file open, try 'help open'
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
tests/qemu-iotests/143.out +1 -1
index dad2024..d24ad20 100644
@@ -1,7 +1,7 @@
 QA output created by 143
 {"return": {}}
 {"return": {}}
-can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: Failed to read export length
+can't open device nbd+unix:///no_such_export?socket=TEST_DIR/nbd: No export with name 'no_such_export' available
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 *** done
Signature · 1 line
2.5.0
#13 lore raw

PATCH v6 12/16 nbd: use "" as a default export name if none provided

In reply to Daniel P. Berrange

If the user does not provide an export name and the server is running the new style protocol, where export names are mandatory, use "" as the default export name if the user has not specified any. "" is defined in the NBD protocol as the default name to use in such scenarios.

---
 nbd/client.c | 4 ++--
 nbd/server.c | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)
nbd/client.c +2 -2
index be5f08d..5e47ac7 100644
@@ -315,8 +315,8 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
             goto fail;
         }
         if (!name) {
-            error_setg(errp, "Server requires an export name");
-            goto fail;
+            TRACE("Using default NBD export name \"\"");
+            name = "";
         }
         if (fixedNewStyle) {
             /* Check our desired export is present in the
nbd/server.c +1 -0
index 3d2fb10..9fee1d4 100644
@@ -220,6 +220,7 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp)
     uint64_t magic, name_len;
     uint32_t opt, type, len;
 
+    TRACE("Advertizing export name '%s'", exp->name ? exp->name : "");
     name_len = strlen(exp->name);
     magic = cpu_to_be64(NBD_REP_MAGIC);
     if (nbd_negotiate_write(ioc, &magic, sizeof(magic)) != sizeof(magic)) {
Signature · 1 line
2.5.0
#14 lore raw

PATCH v6 13/16 nbd: implement TLS support in the protocol negotiation

In reply to Daniel P. Berrange

This extends the NBD protocol handling code so that it is capable of negotiating TLS support during the connection setup. This involves requesting the STARTTLS protocol option before any other NBD options.

---
 block/nbd-client.c  |  12 +++--
 blockdev-nbd.c      |   2 +-
 include/block/nbd.h |   8 ++++
 nbd/client.c        | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 nbd/common.c        |  15 ++++++
 nbd/nbd-internal.h  |  14 ++++++
 nbd/server.c        | 118 ++++++++++++++++++++++++++++++++++++++++++---
 qemu-nbd.c          |   4 +-
 8 files changed, 296 insertions(+), 13 deletions(-)
block/nbd-client.c +9 -3
index 75bb2d9..1c79e4b 100644
@@ -405,7 +405,10 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
     qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
-                                &client->nbdflags, &client->size, errp);
+                                &client->nbdflags,
+                                NULL, NULL,
+                                &client->ioc,
+                                &client->size, errp);
     if (ret < 0) {
         logout("Failed to negotiate with the NBD server\n");
         return ret;
@@ -415,8 +418,11 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
     qemu_co_mutex_init(&client->free_sema);
     client->sioc = sioc;
     object_ref(OBJECT(client->sioc));
-    client->ioc = QIO_CHANNEL(sioc);
-    object_ref(OBJECT(client->ioc));
+
+    if (!client->ioc) {
+        client->ioc = QIO_CHANNEL(sioc);
+        object_ref(OBJECT(client->ioc));
+    }
 
     /* Now that we're connected, set the socket to be non-blocking and
      * kick the reply mechanism.  */
blockdev-nbd.c +1 -1
index 2148753..f181840 100644
@@ -34,7 +34,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
         return TRUE;
     }
 
-    nbd_client_new(NULL, cioc, nbd_client_put);
+    nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
     object_unref(OBJECT(cioc));
     return TRUE;
 }
include/block/nbd.h +8 -0
index 1080ef8..b197adc 100644
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "io/channel-socket.h"
+#include "crypto/tlscreds.h"
 
 struct nbd_request {
     uint32_t magic;
@@ -56,7 +57,10 @@ struct nbd_reply {
 #define NBD_REP_ACK             (1)             /* Data sending finished. */
 #define NBD_REP_SERVER          (2)             /* Export description. */
 #define NBD_REP_ERR_UNSUP       ((UINT32_C(1) << 31) | 1) /* Unknown option. */
+#define NBD_REP_ERR_POLICY      ((UINT32_C(1) << 31) | 2) /* Server denied */
 #define NBD_REP_ERR_INVALID     ((UINT32_C(1) << 31) | 3) /* Invalid length. */
+#define NBD_REP_ERR_TLS_REQD    ((UINT32_C(1) << 31) | 5) /* TLS required */
+
 
 #define NBD_CMD_MASK_COMMAND	0x0000ffff
 #define NBD_CMD_FLAG_FUA	(1 << 16)
@@ -81,6 +85,8 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
                      size_t length,
                      bool do_read);
 int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags,
+                          QCryptoTLSCreds *tlscreds, const char *hostname,
+                          QIOChannel **outioc,
                           off_t *size, Error **errp);
 int nbd_init(int fd, QIOChannelSocket *sioc, uint32_t flags, off_t size);
 ssize_t nbd_send_request(QIOChannel *ioc, struct nbd_request *request);
@@ -106,6 +112,8 @@ void nbd_export_close_all(void);
 
 void nbd_client_new(NBDExport *exp,
                     QIOChannelSocket *sioc,
+                    QCryptoTLSCreds *tlscreds,
+                    const char *tlsaclname,
                     void (*close)(NBDClient *));
 void nbd_client_get(NBDClient *client);
 void nbd_client_put(NBDClient *client);
nbd/client.c +135 -1

174 lines not on this page · raw message

nbd/common.c +15 -0
index 2b19c95..bde673a 100644
@@ -75,3 +75,18 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc,
     g_free(local_iov_head);
     return done;
 }
+
+
+void nbd_tls_handshake(Object *src,
+                       Error *err,
+                       void *opaque)
+{
+    struct NBDTLSHandshakeData *data = opaque;
+
+    if (err) {
+        TRACE("TLS failed %s", error_get_pretty(err));
+        data->error = error_copy(err);
+    }
+    data->complete = true;
+    g_main_loop_quit(data->loop);
+}
nbd/nbd-internal.h +14 -0
index 9960034..db6ab65 100644
@@ -11,6 +11,7 @@
 #define NBD_INTERNAL_H
 #include "block/nbd.h"
 #include "sysemu/block-backend.h"
+#include "io/channel-tls.h"
 
 #include "qemu/coroutine.h"
 #include "qemu/iov.h"
@@ -78,6 +79,8 @@
 #define NBD_OPT_EXPORT_NAME     (1)
 #define NBD_OPT_ABORT           (2)
 #define NBD_OPT_LIST            (3)
+#define NBD_OPT_PEEK_EXPORT     (4)
+#define NBD_OPT_STARTTLS        (5)
 
 /* NBD errors are based on errno numbers, so there is a 1:1 mapping,
  * but only a limited set of errno values is specified in the protocol.
@@ -108,4 +111,15 @@ static inline ssize_t write_sync(QIOChannel *ioc, void *buffer, size_t size)
     return nbd_wr_syncv(ioc, &iov, 1, 0, size, false);
 }
 
+struct NBDTLSHandshakeData {
+    GMainLoop *loop;
+    bool complete;
+    Error *error;
+};
+
+
+void nbd_tls_handshake(Object *src,
+                       Error *err,
+                       void *opaque);
+
 #endif
nbd/server.c +111 -7

194 lines not on this page · raw message

qemu-nbd.c +3 -1
index 9710a26..8acd515 100644
@@ -251,6 +251,7 @@ static void *nbd_client_thread(void *arg)
     }
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), NULL, &nbdflags,
+                                NULL, NULL, NULL,
                                 &size, &local_error);
     if (ret < 0) {
         if (local_error) {
@@ -340,7 +341,8 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
 
     nb_fds++;
     nbd_update_server_watch();
-    nbd_client_new(newproto ? NULL : exp, cioc, nbd_client_closed);
+    nbd_client_new(newproto ? NULL : exp, cioc,
+                   NULL, NULL, nbd_client_closed);
     object_unref(OBJECT(cioc));
 
     return TRUE;
Signature · 1 line
2.5.0
#15 lore raw

PATCH v6 14/16 nbd: enable use of TLS with NBD block driver

In reply to Daniel P. Berrange

This modifies the NBD driver so that it is possible to request use of TLS. This is done by providing the 'tls-creds' parameter with the ID of a previously created QCryptoTLSCreds object.

For example

  $QEMU -object tls-creds-x509,id=tls0,endpoint=client,\
                dir=/home/berrange/security/qemutls \
        -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

The client will drop the connection if the NBD server does not provide TLS.

---
 block/nbd-client.c | 10 ++++---
 block/nbd-client.h |  2 ++
 block/nbd.c        | 78 +++++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 78 insertions(+), 12 deletions(-)
block/nbd-client.c +7 -3
index 1c79e4b..6a9b4c7 100644
@@ -394,8 +394,12 @@ void nbd_client_close(BlockDriverState *bs)
     nbd_teardown_connection(bs);
 }
 
-int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
-                    const char *export, Error **errp)
+int nbd_client_init(BlockDriverState *bs,
+                    QIOChannelSocket *sioc,
+                    const char *export,
+                    QCryptoTLSCreds *tlscreds,
+                    const char *hostname,
+                    Error **errp)
 {
     NbdClientSession *client = nbd_get_client_session(bs);
     int ret;
@@ -406,7 +410,7 @@ int nbd_client_init(BlockDriverState *bs, QIOChannelSocket *sioc,
 
     ret = nbd_receive_negotiate(QIO_CHANNEL(sioc), export,
                                 &client->nbdflags,
-                                NULL, NULL,
+                                tlscreds, hostname,
                                 &client->ioc,
                                 &client->size, errp);
     if (ret < 0) {
block/nbd-client.h +2 -0
index e8b3283..53f116d 100644
@@ -39,6 +39,8 @@ NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
 int nbd_client_init(BlockDriverState *bs,
                     QIOChannelSocket *sock,
                     const char *export_name,
+                    QCryptoTLSCreds *tlscreds,
+                    const char *hostname,
                     Error **errp);
 void nbd_client_close(BlockDriverState *bs);
 
block/nbd.c +69 -9

117 lines not on this page · raw message

Signature · 1 line
2.5.0
#16 lore raw

PATCH v6 15/16 nbd: enable use of TLS with qemu-nbd server

In reply to Daniel P. Berrange

This modifies the qemu-nbd program so that it is possible to request the use of TLS with the server. It simply adds a new command line option --tls-creds which is used to provide the ID of a QCryptoTLSCreds object previously created via the --object command line option.

For example

  qemu-nbd --object tls-creds-x509,id=tls0,endpoint=server,\
                    dir=/home/berrange/security/qemutls \
           --tls-creds tls0 \
           --exportname default

TLS requires the new style NBD protocol, so if no export name is set (via --export-name), then we use the default NBD protocol export name ""

TLS is only supported when using an IPv4/IPv6 socket listener. It is not possible to use with UNIX sockets, which includes when connecting the NBD server to a host device.

---
 qemu-nbd.c    | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-nbd.texi |  4 ++++
 2 files changed, 65 insertions(+), 1 deletion(-)
qemu-nbd.c +61 -1
index 8acd515..933ca4a 100644
@@ -42,6 +42,7 @@
 #define QEMU_NBD_OPT_DISCARD       3
 #define QEMU_NBD_OPT_DETECT_ZEROES 4
 #define QEMU_NBD_OPT_OBJECT        5
+#define QEMU_NBD_OPT_TLSCREDS      6
 
 static NBDExport *exp;
 static bool newproto;
@@ -54,6 +55,7 @@ static int shared = 1;
 static int nb_fds;
 static QIOChannelSocket *server_ioc;
 static int server_watch = -1;
+static QCryptoTLSCreds *tlscreds;
 
 static void usage(const char *name)
 {
@@ -342,7 +344,7 @@ static gboolean nbd_accept(QIOChannel *ioc, GIOCondition cond, gpointer opaque)
     nb_fds++;
     nbd_update_server_watch();
     nbd_client_new(newproto ? NULL : exp, cioc,
-                   NULL, NULL, nbd_client_closed);
+                   tlscreds, NULL, nbd_client_closed);
     object_unref(OBJECT(cioc));
 
     return TRUE;
@@ -402,6 +404,37 @@ static QemuOptsList qemu_object_opts = {
 };
 
 
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+    Object *obj;
+    QCryptoTLSCreds *creds;
+
+    obj = object_resolve_path_component(
+        object_get_objects_root(), id);
+    if (!obj) {
+        error_setg(errp, "No TLS credentials with id '%s'",
+                   id);
+        return NULL;
+    }
+    creds = (QCryptoTLSCreds *)
+        object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+    if (!creds) {
+        error_setg(errp, "Object with id '%s' is not TLS credentials",
+                   id);
+        return NULL;
+    }
+
+    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+        error_setg(errp,
+                   "Expecting TLS credentials with a server endpoint");
+        return NULL;
+    }
+    object_ref(obj);
+    return creds;
+}
+
+
 int main(int argc, char **argv)
 {
     BlockBackend *blk;
@@ -441,6 +474,7 @@ int main(int argc, char **argv)
         { "verbose", 0, NULL, 'v' },
         { "object", 1, NULL, QEMU_NBD_OPT_OBJECT },
         { "export-name", 1, NULL, 'x' },
+        { "tls-creds", 1, NULL, QEMU_NBD_OPT_TLSCREDS },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -458,6 +492,7 @@ int main(int argc, char **argv)
     BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
     QDict *options = NULL;
     const char *export_name = NULL;
+    const char *tlscredsid = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -634,6 +669,9 @@ int main(int argc, char **argv)
                 exit(EXIT_FAILURE);
             }
         }   break;
+        case QEMU_NBD_OPT_TLSCREDS:
+            tlscredsid = optarg;
+            break;
         }
     }
 
@@ -650,6 +688,28 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
+    if (tlscredsid) {
+        if (sockpath) {
+            error_report("TLS is only supported with IPv4/IPv6");
+            exit(EXIT_FAILURE);
+        }
+        if (device) {
+            error_report("TLS is not supported with a host device");
+            exit(EXIT_FAILURE);
+        }
+        if (!export_name) {
+            /* Set the default NBD protocol export name, since
+             * we *must* use new style protocol for TLS */
+            export_name = "";
+        }
+        tlscreds = nbd_get_tls_creds(tlscredsid, &local_err);
+        if (local_err) {
+            error_report("Failed to get TLS creds %s",
+                         error_get_pretty(local_err));
+            exit(EXIT_FAILURE);
+        }
+    }
+
     if (disconnect) {
         int nbdfd = open(argv[optind], O_RDWR);
         if (nbdfd < 0) {
qemu-nbd.texi +4 -0
index 2516963..417f9c6 100644
@@ -76,6 +76,10 @@ Don't exit on the last connection
 @item -x NAME, --export-name=NAME
 Set the NDB volume export name. This switches the server to use
 the new style NBD protocol negotiation
+@item --tls-creds=ID
+Enable mandatory TLS encryption for the server by setting the ID
+of the TLS credentials object previously created with the --object
+option.
 @item -v, --verbose
 Display extra debugging information
 @item -h, --help
Signature · 1 line
2.5.0
#17 lore raw

PATCH v6 16/16 nbd: enable use of TLS with nbd-server-start command

In reply to Daniel P. Berrange

This modifies the nbd-server-start QMP command so that it is possible to request use of TLS. This is done by adding a new optional parameter "tls-creds" which provides the ID of a previously created QCryptoTLSCreds object instance.

TLS is only supported when using an IPv4/IPv6 socket listener.

---
 blockdev-nbd.c  | 122 ++++++++++++++++++++++++++++++++++++++++++++++----------
 hmp.c           |   2 +-
 qapi/block.json |   4 +-
 qmp-commands.hx |   2 +-
 4 files changed, 105 insertions(+), 25 deletions(-)
blockdev-nbd.c +100 -22
index f181840..12cae0e 100644
@@ -20,42 +20,126 @@
 #include "block/nbd.h"
 #include "io/channel-socket.h"
 
-static QIOChannelSocket *server_ioc;
-static int server_watch = -1;
+typedef struct NBDServerData {
+    QIOChannelSocket *listen_ioc;
+    int watch;
+    QCryptoTLSCreds *tlscreds;
+} NBDServerData;
+
+static NBDServerData *nbd_server;
+
 
 static gboolean nbd_accept(QIOChannel *ioc, GIOCondition condition,
                            gpointer opaque)
 {
     QIOChannelSocket *cioc;
 
+    if (!nbd_server) {
+        return FALSE;
+    }
+
     cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc),
                                      NULL);
     if (!cioc) {
         return TRUE;
     }
 
-    nbd_client_new(NULL, cioc, NULL, NULL, nbd_client_put);
+    nbd_client_new(NULL, cioc,
+                   nbd_server->tlscreds, NULL,
+                   nbd_client_put);
     object_unref(OBJECT(cioc));
     return TRUE;
 }
 
-void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
+
+static void nbd_server_free(NBDServerData *server)
 {
-    if (server_ioc) {
-        error_setg(errp, "NBD server already running");
+    if (!server) {
         return;
     }
 
-    server_ioc = qio_channel_socket_new();
-    if (qio_channel_socket_listen_sync(server_ioc, addr, errp) < 0) {
+    if (server->watch != -1) {
+        g_source_remove(server->watch);
+    }
+    object_unref(OBJECT(server->listen_ioc));
+    if (server->tlscreds) {
+        object_unref(OBJECT(server->tlscreds));
+    }
+
+    g_free(server);
+}
+
+static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
+{
+    Object *obj;
+    QCryptoTLSCreds *creds;
+
+    obj = object_resolve_path_component(
+        object_get_objects_root(), id);
+    if (!obj) {
+        error_setg(errp, "No TLS credentials with id '%s'",
+                   id);
+        return NULL;
+    }
+    creds = (QCryptoTLSCreds *)
+        object_dynamic_cast(obj, TYPE_QCRYPTO_TLS_CREDS);
+    if (!creds) {
+        error_setg(errp, "Object with id '%s' is not TLS credentials",
+                   id);
+        return NULL;
+    }
+
+    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
+        error_setg(errp,
+                   "Expecting TLS credentials with a server endpoint");
+        return NULL;
+    }
+    object_ref(obj);
+    return creds;
+}
+
+
+void qmp_nbd_server_start(SocketAddress *addr,
+                          bool has_tls_creds, const char *tls_creds,
+                          Error **errp)
+{
+    if (nbd_server) {
+        error_setg(errp, "NBD server already running");
         return;
     }
 
-    server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
-                                         G_IO_IN,
-                                         nbd_accept,
-                                         NULL,
-                                         NULL);
+    nbd_server = g_new0(NBDServerData, 1);
+    nbd_server->watch = -1;
+    nbd_server->listen_ioc = qio_channel_socket_new();
+    if (qio_channel_socket_listen_sync(
+            nbd_server->listen_ioc, addr, errp) < 0) {
+        goto error;
+    }
+
+    if (has_tls_creds) {
+        nbd_server->tlscreds = nbd_get_tls_creds(tls_creds, errp);
+        if (!nbd_server->tlscreds) {
+            goto error;
+        }
+
+        if (addr->type != SOCKET_ADDRESS_KIND_INET) {
+            error_setg(errp, "TLS is only supported with IPv4/IPv6");
+            goto error;
+        }
+    }
+
+    nbd_server->watch = qio_channel_add_watch(
+        QIO_CHANNEL(nbd_server->listen_ioc),
+        G_IO_IN,
+        nbd_accept,
+        NULL,
+        NULL);
+
+    return;
+
+ error:
+    nbd_server_free(nbd_server);
+    nbd_server = NULL;
 }
 
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
@@ -64,7 +148,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
     BlockBackend *blk;
     NBDExport *exp;
 
-    if (!server_ioc) {
+    if (!nbd_server) {
         error_setg(errp, "NBD server not running");
         return;
     }
@@ -110,12 +194,6 @@ void qmp_nbd_server_stop(Error **errp)
 {
     nbd_export_close_all();
 
-    if (server_watch != -1) {
-        g_source_remove(server_watch);
-        server_watch = -1;
-    }
-    if (server_ioc) {
-        object_unref(OBJECT(server_ioc));
-        server_ioc = NULL;
-    }
+    nbd_server_free(nbd_server);
+    nbd_server = NULL;
 }
hmp.c +1 -1
index 41fb9ca..996cb91 100644
@@ -1793,7 +1793,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         goto exit;
     }
 
-    qmp_nbd_server_start(addr, &local_err);
+    qmp_nbd_server_start(addr, false, NULL, &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
qapi/block.json +3 -1
index ed61f82..58e6b30 100644
@@ -146,13 +146,15 @@
 # QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
 #
 # @addr: Address on which to listen.
+# @tls-creds: (optional) ID of the TLS credentials object. Since 2.6
 #
 # Returns: error if the server is already running.
 #
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-start',
-  'data': { 'addr': 'SocketAddress' } }
+  'data': { 'addr': 'SocketAddress',
+            '*tls-creds': 'str'} }
 
 ##
 # @nbd-server-add:
qmp-commands.hx +1 -1
index 020e5ee..9fb0d78 100644
@@ -3825,7 +3825,7 @@ EQMP
 
     {
         .name       = "nbd-server-start",
-        .args_type  = "addr:q",
+        .args_type  = "addr:q,tls-creds:s?",
         .mhandler.cmd_new = qmp_marshal_nbd_server_start,
     },
     {
Signature · 1 line
2.5.0
#18 lore raw
In reply to Daniel P. Berrange

On Wed, Feb 10, 2016 at 06:40:58PM +0000, Daniel P. Berrange wrote:

[...]

I've applied all the series in this patches, to yesterday's Git master, so I'm here:

    $ git describe
    pull-qcrypto-next-2016-02-02-1-378-gf9375d2
Quoted 6 lines

Starting a QEMU system emulator built-in NBD server

  $ qemu-system-x86_64 \
     -qmp unix:/tmp/qmp,server \
     -hda /home/berrange/Fedora-Server-netinst-x86_64-23.iso \
     -object tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,endpoint=server

Instead of an ISO, I have this command-line:

    $QEMU \
      -display none \
      -nodefconfig \
      -nodefaults \
      -m 2048 \
      -device virtio-scsi-pci,id=scsi \
      -device virtio-serial-pci \
      -serial stdio \
      -drive file=./cirros-0.3.3.qcow2,format=qcow2,if=virtio \
      -object tls-creds-x509,id=tls0,dir=/export/security/gnutls,endpoint=server \
      -qmp unix:./qmp-sock,server
Quoted 3 lines
  $ qmp-shell /tmp/qmp
     (qmp) nbd-server-start addr={"host":"localhost","port":"9000"}
     tls-creds=tls0

However, this invocation seem to work for me, am I doing something wrong?

    $ ./qmp-shell ./qmp-sock
    Welcome to the QMP low-level shell!
    Connected to QEMU 2.5.50
    (QEMU) nbd-server-start addr={'host:'localhost','port':'9000'} tls-creds=tls0
    {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'addr', expected: QDict"}}
    (QEMU) 

I also tried the rlwrap with UNIX socket approach, the below way. You corrected my invocation on #qemu (where I was incorrectly using 'tls-creds' as part of the "addr" struct); hope I got it right this time:

    $ rlwrap -H ~/.qmp_history socat UNIX-CONNECT:./qmp-sock STDIO
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 5, "major": 2}, "package": ""}, "capabilities": []}}
    { "execute": "qmp_capabilities" }
    {"return": {}}
    {"execute":"nbd-server-start","arguments":{"addr":{"type":"inet","data":{"host":"localhost","port":"9000"}}, "tls-creds":"tls0"}

Which just seems to be running forever, and does not yield anything (I was expecting a: {"return": {}}).

[...]

Signature · 1 line
/kashyap
#19 lore raw
Quoted 37 lines · Kashyap Chamarthy

On Wed, Feb 10, 2016 at 06:40:58PM +0000, Daniel P. Berrange wrote:

[...]

I've applied all the series in this patches, to yesterday's Git master, so I'm here:

    $ git describe
    pull-qcrypto-next-2016-02-02-1-378-gf9375d2
Quoted 6 lines

Starting a QEMU system emulator built-in NBD server

  $ qemu-system-x86_64 \
     -qmp unix:/tmp/qmp,server \
     -hda /home/berrange/Fedora-Server-netinst-x86_64-23.iso \
     -object tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,endpoint=server

Instead of an ISO, I have this command-line:

    $QEMU \
      -display none \
      -nodefconfig \
      -nodefaults \
      -m 2048 \
      -device virtio-scsi-pci,id=scsi \
      -device virtio-serial-pci \
      -serial stdio \
      -drive file=./cirros-0.3.3.qcow2,format=qcow2,if=virtio \
      -object tls-creds-x509,id=tls0,dir=/export/security/gnutls,endpoint=server \
      -qmp unix:./qmp-sock,server
Quoted 3 lines
  $ qmp-shell /tmp/qmp
     (qmp) nbd-server-start addr={"host":"localhost","port":"9000"}
     tls-creds=tls0

However, this invocation seem to work for me, am I doing something wrong?

Quoted 7 lines
    $ ./qmp-shell ./qmp-sock
    Welcome to the QMP low-level shell!
    Connected to QEMU 2.5.50
    (QEMU) nbd-server-start addr={'host:'localhost','port':'9000'} tls-creds=tls0
    {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'addr', expected: QDict"}}
    (QEMU)

Yes, my bad - I provided the wrong syntax here. it should be

(QEMU) nbd-server-start addr={"type":"inet","data":{"host":"localhost","port":"9000"}} tls-creds=tls0 {"return": {}}
(QEMU) nbd-server-add device=ide0-hd0
{"return": {}}

I've genuinely tested it this time :-)

Regards,
Daniel

Signature · 4 lines
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
#20 lore raw
Quoted 6 lines · Daniel P. Berrange
Quoted 5 lines · Kashyap Chamarthy

On Wed, Feb 10, 2016 at 06:40:58PM +0000, Daniel P. Berrange wrote:

[...]

I've applied all the series in this patches,

s/series in this patches/patches in this series/

Quoted 6 lines
Quoted 6 lines

to yesterday's Git master,
so I'm here:

    $ git describe
    pull-qcrypto-next-2016-02-02-1-378-gf9375d2

[...]

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
  $ qmp-shell /tmp/qmp
     (qmp) nbd-server-start addr={"host":"localhost","port":"9000"}
     tls-creds=tls0

However, this invocation seem to work for me, am I doing something

Typo: s/seem to work/doesn't seem to work/

Quoted 11 lines
Quoted 10 lines

wrong?

    $ ./qmp-shell ./qmp-sock
    Welcome to the QMP low-level shell!
    Connected to QEMU 2.5.50
    (QEMU) nbd-server-start addr={'host:'localhost','port':'9000'} tls-creds=tls0
    {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'addr', expected: QDict"}}
    (QEMU)
Quoted 8 lines

Yes, my bad - I provided the wrong syntax here. it should be

(QEMU) nbd-server-start addr={"type":"inet","data":{"host":"localhost","port":"9000"}} tls-creds=tls0 {"return": {}}
(QEMU) nbd-server-add device=ide0-hd0
{"return": {}}

I've genuinely tested it this time :-)

Heh, right, this works. I should've figured this out, given that I've already used it in the complete JSON invocation in my previous email.

    [...] "addr":{"type":"inet","data":{"host":"localhost","port":"9000"}}[...]

So, 'qmp-shell' invocation works for me too:

    $ ./qmp-shell ./qmp-sock
    [...]
    (QEMU) nbd-server-start addr={"type":"inet","data":{"host":"localhost","port":"9000"}} tls-creds=tls0
    {"return": {}}
    (QEMU)
    (QEMU) nbd-server-add device=virtio0
    {"return": {}}
    (QEMU)
Signature · 1 line
/kashyap
#21 lore raw

On Fri, Feb 12, 2016 at 05:03:07PM +0100, Kashyap Chamarthy wrote:

[...]

Quoted 11 lines

So, 'qmp-shell' invocation works for me too:

    $ ./qmp-shell ./qmp-sock
    [...]
    (QEMU) nbd-server-start addr={"type":"inet","data":{"host":"localhost","port":"9000"}} tls-creds=tls0
    {"return": {}}
    (QEMU)
    (QEMU) nbd-server-add device=virtio0
    {"return": {}}
    (QEMU)

Noting so I can remember in future, thanks to commit 1ceca07 ("scripts:
qmp-shell: Add verbose flag"), if I supply '-v' flag to 'qmp-shell', it will conveniently print out the full QMP command-line it constructed:

    $ ./qmp-shell -v /var/tmp/qmp-sock
    [...]
    (QEMU) nbd-server-start addr={"type":"inet","data":{"host":"localhost","port":"9000"}} tls-creds=tls0
    {"execute": "nbd-server-start", "arguments": {"addr": {"data": {"host": "localhost", "port": "9000"}, "type": "inet"}, "tls-creds": "tls0"}}
    {"return": {}}
    (QEMU) nbd-server-add device=virtio0
    {"execute": "nbd-server-add", "arguments": {"device": "virtio0"}}
    {"return": {}}
    (QEMU) nbd-server-stop
    {"execute": "nbd-server-stop", "arguments": {}}
    {"return": {}}
    (QEMU) 
Signature · 1 line
/kashyap
#22 lore raw
In reply to Daniel P. Berrange
Quoted 147 lines · Daniel P. Berrange

This is an update of the series previously posted:

v1: https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06126.html

    v2: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01580.html
      v3: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03440.html
        v4: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04160.html

This series of patches implements support for TLS in the QEMU NBD server and client code.

It is implementing the NBD_OPT_STARTTLS option that was previously discussed here:

https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html

And is also described in the NBD spec here:

https://github.com/yoe/nbd/blob/master/doc/proto.md

To ensure that clients always get a suitable error message from the NBD server when it is configured with TLS, a client speaking the new style protocol will always send NBD_OPT_LIST as the first thing it does, so that we can see the NBD_REP_ERR_TLS_REQD response. This should all be backwards & forwards compatible with previous QEMU impls of NBD

Usage of TLS is described in the commit messages for each patch, but for sake of people who don't want to explore the series, here's the summary

Starting QEMU system emulator with a disk backed by an TLS encrypted NBD export

  $ qemu-system-x86_64 \
     -object tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls \
     -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0

Starting a standalone NBD server providing a TLS encrypted NBD export

  $ qemu-nbd \
     --object tls-creds-x509,id=tls0,endpoint=server,dir=/home/berrange/security/qemutls
     --tls-creds tls0 \
     --export-name default \
     $IMAGEFILE

The --export-name is optional, if omitted, the default "" will be used.

Starting a QEMU system emulator built-in NBD server

  $ qemu-system-x86_64 \
     -qmp unix:/tmp/qmp,server \
     -hda /home/berrange/Fedora-Server-netinst-x86_64-23.iso \
     -object tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,endpoint=server
  $ qmp-shell /tmp/qmp
     (qmp) nbd-server-start addr={"host":"localhost","port":"9000"}
     tls-creds=tls0
     (qmp) nbd-server-add device=ide0-hd0

The first 2 patches are taken from this other pending patch series in order to facilitate merge:

https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00296.html

The first 4 patches are the conversion to the I/O channels framework.

The next 6 patches are general tweaks to QEMU's impl of the NBD protocol for better compliance and/or future proofing.

The next patch provides the NBD protocol TLS implementation.

The final 3 patches allow TLS to be enabled in the QEMU NBD client and servers.

Changed in v6:

- Rebase to resolve conflicts with recent qapi changes (Eric) and recent nbd changes (Paolo) which merged
- Add MODULE_INIT_QOM to qemu-io & qemu-img to ensure they can load the QIOChannel object types

Changed in v5:

- Pulled in https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00297.html
and applied fixes for issues Eric mentioned in that review
- Pulled in https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00302.html
- Rebased to latest git master

Changed in v4:

- Don't pick the first export name in the list if no export name is provided (Paolo)
- Set client requested export name to "" if none is provided by the user (Paolo)
- Set server advertized export name to "" if TLS is enabled and none is provided by the user (Paolo)
- Rename qemu-nbd --exportname to --export-name (Paolo)
- Use iov_discard_front() to simplify iov handling (Paolo)

Changed in v3:

- Rebase to resolve conflicts with recently merged NBD patches

Changed in v2:

- Fix error codes used during NBD TLS option negotiate
- Update patch with helpers for UserCreatable object types

Daniel P. Berrange (16):
qom: add helpers for UserCreatable object types
qemu-nbd: add support for --object command line arg
nbd: convert block client to use I/O channels for connection setup
nbd: convert qemu-nbd server to use I/O channels for connection setup
nbd: convert blockdev NBD server to use I/O channels for connection
setup
nbd: convert to using I/O channels for actual socket I/O
nbd: invert client logic for negotiating protocol version
nbd: make server compliant with fixed newstyle spec
nbd: make client request fixed new style if advertized
nbd: allow setting of an export name for qemu-nbd server
nbd: always query export list in fixed new style protocol
nbd: use "" as a default export name if none provided
nbd: implement TLS support in the protocol negotiation
nbd: enable use of TLS with NBD block driver
nbd: enable use of TLS with qemu-nbd server
nbd: enable use of TLS with nbd-server-start command

 Makefile                        |   6 +-
 block/nbd-client.c              |  91 ++++++---
 block/nbd-client.h              |  10 +-
 block/nbd.c                     | 105 ++++++++--
 blockdev-nbd.c                  | 131 ++++++++++--
 hmp.c                           |  54 ++---
 include/block/nbd.h             |  28 ++-
 include/monitor/monitor.h       |   3 -
 include/qom/object_interfaces.h |  92 +++++++++
 nbd/client.c                    | 440 +++++++++++++++++++++++++++++++++++-----
 nbd/common.c                    |  83 +++++---
 nbd/nbd-internal.h              |  32 ++-
 nbd/server.c                    | 334 +++++++++++++++++++++---------
 qapi/block.json                 |   4 +-
 qemu-img.c                      |   1 +
 qemu-io.c                       |   1 +
 qemu-nbd.c                      | 195 ++++++++++++++----
 qemu-nbd.texi                   |  13 ++
Quoted 9 lines
 qmp-commands.hx                 |   2 +-
 qmp.c                           |  76 +------
 qom/object_interfaces.c         | 174 ++++++++++++++++
 tests/Makefile                  |   2 +-
 tests/qemu-iotests/140.out      |   2 +-
 tests/qemu-iotests/143.out      |   2 +-
 vl.c                            |  68 +------
 25 files changed, 1459 insertions(+), 490 deletions(-)

Queued, thanks.

Paolo