[Qemu-devel] [PATCH v5 00/13] qapi: Allow blockdev-add for NBD

[Qemu-devel] [PATCH v5 00/13] qapi: Allow blockdev-add for NBD

[Max Reitz]

This series adds blockdev-add support for NBD clients.

Patches 1, 2, 3, and 4 are minor patches with no functional relation to
this series, other than the fact that later patches will touch the code
they touch, too.

Patch 5 prepares the code for the addition of a new option prefix, which
is "server.".

Patch 6 makes the NBD client accept a SocketAddress under the "server"
option (or rather, a flattened SocketAddress QDict with its keys
prefixed by "server."). The old options "host", "port", and "path" are
supported as legacy options and translated to the respective
SocketAddress representation.

Patch 7 drops usage of "host", "port", and "path" outside of
nbd_has_filename_options_conflict(),
nbd_process_legacy_socket_options(), and nbd_refresh_filename(), making
those options nothing but legacy.

Patch 8, the goal of this series, is again not very complicated.

Patches 9, 10, 11, and 12 are required for the iotest added in patch 13.
It will invoke qemu-nbd, so patch 9 is required. Besides qemu-nbd, it
will launch an NBD server VM concurrently to the client VM, which is why
patch 10 is required. And finally, it will test whether we can add an
NBD BDS by passing it a file descriptor, which patch 11 is needed for
(so we use the socket_scm_helper to pass sockets to qemu).

During rebase of v5 I noticed that the order of keys in has changed when
querying a json: filename. Instead of just adjusting the reference
string, I decided to write a function for actually parsing such
filenames and comparing them against reference dicts. This is
implemented in patch 12.

Patch 13 then adds the iotest for NBD's blockdev-add interface.


***This series depends on qdict_crumple() as it has been merged by
   Markus to his qapi-next branch (i.e. without @recursive).***


v5:
- Patch 6:
  - "server" instead of "address"
  - Dropped comparison of key against "server" (or "address",
    previously), because all options are flattened anyway and it looks
    strange to assume otherwise here
  - Use strstart() instead of strncmp()
  - Reject use of the new server option and the legacy path/host/port
    options at the same time
  - qdict_crumple() no longer has a @recursive parameter
  - The former QMP visitors are now called QObject visitors
- Patch 7: Only rebase conflicts because of %s/address/server/g
- Patch 8: Same
- Patch 12: As explained above, comparing json: filenames against
            reference strings is not extremely clever. This new patch
            adds a function to compare such filenames against reference
            Python dicts (which is thus order-independent).
- Patch 13:
  - %s/address/server/g
  - Rebase conflict because of the blockdev-add parameter change
  - Use the new assert_json_filename_equal() function from patch 12
  - Delete the Unix socket file (if there is one) in tearDown()


git-backport-diff against v4:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/13:[----] [--] 'block/nbd: Drop trailing "." in error messages'
002/13:[----] [--] 'block/nbd: Reject port parameter without host'
003/13:[----] [--] 'block/nbd: Default port in nbd_refresh_filename()'
004/13:[----] [--] 'block/nbd: Use qdict_put()'
005/13:[----] [--] 'block/nbd: Add nbd_has_filename_options_conflict()'
006/13:[0093] [FC] 'block/nbd: Accept SocketAddress'
007/13:[0020] [FC] 'block/nbd: Use SocketAddress options'
008/13:[0004] [FC] 'qapi: Allow blockdev-add for NBD'
009/13:[----] [--] 'iotests.py: Add qemu_nbd function'
010/13:[----] [--] 'iotests.py: Allow concurrent qemu instances'
011/13:[----] [--] 'socket_scm_helper: Accept fd directly'
012/13:[down] 'iotests: Add assert_json_filename_equal() method'
013/13:[0081] [FC] 'iotests: Add test for NBD's blockdev-add interface'


Max Reitz (13):
  block/nbd: Drop trailing "." in error messages
  block/nbd: Reject port parameter without host
  block/nbd: Default port in nbd_refresh_filename()
  block/nbd: Use qdict_put()
  block/nbd: Add nbd_has_filename_options_conflict()
  block/nbd: Accept SocketAddress
  block/nbd: Use SocketAddress options
  qapi: Allow blockdev-add for NBD
  iotests.py: Add qemu_nbd function
  iotests.py: Allow concurrent qemu instances
  socket_scm_helper: Accept fd directly
  iotests: Add assert_json_filename_equal() method
  iotests: Add test for NBD's blockdev-add interface

 block/nbd.c                            | 234 +++++++++++++++++++++------------
 qapi/block-core.json                   |  25 +++-
 tests/qemu-iotests/051.out             |   4 +-
 tests/qemu-iotests/051.pc.out          |   4 +-
 tests/qemu-iotests/147                 | 196 +++++++++++++++++++++++++++
 tests/qemu-iotests/147.out             |   5 +
 tests/qemu-iotests/group               |   1 +
 tests/qemu-iotests/iotests.py          |  34 ++++-
 tests/qemu-iotests/socket_scm_helper.c |  29 ++--
 9 files changed, 428 insertions(+), 104 deletions(-)
 create mode 100755 tests/qemu-iotests/147
 create mode 100644 tests/qemu-iotests/147.out

-- 
2.10.1

[Qemu-devel] [PATCH v5 01/13] block/nbd: Drop trailing "." in error messages

[Max Reitz]

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c                   | 4 ++--
 tests/qemu-iotests/051.out    | 4 ++--
 tests/qemu-iotests/051.pc.out | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 6bc06d6..ce7c14f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -200,9 +200,9 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
 
     if (!s->path == !s->host) {
         if (s->path) {
-            error_setg(errp, "path and host may not be used at the same time.");
+            error_setg(errp, "path and host may not be used at the same time");
         } else {
-            error_setg(errp, "one of path and host must be specified.");
+            error_setg(errp, "one of path and host must be specified");
         }
         return NULL;
     }
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 408d613..9e584fe 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -222,7 +222,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: one of path and host must be specified.
+QEMU_PROG: -drive driver=nbd: one of path and host must be specified
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: Can't use 'raw' as a block driver for the protocol level
@@ -231,7 +231,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified.
+QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: Can't use 'raw' as a block driver for the protocol level
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index ec6d222..6395a30 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -316,7 +316,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: one of path and host must be specified.
+QEMU_PROG: -drive driver=nbd: one of path and host must be specified
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: Can't use 'raw' as a block driver for the protocol level
@@ -325,7 +325,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified.
+QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: Can't use 'raw' as a block driver for the protocol level
-- 
2.10.1

[Qemu-devel] [PATCH v5 02/13] block/nbd: Reject port parameter without host

[Max Reitz]

Currently, a port that is passed along with a UNIX socket path is
silently ignored. That is not exactly ideal, it should be an error
instead.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index ce7c14f..eaca33c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -197,6 +197,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
 
     s->path = g_strdup(qemu_opt_get(opts, "path"));
     s->host = g_strdup(qemu_opt_get(opts, "host"));
+    s->port = g_strdup(qemu_opt_get(opts, "port"));
 
     if (!s->path == !s->host) {
         if (s->path) {
@@ -206,6 +207,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
         }
         return NULL;
     }
+    if (s->port && !s->host) {
+        error_setg(errp, "port may not be used without host");
+        return NULL;
+    }
 
     saddr = g_new0(SocketAddress, 1);
 
@@ -217,8 +222,6 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
     } else {
         InetSocketAddress *inet;
 
-        s->port = g_strdup(qemu_opt_get(opts, "port"));
-
         saddr->type = SOCKET_ADDRESS_KIND_INET;
         inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
         inet->host = g_strdup(s->host);
-- 
2.10.1

[Qemu-devel] [PATCH v5 03/13] block/nbd: Default port in nbd_refresh_filename()

[Max Reitz]

Instead of not emitting the port in nbd_refresh_filename(), just set it
to the default if the user did not specify it. This makes the logic a
bit simpler.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index eaca33c..c77a969 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -444,6 +444,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     BDRVNBDState *s = bs->opaque;
     QDict *opts = qdict_new();
+    const char *port = s->port ?: stringify(NBD_DEFAULT_PORT);
 
     qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
 
@@ -453,27 +454,19 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     } else if (s->path && !s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
                  "nbd+unix://?socket=%s", s->path);
-    } else if (!s->path && s->export && s->port) {
+    } else if (!s->path && s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s/%s", s->host, s->port, s->export);
-    } else if (!s->path && s->export && !s->port) {
+                 "nbd://%s:%s/%s", s->host, port, s->export);
+    } else if (!s->path && !s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s/%s", s->host, s->export);
-    } else if (!s->path && !s->export && s->port) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s", s->host, s->port);
-    } else if (!s->path && !s->export && !s->port) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s", s->host);
+                 "nbd://%s:%s", s->host, port);
     }
 
     if (s->path) {
         qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(s->path)));
-    } else if (s->port) {
-        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(s->host)));
-        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(s->port)));
     } else {
         qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(s->host)));
+        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
     }
     if (s->export) {
         qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(s->export)));
-- 
2.10.1

[Qemu-devel] [PATCH v5 04/13] block/nbd: Use qdict_put()

[Max Reitz]

Instead of inlining this nice macro (i.e. resorting to
qdict_put_obj(..., QOBJECT(...))), use it.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index c77a969..c539fb5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -446,7 +446,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     QDict *opts = qdict_new();
     const char *port = s->port ?: stringify(NBD_DEFAULT_PORT);
 
-    qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
+    qdict_put(opts, "driver", qstring_from_str("nbd"));
 
     if (s->path && s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
@@ -463,17 +463,16 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     }
 
     if (s->path) {
-        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(s->path)));
+        qdict_put(opts, "path", qstring_from_str(s->path));
     } else {
-        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(s->host)));
-        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
+        qdict_put(opts, "host", qstring_from_str(s->host));
+        qdict_put(opts, "port", qstring_from_str(port));
     }
     if (s->export) {
-        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(s->export)));
+        qdict_put(opts, "export", qstring_from_str(s->export));
     }
     if (s->tlscredsid) {
-        qdict_put_obj(opts, "tls-creds",
-                      QOBJECT(qstring_from_str(s->tlscredsid)));
+        qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
     }
 
     bs->full_open_options = opts;
-- 
2.10.1

[Qemu-devel] [PATCH v5 05/13] block/nbd: Add nbd_has_filename_options_conflict()

[Max Reitz]

Right now, we have four possible options that conflict with specifying
an NBD filename, and a future patch will add another one ("address").
This future option is a nested QDict that is flattened at this point,
requiring us to test each option whether its key has an "address."
prefix. Therefore, we will then need to iterate through all options
(including the "export" option which was not covered so far).

Adding this iteration logic now will simplify adding the new option
later. A nice side effect is that the user will not receive a long list
of five options which are not supposed to be specified with a filename,
but we can actually print the problematic option.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index c539fb5..cdab20f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -123,6 +123,25 @@ out:
     return ret;
 }
 
+static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
+{
+    const QDictEntry *e;
+
+    for (e = qdict_first(options); e; e = qdict_next(options, e)) {
+        if (!strcmp(e->key, "host") ||
+            !strcmp(e->key, "port") ||
+            !strcmp(e->key, "path") ||
+            !strcmp(e->key, "export"))
+        {
+            error_setg(errp, "Option '%s' cannot be used with a file name",
+                       e->key);
+            return true;
+        }
+    }
+
+    return false;
+}
+
 static void nbd_parse_filename(const char *filename, QDict *options,
                                Error **errp)
 {
@@ -131,12 +150,7 @@ static void nbd_parse_filename(const char *filename, QDict *options,
     const char *host_spec;
     const char *unixpath;
 
-    if (qdict_haskey(options, "host")
-        || qdict_haskey(options, "port")
-        || qdict_haskey(options, "path"))
-    {
-        error_setg(errp, "host/port/path and a file name may not be specified "
-                         "at the same time");
+    if (nbd_has_filename_options_conflict(options, errp)) {
         return;
     }
 
-- 
2.10.1

[Qemu-devel] [PATCH v5 06/13] block/nbd: Accept SocketAddress

[Max Reitz]

Add a new option "server" to the NBD block driver which accepts a
SocketAddress.

"path", "host" and "port" are still supported as legacy options and are
mapped to their corresponding SocketAddress representation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c                   | 175 +++++++++++++++++++++++++++---------------
 tests/qemu-iotests/051.out    |   4 +-
 tests/qemu-iotests/051.pc.out |   4 +-
 3 files changed, 117 insertions(+), 66 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index cdab20f..a778692 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -32,6 +32,9 @@
 #include "qemu/uri.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
+#include "qapi-visit.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qint.h"
@@ -44,7 +47,8 @@ typedef struct BDRVNBDState {
     NbdClientSession client;
 
     /* For nbd_refresh_filename() */
-    char *path, *host, *port, *export, *tlscredsid;
+    SocketAddress *saddr;
+    char *export, *tlscredsid;
 } BDRVNBDState;
 
 static int nbd_parse_uri(const char *filename, QDict *options)
@@ -131,7 +135,8 @@ static bool nbd_has_filename_options_conflict(QDict *options, Error **errp)
         if (!strcmp(e->key, "host") ||
             !strcmp(e->key, "port") ||
             !strcmp(e->key, "path") ||
-            !strcmp(e->key, "export"))
+            !strcmp(e->key, "export") ||
+            strstart(e->key, "server.", NULL))
         {
             error_setg(errp, "Option '%s' cannot be used with a file name",
                        e->key);
@@ -205,50 +210,81 @@ out:
     g_free(file);
 }
 
-static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
+static bool nbd_process_legacy_socket_options(QDict *output_options,
+                                              QemuOpts *legacy_opts,
+                                              Error **errp)
 {
-    SocketAddress *saddr;
+    const char *path = qemu_opt_get(legacy_opts, "path");
+    const char *host = qemu_opt_get(legacy_opts, "host");
+    const char *port = qemu_opt_get(legacy_opts, "port");
+    const QDictEntry *e;
 
-    s->path = g_strdup(qemu_opt_get(opts, "path"));
-    s->host = g_strdup(qemu_opt_get(opts, "host"));
-    s->port = g_strdup(qemu_opt_get(opts, "port"));
+    if (!path && !host && !port) {
+        return true;
+    }
 
-    if (!s->path == !s->host) {
-        if (s->path) {
-            error_setg(errp, "path and host may not be used at the same time");
-        } else {
-            error_setg(errp, "one of path and host must be specified");
+    for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
+    {
+        if (strstart(e->key, "server.", NULL)) {
+            error_setg(errp, "Cannot use 'server' and path/host/port at the "
+                       "same time");
+            return false;
         }
-        return NULL;
     }
-    if (s->port && !s->host) {
-        error_setg(errp, "port may not be used without host");
-        return NULL;
+
+    if (path && host) {
+        error_setg(errp, "path and host may not be used at the same time");
+        return false;
+    } else if (path) {
+        if (port) {
+            error_setg(errp, "port may not be used without host");
+            return false;
+        }
+
+        qdict_put(output_options, "server.type", qstring_from_str("unix"));
+        qdict_put(output_options, "server.data.path", qstring_from_str(path));
+    } else if (host) {
+        qdict_put(output_options, "server.type", qstring_from_str("inet"));
+        qdict_put(output_options, "server.data.host", qstring_from_str(host));
+        qdict_put(output_options, "server.data.port",
+                  qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
     }
 
-    saddr = g_new0(SocketAddress, 1);
+    return true;
+}
 
-    if (s->path) {
-        UnixSocketAddress *q_unix;
-        saddr->type = SOCKET_ADDRESS_KIND_UNIX;
-        q_unix = saddr->u.q_unix.data = g_new0(UnixSocketAddress, 1);
-        q_unix->path = g_strdup(s->path);
-    } else {
-        InetSocketAddress *inet;
-
-        saddr->type = SOCKET_ADDRESS_KIND_INET;
-        inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
-        inet->host = g_strdup(s->host);
-        inet->port = g_strdup(s->port);
-        if (!inet->port) {
-            inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
-        }
+static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
+{
+    SocketAddress *saddr = NULL;
+    QDict *addr = NULL;
+    QObject *crumpled_addr = NULL;
+    Visitor *iv = NULL;
+    Error *local_err = NULL;
+
+    qdict_extract_subqdict(options, &addr, "server.");
+    if (!qdict_size(addr)) {
+        error_setg(errp, "NBD server address missing");
+        goto done;
     }
 
-    s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
+    crumpled_addr = qdict_crumple(addr, errp);
+    if (!crumpled_addr) {
+        goto done;
+    }
 
-    s->export = g_strdup(qemu_opt_get(opts, "export"));
+    iv = qobject_input_visitor_new(crumpled_addr, true);
+    visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto done;
+    }
 
+    s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
+
+done:
+    QDECREF(addr);
+    qobject_decref(crumpled_addr);
+    visit_free(iv);
     return saddr;
 }
 
@@ -349,7 +385,6 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     QIOChannelSocket *sioc = NULL;
-    SocketAddress *saddr = NULL;
     QCryptoTLSCreds *tlscreds = NULL;
     const char *hostname = NULL;
     int ret = -EINVAL;
@@ -361,12 +396,19 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
         goto error;
     }
 
+    /* Translate @host, @port, and @path to a SocketAddress */
+    if (!nbd_process_legacy_socket_options(options, opts, errp)) {
+        goto error;
+    }
+
     /* Pop the config into our state object. Exit if invalid. */
-    saddr = nbd_config(s, opts, errp);
-    if (!saddr) {
+    s->saddr = nbd_config(s, options, errp);
+    if (!s->saddr) {
         goto error;
     }
 
+    s->export = g_strdup(qemu_opt_get(opts, "export"));
+
     s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
     if (s->tlscredsid) {
         tlscreds = nbd_get_tls_creds(s->tlscredsid, errp);
@@ -374,17 +416,17 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
             goto error;
         }
 
-        if (saddr->type != SOCKET_ADDRESS_KIND_INET) {
+        if (s->saddr->type != SOCKET_ADDRESS_KIND_INET) {
             error_setg(errp, "TLS only supported over IP sockets");
             goto error;
         }
-        hostname = saddr->u.inet.data->host;
+        hostname = s->saddr->u.inet.data->host;
     }
 
     /* establish TCP connection, return error if it fails
      * TODO: Configurable retry-until-timeout behaviour.
      */
-    sioc = nbd_establish_connection(saddr, errp);
+    sioc = nbd_establish_connection(s->saddr, errp);
     if (!sioc) {
         ret = -ECONNREFUSED;
         goto error;
@@ -401,13 +443,10 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
         object_unref(OBJECT(tlscreds));
     }
     if (ret < 0) {
-        g_free(s->path);
-        g_free(s->host);
-        g_free(s->port);
+        qapi_free_SocketAddress(s->saddr);
         g_free(s->export);
         g_free(s->tlscredsid);
     }
-    qapi_free_SocketAddress(saddr);
     qemu_opts_del(opts);
     return ret;
 }
@@ -429,9 +468,7 @@ static void nbd_close(BlockDriverState *bs)
 
     nbd_client_close(bs);
 
-    g_free(s->path);
-    g_free(s->host);
-    g_free(s->port);
+    qapi_free_SocketAddress(s->saddr);
     g_free(s->export);
     g_free(s->tlscredsid);
 }
@@ -458,30 +495,43 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     BDRVNBDState *s = bs->opaque;
     QDict *opts = qdict_new();
-    const char *port = s->port ?: stringify(NBD_DEFAULT_PORT);
+    QObject *saddr_qdict;
+    Visitor *ov;
+    const char *host = NULL, *port = NULL, *path = NULL;
+
+    if (s->saddr->type == SOCKET_ADDRESS_KIND_INET) {
+        const InetSocketAddress *inet = s->saddr->u.inet.data;
+        if (!inet->has_ipv4 && !inet->has_ipv6 && !inet->has_to) {
+            host = inet->host;
+            port = inet->port;
+        }
+    } else if (s->saddr->type == SOCKET_ADDRESS_KIND_UNIX) {
+        path = s->saddr->u.q_unix.data->path;
+    }
 
     qdict_put(opts, "driver", qstring_from_str("nbd"));
 
-    if (s->path && s->export) {
+    if (path && s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix:///%s?socket=%s", s->export, s->path);
-    } else if (s->path && !s->export) {
+                 "nbd+unix:///%s?socket=%s", s->export, path);
+    } else if (path && !s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix://?socket=%s", s->path);
-    } else if (!s->path && s->export) {
+                 "nbd+unix://?socket=%s", path);
+    } else if (host && s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s/%s", s->host, port, s->export);
-    } else if (!s->path && !s->export) {
+                 "nbd://%s:%s/%s", host, port, s->export);
+    } else if (host && !s->export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s", s->host, port);
+                 "nbd://%s:%s", host, port);
     }
 
-    if (s->path) {
-        qdict_put(opts, "path", qstring_from_str(s->path));
-    } else {
-        qdict_put(opts, "host", qstring_from_str(s->host));
-        qdict_put(opts, "port", qstring_from_str(port));
-    }
+    ov = qobject_output_visitor_new(&saddr_qdict);
+    visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort);
+    visit_complete(ov, &saddr_qdict);
+    assert(qobject_type(saddr_qdict) == QTYPE_QDICT);
+
+    qdict_put_obj(opts, "server", saddr_qdict);
+
     if (s->export) {
         qdict_put(opts, "export", qstring_from_str(s->export));
     }
@@ -489,6 +539,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
         qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
     }
 
+    qdict_flatten(opts);
     bs->full_open_options = opts;
 }
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 9e584fe..42bf416 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -222,7 +222,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: one of path and host must be specified
+QEMU_PROG: -drive driver=nbd: NBD server address missing
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: Can't use 'raw' as a block driver for the protocol level
@@ -231,7 +231,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified
+QEMU_PROG: -drive file.driver=nbd: NBD server address missing
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: Can't use 'raw' as a block driver for the protocol level
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 6395a30..603bb76 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -316,7 +316,7 @@ Testing: -drive driver=file
 QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name
 
 Testing: -drive driver=nbd
-QEMU_PROG: -drive driver=nbd: one of path and host must be specified
+QEMU_PROG: -drive driver=nbd: NBD server address missing
 
 Testing: -drive driver=raw
 QEMU_PROG: -drive driver=raw: Can't use 'raw' as a block driver for the protocol level
@@ -325,7 +325,7 @@ Testing: -drive file.driver=file
 QEMU_PROG: -drive file.driver=file: The 'file' block driver requires a file name
 
 Testing: -drive file.driver=nbd
-QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified
+QEMU_PROG: -drive file.driver=nbd: NBD server address missing
 
 Testing: -drive file.driver=raw
 QEMU_PROG: -drive file.driver=raw: Can't use 'raw' as a block driver for the protocol level
-- 
2.10.1

Re: [Qemu-devel] [PATCH v5 06/13] block/nbd: Accept SocketAddress

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> Add a new option "server" to the NBD block driver which accepts a
> SocketAddress.
> 
> "path", "host" and "port" are still supported as legacy options and are
> mapped to their corresponding SocketAddress representation.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 07/13] block/nbd: Use SocketAddress options

[Max Reitz]

Drop the use of legacy options in favor of the SocketAddress
representation, even for internal use (i.e. for storing the result of
the filename parsing).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index a778692..8ef1438 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -94,9 +94,13 @@ static int nbd_parse_uri(const char *filename, QDict *options)
             ret = -EINVAL;
             goto out;
         }
-        qdict_put(options, "path", qstring_from_str(qp->p[0].value));
+        qdict_put(options, "server.type", qstring_from_str("unix"));
+        qdict_put(options, "server.data.path",
+                  qstring_from_str(qp->p[0].value));
     } else {
         QString *host;
+        char *port_str;
+
         /* nbd[+tcp]://host[:port]/export */
         if (!uri->server) {
             ret = -EINVAL;
@@ -111,12 +115,12 @@ static int nbd_parse_uri(const char *filename, QDict *options)
             host = qstring_from_str(uri->server);
         }
 
-        qdict_put(options, "host", host);
-        if (uri->port) {
-            char* port_str = g_strdup_printf("%d", uri->port);
-            qdict_put(options, "port", qstring_from_str(port_str));
-            g_free(port_str);
-        }
+        qdict_put(options, "server.type", qstring_from_str("inet"));
+        qdict_put(options, "server.data.host", host);
+
+        port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
+        qdict_put(options, "server.data.port", qstring_from_str(port_str));
+        g_free(port_str);
     }
 
 out:
@@ -192,7 +196,8 @@ static void nbd_parse_filename(const char *filename, QDict *options,
 
     /* are we a UNIX or TCP socket? */
     if (strstart(host_spec, "unix:", &unixpath)) {
-        qdict_put(options, "path", qstring_from_str(unixpath));
+        qdict_put(options, "server.type", qstring_from_str("unix"));
+        qdict_put(options, "server.data.path", qstring_from_str(unixpath));
     } else {
         InetSocketAddress *addr = NULL;
 
@@ -201,8 +206,9 @@ static void nbd_parse_filename(const char *filename, QDict *options,
             goto out;
         }
 
-        qdict_put(options, "host", qstring_from_str(addr->host));
-        qdict_put(options, "port", qstring_from_str(addr->port));
+        qdict_put(options, "server.type", qstring_from_str("inet"));
+        qdict_put(options, "server.data.host", qstring_from_str(addr->host));
+        qdict_put(options, "server.data.port", qstring_from_str(addr->port));
         qapi_free_InetSocketAddress(addr);
     }
 
-- 
2.10.1

Re: [Qemu-devel] [PATCH v5 07/13] block/nbd: Use SocketAddress options

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> Drop the use of legacy options in favor of the SocketAddress
> representation, even for internal use (i.e. for storing the result of
> the filename parsing).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 08/13] qapi: Allow blockdev-add for NBD

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 97b1205..4b4a74c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1703,14 +1703,15 @@
 #
 # @host_device, @host_cdrom: Since 2.1
 # @gluster: Since 2.7
+# @nbd: Since 2.8
 #
 # Since: 2.0
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
-            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
-            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
+            'host_device', 'http', 'https', 'luks', 'nbd', 'null-aio',
+            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
 	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
 
 ##
@@ -2220,6 +2221,24 @@
   'data': { 'filename': 'str' } }
 
 ##
+# @BlockdevOptionsNbd
+#
+# Driver specific block device options for NBD.
+#
+# @server:      NBD server address
+#
+# @export:      #optional export name
+#
+# @tls-creds:   #optional TLS credentials ID
+#
+# Since: 2.8
+##
+{ 'struct': 'BlockdevOptionsNbd',
+  'data': { 'server': 'SocketAddress',
+            '*export': 'str',
+            '*tls-creds': 'str' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.  Many options are available for all
@@ -2264,7 +2283,7 @@
       'https':      'BlockdevOptionsCurl',
 # TODO iscsi: Wait for structured options
       'luks':       'BlockdevOptionsLUKS',
-# TODO nbd: Should take InetSocketAddress for 'host'?
+      'nbd':        'BlockdevOptionsNbd',
 # TODO nfs: Wait for structured options
       'null-aio':   'BlockdevOptionsNull',
       'null-co':    'BlockdevOptionsNull',
-- 
2.10.1

Re: [Qemu-devel] [PATCH v5 08/13] qapi: Allow blockdev-add for NBD

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On 10/25/2016 08:11 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 97b1205..4b4a74c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1703,14 +1703,15 @@
>  #
>  # @host_device, @host_cdrom: Since 2.1
>  # @gluster: Since 2.7
> +# @nbd: Since 2.8

'replication' was also added in 2.8; we should mention it while touching
this.

>  #
>  # Since: 2.0
>  ##
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> +            'host_device', 'http', 'https', 'luks', 'nbd', 'null-aio',
> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>  	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }

Can we fix the TAB damage while at it?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 08/13] qapi: Allow blockdev-add for NBD

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

Am 26.10.2016 um 01:57 hat Eric Blake geschrieben:
> On 10/25/2016 08:11 AM, Max Reitz wrote:
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  qapi/block-core.json | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 97b1205..4b4a74c 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1703,14 +1703,15 @@
> >  #
> >  # @host_device, @host_cdrom: Since 2.1
> >  # @gluster: Since 2.7
> > +# @nbd: Since 2.8
> 
> 'replication' was also added in 2.8; we should mention it while touching
> this.

In a patch separate from this series, please.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v5 08/13] qapi: Allow blockdev-add for NBD

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

On 26.10.2016 01:57, Eric Blake wrote:
> On 10/25/2016 08:11 AM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qapi/block-core.json | 25 ++++++++++++++++++++++---
>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 97b1205..4b4a74c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1703,14 +1703,15 @@
>>  #
>>  # @host_device, @host_cdrom: Since 2.1
>>  # @gluster: Since 2.7
>> +# @nbd: Since 2.8
> 
> 'replication' was also added in 2.8; we should mention it while touching
> this.
> 
>>  #
>>  # Since: 2.0
>>  ##
>>  { 'enum': 'BlockdevDriver',
>>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>> -            'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>> -            'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> +            'host_device', 'http', 'https', 'luks', 'nbd', 'null-aio',
>> +            'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>>  	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> 
> Can we fix the TAB damage while at it?
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

You're completely right with both comments, but I think putting them in
separate patches might be better (we even have time after soft freeze
for this). If the maintainer applying this patch decides to put the tab
fix into this patch while applying, I wouldn't mind, though.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v5 08/13] qapi: Allow blockdev-add for NBD

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 09/13] iotests.py: Add qemu_nbd function

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3329bc1..5a2678f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -39,6 +39,10 @@ qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
 if os.environ.get('QEMU_IO_OPTIONS'):
     qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
 
+qemu_nbd_args = [os.environ.get('QEMU_NBD_PROG', 'qemu-nbd')]
+if os.environ.get('QEMU_NBD_OPTIONS'):
+    qemu_nbd_args += os.environ['QEMU_NBD_OPTIONS'].strip().split(' ')
+
 qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
 qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
 
@@ -87,6 +91,10 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
+def qemu_nbd(*args):
+    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
+    return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
+
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
     return qemu_img('compare', '-f', fmt1,
-- 
2.10.1

Re: [Qemu-devel] [PATCH v5 09/13] iotests.py: Add qemu_nbd function

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 10/13] iotests.py: Allow concurrent qemu instances

[Max Reitz]

By adding an optional suffix to the files used for communication with a
VM, we can launch multiple VM instances concurrently.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5a2678f..c589deb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -140,8 +140,10 @@ def log(msg, filters=[]):
 class VM(qtest.QEMUQtestMachine):
     '''A QEMU VM'''
 
-    def __init__(self):
-        super(VM, self).__init__(qemu_prog, qemu_opts, test_dir=test_dir,
+    def __init__(self, path_suffix=''):
+        name = "qemu%s-%d" % (path_suffix, os.getpid())
+        super(VM, self).__init__(qemu_prog, qemu_opts, name=name,
+                                 test_dir=test_dir,
                                  socket_scm_helper=socket_scm_helper)
         if debug:
             self._debug = True
-- 
2.10.1

[Qemu-devel] [PATCH v5 11/13] socket_scm_helper: Accept fd directly

[Max Reitz]

This gives us more freedom about the fd that is passed to qemu, allowing
us to e.g. pass sockets.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/socket_scm_helper.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c
index 80cadf4..eb76d31 100644
--- a/tests/qemu-iotests/socket_scm_helper.c
+++ b/tests/qemu-iotests/socket_scm_helper.c
@@ -60,7 +60,7 @@ static int send_fd(int fd, int fd_to_send)
 }
 
 /* Convert string to fd number. */
-static int get_fd_num(const char *fd_str)
+static int get_fd_num(const char *fd_str, bool silent)
 {
     int sock;
     char *err;
@@ -68,12 +68,16 @@ static int get_fd_num(const char *fd_str)
     errno = 0;
     sock = strtol(fd_str, &err, 10);
     if (errno) {
-        fprintf(stderr, "Failed in strtol for socket fd, reason: %s\n",
-                strerror(errno));
+        if (!silent) {
+            fprintf(stderr, "Failed in strtol for socket fd, reason: %s\n",
+                    strerror(errno));
+        }
         return -1;
     }
     if (!*fd_str || *err || sock < 0) {
-        fprintf(stderr, "bad numerical value for socket fd '%s'\n", fd_str);
+        if (!silent) {
+            fprintf(stderr, "bad numerical value for socket fd '%s'\n", fd_str);
+        }
         return -1;
     }
 
@@ -104,18 +108,21 @@ int main(int argc, char **argv, char **envp)
     }
 
 
-    sock = get_fd_num(argv[1]);
+    sock = get_fd_num(argv[1], false);
     if (sock < 0) {
         return EXIT_FAILURE;
     }
 
-    /* Now only open a file in readonly mode for test purpose. If more precise
-       control is needed, use python script in file operation, which is
-       supposed to fork and exec this program. */
-    fd = open(argv[2], O_RDONLY);
+    fd = get_fd_num(argv[2], true);
     if (fd < 0) {
-        fprintf(stderr, "Failed to open file '%s'\n", argv[2]);
-        return EXIT_FAILURE;
+        /* Now only open a file in readonly mode for test purpose. If more
+           precise control is needed, use python script in file operation, which
+           is supposed to fork and exec this program. */
+        fd = open(argv[2], O_RDONLY);
+        if (fd < 0) {
+            fprintf(stderr, "Failed to open file '%s'\n", argv[2]);
+            return EXIT_FAILURE;
+        }
     }
 
     ret = send_fd(sock, fd);
-- 
2.10.1

[Qemu-devel] [PATCH v5 12/13] iotests: Add assert_json_filename_equal() method

[Max Reitz]

Since the order of keys in JSON filenames is not necessarily fixed, they
should not be compared to fixed strings. This method takes a Python dict
as a reference, parses a given JSON filename and compares both.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c589deb..1f30cfc 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -222,6 +222,19 @@ class QMPTestCase(unittest.TestCase):
                     self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
         return d
 
+    def flatten_qmp_object(self, obj, output=None, basestr=''):
+        if output is None:
+            output = dict()
+        if isinstance(obj, list):
+            for i in range(len(obj)):
+                self.flatten_qmp_object(obj[i], output, basestr + str(i) + '.')
+        elif isinstance(obj, dict):
+            for key in obj:
+                self.flatten_qmp_object(obj[key], output, basestr + key + '.')
+        else:
+            output[basestr[:-1]] = obj # Strip trailing '.'
+        return output
+
     def assert_qmp_absent(self, d, path):
         try:
             result = self.dictpath(d, path)
@@ -252,6 +265,13 @@ class QMPTestCase(unittest.TestCase):
         self.assertTrue(False, "Cannot find %s %s in result:\n%s" % \
                 (node_name, file_name, result))
 
+    def assert_json_filename_equal(self, json_filename, reference):
+        '''Asserts that the given filename is a json: filename and that its
+           content is equal to the given reference object'''
+        self.assertEqual(json_filename[:5], 'json:')
+        self.assertEqual(self.flatten_qmp_object(json.loads(json_filename[5:])),
+                         self.flatten_qmp_object(reference))
+
     def cancel_and_wait(self, drive='drive0', force=False, resume=False):
         '''Cancel a block job and wait for it to finish, returning the event'''
         result = self.vm.qmp('block-job-cancel', device=drive, force=force)
-- 
2.10.1

Re: [Qemu-devel] [PATCH v5 12/13] iotests: Add assert_json_filename_equal() method

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> Since the order of keys in JSON filenames is not necessarily fixed, they
> should not be compared to fixed strings. This method takes a Python dict
> as a reference, parses a given JSON filename and compares both.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index c589deb..1f30cfc 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -222,6 +222,19 @@ class QMPTestCase(unittest.TestCase):
>                      self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
>          return d
>  
> +    def flatten_qmp_object(self, obj, output=None, basestr=''):
> +        if output is None:
> +            output = dict()
> +        if isinstance(obj, list):
> +            for i in range(len(obj)):
> +                self.flatten_qmp_object(obj[i], output, basestr + str(i) + '.')
> +        elif isinstance(obj, dict):
> +            for key in obj:
> +                self.flatten_qmp_object(obj[key], output, basestr + key + '.')
> +        else:
> +            output[basestr[:-1]] = obj # Strip trailing '.'
> +        return output
> +
>      def assert_qmp_absent(self, d, path):
>          try:
>              result = self.dictpath(d, path)
> @@ -252,6 +265,13 @@ class QMPTestCase(unittest.TestCase):
>          self.assertTrue(False, "Cannot find %s %s in result:\n%s" % \
>                  (node_name, file_name, result))
>  
> +    def assert_json_filename_equal(self, json_filename, reference):
> +        '''Asserts that the given filename is a json: filename and that its
> +           content is equal to the given reference object'''
> +        self.assertEqual(json_filename[:5], 'json:')
> +        self.assertEqual(self.flatten_qmp_object(json.loads(json_filename[5:])),
> +                         self.flatten_qmp_object(reference))

Why do we have to flatten the dicts instead of comparing them directly?

Anyway, it seems to be correct:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 12/13] iotests: Add assert_json_filename_equal() method

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

On 26.10.2016 12:41, Kevin Wolf wrote:
> Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
>> Since the order of keys in JSON filenames is not necessarily fixed, they
>> should not be compared to fixed strings. This method takes a Python dict
>> as a reference, parses a given JSON filename and compares both.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index c589deb..1f30cfc 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -222,6 +222,19 @@ class QMPTestCase(unittest.TestCase):
>>                      self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
>>          return d
>>  
>> +    def flatten_qmp_object(self, obj, output=None, basestr=''):
>> +        if output is None:
>> +            output = dict()
>> +        if isinstance(obj, list):
>> +            for i in range(len(obj)):
>> +                self.flatten_qmp_object(obj[i], output, basestr + str(i) + '.')
>> +        elif isinstance(obj, dict):
>> +            for key in obj:
>> +                self.flatten_qmp_object(obj[key], output, basestr + key + '.')
>> +        else:
>> +            output[basestr[:-1]] = obj # Strip trailing '.'
>> +        return output
>> +
>>      def assert_qmp_absent(self, d, path):
>>          try:
>>              result = self.dictpath(d, path)
>> @@ -252,6 +265,13 @@ class QMPTestCase(unittest.TestCase):
>>          self.assertTrue(False, "Cannot find %s %s in result:\n%s" % \
>>                  (node_name, file_name, result))
>>  
>> +    def assert_json_filename_equal(self, json_filename, reference):
>> +        '''Asserts that the given filename is a json: filename and that its
>> +           content is equal to the given reference object'''
>> +        self.assertEqual(json_filename[:5], 'json:')
>> +        self.assertEqual(self.flatten_qmp_object(json.loads(json_filename[5:])),
>> +                         self.flatten_qmp_object(reference))
> 
> Why do we have to flatten the dicts instead of comparing them directly?

Well, I believe flattened and unflattened dicts to be equal when it
comes to JSON filenames, and I wanted to express this here. This is my
personal opinion, though, and I can see how others might disagree.

In any case, the block layer does not necessarily report JSON filenames
fully unflattened, at least not today. The test added in patch 13
compares these filenames against unflattened dicts, however, because
that was simpler to write. So there's a practical reason for flattening
them.

Maybe unflattening (or crumpling) the block layer options will be our
next great project. :-)

> Anyway, it seems to be correct:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks for reviewing,

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

[Qemu-devel] [PATCH v5 13/13] iotests: Add test for NBD's blockdev-add interface

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/147     | 196 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/147.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 202 insertions(+)
 create mode 100755 tests/qemu-iotests/147
 create mode 100644 tests/qemu-iotests/147.out

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
new file mode 100755
index 0000000..32d2a03
--- /dev/null
+++ b/tests/qemu-iotests/147
@@ -0,0 +1,196 @@
+#!/usr/bin/env python
+#
+# Test case for NBD's blockdev-add interface
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import socket
+import stat
+import time
+import iotests
+from iotests import cachemode, imgfmt, qemu_img, qemu_nbd
+
+NBD_PORT = 10811
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+unix_socket = os.path.join(iotests.test_dir, 'nbd.socket')
+
+class NBDBlockdevAddBase(iotests.QMPTestCase):
+    def blockdev_add_options(self, address, export=None):
+        options = { 'node-name': 'nbd-blockdev',
+                    'driver': 'raw',
+                    'file': {
+                        'driver': 'nbd',
+                        'server': address
+                    } }
+        if export is not None:
+            options['file']['export'] = export
+        return options
+
+    def client_test(self, filename, address, export=None):
+        bao = self.blockdev_add_options(address, export)
+        result = self.vm.qmp('blockdev-add', **bao)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-named-block-nodes')
+        for node in result['return']:
+            if node['node-name'] == 'nbd-blockdev':
+                if isinstance(filename, str):
+                    self.assert_qmp(node, 'image/filename', filename)
+                else:
+                    self.assert_json_filename_equal(node['image']['filename'],
+                                                    filename)
+                break
+
+        result = self.vm.qmp('x-blockdev-del', node_name='nbd-blockdev')
+        self.assert_qmp(result, 'return', {})
+
+
+class QemuNBD(NBDBlockdevAddBase):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img, '64k')
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        try:
+            os.remove(unix_socket)
+        except OSError:
+            pass
+
+    def _server_up(self, *args):
+        self.assertEqual(qemu_nbd('-f', imgfmt, test_img, *args), 0)
+
+    def test_inet(self):
+        self._server_up('-p', str(NBD_PORT))
+        address = { 'type': 'inet',
+                    'data': {
+                        'host': 'localhost',
+                        'port': str(NBD_PORT)
+                    } }
+        self.client_test('nbd://localhost:%i' % NBD_PORT, address)
+
+    def test_unix(self):
+        self._server_up('-k', unix_socket)
+        address = { 'type': 'unix',
+                    'data': { 'path': unix_socket } }
+        self.client_test('nbd+unix://?socket=' + unix_socket, address)
+
+
+class BuiltinNBD(NBDBlockdevAddBase):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, test_img, '64k')
+        self.vm = iotests.VM()
+        self.vm.launch()
+        self.server = iotests.VM('.server')
+        self.server.add_drive_raw('if=none,id=nbd-export,' +
+                                  'file=%s,' % test_img +
+                                  'format=%s,' % imgfmt +
+                                  'cache=%s' % cachemode)
+        self.server.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        self.server.shutdown()
+        os.remove(test_img)
+        try:
+            os.remove(unix_socket)
+        except OSError:
+            pass
+
+    def _server_up(self, address):
+        result = self.server.qmp('nbd-server-start', addr=address)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.server.qmp('nbd-server-add', device='nbd-export')
+        self.assert_qmp(result, 'return', {})
+
+    def _server_down(self):
+        result = self.server.qmp('nbd-server-stop')
+        self.assert_qmp(result, 'return', {})
+
+    def test_inet(self):
+        address = { 'type': 'inet',
+                    'data': {
+                        'host': 'localhost',
+                        'port': str(NBD_PORT)
+                    } }
+        self._server_up(address)
+        self.client_test('nbd://localhost:%i/nbd-export' % NBD_PORT,
+                         address, 'nbd-export')
+        self._server_down()
+
+    def test_inet6(self):
+        address = { 'type': 'inet',
+                    'data': {
+                        'host': '::1',
+                        'port': str(NBD_PORT),
+                        'ipv4': False,
+                        'ipv6': True
+                    } }
+        filename = { 'driver': 'raw',
+                     'file': {
+                         'driver': 'nbd',
+                         'export': 'nbd-export',
+                         'server': address
+                     } }
+        self._server_up(address)
+        self.client_test(filename, address, 'nbd-export')
+        self._server_down()
+
+    def test_unix(self):
+        address = { 'type': 'unix',
+                    'data': { 'path': unix_socket } }
+        self._server_up(address)
+        self.client_test('nbd+unix:///nbd-export?socket=' + unix_socket,
+                         address, 'nbd-export')
+        self._server_down()
+
+    def test_fd(self):
+        self._server_up({ 'type': 'unix',
+                          'data': { 'path': unix_socket } })
+
+        sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+        sockfd.connect(unix_socket)
+
+        result = self.vm.send_fd_scm(str(sockfd.fileno()))
+        self.assertEqual(result, 0, 'Failed to send socket FD')
+
+        result = self.vm.qmp('getfd', fdname='nbd-fifo')
+        self.assert_qmp(result, 'return', {})
+
+        address = { 'type': 'fd',
+                    'data': { 'str': 'nbd-fifo' } }
+        filename = { 'driver': 'raw',
+                     'file': {
+                         'driver': 'nbd',
+                         'export': 'nbd-export',
+                         'server': address
+                     } }
+        self.client_test(filename, address, 'nbd-export')
+
+        self._server_down()
+
+
+if __name__ == '__main__':
+    # Need to support image creation
+    iotests.main(supported_fmts=['vpc', 'parallels', 'qcow', 'vdi', 'qcow2',
+                                 'vmdk', 'raw', 'vhdx', 'qed'])
+
diff --git a/tests/qemu-iotests/147.out b/tests/qemu-iotests/147.out
new file mode 100644
index 0000000..3f8a935
--- /dev/null
+++ b/tests/qemu-iotests/147.out
@@ -0,0 +1,5 @@
+......
+----------------------------------------------------------------------
+Ran 6 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7eb1770..d7d50cf 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -149,6 +149,7 @@
 144 rw auto quick
 145 auto quick
 146 auto quick
+147 auto
 148 rw auto quick
 149 rw auto sudo
 150 rw auto quick
-- 
2.10.1

Re: [Qemu-devel] [PATCH v5 13/13] iotests: Add test for NBD's blockdev-add interface

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/147     | 196 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/147.out |   5 ++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 202 insertions(+)
>  create mode 100755 tests/qemu-iotests/147
>  create mode 100644 tests/qemu-iotests/147.out
>
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> new file mode 100755
> index 0000000..32d2a03
> --- /dev/null
> +++ b/tests/qemu-iotests/147
> @@ -0,0 +1,196 @@
[...]
> +if __name__ == '__main__':
> +    # Need to support image creation
> +    iotests.main(supported_fmts=['vpc', 'parallels', 'qcow', 'vdi', 'qcow2',
> +                                 'vmdk', 'raw', 'vhdx', 'qed'])
> +

I just fed the series to git-am to make sure it still applies after me
rewriting qapi-next a bit, and git-am is unhappy with you adding a
trailing blank line.

[...]

Re: [Qemu-devel] [PATCH v5 13/13] iotests: Add test for NBD's blockdev-add interface

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

On 25.10.2016 16:41, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/147     | 196 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/147.out |   5 ++
>>  tests/qemu-iotests/group   |   1 +
>>  3 files changed, 202 insertions(+)
>>  create mode 100755 tests/qemu-iotests/147
>>  create mode 100644 tests/qemu-iotests/147.out
>>
>> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
>> new file mode 100755
>> index 0000000..32d2a03
>> --- /dev/null
>> +++ b/tests/qemu-iotests/147
>> @@ -0,0 +1,196 @@
> [...]
>> +if __name__ == '__main__':
>> +    # Need to support image creation
>> +    iotests.main(supported_fmts=['vpc', 'parallels', 'qcow', 'vdi', 'qcow2',
>> +                                 'vmdk', 'raw', 'vhdx', 'qed'])
>> +
> 
> I just fed the series to git-am to make sure it still applies after me
> rewriting qapi-next a bit, and git-am is unhappy with you adding a
> trailing blank line.

Oops, sorry. Would fix in v6, but if this is the only issue with this
series (other than Eric's tab issue in patch 8), I'd like to leave this
to the maintainer.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v5 13/13] iotests: Add test for NBD's blockdev-add interface

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

With the trailing newline fixed that Markus mentioned:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 00/13] qapi: Allow blockdev-add for NBD

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

On 25.10.2016 15:11, Max Reitz wrote:

[...]

> ***This series depends on qdict_crumple() as it has been merged by
>    Markus to his qapi-next branch (i.e. without @recursive).***

Actually, it depends on the whole branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v5 00/13] qapi: Allow blockdev-add for NBD

[Kevin Wolf]

Am 25.10.2016 um 15:11 hat Max Reitz geschrieben:
> This series adds blockdev-add support for NBD clients.

Thanks, applied to the block branch.

Kevin