Implement TLS support to QEMU NBD server & client
Subsystems (2)
v3 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
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=tls0Starting 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 \
--exportname default \
$IMAGEFILENB, exportname is mandatory, since TLS requires the fixed-new style NBD protocol negotiation.
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-hd0This series depends on this bug fix I recently sent:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03406.html
And the qemu-nbd/etc command line options work
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03381.html
The first 4 patches are the conversion to the I/O channels framework.
The next 5 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 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 (13):
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: pick first exported volume if no export name is requested
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 | 74 +++++---- block/nbd-client.h | 10 +- block/nbd.c | 105 ++++++++++--- blockdev-nbd.c | 131 +++++++++++++--- hmp.c | 2 +- include/block/nbd.h | 28 +++- nbd/client.c | 442 +++++++++++++++++++++++++++++++++++++++++++++------- nbd/common.c | 125 ++++++++++++--- nbd/nbd-internal.h | 32 ++-- nbd/server.c | 333 ++++++++++++++++++++++++++++----------- qapi/block.json | 4 +- qemu-nbd.c | 158 ++++++++++++++----- qemu-nbd.texi | 7 + qmp-commands.hx | 2 +- tests/Makefile | 2 +- 16 files changed, 1152 insertions(+), 309 deletions(-)
Signature · 1 line
2.5.0
PATCH v3 1/13 nbd: convert block client to use I/O channels for connection setup
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.
In this initial conversion though, all I/O is still actually done using the raw POSIX sockets APIs.
--- Makefile | 6 +++--- block/nbd-client.c | 59 ++++++++++++++++++++++++++++-------------------------- block/nbd-client.h | 8 ++++++-- block/nbd.c | 39 ++++++++++++++++++------------------ tests/Makefile | 2 +- 5 files changed, 61 insertions(+), 53 deletions(-)
Makefile +3 -3
@@ -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 +31 -28
144 lines not on this page · raw message
block/nbd-client.h +6 -2
@@ -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
@@ -30,7 +30,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"
@@ -239,25 +238,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,
@@ -265,7 +264,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. */
@@ -277,15 +277,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; }
tests/Makefile +1 -1
@@ -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
PATCH v3 2/13 nbd: convert qemu-nbd server to use I/O channels for connection setup
In reply to Daniel P. BerrangeThis 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 | 89 +++++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 51 insertions(+), 38 deletions(-)
qemu-nbd.c +51 -38
@@ -21,7 +21,6 @@#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"
@@ -29,15 +28,12 @@#include "qapi/qmp/qstring.h" #include "qapi/opts-visitor.h" #include "qom/object_interfaces.h" +#include "io/channel-socket.h" #include <stdarg.h> #include <stdio.h> #include <getopt.h> #include <sys/types.h> -#include <sys/socket.h> -#include <netinet/in.h> -#include <netinet/tcp.h> -#include <arpa/inet.h> #include <signal.h> #include <libgen.h> #include <pthread.h>
@@ -58,7 +54,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) {
@@ -241,19 +238,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) {
@@ -269,7 +268,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; }
@@ -290,13 +289,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 ;
@@ -313,7 +313,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) {
@@ -321,37 +321,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 ; + } } }
@@ -469,7 +483,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;
@@ -671,15 +684,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]);
@@ -831,8 +844,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 ; }
@@ -850,8 +864,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
PATCH v3 3/13 nbd: convert blockdev NBD server to use I/O channels for connection setup
In reply to Daniel P. BerrangeThis 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
@@ -17,32 +17,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 ); }/*
@@ -77,7 +93,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,NBDExport *exp;NBDCloseNotifier *n; -if (server_fd ==-1 ) { +if (!server_ioc) {error_setg (errp,"NBD server not running" );return ; }
@@ -127,9 +143,12 @@ void qmp_nbd_server_stop(Error **errp)nbd_close_notifier (&cn->n,nbd_export_get_blockdev (cn->exp)); } -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
PATCH v3 4/13 nbd: convert to using I/O channels for actual socket I/O
In reply to Daniel P. BerrangeNow 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 | 112 ++++++++++++++++++++++++++++++--------- nbd/nbd-internal.h | 18 +++---- nbd/server.c | 150 +++++++++++++++++++++++++++++----------------------- qemu-nbd.c | 10 ++-- 8 files changed, 226 insertions(+), 149 deletions(-)
block/nbd-client.c +10 -9
@@ -71,7 +71,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 ; }
@@ -122,6 +122,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); s->send_coroutine =qemu_coroutine_self ();
@@ -131,17 +132,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);
@@ -164,8 +165,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 ; }
@@ -372,7 +373,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); }
@@ -387,7 +388,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
@@ -26,7 +26,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 );
@@ -34,10 +33,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
@@ -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
@@ -70,7 +70,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 ];
@@ -82,7 +82,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; }
@@ -108,7 +108,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; }
@@ -129,35 +129,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; }
@@ -174,7 +174,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; }
@@ -182,19 +182,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; }
@@ -205,11 +205,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;
@@ -282,7 +282,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 ; }
@@ -293,7 +293,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;
@@ -308,7 +308,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; }
@@ -320,13 +320,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 +86 -26
133 lines not on this page · raw message
nbd/nbd-internal.h +8 -10
@@ -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
@@ -252,7 +252,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) {
@@ -268,7 +268,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; }
@@ -328,7 +328,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 );
@@ -343,10 +342,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
Re: [Qemu-devel] [PATCH v3 04/13] nbd: convert to using I/O channels for actual socket I/O
Quoted 26 lines · Daniel P. Berrange
+ * This will update @iov so that its head is advanced
+ * by @skip bytes. To do this, zero or more complete
+ * elements of @iov will be skipped over. The new head
+ * of @iov will then have its base & len updated to
+ * skip the remaining number of bytes. @oldiov will
+ * hold the original data from the new head of @iov.
+ */nbd/common.c +10
-7+static void nbd_skip_iov (size_t skip, +struct iovec **iov, +int *niov, +struct iovec *oldiov) { -size_t offset =0 ; -int err; - -if (qemu_in_coroutine ()) { -if (do_read) { -return qemu_co_recv (fd, buffer, size); +ssize_t done =0 ; +size_t i; +for (i =0 ; i < *niov; i++) { +if ((*iov)[i].iov_len <= skip) { + done += (*iov)[i].iov_len; + skip -= (*iov)[i].iov_len; }else { -return qemu_co_send (fd, buffer, size);
nbd/common.c line 55
Quoted 10 lines
nbd/common.c +8
-0+ done += skip; + oldiov->iov_base = (*iov)[i].iov_base; + oldiov->iov_len = (*iov)[i].iov_len; + (*iov)[i].iov_len -= skip; + (*iov)[i].iov_base += skip; + *iov = *iov + i; + *niov = *niov - i; +break ; } }
Can you use iov_discard_front instead?
nbd/common.c line 103
Quoted 5 lines
nbd/common.c +4
-0+/* XXX see about using qio_channel_yield() in +* future if we can use normal watches instead +* of the AIO handlers */ +qemu_coroutine_yield ();
This breaks dataplane. The simplest solution is to change qio_channel_yield() to just
qio_channel_aio_yield(ioc, qemu_get_aio_context(), condition);
where qio_channel_aio_yield(QIOChannel*, AioContext *, int) is implemented by the backends. The implementation could then use aio_set_fd_handler, and it would be almost trivial because it would reuse common code just like the one in io/channel-watch.c.
Websock is the only complicated case. I think it's fine if you simply must not use qio_channel_{,aio_}yield() for websock channels.
That said, I'm not sure you can even rely on nbd_negotiate_continue to run in the main I/O thread. So, another possibility is to add an alternative watch API similar to AioContext's:
qio_channel_set_handler(QIOChannel*, AioContext*, void(*in)(void*),
void(*out)(void*), void *opaque)
whose implementation would again be simple for backends other than websock, and probably trivial for backends other than buffer and websock. Again, it wouldn't be a problem to skip buffer and websock for the time being (read, until someone actually needs it).
Converting the block layer to use GMainLoop, unfortunately, would lose too much performance.
Paolo
Re: [Qemu-devel] [PATCH v3 04/13] nbd: convert to using I/O channels for actual socket I/O
Quoted 33 lines · Paolo Bonzini
Quoted 30 lines · Daniel P. Berrange
+ * This will update @iov so that its head is advanced
+ * by @skip bytes. To do this, zero or more complete
+ * elements of @iov will be skipped over. The new head
+ * of @iov will then have its base & len updated to
+ * skip the remaining number of bytes. @oldiov will
+ * hold the original data from the new head of @iov.
+ */nbd/common.c +14
-7+static void nbd_skip_iov (size_t skip, +struct iovec **iov, +int *niov, +struct iovec *oldiov) { -size_t offset =0 ; -int err; - -if (qemu_in_coroutine ()) { -if (do_read) { -return qemu_co_recv (fd, buffer, size); +ssize_t done =0 ; +size_t i; +for (i =0 ; i < *niov; i++) { +if ((*iov)[i].iov_len <= skip) { + done += (*iov)[i].iov_len; + skip -= (*iov)[i].iov_len; }else { -return qemu_co_send (fd, buffer, size); + done += skip; + oldiov->iov_base = (*iov)[i].iov_base; + oldiov->iov_len = (*iov)[i].iov_len; + (*iov)[i].iov_len -= skip;
Quoted 8 lines
nbd/common.c line 55
Quoted 6 lines
nbd/common.c +4
-0+ (*iov)[i].iov_base += skip; + *iov = *iov + i; + *niov = *niov - i; +break ; } }Can you use
iov_discard_frontinstead?
I didn't know that existed - i'll investigate whether it is possible to use it.
Quoted 22 lines
nbd/common.c line 103
Quoted 4 lines
nbd/common.c +4
-0+/* XXX see about using qio_channel_yield() in +* future if we can use normal watches instead +* of the AIO handlers */ +qemu_coroutine_yield ();This breaks dataplane. The simplest solution is to change
qio_channel_yield()to justqio_channel_aio_yield(ioc, qemu_get_aio_context(), condition);where
qio_channel_aio_yield(QIOChannel*, AioContext *, int)is implemented by the backends. The implementation could then useaio_set_fd_handler, and it would be almost trivial because it would reuse common code just like the one inio/channel-watch.c.Websock is the only complicated case. I think it's fine if you simply must not use qio_channel_{,aio_}yield() for websock channels.
That said, I'm not sure you can even rely on
nbd_negotiate_continueto run in the main I/O thread. So, another possibility is to add an alternative watch API similar to AioContext's:
Quoted 7 lines
qio_channel_set_handler(QIOChannel*, AioContext*, void(*in)(void*),
void(*out)(void*), void *opaque)whose implementation would again be simple for backends other than websock, and probably trivial for backends other than buffer and websock. Again, it wouldn't be a problem to skip buffer and websock for the time being (read, until someone actually needs it).
I'll have a look at this and see what I can do. It shouldn't block merging of this patch I assume, since the current call to qemu_coroutine_yield() should be fine - it just my comment suggestion that would not work. So I'll investigate what you suggest here wrt to AioContext & QIOChannel as a later followup patch.
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 :|
Re: [Qemu-devel] [PATCH v3 04/13] nbd: convert to using I/O channels for actual socket I/O
Quoted 4 lines · Daniel P. Berrange
I'll have a look at this and see what I can do. It shouldn't block merging of this patch I assume, since the current call to
qemu_coroutine_yield()should be fine - it just my comment suggestion that would not work.
Yes, that is tied to negotiations being always in the main loop. I'm not entirely sure that is the case (e.g. a call bdrv_reopen could perhaps be from somewhere else) but it's anyway no worse than the current state.
Paolo
PATCH v3 5/13 nbd: invert client logic for negotiating protocol version
In reply to Daniel P. BerrangeThe 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
@@ -115,20 +115,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;
@@ -141,6 +132,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" );
@@ -161,39 +156,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
PATCH v3 6/13 nbd: make server compliant with fixed newstyle spec
In reply to Daniel P. BerrangeIf 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
@@ -307,6 +307,7 @@ fail:static int nbd_negotiate_options (NBDClient *client) {uint32_t flags; +bool fixedNewstyle = false;/* Client sends: [ 0 .. 3] client flags
@@ -329,14 +330,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)) !=
@@ -350,10 +356,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)) {
@@ -362,26 +370,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
PATCH v3 7/13 nbd: make client request fixed new style if advertized
In reply to Daniel P. BerrangeIf 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
@@ -75,7 +75,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." );
@@ -116,19 +115,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 */
@@ -164,11 +170,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
PATCH v3 8/13 nbd: allow setting of an export name for qemu-nbd server
In reply to Daniel P. BerrangeThe 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 a "--exportname NAME" argument to qemu-nbd which allows the user to set an explicit export name. When --exportname is set the server will always use the new style protocol.
--- qemu-nbd.c | 14 ++++++++++++-- qemu-nbd.texi | 3 +++ 2 files changed, 15 insertions(+), 2 deletions(-)
qemu-nbd.c +12 -2
@@ -47,6 +47,7 @@#define QEMU_NBD_OPT_IMAGE_OPTS 261static NBDExport *exp; +static bool newproto;static int verbose;static char *srcpath;static SocketAddress *saddr;
@@ -342,7 +343,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;
@@ -445,7 +446,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" , no_argument,NULL ,'h' }, {"version" , no_argument,NULL ,'V' },
@@ -471,6 +472,7 @@ int main(int argc, char **argv) { "verbose", no_argument, NULL, 'v' }, { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT }, { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, + { "exportname", required_argument, NULL, 'x' }, { NULL, 0, NULL, 0 } };int ch;
@@ -489,6 +491,7 @@ int main(int argc, char **argv)QDict *options =NULL ;QemuOpts *opts;bool imageOpts = false; +const char *exportname =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.
@@ -640,6 +643,9 @@ int main(int argc, char **argv)case 't' : persistent =1 ;break ; +case 'x' : + exportname = optarg; +break ;case 'v' : verbose =1 ;break ;
@@ -839,6 +845,10 @@ int main(int argc, char **argv)error_report_err (local_err);exit (EXIT_FAILURE ); } +if (exportname) { +nbd_export_set_name (exp, exportname); + 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
@@ -63,6 +63,9 @@ Export QEMU disk image using NBD protocol. force block driver for format @var{fmt} instead of auto-detecting @item -t, --persistent don't exit on the last connection +@item -x NAME, --exportname=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
Re: [Qemu-devel] [PATCH v3 08/13] nbd: allow setting of an export name for qemu-nbd server
Quoted 8 lines · 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 a "--exportname NAME" argument to qemu-nbd which allows the user to set an explicit export name. When --exportname is set the server will always use the new style protocol.
Let's make this --export-name.
Paolo
Re: [Qemu-devel] [PATCH v3 08/13] nbd: allow setting of an export name for qemu-nbd server
Quoted 8 lines · Paolo Bonzini
Quoted 5 lines · 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.
Quoted 5 lines
Quoted 3 lines
This adds a "--exportname NAME" argument to qemu-nbd which allows the user to set an explicit export name. When --exportname is set the server will always use the new style protocol.
Let's make this --export-name.
Sure, will do.
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 :|
PATCH v3 9/13 nbd: pick first exported volume if no export name is requested
In reply to Daniel P. BerrangeThe NBD client is currently only capable of using the new style protocol negotiation if an explicit export name is provided. This is a problem, because TLS support will require use of the new style protocol in all cases, and we wish to keep the export name as an optional request for backwards compatibility.
The trivial way to deal with this is to use the NBD protocol option for listing exports and then pick the first returned export as the one to use. This makes the client "do the right thing" in the normal case where the qemu-nbd server only exports a single volume.
Furthermore, in order to get proper error reporting of unknown options with fixed new style protocol, we must not send the NBD_OPT_EXPORT_NAME as the first thing. Thus, even if an export name is provided we still send NBD_OPT_LIST to enumerate server exports. This also gives us clearer error reporting in the case that the requested export name does not exist. If NBD_OPT_LIST is not supported, we still fallback to using the specified export name, so as not to break compatibility with old QEMU NBD server impls that predated NBD_OPT_LIST
--- nbd/client.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- nbd/server.c | 2 + 2 files changed, 198 insertions(+), 5 deletions(-)
nbd/client.c +196 -5
@@ -70,12 +70,187 @@ 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 char *nbd_receive_pick_export (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 ; +char *chosenname =NULL ; + +TRACE ("Picking export name" ); +if (write_sync (ioc, &magic,sizeof (magic)) !=sizeof (magic)) { +error_setg (errp,"Failed to send list option magic" ); + goto fail; + } + +if (write_sync (ioc, &opt,sizeof (opt)) !=sizeof (opt)) { +error_setg (errp,"Failed to send list option number" ); + goto fail; + } + +if (write_sync (ioc, &length,sizeof (length)) !=sizeof (length)) { +error_setg (errp,"Failed to send list option length" ); + goto fail; + } + +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 ; + goto fail; + } +if (ret ==0 ) {/* Server doesn't support export listing */ +if (wantname ==NULL ) { +error_setg (errp,"Export name is required" ); + goto fail; + } + chosenname =g_strdup (wantname); +break ; + } +if (name ==NULL ) { +TRACE ("End of export name list" ); +break ; + } +if (!chosenname && (!wantname ||g_str_equal (name, wantname))) { + chosenname = name; +TRACE ("Picked export name %s" , name); + }else { +TRACE ("Ignored export name %s" , name); +g_free (name); + } + } + +if (wantname && !chosenname) { +error_setg (errp,"No export with name %s available" , wantname); + goto fail; + } + + fail: +TRACE ("Chosen export name %s" , chosenname ? chosenname :"<null>" ); +return chosenname; +} +int nbd_receive_negotiate (QIOChannel *ioc,const char *name,uint32_t *flags,off_t *size,Error **errp) {char buf[256 ];uint64_t magic, s;int rc; +char *autoname =NULL ;TRACE ("Receiving negotiation." );
@@ -120,27 +295,40 @@ 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) { + autoname =nbd_receive_pick_export (ioc, name, errp); +if (!autoname) { + goto fail; + } + name = autoname; + }else { +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)) {
@@ -175,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" );
@@ -206,6 +396,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, rc =0 ; fail: +free (autoname);return rc; }
nbd/server.c +2 -0
@@ -291,6 +291,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" );
Signature · 1 line
2.5.0
Re: [Qemu-devel] [PATCH v3 09/13] nbd: pick first exported volume if no export name is requested
Quoted 22 lines · Daniel P. Berrange
The NBD client is currently only capable of using the new style protocol negotiation if an explicit export name is provided. This is a problem, because TLS support will require use of the new style protocol in all cases, and we wish to keep the export name as an optional request for backwards compatibility.
The trivial way to deal with this is to use the NBD protocol option for listing exports and then pick the first returned export as the one to use. This makes the client "do the right thing" in the normal case where the qemu-nbd server only exports a single volume.
Furthermore, in order to get proper error reporting of unknown options with fixed new style protocol, we must not send the
NBD_OPT_EXPORT_NAMEas the first thing. Thus, even if an export name is provided we still sendNBD_OPT_LISTto enumerate server exports. This also gives us clearer error reporting in the case that the requested export name does not exist. IfNBD_OPT_LISTis not supported, we still fallback to using the specified export name, so as not to break compatibility with old QEMU NBD server impls that predatedNBD_OPT_LIST
Quoted 1 line
Signed-off-by Daniel P. Berrange <[email protected]>
As a first reaction, I would really avoid magic unless the server provides a single exports. But even in that case, I would prefer to have some synchronization between the server and client command-line.
Is an empty NBD_OPT_EXPORT_NAME valid? What about using new-style negotiation with empty NBD_OPT_EXPORT_NAME if TLS is requested?
Paolo
Re: [Qemu-devel] [PATCH v3 09/13] nbd: pick first exported volume if no export name is requested
Quoted 25 lines · Paolo Bonzini
Quoted 22 lines · Daniel P. Berrange
The NBD client is currently only capable of using the new style protocol negotiation if an explicit export name is provided. This is a problem, because TLS support will require use of the new style protocol in all cases, and we wish to keep the export name as an optional request for backwards compatibility.
The trivial way to deal with this is to use the NBD protocol option for listing exports and then pick the first returned export as the one to use. This makes the client "do the right thing" in the normal case where the qemu-nbd server only exports a single volume.
Furthermore, in order to get proper error reporting of unknown options with fixed new style protocol, we must not send the
NBD_OPT_EXPORT_NAMEas the first thing. Thus, even if an export name is provided we still sendNBD_OPT_LISTto enumerate server exports. This also gives us clearer error reporting in the case that the requested export name does not exist. IfNBD_OPT_LISTis not supported, we still fallback to using the specified export name, so as not to break compatibility with old QEMU NBD server impls that predatedNBD_OPT_LIST
Quoted 8 lines
Quoted 1 line
Signed-off-by Daniel P. Berrange <[email protected]>As a first reaction, I would really avoid magic unless the server provides a single exports. But even in that case, I would prefer to have some synchronization between the server and client command-line.
Is an empty
NBD_OPT_EXPORT_NAMEvalid? What about using new-style negotiation with emptyNBD_OPT_EXPORT_NAMEif TLS is requested?
The main goal here is to ensure the NBD client gets a decent error message if it tries to connect without TLS. Even if we are using the fixed new style protocol, the client code will send NBD_OPT_EXPORT_NAME as the first thing it does. Thanks to a bit of crazyness is the NBD protocol spec, the server is unable to reply with an error message to NBD_OPT_EXPORT_NAME.
So if the client connected to a server reqiuring TLS and does not request TLS enablement, the server will have no choice but to just close the connection with no error. I think this will be pretty nasty for users trying to debug problems with TLS.
The NBD_OPT_LIST option is the only option available which we can unconditionally invoke from the client which will get us a clear response from the server that TLS is required. So AFAICT we have no choice but to use that if we want decent error reporting.
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 :|
Re: [Qemu-devel] [PATCH v3 09/13] nbd: pick first exported volume if no export name is requested
Quoted 13 lines · Daniel P. Berrange
Quoted 6 lines
Quoted 6 lines
As a first reaction, I would really avoid magic unless the server provides a single exports. But even in that case, I would prefer to have some synchronization between the server and client command-line.
Is an empty
NBD_OPT_EXPORT_NAMEvalid? What about using new-style negotiation with emptyNBD_OPT_EXPORT_NAMEif TLS is requested?The main goal here is to ensure the NBD client gets a decent error message if it tries to connect without TLS. Even if we are using the fixed new style protocol, the client code will send
NBD_OPT_EXPORT_NAMEas the first thing it does. Thanks to a bit of crazyness is the NBD protocol spec, the server is unable to reply with an error message toNBD_OPT_EXPORT_NAME.
Quoted 4 lines
So if the client connected to a server reqiuring TLS and does not request TLS enablement, the server will have no choice but to just close the connection with no error. I think this will be pretty nasty for users trying to debug problems with TLS.
That's fine. I'm just not sold on using the first answer from NBD_OPT_LIST as the argument to the subsequent NBD_OPT_EXPORT_NAME.
In other words, I would prefer to do the following for no export name:
1) server, no TLS: accept either old-style negotiation or new-style negotation with an empty ("") export name; NBD_OPT_LIST returns a single export name, "".
2) server, TLS: accept only new-style negotiation with an empty ("") export name; NBD_OPT_LIST returns a single export name, "".
3) client, no TLS: use old-style negotiation; if the server rejects old-style negotiation, mention the possibility that the server requires TLS
4) client, TLS: use new-style negotiation with an empty ("") export name.
The only interesting case for named exports is client, no TLS. Then you can just send a dummy NBD_OPT_LIST unconditionally, and use the result to provide a good error message if the server requires TLS. If it makes the code simpler to use NBD_OPT_LIST always, even if the client supports TLS (making the sequence NBD_OPT_STARTTLS, NBD_OPT_LIST, NBD_OPT_EXPOR_NAME), then that's fine too.
Paolo
Re: [Qemu-devel] [PATCH v3 09/13] nbd: pick first exported volume if no export name is requested
Quoted 36 lines · Paolo Bonzini
Quoted 13 lines · Daniel P. Berrange
Quoted 6 lines
Quoted 6 lines
As a first reaction, I would really avoid magic unless the server provides a single exports. But even in that case, I would prefer to have some synchronization between the server and client command-line.
Is an empty
NBD_OPT_EXPORT_NAMEvalid? What about using new-style negotiation with emptyNBD_OPT_EXPORT_NAMEif TLS is requested?The main goal here is to ensure the NBD client gets a decent error message if it tries to connect without TLS. Even if we are using the fixed new style protocol, the client code will send
NBD_OPT_EXPORT_NAMEas the first thing it does. Thanks to a bit of crazyness is the NBD protocol spec, the server is unable to reply with an error message toNBD_OPT_EXPORT_NAME.Quoted 4 lines
So if the client connected to a server reqiuring TLS and does not request TLS enablement, the server will have no choice but to just close the connection with no error. I think this will be pretty nasty for users trying to debug problems with TLS.
That's fine. I'm just not sold on using the first answer from
NBD_OPT_LISTas the argument to the subsequentNBD_OPT_EXPORT_NAME.In other words, I would prefer to do the following for no export name:
1) server, no TLS: accept either old-style negotiation or new-style negotation with an empty ("") export name;
NBD_OPT_LISTreturns a single export name, "".2) server, TLS: accept only new-style negotiation with an empty ("") export name;
NBD_OPT_LISTreturns a single export name, "".3) client, no TLS: use old-style negotiation; if the server rejects old-style negotiation, mention the possibility that the server requires TLS
Quoted 8 lines
4) client, TLS: use new-style negotiation with an empty ("") export name.
The only interesting case for named exports is client, no TLS. Then you can just send a dummy
NBD_OPT_LISTunconditionally, and use the result to provide a good error message if the server requires TLS. If it makes the code simpler to useNBD_OPT_LISTalways, even if the client supports TLS (making the sequenceNBD_OPT_STARTTLS,NBD_OPT_LIST,NBD_OPT_EXPOR_NAME), then that's fine too.
Ok, I'll have a go at this approach
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 :|
PATCH v3 10/13 nbd: implement TLS support in the protocol negotiation
In reply to Daniel P. BerrangeThis 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
@@ -389,7 +389,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;
@@ -399,8 +402,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
@@ -33,7 +33,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
@@ -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
@@ -122,3 +122,18 @@ ssize_t nbd_wr_syncv(QIOChannel *ioc, }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
@@ -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
@@ -254,6 +254,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) {
@@ -343,7 +344,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
PATCH v3 11/13 nbd: enable use of TLS with NBD block driver
In reply to Daniel P. BerrangeThis 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=tls0The 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
@@ -378,8 +378,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;
@@ -390,7 +394,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
@@ -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
PATCH v3 12/13 nbd: enable use of TLS with qemu-nbd server
In reply to Daniel P. BerrangeThis 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 defaultNote that it is mandatory to supply the --exportname argument when requesting TLS, since it requires use of the new style NBD protocol where the client requests a volume name explicitly.
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 | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- qemu-nbd.texi | 4 ++++ 2 files changed, 64 insertions(+), 1 deletion(-)
qemu-nbd.c +60 -1
@@ -45,6 +45,7 @@#define QEMU_NBD_OPT_DETECT_ZEROES 259#define QEMU_NBD_OPT_OBJECT 260#define QEMU_NBD_OPT_IMAGE_OPTS 261 +#define QEMU_NBD_OPT_TLSCREDS 262static NBDExport *exp;static bool newproto;
@@ -57,6 +58,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) {
@@ -345,7 +347,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;
@@ -434,6 +436,37 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)return 0 ; } + +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;
@@ -475,6 +508,7 @@ int main(int argc, char **argv) { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT }, { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, { "exportname", required_argument, NULL, 'x' }, + { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, { NULL, 0, NULL, 0 } };int ch;
@@ -494,6 +528,7 @@ int main(int argc, char **argv)QemuOpts *opts;bool imageOpts = false;const char *exportname =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.
@@ -669,6 +704,9 @@ int main(int argc, char **argv)case QEMU_NBD_OPT_IMAGE_OPTS : imageOpts = true;break ; +case QEMU_NBD_OPT_TLSCREDS : + tlscredsid = optarg; +break ;case '?' :error_report ("Try `%s --help' for more information." , argv[0 ]);exit (EXIT_FAILURE );
@@ -687,6 +725,27 @@ int main(int argc, char **argv)exit (1 ); } +if (tlscredsid) { +if (!exportname) { +error_report ("Export name is required when using TLS" ); +exit (EXIT_FAILURE ); + } +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 ); + } + 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
@@ -66,6 +66,10 @@ Export QEMU disk image using NBD protocol. @item -x NAME, --exportname=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
PATCH v3 13/13 nbd: enable use of TLS with nbd-server-start command
In reply to Daniel P. BerrangeThis 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
@@ -19,42 +19,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 ; }/*
@@ -89,7 +173,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,NBDExport *exp;NBDCloseNotifier *n; -if (!server_ioc) { +if (!nbd_server) {error_setg (errp,"NBD server not running" );return ; }
@@ -139,12 +223,6 @@ void qmp_nbd_server_stop(Error **errp)nbd_close_notifier (&cn->n,nbd_export_get_blockdev (cn->exp)); } -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
@@ -1792,7 +1792,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
@@ -130,13 +130,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
@@ -3802,7 +3802,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