[Qemu-devel] [RFC PATCH 00/13] Embedded NBD server

[Qemu-devel] [RFC PATCH 00/13] Embedded NBD server

[Paolo Bonzini]

Hi all,

this is an RFC series implementing an NBD server embedded inside QEMU.
This can be used in various cases, including migration with non-shared
storage.

Three new commands are introduced at the QMP level

  { 'command': 'nbd-server-start',  'data': { 'addr': 'IPSocketAddress' } }
  { 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
  { 'command': 'nbd-server-stop' }

At the HMP level there is no nbd_server_add command.  nbd_server_start
automatically exposes all of the VM's block devices, and an option -w
makes them writable.

The NBD server exports multiple devices on a single port; they can be
accessed using "nbd:HOST:PART:exportname=NAME".

The patches are mostly boring touching nbd.c.  The part where I need
a second opinion and/or ack is patch 12 and 13.  They fix the case of
a disk being unplugged while NBD export is active.  To do this I add a
NotifierList to a BlockDriverState.  Does this look okay, or is it too
ad hoc?

Paolo Bonzini (13):
  nbd: add more constants
  nbd: pass NBDClient to nbd_send_negotiate
  nbd: do not leak nbd_trip coroutines when a connection is torn down
  nbd: close all clients on deleting export
  nbd: register named exports
  nbd: negotiate with named exports
  nbd: do not close BlockDriverState in nbd_export_close
  qemu-sockets: publish dummy_opts
  qmp: add NBD server commands
  qemu-sockets: make inet_parse public
  hmp: add NBD server commands
  block: add close notifiers
  nbd: add notifier to close exports when the image is closed

 Makefile.objs       |   5 +-
 block.c             |  19 +++-
 block.h             |   1 +
 block_int.h         |   2 +
 blockdev-nbd.c      | 131 ++++++++++++++++++++++
 hmp-commands.hx     |  29 +++++
 hmp.c               |  66 +++++++++++
 hmp.h               |   2 +
 nbd.c               | 311 +++++++++++++++++++++++++++++++++++++++++-----------
 nbd.h               |   6 +
 qapi-schema.json    |  69 ++++++++++++
 qapi/opts-visitor.c |  48 ++++----
 qemu-nbd.c          |   1 +
 qemu-sockets.c      |  16 +--
 qemu_socket.h       |   3 +
 qmp-commands.hx     |  16 +++
 16 file modificati, 621 inserzioni(+), 104 rimozioni(-)
 create mode 100644 blockdev-nbd.c

-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 01/13] nbd: add more constants

[Paolo Bonzini]

Avoid magic numbers and magic size computations; hide them behind #defines.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 17 ++++++++++-------
 1 file modificato, 10 inserzioni(+), 7 rimozioni(-)

diff --git a/nbd.c b/nbd.c
index 0dd60c5..8201b7a 100644
--- a/nbd.c
+++ b/nbd.c
@@ -57,9 +57,12 @@
 
 /* This is all part of the "official" NBD API */
 
+#define NBD_REQUEST_SIZE        (4 + 4 + 8 + 8 + 4)
 #define NBD_REPLY_SIZE          (4 + 4 + 8)
 #define NBD_REQUEST_MAGIC       0x25609513
 #define NBD_REPLY_MAGIC         0x67446698
+#define NBD_OPTS_MAGIC          0x49484156454F5054LL
+#define NBD_CLIENT_MAGIC        0x0000420281861253LL
 
 #define NBD_SET_SOCK            _IO(0xab, 0)
 #define NBD_SET_BLKSIZE         _IO(0xab, 1)
@@ -213,7 +216,7 @@ static int nbd_send_negotiate(int csock, off_t size, uint32_t flags)
 
     /* Negotiate
         [ 0 ..   7]   passwd   ("NBDMAGIC")
-        [ 8 ..  15]   magic    (0x00420281861253)
+        [ 8 ..  15]   magic    (NBD_CLIENT_MAGIC)
         [16 ..  23]   size
         [24 ..  27]   flags
         [28 .. 151]   reserved (0)
@@ -224,7 +227,7 @@ static int nbd_send_negotiate(int csock, off_t size, uint32_t flags)
 
     TRACE("Beginning negotiation.");
     memcpy(buf, "NBDMAGIC", 8);
-    cpu_to_be64w((uint64_t*)(buf + 8), 0x00420281861253LL);
+    cpu_to_be64w((uint64_t*)(buf + 8), NBD_CLIENT_MAGIC);
     cpu_to_be64w((uint64_t*)(buf + 16), size);
     cpu_to_be32w((uint32_t*)(buf + 24),
                  flags | NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
@@ -295,7 +298,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
         uint32_t namesize;
 
         TRACE("Checking magic (opts_magic)");
-        if (magic != 0x49484156454F5054LL) {
+        if (magic != NBD_OPTS_MAGIC) {
             LOG("Bad magic received");
             goto fail;
         }
@@ -334,7 +337,7 @@ int nbd_receive_negotiate(int csock, const char *name, uint32_t *flags,
     } else {
         TRACE("Checking magic (cli_magic)");
 
-        if (magic != 0x00420281861253LL) {
+        if (magic != NBD_CLIENT_MAGIC) {
             LOG("Bad magic received");
             goto fail;
         }
@@ -477,7 +480,7 @@ int nbd_client(int fd)
 
 ssize_t nbd_send_request(int csock, struct nbd_request *request)
 {
-    uint8_t buf[4 + 4 + 8 + 8 + 4];
+    uint8_t buf[NBD_REQUEST_SIZE];
     ssize_t ret;
 
     cpu_to_be32w((uint32_t*)buf, NBD_REQUEST_MAGIC);
@@ -504,7 +507,7 @@ ssize_t nbd_send_request(int csock, struct nbd_request *request)
 
 static ssize_t nbd_receive_request(int csock, struct nbd_request *request)
 {
-    uint8_t buf[4 + 4 + 8 + 8 + 4];
+    uint8_t buf[NBD_REQUEST_SIZE];
     uint32_t magic;
     ssize_t ret;
 
@@ -582,7 +585,7 @@ ssize_t nbd_receive_reply(int csock, struct nbd_reply *reply)
 
 static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
 {
-    uint8_t buf[4 + 4 + 8];
+    uint8_t buf[NBD_REPLY_SIZE];
     ssize_t ret;
 
     /* Reply
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 02/13] nbd: pass NBDClient to nbd_send_negotiate

[Paolo Bonzini]

We will need the NBDClient in nbd_send_negotiate to store the
export requested by the client.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 76 ++++++++++++++++++++++++++++++++++++-------------------------------
 1 file modificato, 41 inserzioni(+), 35 rimozioni(-)

diff --git a/nbd.c b/nbd.c
index 8201b7a..c1edbeb 100644
--- a/nbd.c
+++ b/nbd.c
@@ -78,6 +78,39 @@
 
 #define NBD_OPT_EXPORT_NAME     (1 << 0)
 
+/* Definitions for opaque data types */
+
+typedef struct NBDRequest NBDRequest;
+
+struct NBDRequest {
+    QSIMPLEQ_ENTRY(NBDRequest) entry;
+    NBDClient *client;
+    uint8_t *data;
+};
+
+struct NBDExport {
+    BlockDriverState *bs;
+    off_t dev_offset;
+    off_t size;
+    uint32_t nbdflags;
+    QSIMPLEQ_HEAD(, NBDRequest) requests;
+};
+
+struct NBDClient {
+    int refcount;
+    void (*close)(NBDClient *client);
+
+    NBDExport *exp;
+    int sock;
+
+    Coroutine *recv_coroutine;
+
+    CoMutex send_lock;
+    Coroutine *send_coroutine;
+
+    int nb_requests;
+};
+
 /* That's all folks */
 
 ssize_t nbd_wr_sync(int fd, void *buffer, size_t size, bool do_read)
@@ -209,8 +242,9 @@ int unix_socket_outgoing(const char *path)
                   Request (type == 2)
 */
 
-static int nbd_send_negotiate(int csock, off_t size, uint32_t flags)
+static int nbd_send_negotiate(NBDClient *client)
 {
+    int csock = client->sock;
     char buf[8 + 8 + 8 + 128];
     int rc;
 
@@ -228,9 +262,9 @@ static int nbd_send_negotiate(int csock, off_t size, uint32_t flags)
     TRACE("Beginning negotiation.");
     memcpy(buf, "NBDMAGIC", 8);
     cpu_to_be64w((uint64_t*)(buf + 8), NBD_CLIENT_MAGIC);
-    cpu_to_be64w((uint64_t*)(buf + 16), size);
+    cpu_to_be64w((uint64_t*)(buf + 16), client->exp->size);
     cpu_to_be32w((uint32_t*)(buf + 24),
-                 flags | NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
+                 client->exp->nbdflags | NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
                  NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
     memset(buf + 28, 0, 124);
 
@@ -615,35 +649,6 @@ static ssize_t nbd_send_reply(int csock, struct nbd_reply *reply)
 
 typedef struct NBDRequest NBDRequest;
 
-struct NBDRequest {
-    QSIMPLEQ_ENTRY(NBDRequest) entry;
-    NBDClient *client;
-    uint8_t *data;
-};
-
-struct NBDExport {
-    BlockDriverState *bs;
-    off_t dev_offset;
-    off_t size;
-    uint32_t nbdflags;
-    QSIMPLEQ_HEAD(, NBDRequest) requests;
-};
-
-struct NBDClient {
-    int refcount;
-    void (*close)(NBDClient *client);
-
-    NBDExport *exp;
-    int sock;
-
-    Coroutine *recv_coroutine;
-
-    CoMutex send_lock;
-    Coroutine *send_coroutine;
-
-    int nb_requests;
-};
-
 static void nbd_client_get(NBDClient *client)
 {
     client->refcount++;
@@ -977,13 +982,14 @@ NBDClient *nbd_client_new(NBDExport *exp, int csock,
                           void (*close)(NBDClient *))
 {
     NBDClient *client;
-    if (nbd_send_negotiate(csock, exp->size, exp->nbdflags) < 0) {
-        return NULL;
-    }
     client = g_malloc0(sizeof(NBDClient));
     client->refcount = 1;
     client->exp = exp;
     client->sock = csock;
+    if (nbd_send_negotiate(client) < 0) {
+        g_free(client);
+        return NULL;
+    }
     client->close = close;
     qemu_co_mutex_init(&client->send_lock);
     qemu_set_fd_handler2(csock, nbd_can_read, nbd_read, NULL, client);
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 03/13] nbd: do not leak nbd_trip coroutines when a connection is torn down

[Paolo Bonzini]

Because nbd_client_close removes the I/O handlers for the client
socket, there is no way that any suspended coroutines are restarted.
This will be a problem with the QEMU embedded NBD server, because
we will have a QMP command to forcibly close all connections with
the clients.

Instead, we can exploit the reference counting of NBDClients; shutdown
the client socket, which will make it readable and writeable.  The
coroutines then will fail and exit cleanly.  The last refcount finally
triggers the closure of the client.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 18 +++++++++++-------
 nbd.h |  3 +++
 2 file modificati, 14 inserzioni(+), 7 rimozioni(-)

diff --git a/nbd.c b/nbd.c
index c1edbeb..cb4e8fe 100644
--- a/nbd.c
+++ b/nbd.c
@@ -657,18 +657,22 @@ static void nbd_client_get(NBDClient *client)
 static void nbd_client_put(NBDClient *client)
 {
     if (--client->refcount == 0) {
+        qemu_set_fd_handler2(client->sock, NULL, NULL, NULL, NULL);
+        close(client->sock);
+        client->sock = -1;
+        if (client->close) {
+            client->close(client);
+        }
         g_free(client);
     }
 }
 
-static void nbd_client_close(NBDClient *client)
+void nbd_client_close(NBDClient *client)
 {
-    qemu_set_fd_handler2(client->sock, NULL, NULL, NULL, NULL);
-    close(client->sock);
-    client->sock = -1;
-    if (client->close) {
-        client->close(client);
-    }
+    /* Force requests to finish.  They will drop their own references,
+     * then we'll close the socket and free the NBDClient.
+     */
+    shutdown(client->sock, 2);
     nbd_client_put(client);
 }
 
diff --git a/nbd.h b/nbd.h
index 40d58d3..6305d68 100644
--- a/nbd.h
+++ b/nbd.h
@@ -81,7 +81,10 @@ typedef struct NBDClient NBDClient;
 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
                           off_t size, uint32_t nbdflags);
 void nbd_export_close(NBDExport *exp);
+
 NBDClient *nbd_client_new(NBDExport *exp, int csock,
                           void (*close)(NBDClient *));
+void nbd_client_close(NBDClient *client);
+
 
 #endif
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 04/13] nbd: close all clients on deleting export

[Paolo Bonzini]

Clients have a pointer to the NBDExport that they serve.  Do not
let a dangling pointer escape.  Also flush all pending I/O so that
coroutines are forced to exit.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 14 ++++++++++++++
 1 file modificato, 14 inserzioni(+)

diff --git a/nbd.c b/nbd.c
index cb4e8fe..f6eaea3 100644
--- a/nbd.c
+++ b/nbd.c
@@ -93,6 +93,7 @@ struct NBDExport {
     off_t dev_offset;
     off_t size;
     uint32_t nbdflags;
+    QTAILQ_HEAD(, NBDClient) clients;
     QSIMPLEQ_HEAD(, NBDRequest) requests;
 };
 
@@ -108,6 +109,7 @@ struct NBDClient {
     CoMutex send_lock;
     Coroutine *send_coroutine;
 
+    QTAILQ_ENTRY(NBDClient) next;
     int nb_requests;
 };
 
@@ -658,6 +660,9 @@ static void nbd_client_put(NBDClient *client)
 {
     if (--client->refcount == 0) {
         qemu_set_fd_handler2(client->sock, NULL, NULL, NULL, NULL);
+        if (client->exp) {
+            QTAILQ_REMOVE(&client->exp->clients, client, next);
+        }
         close(client->sock);
         client->sock = -1;
         if (client->close) {
@@ -711,6 +716,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
 {
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
     QSIMPLEQ_INIT(&exp->requests);
+    QTAILQ_INIT(&exp->clients);
     exp->bs = bs;
     exp->dev_offset = dev_offset;
     exp->nbdflags = nbdflags;
@@ -720,6 +726,11 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
 
 void nbd_export_close(NBDExport *exp)
 {
+    while (!QTAILQ_EMPTY(&exp->clients)) {
+        NBDClient *client = QTAILQ_FIRST(&exp->clients);
+        nbd_client_close(client);
+    }
+
     while (!QSIMPLEQ_EMPTY(&exp->requests)) {
         NBDRequest *first = QSIMPLEQ_FIRST(&exp->requests);
         QSIMPLEQ_REMOVE_HEAD(&exp->requests, entry);
@@ -995,6 +1006,9 @@ NBDClient *nbd_client_new(NBDExport *exp, int csock,
         return NULL;
     }
     client->close = close;
+    if (exp) {
+        QTAILQ_INSERT_TAIL(&exp->clients, client, next);
+    }
     qemu_co_mutex_init(&client->send_lock);
     qemu_set_fd_handler2(csock, nbd_can_read, nbd_read, NULL, client);
     return client;
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 05/13] nbd: register named exports

[Paolo Bonzini]

Add an API to register and find named exports.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 38 ++++++++++++++++++++++++++++++++++++++
 nbd.h |  3 +++
 2 file modificati, 41 inserzioni(+)

diff --git a/nbd.c b/nbd.c
index f6eaea3..1249548 100644
--- a/nbd.c
+++ b/nbd.c
@@ -90,13 +90,17 @@ struct NBDRequest {
 
 struct NBDExport {
     BlockDriverState *bs;
+    char *name;
     off_t dev_offset;
     off_t size;
     uint32_t nbdflags;
     QTAILQ_HEAD(, NBDClient) clients;
     QSIMPLEQ_HEAD(, NBDRequest) requests;
+    QTAILQ_ENTRY(NBDExport) next;
 };
 
+static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
+
 struct NBDClient {
     int refcount;
     void (*close)(NBDClient *client);
@@ -724,6 +728,27 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
     return exp;
 }
 
+NBDExport *nbd_export_find(const char *name)
+{
+    NBDExport *exp;
+    QTAILQ_FOREACH(exp, &exports, next) {
+        if (strcmp(name, exp->name) == 0) {
+            return exp;
+        }
+    }
+
+    return NULL;
+}
+
+void nbd_export_set_name(NBDExport *exp, const char *name)
+{
+    assert(exp->name == NULL);
+    assert(!nbd_export_find(name));
+
+    exp->name = g_strdup(name);
+    QTAILQ_INSERT_TAIL(&exports, exp, next);
+}
+
 void nbd_export_close(NBDExport *exp)
 {
     while (!QTAILQ_EMPTY(&exp->clients)) {
@@ -738,10 +763,23 @@ void nbd_export_close(NBDExport *exp)
         g_free(first);
     }
 
+    if (exp->name) {
+        QTAILQ_REMOVE(&exports, exp, next);
+        g_free(exp->name);
+    }
+
     bdrv_close(exp->bs);
     g_free(exp);
 }
 
+void nbd_export_close_all(void)
+{
+    while (!QTAILQ_EMPTY(&exports)) {
+        NBDExport *export = QTAILQ_FIRST(&exports);
+        nbd_export_close(export);
+    }
+}
+
 static int nbd_can_read(void *opaque);
 static void nbd_read(void *opaque);
 static void nbd_restart_write(void *opaque);
diff --git a/nbd.h b/nbd.h
index 6305d68..88ca324 100644
--- a/nbd.h
+++ b/nbd.h
@@ -80,7 +80,10 @@ typedef struct NBDClient NBDClient;
 
 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset,
                           off_t size, uint32_t nbdflags);
+NBDExport *nbd_export_find(const char *name);
+void nbd_export_set_name(NBDExport *exp, const char *name);
 void nbd_export_close(NBDExport *exp);
+void nbd_export_close_all(void);
 
 NBDClient *nbd_client_new(NBDExport *exp, int csock,
                           void (*close)(NBDClient *));
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 06/13] nbd: negotiate with named exports

[Paolo Bonzini]

Allow negotiation to receive the name of the requested export from
the client.  Passing a NULL export to nbd_client_new will cause
the server to send the extended negotiation header.  The exp field
is then filled during negotiation.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file modificato, 140 inserzioni(+), 15 rimozioni(-)

diff --git a/nbd.c b/nbd.c
index 1249548..fe7551d 100644
--- a/nbd.c
+++ b/nbd.c
@@ -234,11 +234,23 @@ int unix_socket_outgoing(const char *path)
     return unix_connect(path);
 }
 
-/* Basic flow
+/* Basic flow for negotiation
 
    Server         Client
-
    Negotiate
+
+   or
+
+   Server         Client
+   Negotiate #1
+                  Option
+   Negotiate #2
+
+   ----
+
+   followed by
+
+   Server         Client
                   Request
    Response
                   Request
@@ -246,20 +258,110 @@ int unix_socket_outgoing(const char *path)
                   ...
    ...
                   Request (type == 2)
+
 */
 
+static int nbd_receive_options(NBDClient *client)
+{
+    int csock = client->sock;
+    char name[256];
+    uint32_t tmp, length;
+    uint64_t magic;
+    int rc;
+
+    /* Client sends:
+        [ 0 ..   3]   reserved (0)
+        [ 4 ..  11]   NBD_OPTS_MAGIC
+        [12 ..  15]   NBD_OPT_EXPORT_NAME
+        [16 ..  19]   length
+        [20 ..  xx]   export name (length bytes)
+     */
+
+    rc = -EINVAL;
+    if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        LOG("read failed");
+        goto fail;
+    }
+    TRACE("Checking reserved");
+    if (tmp != 0) {
+        LOG("Bad reserved received");
+        goto fail;
+    }
+
+    if (read_sync(csock, &magic, sizeof(magic)) != sizeof(magic)) {
+        LOG("read failed");
+        goto fail;
+    }
+    TRACE("Checking reserved");
+    if (magic != be64_to_cpu(NBD_OPTS_MAGIC)) {
+        LOG("Bad magic received");
+        goto fail;
+    }
+
+    if (read_sync(csock, &tmp, sizeof(tmp)) != sizeof(tmp)) {
+        LOG("read failed");
+        goto fail;
+    }
+    TRACE("Checking option");
+    if (tmp != be32_to_cpu(NBD_OPT_EXPORT_NAME)) {
+        LOG("Bad option received");
+        goto fail;
+    }
+
+    if (read_sync(csock, &length, sizeof(length)) != sizeof(length)) {
+        LOG("read failed");
+        goto fail;
+    }
+    TRACE("Checking length");
+    length = be32_to_cpu(length);
+    if (length > 255) {
+        LOG("Bad length received");
+        goto fail;
+    }
+    if (read_sync(csock, name, length) != length) {
+        LOG("read failed");
+        goto fail;
+    }
+    name[length] = '\0';
+
+    client->exp = nbd_export_find(name);
+    if (!client->exp) {
+        LOG("export not found");
+        goto fail;
+    }
+
+    QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
+    TRACE("Option negotiation succeeded.");
+    rc = 0;
+fail:
+    return rc;
+}
+
 static int nbd_send_negotiate(NBDClient *client)
 {
     int csock = client->sock;
     char buf[8 + 8 + 8 + 128];
     int rc;
+    const int myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
+                         NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
 
-    /* Negotiate
-        [ 0 ..   7]   passwd   ("NBDMAGIC")
-        [ 8 ..  15]   magic    (NBD_CLIENT_MAGIC)
+    /* Negotiation header without options:
+        [ 0 ..   7]   passwd       ("NBDMAGIC")
+        [ 8 ..  15]   magic        (NBD_CLIENT_MAGIC)
         [16 ..  23]   size
-        [24 ..  27]   flags
-        [28 .. 151]   reserved (0)
+        [24 ..  25]   server flags (0)
+        [24 ..  27]   export flags
+        [28 .. 151]   reserved     (0)
+
+       Negotiation header with options, part 1:
+        [ 0 ..   7]   passwd       ("NBDMAGIC")
+        [ 8 ..  15]   magic        (NBD_OPTS_MAGIC)
+        [16 ..  17]   server flags (0)
+
+       part 2 (after options are sent):
+        [18 ..  25]   size
+        [26 ..  27]   export flags
+        [28 .. 151]   reserved     (0)
      */
 
     socket_set_block(csock);
@@ -267,16 +369,39 @@ static int nbd_send_negotiate(NBDClient *client)
 
     TRACE("Beginning negotiation.");
     memcpy(buf, "NBDMAGIC", 8);
-    cpu_to_be64w((uint64_t*)(buf + 8), NBD_CLIENT_MAGIC);
-    cpu_to_be64w((uint64_t*)(buf + 16), client->exp->size);
-    cpu_to_be32w((uint32_t*)(buf + 24),
-                 client->exp->nbdflags | NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
-                 NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA);
+    if (client->exp) {
+        assert ((client->exp->nbdflags & ~65535) == 0);
+        cpu_to_be64w((uint64_t*)(buf + 8), NBD_CLIENT_MAGIC);
+        cpu_to_be64w((uint64_t*)(buf + 16), client->exp->size);
+        cpu_to_be16w((uint16_t*)(buf + 26), client->exp->nbdflags | myflags);
+    } else {
+        cpu_to_be64w((uint64_t*)(buf + 8), NBD_OPTS_MAGIC);
+    }
     memset(buf + 28, 0, 124);
 
-    if (write_sync(csock, buf, sizeof(buf)) != sizeof(buf)) {
-        LOG("write failed");
-        goto fail;
+    if (client->exp) {
+        if (write_sync(csock, buf, sizeof(buf)) != sizeof(buf)) {
+            LOG("write failed");
+            goto fail;
+        }
+    } else {
+        if (write_sync(csock, buf, 18) != 18) {
+            LOG("write failed");
+            goto fail;
+        }
+        rc = nbd_receive_options(client);
+        if (rc < 0) {
+            LOG("option negotiation failed");
+            goto fail;
+        }
+
+        assert ((client->exp->nbdflags & ~65535) == 0);
+        cpu_to_be64w((uint64_t*)(buf + 18), client->exp->size);
+        cpu_to_be16w((uint16_t*)(buf + 26), client->exp->nbdflags | myflags);
+        if (write_sync(csock, buf + 18, sizeof(buf) - 18) != sizeof(buf) - 18) {
+            LOG("write failed");
+            goto fail;
+        }
     }
 
     TRACE("Negotiation succeeded.");
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 07/13] nbd: do not close BlockDriverState in nbd_export_close

[Paolo Bonzini]

This is not desirable when embedding the NBD server inside QEMU.
Move the bdrv_close to qemu-nbd.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c      | 1 -
 qemu-nbd.c | 1 +
 2 file modificati, 1 inserzione(+). 1 rimozione(-)

diff --git a/nbd.c b/nbd.c
index fe7551d..1f65b1f 100644
--- a/nbd.c
+++ b/nbd.c
@@ -893,7 +893,6 @@ void nbd_export_close(NBDExport *exp)
         g_free(exp->name);
     }
 
-    bdrv_close(exp->bs);
     g_free(exp);
 }
 
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 1c1cf6a..23392e0 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -586,6 +586,7 @@ int main(int argc, char **argv)
     } while (!sigterm_reported && (persistent || !nbd_started || nb_fds > 0));
 
     nbd_export_close(exp);
+    bdrv_close(bs);
     if (sockpath) {
         unlink(sockpath);
     }
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 08/13] qemu-sockets: publish dummy_opts

[Paolo Bonzini]

This is needed so that we can set up a QemuOpts instance from QMP
parameters.  The way to go here is to and move qemu-sockets.c away from
QemuOpts and use Laszlo's QemuOptsVisitor whenever *_opts functions are
called now.  This can be done later, however.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 14 +++++++-------
 qemu_socket.h  |  2 ++
 2 file modificati, 9 inserzioni(+), 7 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 361d890..b292311 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -32,9 +32,9 @@
 static const int on=1, off=0;
 
 /* used temporarely until all users are converted to QemuOpts */
-static QemuOptsList dummy_opts = {
-    .name = "dummy",
-    .head = QTAILQ_HEAD_INITIALIZER(dummy_opts.head),
+QemuOptsList socket_opts = {
+    .name = "socket",
+    .head = QTAILQ_HEAD_INITIALIZER(socket_opts.head),
     .desc = {
         {
             .name = "path",
@@ -469,7 +469,7 @@ int inet_listen(const char *str, char *ostr, int olen,
     char *optstr;
     int sock = -1;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
         sock = inet_listen_opts(opts, port_offset, errp);
         if (sock != -1 && ostr) {
@@ -498,7 +498,7 @@ int inet_connect(const char *str, bool block, bool *in_progress, Error **errp)
     QemuOpts *opts;
     int sock = -1;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
         if (block) {
             qemu_opt_set(opts, "block", "on");
@@ -597,7 +597,7 @@ int unix_listen(const char *str, char *ostr, int olen)
     char *path, *optstr;
     int sock, len;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
 
     optstr = strchr(str, ',');
     if (optstr) {
@@ -625,7 +625,7 @@ int unix_connect(const char *path)
     QemuOpts *opts;
     int sock;
 
-    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
     qemu_opt_set(opts, "path", path);
     sock = unix_connect_opts(opts);
     qemu_opts_del(opts);
diff --git a/qemu_socket.h b/qemu_socket.h
index 30ae6af..c87ee57 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -30,6 +30,8 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "error.h"
 #include "qerror.h"
 
+extern QemuOptsList socket_opts;
+
 /* misc helpers */
 int qemu_socket(int domain, int type, int protocol);
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 09/13] qmp: add NBD server commands

[Paolo Bonzini]

Adding an NBD server inside QEMU is trivial, since all the logic is
in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
The main difference is that qemu-nbd serves a single unnamed export,
while QEMU serves named exports.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs    |  2 +-
 blockdev-nbd.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 69 +++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 16 ++++++++++
 4 file modificati, 179 inserzioni(+). 1 rimozione(-)
 create mode 100644 blockdev-nbd.c

diff --git a/Makefile.objs b/Makefile.objs
index 4412757..c42affc 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -59,7 +59,7 @@ endif
 # suppress *all* target specific code in case of system emulation, i.e. a
 # single QEMU executable should support all CPUs and machines.
 
-common-obj-y = $(block-obj-y) blockdev.o
+common-obj-y = $(block-obj-y) blockdev.o blockdev-nbd.o
 common-obj-y += net.o net/
 common-obj-y += qom/
 common-obj-y += readline.o console.o cursor.o
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
new file mode 100644
index 0000000..5a415be
--- /dev/null
+++ b/blockdev-nbd.c
@@ -0,0 +1,93 @@
+/*
+ * QEMU host block devices
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.  See the COPYING file in the top-level directory.
+ */
+
+#include "blockdev.h"
+#include "hw/block-common.h"
+#include "monitor.h"
+#include "qerror.h"
+#include "sysemu.h"
+#include "qmp-commands.h"
+#include "trace.h"
+#include "nbd.h"
+#include "qemu_socket.h"
+
+static int server_fd = -1;
+
+static void nbd_accept(void *opaque)
+{
+    struct sockaddr_in addr;
+    socklen_t addr_len = sizeof(addr);
+
+    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
+    if (fd >= 0) {
+        nbd_client_new(NULL, fd, NULL);
+    }
+}
+
+static void nbd_server_start(QemuOpts *opts, Error **errp)
+{
+    if (server_fd != -1) {
+        /* TODO: error */
+        return;
+    }
+
+    server_fd = inet_listen_opts(opts, 0, errp);
+    if (server_fd != -1) {
+        qemu_set_fd_handler2(server_fd, NULL, nbd_accept, NULL, NULL);
+    }
+}
+
+void qmp_nbd_server_start(IPSocketAddress *addr, Error **errp)
+{
+    QemuOpts *opts;
+
+    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
+    qemu_opt_set(opts, "host", addr->host);
+    qemu_opt_set(opts, "port", addr->port);
+
+    addr->ipv4 |= !addr->has_ipv4;
+    addr->ipv6 |= !addr->has_ipv6;
+    if (!addr->ipv4 || !addr->ipv6) {
+        qemu_opt_set_bool(opts, "ipv4", addr->ipv4);
+        qemu_opt_set_bool(opts, "ipv6", addr->ipv6);
+    }
+
+    nbd_server_start(opts, errp);
+    qemu_opts_del(opts);
+}
+
+
+void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
+                        Error **errp)
+{
+    BlockDriverState *bs;
+    NBDExport *exp;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (nbd_export_find(bdrv_get_device_name(bs))) {
+        /* TODO: error */
+        return;
+    }
+
+    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY);
+    nbd_export_set_name(exp, device);
+}
+
+void qmp_nbd_server_stop(Error **errp)
+{
+    nbd_export_close_all();
+    qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
+    close(server_fd);
+    server_fd = -1;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 3d2b2d1..d792d2c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2275,6 +2275,30 @@
     'opts': 'NetClientOptions' } }
 
 ##
+# @IPSocketAddress
+#
+# Captures the destination address of an IP socket
+#
+# @host: host part of the address
+#
+# @port: port part of the address, or lowest port if @to is present
+#
+# @to: highest port to try
+#
+# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
+#
+# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
+#
+# Since 1.3
+##
+{ 'type': 'IPSocketAddress',
+  'data': {
+    'host': 'str',
+    'port': 'str',
+    '*ipv4': 'bool',
+    '*ipv6': 'bool' } }
+
+##
 # @getfd:
 #
 # Receive a file descriptor via SCM rights and assign it a name
@@ -2454,3 +2478,46 @@
 #
 ##
 { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
+
+##
+# @nbd-server-start:
+#
+# Start an NBD server listening on the given host and port.
+#
+# @addr: Interface on which to listen, nothing for all interfaces.
+#
+# Since: 1.3.0
+#
+##
+{ 'command': 'nbd-server-start',
+  'data': { 'addr': 'IPSocketAddress' } }
+
+##
+# @nbd-server-add:
+#
+# Export a device to QEMU's embedded NBD server.
+#
+# @device: Block device to be exported
+#
+# @writable: Whether clients should be able to write to the device via the
+#     NBD connection (default false).
+#
+# Returns: error if the device is already marked for export.
+#
+# Since: 1.3.0
+#
+##
+{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
+
+##
+# @nbd-server-stop:
+#
+# Stop QEMU's embedded NBD server, and unregister all devices previously
+# added via nbd-server-add
+#
+# Returns: error if the server is already running.
+#
+# Since: 1.3.0
+#
+##
+{ 'command': 'nbd-server-stop' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2ce4ce6..1f7d6fe 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2481,6 +2481,22 @@ EQMP
     },
 
     {
+        .name       = "nbd-server-start",
+        .args_type  = "addr:q",
+        .mhandler.cmd_new = qmp_marshal_input_nbd_server_start,
+    },
+    {
+        .name       = "nbd-server-add",
+        .args_type  = "device:B,writable:b?",
+        .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
+    },
+    {
+        .name       = "nbd-server-stop",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_nbd_server_stop,
+    },
+
+    {
         .name       = "change-vnc-password",
         .args_type  = "password:s",
         .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
-- 
1.7.11.2

Re: [Qemu-devel] [RFC PATCH 09/13] qmp: add NBD server commands

[Luiz Capitulino]

On Mon, 27 Aug 2012 17:00:22 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Adding an NBD server inside QEMU is trivial, since all the logic is
> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
> The main difference is that qemu-nbd serves a single unnamed export,
> while QEMU serves named exports.

Quickly reviewed the qmp part and it looks good to me (apart from TODOs
of course, specially on error handling).

I've only a few minor comments to the schema.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  Makefile.objs    |  2 +-
>  blockdev-nbd.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 69 +++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  | 16 ++++++++++
>  4 file modificati, 179 inserzioni(+). 1 rimozione(-)
>  create mode 100644 blockdev-nbd.c
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 4412757..c42affc 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -59,7 +59,7 @@ endif
>  # suppress *all* target specific code in case of system emulation, i.e. a
>  # single QEMU executable should support all CPUs and machines.
>  
> -common-obj-y = $(block-obj-y) blockdev.o
> +common-obj-y = $(block-obj-y) blockdev.o blockdev-nbd.o
>  common-obj-y += net.o net/
>  common-obj-y += qom/
>  common-obj-y += readline.o console.o cursor.o
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> new file mode 100644
> index 0000000..5a415be
> --- /dev/null
> +++ b/blockdev-nbd.c
> @@ -0,0 +1,93 @@
> +/*
> + * QEMU host block devices
> + *
> + * Copyright (c) 2003-2008 Fabrice Bellard
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.  See the COPYING file in the top-level directory.
> + */
> +
> +#include "blockdev.h"
> +#include "hw/block-common.h"
> +#include "monitor.h"
> +#include "qerror.h"
> +#include "sysemu.h"
> +#include "qmp-commands.h"
> +#include "trace.h"
> +#include "nbd.h"
> +#include "qemu_socket.h"
> +
> +static int server_fd = -1;
> +
> +static void nbd_accept(void *opaque)
> +{
> +    struct sockaddr_in addr;
> +    socklen_t addr_len = sizeof(addr);
> +
> +    int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
> +    if (fd >= 0) {
> +        nbd_client_new(NULL, fd, NULL);
> +    }
> +}
> +
> +static void nbd_server_start(QemuOpts *opts, Error **errp)
> +{
> +    if (server_fd != -1) {
> +        /* TODO: error */
> +        return;
> +    }
> +
> +    server_fd = inet_listen_opts(opts, 0, errp);
> +    if (server_fd != -1) {
> +        qemu_set_fd_handler2(server_fd, NULL, nbd_accept, NULL, NULL);
> +    }
> +}
> +
> +void qmp_nbd_server_start(IPSocketAddress *addr, Error **errp)
> +{
> +    QemuOpts *opts;
> +
> +    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
> +    qemu_opt_set(opts, "host", addr->host);
> +    qemu_opt_set(opts, "port", addr->port);
> +
> +    addr->ipv4 |= !addr->has_ipv4;
> +    addr->ipv6 |= !addr->has_ipv6;
> +    if (!addr->ipv4 || !addr->ipv6) {
> +        qemu_opt_set_bool(opts, "ipv4", addr->ipv4);
> +        qemu_opt_set_bool(opts, "ipv6", addr->ipv6);
> +    }
> +
> +    nbd_server_start(opts, errp);
> +    qemu_opts_del(opts);
> +}
> +
> +
> +void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
> +                        Error **errp)
> +{
> +    BlockDriverState *bs;
> +    NBDExport *exp;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (nbd_export_find(bdrv_get_device_name(bs))) {
> +        /* TODO: error */
> +        return;
> +    }
> +
> +    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY);
> +    nbd_export_set_name(exp, device);
> +}
> +
> +void qmp_nbd_server_stop(Error **errp)
> +{
> +    nbd_export_close_all();
> +    qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
> +    close(server_fd);
> +    server_fd = -1;
> +}
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3d2b2d1..d792d2c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2275,6 +2275,30 @@
>      'opts': 'NetClientOptions' } }
>  
>  ##
> +# @IPSocketAddress
> +#
> +# Captures the destination address of an IP socket
> +#
> +# @host: host part of the address
> +#
> +# @port: port part of the address, or lowest port if @to is present
> +#
> +# @to: highest port to try

Doesn't exist in data. If you add, please use a more descriptive name.

> +#
> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
> +#
> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6

Both are missing #optional.

> +#
> +# Since 1.3
> +##
> +{ 'type': 'IPSocketAddress',
> +  'data': {
> +    'host': 'str',
> +    'port': 'str',
> +    '*ipv4': 'bool',
> +    '*ipv6': 'bool' } }
> +
> +##
>  # @getfd:
>  #
>  # Receive a file descriptor via SCM rights and assign it a name
> @@ -2454,3 +2478,46 @@
>  #
>  ##
>  { 'command': 'query-fdsets', 'returns': ['FdsetInfo'] }
> +
> +##
> +# @nbd-server-start:
> +#
> +# Start an NBD server listening on the given host and port.
> +#
> +# @addr: Interface on which to listen, nothing for all interfaces.
> +#
> +# Since: 1.3.0
> +#
> +##
> +{ 'command': 'nbd-server-start',
> +  'data': { 'addr': 'IPSocketAddress' } }
> +
> +##
> +# @nbd-server-add:
> +#
> +# Export a device to QEMU's embedded NBD server.
> +#
> +# @device: Block device to be exported
> +#
> +# @writable: Whether clients should be able to write to the device via the
> +#     NBD connection (default false).

Missing #optional.

> +#
> +# Returns: error if the device is already marked for export.
> +#
> +# Since: 1.3.0
> +#
> +##
> +{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
> +
> +##
> +# @nbd-server-stop:
> +#
> +# Stop QEMU's embedded NBD server, and unregister all devices previously
> +# added via nbd-server-add
> +#
> +# Returns: error if the server is already running.

That error pertains to nbd-start-start.

> +#
> +# Since: 1.3.0
> +#
> +##
> +{ 'command': 'nbd-server-stop' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2ce4ce6..1f7d6fe 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2481,6 +2481,22 @@ EQMP
>      },
>  
>      {
> +        .name       = "nbd-server-start",
> +        .args_type  = "addr:q",
> +        .mhandler.cmd_new = qmp_marshal_input_nbd_server_start,
> +    },
> +    {
> +        .name       = "nbd-server-add",
> +        .args_type  = "device:B,writable:b?",
> +        .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
> +    },
> +    {
> +        .name       = "nbd-server-stop",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_nbd_server_stop,
> +    },
> +
> +    {
>          .name       = "change-vnc-password",
>          .args_type  = "password:s",
>          .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,

Re: [Qemu-devel] [RFC PATCH 09/13] qmp: add NBD server commands

[Paolo Bonzini]

Il 18/09/2012 22:11, Luiz Capitulino ha scritto:
> (apart from TODOs of course, specially on error handling).

Yes, those were waiting for error_setg to get in the tree. :)

Paolo

[Qemu-devel] [RFC PATCH 10/13] qemu-sockets: make inet_parse public

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 2 +-
 qemu_socket.h  | 1 +
 2 file modificati, 2 inserzioni(+). 1 rimozione(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index b292311..7a28715 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -407,7 +407,7 @@ err:
 }
 
 /* compatibility wrapper */
-static int inet_parse(QemuOpts *opts, const char *str)
+int inet_parse(QemuOpts *opts, const char *str)
 {
     const char *optstr, *h;
     char addr[64];
diff --git a/qemu_socket.h b/qemu_socket.h
index c87ee57..e4cb6b1 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -41,6 +41,7 @@ void socket_set_nonblock(int fd);
 int send_all(int fd, const void *buf, int len1);
 
 /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
+int inet_parse(QemuOpts *opts, const char *str);
 int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp);
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 11/13] hmp: add NBD server commands

[Paolo Bonzini]

At the HMP level there is no nbd_server_add command.  nbd_server_start
automatically exposes all of the VM's block devices, and an option -w
makes them writable.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hmp-commands.hx | 29 +++++++++++++++++++++++++
 hmp.c           | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp.h           |  2 ++
 3 file modificati, 97 inserzioni(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index f6104b0..cabb886 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1248,6 +1248,35 @@ Remove all matches from the access control list, and set the default
 policy back to @code{deny}.
 ETEXI
 
+    {
+        .name       = "nbd_server_start",
+        .args_type  = "writable:-w,uri:s",
+        .params     = "nbd_server_start [-w] host:port",
+        .help       = "serve block devices on the given host and port",
+        .mhandler.cmd = hmp_nbd_server_start,
+    },
+STEXI
+@item nbd_server_start @var{host}:@var{port}
+@findex nbd_server_start
+Start an NBD server on the given host and/or port, and serve all of the
+virtual machine's block devices that have an inserted media on it.
+The @option{-w} option makes the devices writable.
+ETEXI
+
+    {
+        .name       = "nbd_server_stop",
+        .args_type  = "",
+        .params     = "nbd_server_stop",
+        .help       = "stop serving block devices using the NBD protocol",
+        .mhandler.cmd = hmp_nbd_server_stop,
+    },
+STEXI
+@item nbd_server_stop
+@findex nbd_server_stop
+Stop the QEMU embedded NBD server.
+ETEXI
+
+
 #if defined(TARGET_I386)
 
     {
diff --git a/hmp.c b/hmp.c
index a9d5675..10ff50d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -18,6 +18,7 @@
 #include "qemu-option.h"
 #include "qemu-timer.h"
 #include "qmp-commands.h"
+#include "qemu_socket.h"
 #include "monitor.h"
 
 static void hmp_handle_error(Monitor *mon, Error **errp)
@@ -1102,3 +1103,68 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
     qmp_closefd(fdname, &errp);
     hmp_handle_error(mon, &errp);
 }
+
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
+{
+    const char *uri = qdict_get_str(qdict, "uri");
+    int writable = qdict_get_try_bool(qdict, "writable", 0);
+    Error *errp = NULL;
+    QemuOpts *opts;
+    BlockDriverState *bs;
+    IPSocketAddress addr;
+
+    /* First check if the address is available and start the server.  */
+    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
+    if (inet_parse(opts, uri) != 0) {
+        error_set(&errp, QERR_SOCKET_CREATE_FAILED);
+	goto exit;
+    }
+
+    memset(&addr, 0, sizeof(addr));
+    addr.host = (char *) qemu_opt_get(opts, "host");
+    addr.port = (char *) qemu_opt_get(opts, "port");
+    addr.ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
+    addr.ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+    addr.has_ipv4 = addr.has_ipv6 = true;
+
+    if (addr.host == NULL || addr.port == NULL) {
+        error_set(&errp, QERR_SOCKET_CREATE_FAILED);
+        goto exit;
+    }
+
+    qmp_nbd_server_start(&addr, &errp);
+    if (errp != NULL) {
+        goto exit;
+    }
+
+    /* Then try adding all block devices.  If one fails, close all and
+     * exit.
+     */
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
+        if (!bdrv_is_inserted(bs)) {
+            continue;
+        }
+
+        qmp_nbd_server_add(bdrv_get_device_name(bs),
+                           true, !bdrv_is_read_only(bs) && writable,
+                           &errp);
+
+        if (errp != NULL) {
+            qmp_nbd_server_stop(NULL);
+            break;
+        }
+    }
+
+exit:
+    qemu_opts_del(opts);
+    hmp_handle_error(mon, &errp);
+}
+
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
+{
+    Error *errp = NULL;
+
+    qmp_nbd_server_stop(&errp);
+    hmp_handle_error(mon, &errp);
+}
diff --git a/hmp.h b/hmp.h
index 7dd93bf..89d3960 100644
--- a/hmp.h
+++ b/hmp.h
@@ -71,5 +71,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
 void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
1.7.11.2

Re: [Qemu-devel] [RFC PATCH 11/13] hmp: add NBD server commands

[Luiz Capitulino]

On Mon, 27 Aug 2012 17:00:24 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> At the HMP level there is no nbd_server_add command.  nbd_server_start
> automatically exposes all of the VM's block devices, and an option -w
> makes them writable.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hmp-commands.hx | 29 +++++++++++++++++++++++++
>  hmp.c           | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h           |  2 ++
>  3 file modificati, 97 inserzioni(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f6104b0..cabb886 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1248,6 +1248,35 @@ Remove all matches from the access control list, and set the default
>  policy back to @code{deny}.
>  ETEXI
>  
> +    {
> +        .name       = "nbd_server_start",
> +        .args_type  = "writable:-w,uri:s",
> +        .params     = "nbd_server_start [-w] host:port",
> +        .help       = "serve block devices on the given host and port",
> +        .mhandler.cmd = hmp_nbd_server_start,
> +    },
> +STEXI
> +@item nbd_server_start @var{host}:@var{port}
> +@findex nbd_server_start
> +Start an NBD server on the given host and/or port, and serve all of the
> +virtual machine's block devices that have an inserted media on it.
> +The @option{-w} option makes the devices writable.
> +ETEXI
> +
> +    {
> +        .name       = "nbd_server_stop",
> +        .args_type  = "",
> +        .params     = "nbd_server_stop",
> +        .help       = "stop serving block devices using the NBD protocol",
> +        .mhandler.cmd = hmp_nbd_server_stop,
> +    },
> +STEXI
> +@item nbd_server_stop
> +@findex nbd_server_stop
> +Stop the QEMU embedded NBD server.
> +ETEXI
> +
> +
>  #if defined(TARGET_I386)
>  
>      {
> diff --git a/hmp.c b/hmp.c
> index a9d5675..10ff50d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -18,6 +18,7 @@
>  #include "qemu-option.h"
>  #include "qemu-timer.h"
>  #include "qmp-commands.h"
> +#include "qemu_socket.h"
>  #include "monitor.h"
>  
>  static void hmp_handle_error(Monitor *mon, Error **errp)
> @@ -1102,3 +1103,68 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
>      qmp_closefd(fdname, &errp);
>      hmp_handle_error(mon, &errp);
>  }
> +
> +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
> +{
> +    const char *uri = qdict_get_str(qdict, "uri");
> +    int writable = qdict_get_try_bool(qdict, "writable", 0);
> +    Error *errp = NULL;
> +    QemuOpts *opts;
> +    BlockDriverState *bs;
> +    IPSocketAddress addr;
> +
> +    /* First check if the address is available and start the server.  */
> +    opts = qemu_opts_create(&socket_opts, NULL, 0, NULL);
> +    if (inet_parse(opts, uri) != 0) {
> +        error_set(&errp, QERR_SOCKET_CREATE_FAILED);
> +	goto exit;
> +    }
> +
> +    memset(&addr, 0, sizeof(addr));
> +    addr.host = (char *) qemu_opt_get(opts, "host");
> +    addr.port = (char *) qemu_opt_get(opts, "port");
> +    addr.ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
> +    addr.ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
> +    addr.has_ipv4 = addr.has_ipv6 = true;
> +
> +    if (addr.host == NULL || addr.port == NULL) {
> +        error_set(&errp, QERR_SOCKET_CREATE_FAILED);

You can and should use monitor_printf() directly.

> +        goto exit;
> +    }
> +
> +    qmp_nbd_server_start(&addr, &errp);
> +    if (errp != NULL) {

Please, use error_is_set() instead.

> +        goto exit;
> +    }
> +
> +    /* Then try adding all block devices.  If one fails, close all and
> +     * exit.
> +     */
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {

To keep hmp.c an honest qmp client, you should use qmp_query_block() instead
of looping through BSs directly. You'll find other examples if you find for
qmp_query_block() in this file.

> +        if (!bdrv_is_inserted(bs)) {
> +            continue;
> +        }
> +
> +        qmp_nbd_server_add(bdrv_get_device_name(bs),
> +                           true, !bdrv_is_read_only(bs) && writable,
> +                           &errp);
> +
> +        if (errp != NULL) {
> +            qmp_nbd_server_stop(NULL);
> +            break;
> +        }
> +    }
> +
> +exit:
> +    qemu_opts_del(opts);
> +    hmp_handle_error(mon, &errp);
> +}
> +
> +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> +{
> +    Error *errp = NULL;
> +
> +    qmp_nbd_server_stop(&errp);
> +    hmp_handle_error(mon, &errp);
> +}
> diff --git a/hmp.h b/hmp.h
> index 7dd93bf..89d3960 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -71,5 +71,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>  void hmp_getfd(Monitor *mon, const QDict *qdict);
>  void hmp_closefd(Monitor *mon, const QDict *qdict);
> +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
> +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  
>  #endif

Re: [Qemu-devel] [RFC PATCH 11/13] hmp: add NBD server commands

[Paolo Bonzini]

Il 18/09/2012 22:22, Luiz Capitulino ha scritto:
>> > +    if (addr.host == NULL || addr.port == NULL) {
>> > +        error_set(&errp, QERR_SOCKET_CREATE_FAILED);
> You can and should use monitor_printf() directly.

Hmm, why? Then I would have two error paths, one with monitor_printf and
one with hmp_handle_error.  But I will change this to add an Error* to
inet_parse.

>> > +        goto exit;
>> > +    }
>> > +
>> > +    qmp_nbd_server_start(&addr, &errp);
>> > +    if (errp != NULL) {
> Please, use error_is_set() instead.

I'll just rename errp to local_err, which is the usual convention.

>> > +        goto exit;
>> > +    }
>> > +
>> > +    /* Then try adding all block devices.  If one fails, close all and
>> > +     * exit.
>> > +     */
>> > +    bs = NULL;
>> > +    while ((bs = bdrv_next(bs))) {
> To keep hmp.c an honest qmp client, you should use qmp_query_block() instead
> of looping through BSs directly. You'll find other examples if you find for
> qmp_query_block() in this file.

Right, thanks!

Paolo

[Qemu-devel] [RFC PATCH 12/13] block: add close notifiers

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs |  3 ++-
 block.c       | 19 ++++++++++++++-----
 block.h       |  1 +
 block_int.h   |  2 ++
 4 file modificati, 19 inserzioni(+), 6 rimozioni(-)

diff --git a/Makefile.objs b/Makefile.objs
index c42affc..8321f81 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -43,6 +43,7 @@ coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
 
 block-obj-y = cutils.o iov.o cache-utils.o qemu-option.o module.o async.o
 block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o qemu-progress.o qemu-sockets.o
+block-obj-y += notify.o
 block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
@@ -91,7 +92,7 @@ common-obj-y += bt-host.o bt-vhci.o
 
 common-obj-y += iov.o acl.o
 common-obj-$(CONFIG_POSIX) += compatfd.o
-common-obj-y += notify.o event_notifier.o
+common-obj-y += event_notifier.o
 common-obj-y += qemu-timer.o qemu-timer-common.o
 
 common-obj-$(CONFIG_SLIRP) += slirp/
diff --git a/block.c b/block.c
index 470bdcc..cb44dac 100644
--- a/block.c
+++ b/block.c
@@ -28,6 +28,7 @@
 #include "block_int.h"
 #include "module.h"
 #include "qjson.h"
+#include "notify.h"
 #include "qemu-coroutine.h"
 #include "qmp-commands.h"
 #include "qemu-timer.h"
@@ -310,9 +311,16 @@ BlockDriverState *bdrv_new(const char *device_name)
         QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
     }
     bdrv_iostatus_disable(bs);
+    notifier_list_init(&bs->close_notifiers);
+
     return bs;
 }
 
+void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify)
+{
+    notifier_list_add(&bs->close_notifiers, notify);
+}
+
 BlockDriver *bdrv_find_format(const char *format_name)
 {
     BlockDriver *drv1;
@@ -862,12 +870,13 @@ unlink_and_fail:
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);
-    if (bs->drv) {
-        if (bs->job) {
-            block_job_cancel_sync(bs->job);
-        }
-        bdrv_drain_all();
+    if (bs->job) {
+        block_job_cancel_sync(bs->job);
+    }
+    bdrv_drain_all();
+    notifier_list_notify(&bs->close_notifiers, bs);
 
+    if (bs->drv) {
         if (bs == bs_snapshots) {
             bs_snapshots = NULL;
         }
diff --git a/block.h b/block.h
index 2e2be11..921c72e 100644
--- a/block.h
+++ b/block.h
@@ -131,6 +131,7 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
 void bdrv_close(BlockDriverState *bs);
+void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify);
 int bdrv_attach_dev(BlockDriverState *bs, void *dev);
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
 void bdrv_detach_dev(BlockDriverState *bs, void *dev);
diff --git a/block_int.h b/block_int.h
index 4452f6f..4eb79c2 100644
--- a/block_int.h
+++ b/block_int.h
@@ -293,6 +293,8 @@ struct BlockDriverState {
     BlockDriverState *backing_hd;
     BlockDriverState *file;
 
+    NotifierList close_notifiers;
+
     /* number of in-flight copy-on-read requests */
     unsigned int copy_on_read_in_flight;
 
-- 
1.7.11.2

[Qemu-devel] [RFC PATCH 13/13] nbd: add notifier to close exports when the image is closed

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev-nbd.c      | 38 ++++++++++++++++++++++++++++++++++++++
 qapi/opts-visitor.c | 48 ++++++++++++++++++++----------------------------
 2 file modificati, 58 inserzioni(+), 28 rimozioni(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 5a415be..c190caa 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -62,12 +62,39 @@ void qmp_nbd_server_start(IPSocketAddress *addr, Error **errp)
     qemu_opts_del(opts);
 }
 
+/* Hook into the BlockDriverState notifiers to close the export when
+ * the file is closed.
+ */
+typedef struct NBDCloseNotifier {
+    Notifier n;
+    NBDExport *exp;
+    QTAILQ_ENTRY(NBDCloseNotifier) next;
+} NBDCloseNotifier;
+
+static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
+    QTAILQ_HEAD_INITIALIZER(close_notifiers);
+
+static void nbd_close_notifier_remove(NBDCloseNotifier *cn)
+{
+    notifier_remove(&cn->n);
+    QTAILQ_REMOVE(&close_notifiers, cn, next);
+    g_free(cn);
+}
+
+static void nbd_close_notifier(Notifier *n, void *data)
+{
+    NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
+
+    nbd_export_close(cn->exp);
+    nbd_close_notifier_remove(cn);
+}
 
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
                         Error **errp)
 {
     BlockDriverState *bs;
     NBDExport *exp;
+    NBDCloseNotifier *n;
 
     bs = bdrv_find(device);
     if (!bs) {
@@ -82,10 +109,21 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
 
     exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY);
     nbd_export_set_name(exp, device);
+
+    n = g_malloc0(sizeof(NBDCloseNotifier));
+    n->n.notify = nbd_close_notifier;
+    n->exp = exp;
+    bdrv_add_close_notifier(bs, &n->n);
+    QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
 }
 
 void qmp_nbd_server_stop(Error **errp)
 {
+    while (!QTAILQ_EMPTY(&close_notifiers)) {
+        NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
+        nbd_close_notifier_remove(cn);
+    }
+
     nbd_export_close_all();
     qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
     close(server_fd);
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index a59d306..6893d62 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -20,9 +20,6 @@ struct OptsVisitor
 {
     Visitor visitor;
 
-    /* Ownership remains with opts_visitor_new()'s caller. */
-    const QemuOpts *opts_root;
-
     unsigned depth;
 
     /* Non-null iff depth is positive. Each key is a QemuOpt name. Each value
@@ -36,9 +33,9 @@ struct OptsVisitor
     GQueue *repeated_opts;
     bool repeated_opts_first;
 
-    /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
-     * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
-     * not survive or escape the OptsVisitor object.
+    /* If the "id" is set on the QemuOpts, reinstantiate it as a fake QemuOpt
+     * for uniformity. Only its "name" and "str" fields are set. "fake_id_opt"
+     * does not survive or escape the OptsVisitor object.
      */
     QemuOpt *fake_id_opt;
 };
@@ -77,29 +74,9 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
                   const char *name, size_t size, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
-    const QemuOpt *opt;
 
     *obj = g_malloc0(size > 0 ? size : 1);
-    if (ov->depth++ > 0) {
-        return;
-    }
-
-    ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
-                                                 NULL, &destroy_list);
-    QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
-        /* ensured by qemu-option.c::opts_do_parse() */
-        assert(strcmp(opt->name, "id") != 0);
-
-        opts_visitor_insert(ov->unprocessed_opts, opt);
-    }
-
-    if (ov->opts_root->id != NULL) {
-        ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt);
-
-        ov->fake_id_opt->name = "id";
-        ov->fake_id_opt->str = ov->opts_root->id;
-        opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
-    }
+    ov->depth++;
 }
 
 
@@ -372,6 +349,7 @@ OptsVisitor *
 opts_visitor_new(const QemuOpts *opts)
 {
     OptsVisitor *ov;
+    const QemuOpt *opt;
 
     ov = g_malloc0(sizeof *ov);
 
@@ -403,8 +381,22 @@ opts_visitor_new(const QemuOpts *opts)
 
     ov->visitor.start_optional = &opts_start_optional;
 
-    ov->opts_root = opts;
+    ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
+                                                 NULL, &destroy_list);
+    QTAILQ_FOREACH(opt, &opts->head, next) {
+        /* ensured by qemu-option.c::opts_do_parse() */
+        assert(strcmp(opt->name, "id") != 0);
 
+        opts_visitor_insert(ov->unprocessed_opts, opt);
+    }
+
+    if (opts->id != NULL) {
+        ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt);
+
+        ov->fake_id_opt->name = "id";
+        ov->fake_id_opt->str = opts->id;
+        opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
+    }
     return ov;
 }
 
-- 
1.7.11.2

[Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Paolo Bonzini]

Il 27/08/2012 17:00, Paolo Bonzini ha scritto:
> The part where I need
> a second opinion and/or ack is patch 12 and 13.  They fix the case of
> a disk being unplugged while NBD export is active.  To do this I add a
> NotifierList to a BlockDriverState.  Does this look okay, or is it too
> ad hoc?

Ping... Kevin/Stefan, could you look at just these two patches:

http://permalink.gmane.org/gmane.comp.emulators.qemu/167411
[12/13] block: add close notifiers

http://permalink.gmane.org/gmane.comp.emulators.qemu/167410
[13/13] nbd: add notifier to close exports when the image is closed

and if you need some context:

http://permalink.gmane.org/gmane.comp.emulators.qemu/167400
[09/13] qmp: add NBD server commands


Everything else is totally uninteresting.

Paolo

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Kevin Wolf]

Am 07.09.2012 17:50, schrieb Paolo Bonzini:
> Il 27/08/2012 17:00, Paolo Bonzini ha scritto:
>> The part where I need
>> a second opinion and/or ack is patch 12 and 13.  They fix the case of
>> a disk being unplugged while NBD export is active.  To do this I add a
>> NotifierList to a BlockDriverState.  Does this look okay, or is it too
>> ad hoc?
> 
> Ping... Kevin/Stefan, could you look at just these two patches:
> 
> http://permalink.gmane.org/gmane.comp.emulators.qemu/167411
> [12/13] block: add close notifiers
> 
> http://permalink.gmane.org/gmane.comp.emulators.qemu/167410
> [13/13] nbd: add notifier to close exports when the image is closed
> 
> and if you need some context:
> 
> http://permalink.gmane.org/gmane.comp.emulators.qemu/167400
> [09/13] qmp: add NBD server commands
> 
> 
> Everything else is totally uninteresting.

I was planning to review it in more detail next week, but I just had a
quick look. I'm not sure if automatically shutting down the NBD server
when the guest stops using it is always right (for removable media it
could even be an eject from the guest), but introducing a notifier list
doesn't look too bad. We can probably use it for other things that are
currently hardcoded in bdrv_close() with some if statements, like
disabling I/O throttling, cancelling a block job, etc.

Kevin

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Paolo Bonzini]

Il 07/09/2012 18:11, Kevin Wolf ha scritto:
> I was planning to review it in more detail next week, but I just had a
> quick look. I'm not sure if automatically shutting down the NBD server
> when the guest stops using it is always right (for removable media it
> could even be an eject from the guest),

Yes, the removable media case could be a bit too eager.  Note however
that a guest-triggered eject doesn't do bdrv_close, only a
user-triggered eject does, and that's blocked by bdrv_in_use.

Luckily removable media are usually not too interesting, so a slightly
suboptimal behavior is okay as long as it does not break the important
use cases---mostly migration without shared storage, where also
uninteresting images have to be mirrored or exposed via NBD.  Those
should be covered by bdrv_in_use.

> but introducing a notifier list
> doesn't look too bad. We can probably use it for other things that are
> currently hardcoded in bdrv_close() with some if statements, like
> disabling I/O throttling, cancelling a block job, etc.

Yes, though a lot of these could be moved to "filters" and use whatever
filter-specific method is there (e.g. a filter bdrv_close).  This
circles back to the question of whether bdrv_close kills filters or only
the base image...

Paolo

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Kevin Wolf]

Am 17.09.2012 18:43, schrieb Paolo Bonzini:
> Il 07/09/2012 18:11, Kevin Wolf ha scritto:
>> I was planning to review it in more detail next week, but I just had a
>> quick look. I'm not sure if automatically shutting down the NBD server
>> when the guest stops using it is always right (for removable media it
>> could even be an eject from the guest),
> 
> Yes, the removable media case could be a bit too eager.  Note however
> that a guest-triggered eject doesn't do bdrv_close, only a
> user-triggered eject does, and that's blocked by bdrv_in_use.
> 
> Luckily removable media are usually not too interesting, so a slightly
> suboptimal behavior is okay as long as it does not break the important
> use cases---mostly migration without shared storage, where also
> uninteresting images have to be mirrored or exposed via NBD.  Those
> should be covered by bdrv_in_use.

It sounds like it could be acceptable, yes. But what's even the
motivation to close the server on bdrv_close? The commit message is a
bit... well, not just terse, but even empty.

The standard case for closing images is that qemu exits. In this case,
the NBD server would automatically exit as well. An interesting case for
the NBD server would be when the migration has completed; but do we even
get a bdrv_close there?

>> but introducing a notifier list
>> doesn't look too bad. We can probably use it for other things that are
>> currently hardcoded in bdrv_close() with some if statements, like
>> disabling I/O throttling, cancelling a block job, etc.
> 
> Yes, though a lot of these could be moved to "filters" and use whatever
> filter-specific method is there (e.g. a filter bdrv_close).  This
> circles back to the question of whether bdrv_close kills filters or only
> the base image...

Note that after completing the refactoring, we'll only have one combined
bdrv_close/delete function and so there won't be BlockDriverStates that
are closed. In this case, I think it's quite obvious that not closing
the filters wouldn't make any sense.

Kevin

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Paolo Bonzini]

> > Luckily removable media are usually not too interesting, so a
> > slightly suboptimal behavior is okay as long as it does not break the
> > important use cases---mostly migration without shared storage, where also
> > uninteresting images have to be mirrored or exposed via NBD.  Those
> > should be covered by bdrv_in_use.
> 
> It sounds like it could be acceptable, yes. But what's even the
> motivation to close the server on bdrv_close? The commit message is a
> bit... well, not just terse, but even empty.

The motivation is two-fold:

1) for device hot-unplug, not closing the server would impede removal
of the blockdev until after all clients have closed their connections.

2) for the removable media case, clients risk reading data from two
different images and merging it somehow.

In either case (hot-unplug and eject) after bdrv_close I/O requests would
return ENOMEDIUM, so there is not much benefit in leaving the connection
open.  Clients can reconnect with the understanding that the medium has
changed (medium change is not part of the NBD specification, but we can
retrofit it this way).

> The standard case for closing images is that qemu exits. In this case,
> the NBD server would automatically exit as well. An interesting case
> for the NBD server would be when the migration has completed; but do we
> even get a bdrv_close there?

No, you don't.

> > Yes, though a lot of these could be moved to "filters" and use
> > whatever filter-specific method is there (e.g. a filter bdrv_close).
> > This circles back to the question of whether bdrv_close kills filters
> > or only the base image...
> 
> Note that after completing the refactoring, we'll only have one combined
> bdrv_close/delete function and so there won't be BlockDriverStates
> that are closed. In this case, I think it's quite obvious that not closing
> the filters wouldn't make any sense.

Does that mean that any I/O throttling must be applied again on every
medium change?  That would be a behavioral change.

Paolo

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Kevin Wolf]

Am 18.09.2012 11:09, schrieb Paolo Bonzini:
>>> Luckily removable media are usually not too interesting, so a
>>> slightly suboptimal behavior is okay as long as it does not break the
>>> important use cases---mostly migration without shared storage, where also
>>> uninteresting images have to be mirrored or exposed via NBD.  Those
>>> should be covered by bdrv_in_use.
>>
>> It sounds like it could be acceptable, yes. But what's even the
>> motivation to close the server on bdrv_close? The commit message is a
>> bit... well, not just terse, but even empty.
> 
> The motivation is two-fold:
> 
> 1) for device hot-unplug, not closing the server would impede removal
> of the blockdev until after all clients have closed their connections.
> 
> 2) for the removable media case, clients risk reading data from two
> different images and merging it somehow.
> 
> In either case (hot-unplug and eject) after bdrv_close I/O requests would
> return ENOMEDIUM, so there is not much benefit in leaving the connection
> open.  Clients can reconnect with the understanding that the medium has
> changed (medium change is not part of the NBD specification, but we can
> retrofit it this way).

I think I can buy this, but please add it to the commit message.

>>> Yes, though a lot of these could be moved to "filters" and use
>>> whatever filter-specific method is there (e.g. a filter bdrv_close).
>>> This circles back to the question of whether bdrv_close kills filters
>>> or only the base image...
>>
>> Note that after completing the refactoring, we'll only have one combined
>> bdrv_close/delete function and so there won't be BlockDriverStates
>> that are closed. In this case, I think it's quite obvious that not closing
>> the filters wouldn't make any sense.
> 
> Does that mean that any I/O throttling must be applied again on every
> medium change?  That would be a behavioral change.

Hm, I guess so, at least on the lowest level. The only thing I know for
certain is that maintaining compatibility for the old commands will be
fun, but if possible at all we shouldn't let that compromise our design.

Kevin

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Paolo Bonzini]

Il 18/09/2012 11:40, Kevin Wolf ha scritto:
>>> >> Note that after completing the refactoring, we'll only have one combined
>>> >> bdrv_close/delete function and so there won't be BlockDriverStates
>>> >> that are closed. In this case, I think it's quite obvious that not closing
>>> >> the filters wouldn't make any sense.
>> > 
>> > Does that mean that any I/O throttling must be applied again on every
>> > medium change?  That would be a behavioral change.
> Hm, I guess so, at least on the lowest level. The only thing I know for
> certain is that maintaining compatibility for the old commands will be
> fun, but if possible at all we shouldn't let that compromise our design.

Yeah, originally we had the idea of a "proxy" driver where you could
stack all your persistent filters.  The proxy driver would be needed of
course for removable media, but it could also subsume things like
bdrv_swap and bdrv_append.

Paolo

Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server

[Kevin Wolf]

Am 18.09.2012 11:48, schrieb Paolo Bonzini:
> Il 18/09/2012 11:40, Kevin Wolf ha scritto:
>>>>>> Note that after completing the refactoring, we'll only have one combined
>>>>>> bdrv_close/delete function and so there won't be BlockDriverStates
>>>>>> that are closed. In this case, I think it's quite obvious that not closing
>>>>>> the filters wouldn't make any sense.
>>>>
>>>> Does that mean that any I/O throttling must be applied again on every
>>>> medium change?  That would be a behavioral change.
>> Hm, I guess so, at least on the lowest level. The only thing I know for
>> certain is that maintaining compatibility for the old commands will be
>> fun, but if possible at all we shouldn't let that compromise our design.
> 
> Yeah, originally we had the idea of a "proxy" driver where you could
> stack all your persistent filters.  The proxy driver would be needed of
> course for removable media, but it could also subsume things like
> bdrv_swap and bdrv_append.

That would be filters directly for a BlockBackend, which we need in
order to support filter that stay on top of snapshots. However, they
only help in this specific case if we leave the same BlockBackend around
across media change, which wasn't part of the plan, I think. But maybe
we'll have to.

Kevin

Re: [Qemu-devel] [RFC PATCH 00/13] Embedded NBD server

[Daniel P. Berrange]

On Mon, Aug 27, 2012 at 05:00:13PM +0200, Paolo Bonzini wrote:
> Hi all,
> 
> this is an RFC series implementing an NBD server embedded inside QEMU.
> This can be used in various cases, including migration with non-shared
> storage.
> 
> Three new commands are introduced at the QMP level
> 
>   { 'command': 'nbd-server-start',  'data': { 'addr': 'IPSocketAddress' } }

It is probably desirable/required to also have a command where libvirt
can pass in a pre-opened listening socket, since I think SELinux policy
will not want to allow QEMU to create listening sockets itself.

Regards,
Daniel
-- 
|: 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] [RFC PATCH 00/13] Embedded NBD server

[Paolo Bonzini]

Il 19/09/2012 12:16, Daniel P. Berrange ha scritto:
>> > Three new commands are introduced at the QMP level
>> > 
>> >   { 'command': 'nbd-server-start',  'data': { 'addr': 'IPSocketAddress' } }
> It is probably desirable/required to also have a command where libvirt
> can pass in a pre-opened listening socket, since I think SELinux policy
> will not want to allow QEMU to create listening sockets itself.

Ok, I'll make it like this:

{ 'type': 'UnixSocketAddress',
  'data': {
    'path': 'str' } }

{ 'type': 'String',
  'data': {
    'str': 'str' } }

{'union': 'SocketAddress',
  'data': {
       'inet': 'IPSocketAddress',
       'unix': 'UnixSocketAddress',
       'fd': 'String',
   } }

Paolo