[Qemu-devel] [PATCH v3 for-2.7 00/14] qapi: Allow blockdev-add for NBD

[Qemu-devel] [PATCH v3 for-2.7 00/14] qapi: Allow blockdev-add for NBD

[Max Reitz]

Turns out NBD is not so simple to do if you do it right. Anyway, this
series adds blockdev-add support for NBD clients.

Patch 1 adds a QDict function which I needed in later NBD patches for
handling legacy options (move "host" to "address.data.host" etc.).

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

Patches 6 and 7 prepare the code for the addition of a new option
prefix, which is "address.".

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

Patch 9 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 10, the goal of this series, is again not very complicated.

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

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


*** This series requires Daniel's qdict_crumple() function (from his
    "Provide a QOM-based authorization API" series). ***


v2:
- Dropped patches 2 and 3; use Daniel's qdict_crumple() instead.
- Patch 7: Not sure why the diff differs, seems functionally like the
  same patch to me. Maybe git changed something about its default diff
  algorithm.
- Patch 8: Use qdict_crumple() instead of qdict_unflatten()
- Patch 10: Rebase conflicts


git-backport-diff against v2:

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/14:[----] [--] 'qdict: Add qdict_change_key()'
002/14:[----] [--] 'block/nbd: Drop trailing "." in error messages'
003/14:[----] [--] 'block/nbd: Reject port parameter without host'
004/14:[----] [--] 'block/nbd: Default port in nbd_refresh_filename()'
005/14:[----] [--] 'block/nbd: Use qdict_put()'
006/14:[----] [--] 'block/nbd: Add nbd_has_filename_options_conflict()'
007/14:[0008] [FC] 'block/nbd: "address" in nbd_refresh_filename()'
008/14:[0024] [FC] 'block/nbd: Accept SocketAddress'
009/14:[----] [--] 'block/nbd: Use SocketAddress options'
010/14:[0004] [FC] 'qapi: Allow blockdev-add for NBD'
011/14:[----] [-C] 'iotests.py: Add qemu_nbd function'
012/14:[----] [--] 'iotests.py: Allow concurrent qemu instances'
013/14:[----] [--] 'socket_scm_helper: Accept fd directly'
014/14:[----] [-C] 'iotests: Add test for NBD's blockdev-add interface'


Max Reitz (14):
  qdict: Add qdict_change_key()
  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: "address" in nbd_refresh_filename()
  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 test for NBD's blockdev-add interface

 block/nbd.c                            | 235 ++++++++++++++++++++++-----------
 include/qapi/qmp/qdict.h               |   1 +
 qapi/block-core.json                   |  23 +++-
 qobject/qdict.c                        |  23 ++++
 tests/qemu-iotests/051.out             |   4 +-
 tests/qemu-iotests/051.pc.out          |   4 +-
 tests/qemu-iotests/147                 | 194 +++++++++++++++++++++++++++
 tests/qemu-iotests/147.out             |   5 +
 tests/qemu-iotests/group               |   1 +
 tests/qemu-iotests/iotests.py          |  22 ++-
 tests/qemu-iotests/socket_scm_helper.c |  29 ++--
 11 files changed, 443 insertions(+), 98 deletions(-)
 create mode 100755 tests/qemu-iotests/147
 create mode 100644 tests/qemu-iotests/147.out

-- 
2.8.0

[Qemu-devel] [PATCH v3 01/14] qdict: Add qdict_change_key()

[Max Reitz]

This is a shorthand function for changing a QDict's entry's key.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qdict.h |  1 +
 qobject/qdict.c          | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 8a3ac13..3e0d7df 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -39,6 +39,7 @@ size_t qdict_size(const QDict *qdict);
 void qdict_put_obj(QDict *qdict, const char *key, QObject *value);
 void qdict_del(QDict *qdict, const char *key);
 int qdict_haskey(const QDict *qdict, const char *key);
+bool qdict_change_key(QDict *qdict, const char *old_key, const char *new_key);
 QObject *qdict_get(const QDict *qdict, const char *key);
 QDict *qobject_to_qdict(const QObject *obj);
 void qdict_iter(const QDict *qdict,
diff --git a/qobject/qdict.c b/qobject/qdict.c
index ae6ad06..2eacb3d 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -170,6 +170,29 @@ int qdict_haskey(const QDict *qdict, const char *key)
 }
 
 /**
+ * qdict_change_key(): Changes an entry's key
+ *
+ * Removes the entry with the key 'old_key' and inserts its associated value as
+ * a new entry with the key 'new_key'.
+ *
+ * Returns false if 'old_key' does not exist, true otherwise.
+ */
+bool qdict_change_key(QDict *qdict, const char *old_key, const char *new_key)
+{
+    QObject *value = qdict_get(qdict, old_key);
+
+    if (!value) {
+        return false;
+    }
+
+    qobject_incref(value);
+    qdict_del(qdict, old_key);
+    qdict_put_obj(qdict, new_key, value);
+
+    return true;
+}
+
+/**
  * qdict_size(): Return the size of the dictionary
  */
 size_t qdict_size(const QDict *qdict)
-- 
2.8.0

[Qemu-devel] [PATCH v3 02/14] block/nbd: Drop trailing "." in error messages

[Max Reitz]

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 f7ea3b3..d9b946f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -195,9 +195,9 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
 
     if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
         if (qdict_haskey(options, "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 c1291ff..5b3e991 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.8.0

[Qemu-devel] [PATCH v3 03/14] block/nbd: Reject port parameter without host

[Max Reitz]

This is better than the generic block layer finding out later that the
port parameter has not been used.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index d9b946f..2112ec0 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -201,6 +201,10 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
         }
         return NULL;
     }
+    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
+        error_setg(errp, "port may not be used without host");
+        return NULL;
+    }
 
     saddr = g_new0(SocketAddress, 1);
 
-- 
2.8.0

[Qemu-devel] [PATCH v3 04/14] 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.

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

diff --git a/block/nbd.c b/block/nbd.c
index 2112ec0..efa5d3d 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -433,6 +433,10 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     const char *export = qdict_get_try_str(options, "export");
     const char *tlscreds = qdict_get_try_str(options, "tls-creds");
 
+    if (host && !port) {
+        port = stringify(NBD_DEFAULT_PORT);
+    }
+
     qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
 
     if (path && export) {
@@ -441,27 +445,19 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     } else if (path && !export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
                  "nbd+unix://?socket=%s", path);
-    } else if (!path && export && port) {
+    } else if (!path && export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
                  "nbd://%s:%s/%s", host, port, export);
-    } else if (!path && export && !port) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s/%s", host, export);
-    } else if (!path && !export && port) {
+    } else if (!path && !export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
                  "nbd://%s:%s", host, port);
-    } else if (!path && !export && !port) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s", host);
     }
 
     if (path) {
         qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
-    } else if (port) {
-        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
-        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
     } else {
         qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
+        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
     }
     if (export) {
         qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
-- 
2.8.0

[Qemu-devel] [PATCH v3 05/14] block/nbd: Use qdict_put()

[Max Reitz]

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

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

diff --git a/block/nbd.c b/block/nbd.c
index efa5d3d..d12bcc6 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -437,7 +437,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
         port = stringify(NBD_DEFAULT_PORT);
     }
 
-    qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
+    qdict_put(opts, "driver", qstring_from_str("nbd"));
 
     if (path && export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
@@ -454,16 +454,16 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     }
 
     if (path) {
-        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
+        qdict_put(opts, "path", qstring_from_str(path));
     } else {
-        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
-        qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
+        qdict_put(opts, "host", qstring_from_str(host));
+        qdict_put(opts, "port", qstring_from_str(port));
     }
     if (export) {
-        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
+        qdict_put(opts, "export", qstring_from_str(export));
     }
     if (tlscreds) {
-        qdict_put_obj(opts, "tls-creds", QOBJECT(qstring_from_str(tlscreds)));
+        qdict_put(opts, "tls-creds", qstring_from_str(tlscreds));
     }
 
     bs->full_open_options = opts;
-- 
2.8.0

[Qemu-devel] [PATCH v3 06/14] 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 as to test each option whether its key has an "address."
prefix. Therefore, we will then need to iterate through all options.

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.

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 d12bcc6..1736f68 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -120,6 +120,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)
 {
@@ -128,12 +147,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.8.0

[Qemu-devel] [PATCH v3 07/14] block/nbd: "address" in nbd_refresh_filename()

[Max Reitz]

As of a future patch, the NBD block driver will accept a SocketAddress
structure for a new "address" option. In order to support this,
nbd_refresh_filename() needs some changes.

The two TODOs introduced by this patch will be removed in the very next
one. They exist to explain that it is currently impossible for
nbd_refresh_filename() to emit an "address.*" option (which the NBD
block driver does not handle yet). The next patch will arm these code
paths, but it will also enable handling of these options.

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

diff --git a/block/nbd.c b/block/nbd.c
index 1736f68..3adf302 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -441,37 +441,75 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
 static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     QDict *opts = qdict_new();
-    const char *path   = qdict_get_try_str(options, "path");
-    const char *host   = qdict_get_try_str(options, "host");
-    const char *port   = qdict_get_try_str(options, "port");
+    bool can_generate_filename = true;
+    const char *path = NULL, *host = NULL, *port = NULL;
     const char *export = qdict_get_try_str(options, "export");
     const char *tlscreds = qdict_get_try_str(options, "tls-creds");
 
-    if (host && !port) {
-        port = stringify(NBD_DEFAULT_PORT);
+    if (qdict_get_try_str(options, "address.type")) {
+        /* This path will only be possible as of a future patch;
+         * TODO: Remove this note once it is */
+
+        const char *type = qdict_get_str(options, "address.type");
+
+        if (!strcmp(type, "unix")) {
+            path = qdict_get_str(options, "address.data.path");
+        } else if (!strcmp(type, "inet")) {
+            host = qdict_get_str(options, "address.data.host");
+            port = qdict_get_str(options, "address.data.port");
+
+            can_generate_filename = !qdict_haskey(options, "address.data.to")
+                                 && !qdict_haskey(options, "address.data.ipv4")
+                                 && !qdict_haskey(options, "address.data.ipv6");
+        } else {
+            can_generate_filename = false;
+        }
+    } else {
+        path = qdict_get_try_str(options, "path");
+        host = qdict_get_try_str(options, "host");
+        port = qdict_get_try_str(options, "port");
+
+        if (host && !port) {
+            port = stringify(NBD_DEFAULT_PORT);
+        }
     }
 
     qdict_put(opts, "driver", qstring_from_str("nbd"));
 
-    if (path && export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix:///%s?socket=%s", export, path);
-    } else if (path && !export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix://?socket=%s", path);
-    } else if (!path && export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s/%s", host, port, export);
-    } else if (!path && !export) {
-        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd://%s:%s", host, port);
-    }
-
-    if (path) {
-        qdict_put(opts, "path", qstring_from_str(path));
+    if (can_generate_filename) {
+        if (path && export) {
+            snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                     "nbd+unix:///%s?socket=%s", export, path);
+        } else if (path && !export) {
+            snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                     "nbd+unix://?socket=%s", path);
+        } else if (!path && export) {
+            snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                     "nbd://%s:%s/%s", host, port, export);
+        } else if (!path && !export) {
+            snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                     "nbd://%s:%s", host, port);
+        }
+    }
+
+    if (qdict_get_try_str(options, "address.type")) {
+        /* This path will only be possible as of a future patch;
+         * TODO: Remove this note once it is */
+
+        const QDictEntry *e;
+        for (e = qdict_first(options); e; e = qdict_next(options, e)) {
+            if (!strncmp(e->key, "address.", 8)) {
+                qobject_incref(e->value);
+                qdict_put_obj(opts, e->key, e->value);
+            }
+        }
     } else {
-        qdict_put(opts, "host", qstring_from_str(host));
-        qdict_put(opts, "port", qstring_from_str(port));
+        if (path) {
+            qdict_put(opts, "path", qstring_from_str(path));
+        } else {
+            qdict_put(opts, "host", qstring_from_str(host));
+            qdict_put(opts, "port", qstring_from_str(port));
+        }
     }
     if (export) {
         qdict_put(opts, "export", qstring_from_str(export));
-- 
2.8.0

[Qemu-devel] [PATCH v3 08/14] block/nbd: Accept SocketAddress

[Max Reitz]

Add a new option "address" 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                   | 97 ++++++++++++++++++++++++++-----------------
 tests/qemu-iotests/051.out    |  4 +-
 tests/qemu-iotests/051.pc.out |  4 +-
 3 files changed, 64 insertions(+), 41 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 3adf302..9ae66ba 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -32,6 +32,8 @@
 #include "qemu/uri.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
+#include "qapi-visit.h"
+#include "qapi/qmp-input-visitor.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qint.h"
@@ -128,7 +130,9 @@ 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")
+            || !strcmp(e->key, "address")
+            || !strncmp(e->key, "address.", 8))
         {
             error_setg(errp, "Option '%s' cannot be used with a file name",
                        e->key);
@@ -202,46 +206,66 @@ out:
     g_free(file);
 }
 
+static bool nbd_process_legacy_socket_options(QDict *options, Error **errp)
+{
+    if (qdict_haskey(options, "path") && qdict_haskey(options, "host")) {
+        error_setg(errp, "path and host may not be used at the same time");
+        return false;
+    } else if (qdict_haskey(options, "path")) {
+        if (qdict_haskey(options, "port")) {
+            error_setg(errp, "port may not be used without host");
+            return false;
+        }
+
+        qdict_put(options, "address.type", qstring_from_str("unix"));
+        qdict_change_key(options, "path", "address.data.path");
+    } else if (qdict_haskey(options, "host")) {
+        qdict_put(options, "address.type", qstring_from_str("inet"));
+        qdict_change_key(options, "host", "address.data.host");
+        if (!qdict_change_key(options, "port", "address.data.port")) {
+            qdict_put(options, "address.data.port",
+                      qstring_from_str(stringify(NBD_DEFAULT_PORT)));
+        }
+    }
+
+    return true;
+}
+
 static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
                                  Error **errp)
 {
-    SocketAddress *saddr;
+    SocketAddress *saddr = NULL;
+    QDict *addr = NULL;
+    QObject *crumpled_addr;
+    QmpInputVisitor *iv;
+    Error *local_err = NULL;
 
-    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
-        if (qdict_haskey(options, "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");
-        }
-        return NULL;
+    if (!nbd_process_legacy_socket_options(options, errp)) {
+        goto fail;
     }
-    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
-        error_setg(errp, "port may not be used without host");
-        return NULL;
+
+    qdict_extract_subqdict(options, &addr, "address.");
+    if (!qdict_size(addr)) {
+        error_setg(errp, "NBD server address missing");
+        goto fail;
     }
 
-    saddr = g_new0(SocketAddress, 1);
+    crumpled_addr = qdict_crumple(addr, true, errp);
+    if (!crumpled_addr) {
+        goto fail;
+    }
 
-    if (qdict_haskey(options, "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(qdict_get_str(options, "path"));
-        qdict_del(options, "path");
-    } else {
-        InetSocketAddress *inet;
-        saddr->type = SOCKET_ADDRESS_KIND_INET;
-        inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
-        inet->host = g_strdup(qdict_get_str(options, "host"));
-        if (!qdict_get_try_str(options, "port")) {
-            inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
-        } else {
-            inet->port = g_strdup(qdict_get_str(options, "port"));
-        }
-        qdict_del(options, "host");
-        qdict_del(options, "port");
+    iv = qmp_input_visitor_new(crumpled_addr);
+    visit_type_SocketAddress(qmp_input_get_visitor(iv), NULL, &saddr,
+                             &local_err);
+    qmp_input_visitor_cleanup(iv);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
     }
 
+    /* TODO: Detect superfluous (unused) options under in addr */
+
     s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
 
     *export = g_strdup(qdict_get_try_str(options, "export"));
@@ -249,7 +273,12 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
         qdict_del(options, "export");
     }
 
+    QDECREF(addr);
     return saddr;
+
+fail:
+    QDECREF(addr);
+    return NULL;
 }
 
 NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
@@ -447,9 +476,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     const char *tlscreds = qdict_get_try_str(options, "tls-creds");
 
     if (qdict_get_try_str(options, "address.type")) {
-        /* This path will only be possible as of a future patch;
-         * TODO: Remove this note once it is */
-
         const char *type = qdict_get_str(options, "address.type");
 
         if (!strcmp(type, "unix")) {
@@ -493,9 +519,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     }
 
     if (qdict_get_try_str(options, "address.type")) {
-        /* This path will only be possible as of a future patch;
-         * TODO: Remove this note once it is */
-
         const QDictEntry *e;
         for (e = qdict_first(options); e; e = qdict_next(options, e)) {
             if (!strncmp(e->key, "address.", 8)) {
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 5b3e991..2f5c91b 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.8.0

[Qemu-devel] [PATCH v3 09/14] 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 | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 9ae66ba..82dcb5e 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -89,9 +89,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, "address.type", qstring_from_str("unix"));
+        qdict_put(options, "address.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;
@@ -106,12 +110,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, "address.type", qstring_from_str("inet"));
+        qdict_put(options, "address.data.host", host);
+
+        port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
+        qdict_put(options, "address.data.port", qstring_from_str(port_str));
+        g_free(port_str);
     }
 
 out:
@@ -188,7 +192,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, "address.type", qstring_from_str("unix"));
+        qdict_put(options, "address.data.path", qstring_from_str(unixpath));
     } else {
         InetSocketAddress *addr = NULL;
 
@@ -197,8 +202,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, "address.type", qstring_from_str("inet"));
+        qdict_put(options, "address.data.host", qstring_from_str(addr->host));
+        qdict_put(options, "address.data.port", qstring_from_str(addr->port));
         qapi_free_InetSocketAddress(addr);
     }
 
@@ -528,10 +534,12 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
         }
     } else {
         if (path) {
-            qdict_put(opts, "path", qstring_from_str(path));
+            qdict_put(opts, "address.type", qstring_from_str("unix"));
+            qdict_put(opts, "address.data.path", qstring_from_str(path));
         } else {
-            qdict_put(opts, "host", qstring_from_str(host));
-            qdict_put(opts, "port", qstring_from_str(port));
+            qdict_put(opts, "address.type", qstring_from_str("inet"));
+            qdict_put(opts, "address.data.host", qstring_from_str(host));
+            qdict_put(opts, "address.data.port", qstring_from_str(port));
         }
     }
     if (export) {
-- 
2.8.0

[Qemu-devel] [PATCH v3 10/14] qapi: Allow blockdev-add for NBD

[Max Reitz]

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d09079..6e38ff0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1632,13 +1632,14 @@
 # Drivers that are supported in block device operations.
 #
 # @host_device, @host_cdrom: Since 2.1
+# @nbd: Since 2.6
 #
 # Since: 2.0
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
             'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
-            'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels',
+            'http', 'https', 'luks', 'nbd', 'null-aio', 'null-co', 'parallels',
             'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
             'vmdk', 'vpc', 'vvfat' ] }
 
@@ -2032,6 +2033,24 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @BlockdevOptionsNbd
+#
+# Driver specific block device options for NBD.
+#
+# @address:     NBD server address
+#
+# @export:      #optional export name
+#
+# @tls-creds:   #optional TLS credentials ID
+#
+# Since: 2.6
+##
+{ 'struct': 'BlockdevOptionsNbd',
+  'data': { 'address': 'SocketAddress',
+            '*export': 'str',
+            '*tls-creds': 'str' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.  Many options are available for all
@@ -2101,7 +2120,7 @@
       'https':      'BlockdevOptionsFile',
 # 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.8.0

[Qemu-devel] [PATCH v3 11/14] iotests.py: Add qemu_nbd function

[Max Reitz]

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8499e1b..b3c00dd 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,7 +28,7 @@ import qmp
 import qtest
 import struct
 
-__all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io',
+__all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 'qemu_io', 'qemu_nbd',
            'VM', 'QMPTestCase', 'notrun', 'main', 'verify_image_format',
            'verify_platform', 'filter_test_dir', 'filter_win32',
            'filter_qemu_io', 'filter_chown', 'log']
@@ -43,6 +43,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_args = [os.environ.get('QEMU_PROG', 'qemu')]
 if os.environ.get('QEMU_OPTIONS'):
     qemu_args += os.environ['QEMU_OPTIONS'].strip().split(' ')
@@ -91,6 +95,11 @@ 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 the background'''
+    subp = subprocess.Popen(qemu_nbd_args + list(args))
+    return subp
+
 def compare_images(img1, img2):
     '''Return True if two image files are identical'''
     return qemu_img('compare', '-f', imgfmt,
-- 
2.8.0

[Qemu-devel] [PATCH v3 12/14] 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.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b3c00dd..ef7e52f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -157,10 +157,13 @@ def event_match(event, match=None):
 class VM(object):
     '''A QEMU VM'''
 
-    def __init__(self):
-        self._monitor_path = os.path.join(test_dir, 'qemu-mon.%d' % os.getpid())
-        self._qemu_log_path = os.path.join(test_dir, 'qemu-log.%d' % os.getpid())
-        self._qtest_path = os.path.join(test_dir, 'qemu-qtest.%d' % os.getpid())
+    def __init__(self, path_suffix=''):
+        self._monitor_path = os.path.join(test_dir, 'qemu-mon%s.%d' %
+                                                    (path_suffix, os.getpid()))
+        self._qemu_log_path = os.path.join(test_dir, 'qemu-log%s.%d' %
+                                                     (path_suffix, os.getpid()))
+        self._qtest_path = os.path.join(test_dir, 'qemu-qtest%s.%d' %
+                                                  (path_suffix, os.getpid()))
         self._args = qemu_args + ['-chardev',
                      'socket,id=mon,path=' + self._monitor_path,
                      '-mon', 'chardev=mon,mode=control',
-- 
2.8.0

[Qemu-devel] [PATCH v3 13/14] 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.

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.8.0

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

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/147     | 194 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/147.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 200 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..f31de69
--- /dev/null
+++ b/tests/qemu-iotests/147
@@ -0,0 +1,194 @@
+#!/usr/bin/env python
+#
+# Test case for the QMP 'change' command and all other associated
+# commands
+#
+# 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')
+
+class NBDBlockdevAddBase(iotests.QMPTestCase):
+    def blockdev_add_options(self, address, export=None):
+        options = { 'id': 'drive0',
+                    'driver': 'raw',
+                    'file': {
+                        'driver': 'nbd',
+                        'address': 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', options=bao)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/inserted/image/filename', filename)
+
+        result = self.vm.qmp('x-blockdev-del', id='drive0')
+        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()
+        self.qemu_nbd = None
+
+    def tearDown(self):
+        self.vm.shutdown()
+        if self.qemu_nbd is not None:
+            self.qemu_nbd.wait()
+        os.remove(test_img)
+
+    def _server_up(self, *args):
+        self.qemu_nbd = qemu_nbd('-f', imgfmt, test_img, *args)
+        time.sleep(1)
+
+    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):
+        socket = os.path.join(iotests.test_dir, 'qemu-nbd.socket')
+        self._server_up('-k', socket)
+        address = { 'type': 'unix',
+                    'data': { 'path': socket } }
+        self.client_test('nbd+unix://?socket=' + socket, address)
+        os.remove(socket)
+
+
+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)
+
+    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 = 'json:{"driver": "raw", ' + \
+                         '"file": {"driver": "nbd", ' + \
+                                  '"address.data.host": "::1", ' + \
+                                  '"address.data.ipv4": false, ' + \
+                                  '"address.data.ipv6": true, ' + \
+                                  '"address.type": "inet", ' + \
+                                  '"export": "nbd-export", ' + \
+                                  '"address.data.port": "%i"}}' % NBD_PORT
+        self._server_up(address)
+        self.client_test(filename, address, 'nbd-export')
+        self._server_down()
+
+    def test_unix(self):
+        socket = os.path.join(iotests.test_dir, 'nbd.socket')
+        address = { 'type': 'unix',
+                    'data': { 'path': socket } }
+        self._server_up(address)
+        self.client_test('nbd+unix:///nbd-export?socket=' + socket,
+                         address, 'nbd-export')
+        self._server_down()
+        os.remove(socket)
+
+    def test_fd(self):
+        sockfile = os.path.join(iotests.test_dir, 'nbd.socket')
+        self._server_up({ 'type': 'unix',
+                          'data': { 'path': sockfile } })
+
+        sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+        sockfd.connect(sockfile)
+
+        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', {})
+
+        filename = 'json:{"driver": "raw", ' + \
+                         '"file": {"driver": "nbd", ' + \
+                         '"address.type": "fd", ' + \
+                         '"export": "nbd-export", ' + \
+                         '"address.data.str": "nbd-fifo"}}'
+        self.client_test(filename,
+                         { 'type': 'fd',
+                           'data': { 'str': 'nbd-fifo' } },
+                         'nbd-export')
+
+        self._server_down()
+
+        os.remove(sockfile)
+
+
+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 2952b9d..8bd99ea 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.8.0

Re: [Qemu-devel] [PATCH v3 for-2.7 00/14] qapi: Allow blockdev-add for NBD

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2188 bytes --]

On 06.04.2016 20:28, Max Reitz wrote:
> Turns out NBD is not so simple to do if you do it right. Anyway, this
> series adds blockdev-add support for NBD clients.
> 
> Patch 1 adds a QDict function which I needed in later NBD patches for
> handling legacy options (move "host" to "address.data.host" etc.).
> 
> Patches 2, 3, 4, and 5 are minor patches with no functional relation to
> this series, other than later patches will touch the code they touch,
> too.
> 
> Patches 6 and 7 prepare the code for the addition of a new option
> prefix, which is "address.".
> 
> Patch 8 makes the NBD client accept a SocketAddress under the "address"
> option (or rather, a flattened SocketAddress QDict with its keys
> prefixed by "address."). The old options "host", "port", and "path" are
> supported as legacy options and translated to the respective
> SocketAddress representation.
> 
> Patch 9 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 10, the goal of this series, is again not very complicated.
> 
> Patches 11, 12, and 13 are required for the iotest added in patch 16. It
> will invoke qemu-nbd, so patch 13 is required. Besides qemu-nbd, it will
> launch an NBD server VM concurrently to the client VM, which is why
> patch 14 is required. And finally, it will test whether we can add an
> NBD BDS by passing it a file descriptor, which patch 15 is needed for
> (so we use the socket_scm_helper to pass sockets to qemu).
> 
> Patch 14 then adds the iotest for NBD's blockdev-add interface.
> 
> 
> *** This series requires Daniel's qdict_crumple() function (from his
>     "Provide a QOM-based authorization API" series). ***
> 
> 
> v2:
> - Dropped patches 2 and 3; use Daniel's qdict_crumple() instead.
> - Patch 7: Not sure why the diff differs, seems functionally like the
>   same patch to me. Maybe git changed something about its default diff
>   algorithm.
> - Patch 8: Use qdict_crumple() instead of qdict_unflatten()
> - Patch 10: Rebase conflicts


Ping


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

Re: [Qemu-devel] [PATCH v3 for-2.7 00/14] qapi: Allow blockdev-add for NBD

[Kevin Wolf]

Am 06.04.2016 um 20:28 hat Max Reitz geschrieben:
> Turns out NBD is not so simple to do if you do it right. Anyway, this
> series adds blockdev-add support for NBD clients.

What the series does seems to make sense to me, though things would be a
bit nicer (especially on the command line) if SocketAddress had been a
flat union from the beginning.  It's too late to change now, and I guess
the ugliness isn't worth duplicating it.

I'll leave the in-detail review to our NBD folks.

Kevin

Re: [Qemu-devel] [PATCH v3 01/14] qdict: Add qdict_change_key()

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> This is a shorthand function for changing a QDict's entry's key.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)

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

However, it would be nice to enhance tests/check-qdict.c to cover this.

-- 
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 v3 02/14] block/nbd: Drop trailing "." in error messages

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> 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(-)

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

Could go in via qemu-trivial, if desired.

-- 
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 v3 04/14] block/nbd: Default port in nbd_refresh_filename()

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 2112ec0..efa5d3d 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -433,6 +433,10 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>      const char *export = qdict_get_try_str(options, "export");
>      const char *tlscreds = qdict_get_try_str(options, "tls-creds");
>  
> +    if (host && !port) {
> +        port = stringify(NBD_DEFAULT_PORT);
> +    }

It would be nice to store the port as a number rather than a string -
but that's not your task (I've long thought that port should be a QAPI
alternate type, with both string and number branches, but it's a big
audit and code change to actually make it happen).

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 v3 05/14] block/nbd: Use qdict_put()

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> Instead of inlining this nice macro (i.e. resorting to
> qdict_put_obj(..., QOBJECT(...))), use it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

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 v3 for-2.7 00/14] qapi: Allow blockdev-add for NBD

[Eric Blake]

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

On 05/03/2016 09:23 AM, Kevin Wolf wrote:
> Am 06.04.2016 um 20:28 hat Max Reitz geschrieben:
>> Turns out NBD is not so simple to do if you do it right. Anyway, this
>> series adds blockdev-add support for NBD clients.
> 
> What the series does seems to make sense to me, though things would be a
> bit nicer (especially on the command line) if SocketAddress had been a
> flat union from the beginning.  It's too late to change now, and I guess
> the ugliness isn't worth duplicating it.

I'm sorely tempted to make SocketAddress a flat union anyways (or even
an alternate that can be flat or nested at will), if only for
convenience.  But that's a pipe dream - there's probably not enough time
before 2.7 to actually achieve it.

> 
> I'll leave the in-detail review to our NBD folks.
> 
> Kevin
> 

-- 
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 v3 06/14] block/nbd: Add nbd_has_filename_options_conflict()

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> 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 as to test each option whether its key has an "address."
> prefix. Therefore, we will then need to iterate through all options.
> 
> 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.
> 
> 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 d12bcc6..1736f68 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -120,6 +120,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")

I know there are already instances of breaking before || in this file,
but most of qemu breaks after, as in:

if (!strcmp(e->key, "host") ||
    !strcmp(e->key, "port") ||
...


But choice of formatting is trivial, so:
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 v3 07/14] block/nbd: "address" in nbd_refresh_filename()

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> As of a future patch, the NBD block driver will accept a SocketAddress
> structure for a new "address" option. In order to support this,
> nbd_refresh_filename() needs some changes.
> 
> The two TODOs introduced by this patch will be removed in the very next
> one. They exist to explain that it is currently impossible for
> nbd_refresh_filename() to emit an "address.*" option (which the NBD
> block driver does not handle yet). The next patch will arm these code
> paths, but it will also enable handling of these options.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 61 insertions(+), 23 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 1736f68..3adf302 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -441,37 +441,75 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
>  static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>  {
>      QDict *opts = qdict_new();
> -    const char *path   = qdict_get_try_str(options, "path");
> -    const char *host   = qdict_get_try_str(options, "host");
> -    const char *port   = qdict_get_try_str(options, "port");
> +    bool can_generate_filename = true;
> +    const char *path = NULL, *host = NULL, *port = NULL;
>      const char *export = qdict_get_try_str(options, "export");
>      const char *tlscreds = qdict_get_try_str(options, "tls-creds");
>  
> -    if (host && !port) {
> -        port = stringify(NBD_DEFAULT_PORT);
> +    if (qdict_get_try_str(options, "address.type")) {
> +        /* This path will only be possible as of a future patch;
> +         * TODO: Remove this note once it is */
> +
> +        const char *type = qdict_get_str(options, "address.type");
> +

Oh, I'm sooooo tempted to teach the QAPI generator how to make a
discriminated union have a default 'type' value (thus making the
discriminator optional), so that we don't need a layer of nesting behind
'address.'.

> +        if (!strcmp(type, "unix")) {
> +            path = qdict_get_str(options, "address.data.path");
> +        } else if (!strcmp(type, "inet")) {
> +            host = qdict_get_str(options, "address.data.host");
> +            port = qdict_get_str(options, "address.data.port");

It's especially annoying that because SocketAddress is not flat, we have
to expose the 'data.' layer of nesting, even if we could avoid the
'address.' layer.

> +
> +            can_generate_filename = !qdict_haskey(options, "address.data.to")
> +                                 && !qdict_haskey(options, "address.data.ipv4")
> +                                 && !qdict_haskey(options, "address.data.ipv6");
> +        } else {
> +            can_generate_filename = false;
> +        }
> +    } else {
> +        path = qdict_get_try_str(options, "path");
> +        host = qdict_get_try_str(options, "host");
> +        port = qdict_get_try_str(options, "port");
> +
> +        if (host && !port) {
> +            port = stringify(NBD_DEFAULT_PORT);
> +        }
>      }

Looks clean given the constraints of what you are able to use from QAPI.

> +
> +    if (qdict_get_try_str(options, "address.type")) {
> +        /* This path will only be possible as of a future patch;
> +         * TODO: Remove this note once it is */
> +
> +        const QDictEntry *e;
> +        for (e = qdict_first(options); e; e = qdict_next(options, e)) {
> +            if (!strncmp(e->key, "address.", 8)) {
> +                qobject_incref(e->value);
> +                qdict_put_obj(opts, e->key, e->value);
> +            }
> +        }

This part makes me wonder if we want Dan's qdict_crumple() working first.

>      } else {
> -        qdict_put(opts, "host", qstring_from_str(host));
> -        qdict_put(opts, "port", qstring_from_str(port));
> +        if (path) {
> +            qdict_put(opts, "path", qstring_from_str(path));
> +        } else {
> +            qdict_put(opts, "host", qstring_from_str(host));
> +            qdict_put(opts, "port", qstring_from_str(port));
> +        }
>      }
>      if (export) {
>          qdict_put(opts, "export", qstring_from_str(export));
> 

At this point, I'll reserve giving R-b until I've seen the whole series
(it may need rebasing anyways...)

-- 
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 v3 08/14] block/nbd: Accept SocketAddress

[Eric Blake]

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

On 04/06/2016 12:28 PM, Max Reitz wrote:
> Add a new option "address" 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.

The back-compat work is pretty slick.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c                   | 97 ++++++++++++++++++++++++++-----------------
>  tests/qemu-iotests/051.out    |  4 +-
>  tests/qemu-iotests/051.pc.out |  4 +-
>  3 files changed, 64 insertions(+), 41 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 3adf302..9ae66ba 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -32,6 +32,8 @@
>  #include "qemu/uri.h"
>  #include "block/block_int.h"
>  #include "qemu/module.h"
> +#include "qapi-visit.h"
> +#include "qapi/qmp-input-visitor.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qjson.h"
>  #include "qapi/qmp/qint.h"
> @@ -128,7 +130,9 @@ 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")
> +            || !strcmp(e->key, "address")
> +            || !strncmp(e->key, "address.", 8))

May need a tweak if you break before '||'

>          {
>              error_setg(errp, "Option '%s' cannot be used with a file name",
>                         e->key);
> @@ -202,46 +206,66 @@ out:
>      g_free(file);
>  }
>  
> +static bool nbd_process_legacy_socket_options(QDict *options, Error **errp)
> +{
> +    if (qdict_haskey(options, "path") && qdict_haskey(options, "host")) {
> +        error_setg(errp, "path and host may not be used at the same time");
> +        return false;
> +    } else if (qdict_haskey(options, "path")) {
> +        if (qdict_haskey(options, "port")) {
> +            error_setg(errp, "port may not be used without host");
> +            return false;
> +        }
> +
> +        qdict_put(options, "address.type", qstring_from_str("unix"));
> +        qdict_change_key(options, "path", "address.data.path");
> +    } else if (qdict_haskey(options, "host")) {
> +        qdict_put(options, "address.type", qstring_from_str("inet"));
> +        qdict_change_key(options, "host", "address.data.host");
> +        if (!qdict_change_key(options, "port", "address.data.port")) {
> +            qdict_put(options, "address.data.port",
> +                      qstring_from_str(stringify(NBD_DEFAULT_PORT)));
> +        }

The rewrite from old to modern is rather nice.  I wonder if we could
then use a generated qapi_visit_SocketAddress instead of manually
handling all the complication of SocketAddress proper.

> +    }
> +
> +    return true;
> +}
> +
>  static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
>                                   Error **errp)
>  {
> -    SocketAddress *saddr;
> +    SocketAddress *saddr = NULL;
> +    QDict *addr = NULL;
> +    QObject *crumpled_addr;
> +    QmpInputVisitor *iv;
> +    Error *local_err = NULL;
>  
> -    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
> -        if (qdict_haskey(options, "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");
> -        }
> -        return NULL;
> +    if (!nbd_process_legacy_socket_options(options, errp)) {
> +        goto fail;
>      }
> -    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
> -        error_setg(errp, "port may not be used without host");
> -        return NULL;
> +
> +    qdict_extract_subqdict(options, &addr, "address.");
> +    if (!qdict_size(addr)) {
> +        error_setg(errp, "NBD server address missing");
> +        goto fail;
>      }
>  
> -    saddr = g_new0(SocketAddress, 1);
> +    crumpled_addr = qdict_crumple(addr, true, errp);
> +    if (!crumpled_addr) {
> +        goto fail;
> +    }
>  

Ah, so you ARE depending on Dan's qdict_crumple().

> -    if (qdict_haskey(options, "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(qdict_get_str(options, "path"));
> -        qdict_del(options, "path");
> -    } else {
> -        InetSocketAddress *inet;
> -        saddr->type = SOCKET_ADDRESS_KIND_INET;
> -        inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
> -        inet->host = g_strdup(qdict_get_str(options, "host"));
> -        if (!qdict_get_try_str(options, "port")) {
> -            inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
> -        } else {
> -            inet->port = g_strdup(qdict_get_str(options, "port"));
> -        }
> -        qdict_del(options, "host");
> -        qdict_del(options, "port");
> +    iv = qmp_input_visitor_new(crumpled_addr);
> +    visit_type_SocketAddress(qmp_input_get_visitor(iv), NULL, &saddr,
> +                             &local_err);

and you DO use the generated QAPI code.  Cool!

> +    qmp_input_visitor_cleanup(iv);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto fail;
>      }
>  
> +    /* TODO: Detect superfluous (unused) options under in addr */

Is this TODO addressed anywhere, or mentioned in the commit message?

In fact, I think it's quite easy to address: qmp_input_visitor_new()
recently changed signatures (commit fc471c18), so all you have to do is
pass strict=true to that function as part of rebasing your work, at
which point you will automatically reject any leftover keys in addr that
didn't get parsed by SocketAddress.

Needs a rebase, but looks promising.

-- 
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 v3 08/14] block/nbd: Accept SocketAddress

[Max Reitz]

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

On 15.06.2016 01:14, Eric Blake wrote:
> On 04/06/2016 12:28 PM, Max Reitz wrote:
>> Add a new option "address" 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.
> 
> The back-compat work is pretty slick.
> 
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/nbd.c                   | 97 ++++++++++++++++++++++++++-----------------
>>  tests/qemu-iotests/051.out    |  4 +-
>>  tests/qemu-iotests/051.pc.out |  4 +-
>>  3 files changed, 64 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 3adf302..9ae66ba 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -32,6 +32,8 @@
>>  #include "qemu/uri.h"
>>  #include "block/block_int.h"
>>  #include "qemu/module.h"
>> +#include "qapi-visit.h"
>> +#include "qapi/qmp-input-visitor.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/qjson.h"
>>  #include "qapi/qmp/qint.h"
>> @@ -128,7 +130,9 @@ 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")
>> +            || !strcmp(e->key, "address")
>> +            || !strncmp(e->key, "address.", 8))
> 
> May need a tweak if you break before '||'

Will do. (Same for patch 6.)

> 
>>          {
>>              error_setg(errp, "Option '%s' cannot be used with a file name",
>>                         e->key);
>> @@ -202,46 +206,66 @@ out:
>>      g_free(file);
>>  }
>>  
>> +static bool nbd_process_legacy_socket_options(QDict *options, Error **errp)
>> +{
>> +    if (qdict_haskey(options, "path") && qdict_haskey(options, "host")) {
>> +        error_setg(errp, "path and host may not be used at the same time");
>> +        return false;
>> +    } else if (qdict_haskey(options, "path")) {
>> +        if (qdict_haskey(options, "port")) {
>> +            error_setg(errp, "port may not be used without host");
>> +            return false;
>> +        }
>> +
>> +        qdict_put(options, "address.type", qstring_from_str("unix"));
>> +        qdict_change_key(options, "path", "address.data.path");
>> +    } else if (qdict_haskey(options, "host")) {
>> +        qdict_put(options, "address.type", qstring_from_str("inet"));
>> +        qdict_change_key(options, "host", "address.data.host");
>> +        if (!qdict_change_key(options, "port", "address.data.port")) {
>> +            qdict_put(options, "address.data.port",
>> +                      qstring_from_str(stringify(NBD_DEFAULT_PORT)));
>> +        }
> 
> The rewrite from old to modern is rather nice.  I wonder if we could
> then use a generated qapi_visit_SocketAddress instead of manually
> handling all the complication of SocketAddress proper.
> 
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>  static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
>>                                   Error **errp)
>>  {
>> -    SocketAddress *saddr;
>> +    SocketAddress *saddr = NULL;
>> +    QDict *addr = NULL;
>> +    QObject *crumpled_addr;
>> +    QmpInputVisitor *iv;
>> +    Error *local_err = NULL;
>>  
>> -    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
>> -        if (qdict_haskey(options, "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");
>> -        }
>> -        return NULL;
>> +    if (!nbd_process_legacy_socket_options(options, errp)) {
>> +        goto fail;
>>      }
>> -    if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
>> -        error_setg(errp, "port may not be used without host");
>> -        return NULL;
>> +
>> +    qdict_extract_subqdict(options, &addr, "address.");
>> +    if (!qdict_size(addr)) {
>> +        error_setg(errp, "NBD server address missing");
>> +        goto fail;
>>      }
>>  
>> -    saddr = g_new0(SocketAddress, 1);
>> +    crumpled_addr = qdict_crumple(addr, true, errp);
>> +    if (!crumpled_addr) {
>> +        goto fail;
>> +    }
>>  
> 
> Ah, so you ARE depending on Dan's qdict_crumple().

In v2, I was still using my own version (qdict_unflatten()), written
specifically for this. Since both fulfill the same purpose and I got to
review qdict_crumple() before anyone got to review qdict_unflatten(), I
dropped the qdict_unflatten() patch from v3; also, "crumple" is more
imaginative than "unflatten", so it has to be better. :-)

>> -    if (qdict_haskey(options, "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(qdict_get_str(options, "path"));
>> -        qdict_del(options, "path");
>> -    } else {
>> -        InetSocketAddress *inet;
>> -        saddr->type = SOCKET_ADDRESS_KIND_INET;
>> -        inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1);
>> -        inet->host = g_strdup(qdict_get_str(options, "host"));
>> -        if (!qdict_get_try_str(options, "port")) {
>> -            inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
>> -        } else {
>> -            inet->port = g_strdup(qdict_get_str(options, "port"));
>> -        }
>> -        qdict_del(options, "host");
>> -        qdict_del(options, "port");
>> +    iv = qmp_input_visitor_new(crumpled_addr);
>> +    visit_type_SocketAddress(qmp_input_get_visitor(iv), NULL, &saddr,
>> +                             &local_err);
> 
> and you DO use the generated QAPI code.  Cool!
> 
>> +    qmp_input_visitor_cleanup(iv);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        goto fail;
>>      }
>>  
>> +    /* TODO: Detect superfluous (unused) options under in addr */
> 
> Is this TODO addressed anywhere, or mentioned in the commit message?

Not yet (also, it should be either of "under" and "in" in the comment,
but not both).

> In fact, I think it's quite easy to address: qmp_input_visitor_new()
> recently changed signatures (commit fc471c18), so all you have to do is
> pass strict=true to that function as part of rebasing your work, at
> which point you will automatically reject any leftover keys in addr that
> didn't get parsed by SocketAddress.

Sounds good, will look into it!

> Needs a rebase, but looks promising.

Will do, thanks for reviewing!

Max


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