[Qemu-devel] [PATCH v2 00/16] qapi: Allow blockdev-add for NBD

[Qemu-devel] [PATCH v2 00/16] 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.

Patches 1 and 2 add one less and one more complicated QDict function,
respectively, which I needed in later NBD patches: Patch 1 for handling
legacy options (move "host" to "address.data.host" etc.) and patch 2
because I'd like to use the input visitor for transforming the NBD
options into a SocketAddress. Unfortunately, the block layer uses
flattened QDicts everywhere, so we'll have to unflatten (erect?) them
before we can use that visitor.

Patch 3 adds a test for qdict_unflatten().

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

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

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

Patches 13, 14, and 15 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 16 then adds the iotest for NBD's blockdev-add interface.


Note on the relation to v1: As you can see, most of this series is
completely new. Patch 5 was patch 1 in v1, and the only thing that has
changed is that I removed the full stop at the end of the error message;
consequently I kept Eric's R-b.


Max Reitz (16):
  qdict: Add qdict_change_key()
  qdict: Add qdict_unflatten()
  check-qdict: Add a test for qdict_unflatten()
  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                            | 231 +++++++++++++++++++---------
 include/qapi/qmp/qdict.h               |   2 +
 qapi/block-core.json                   |  23 ++-
 qobject/qdict.c                        | 212 ++++++++++++++++++++++++++
 tests/check-qdict.c                    | 267 +++++++++++++++++++++++++++++++++
 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 ++--
 12 files changed, 898 insertions(+), 96 deletions(-)
 create mode 100755 tests/qemu-iotests/147
 create mode 100644 tests/qemu-iotests/147.out

-- 
2.7.1

[Qemu-devel] [PATCH v2 01/16] 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 71b8eb0..223f746 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 9833bd0..bbfe39f 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -168,6 +168,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.7.1

[Qemu-devel] [PATCH v2 02/16] qdict: Add qdict_unflatten()

[Max Reitz]

The QMP input visitor is rather unhappy with flattened QDicts, which is
how they are generally used in the block layer. This function allows
unflattening a QDict so we can use an input visitor on it.

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

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 223f746..0ec7477 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -70,6 +70,7 @@ void qdict_set_default_str(QDict *dst, const char *key, const char *val);
 
 QDict *qdict_clone_shallow(const QDict *src);
 void qdict_flatten(QDict *qdict);
+bool qdict_unflatten(QDict *qdict, Error **errp);
 
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
diff --git a/qobject/qdict.c b/qobject/qdict.c
index bbfe39f..800af38 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -771,6 +771,195 @@ int qdict_array_entries(QDict *src, const char *subqdict)
 }
 
 /**
+ * qlist_unflatten(): Recursive helper function for qdict_unflatten(). Invokes
+ * qdict_unflatten() and qlist_unflatten() on all its QDict and QList members,
+ * respectively.
+ */
+static bool qlist_unflatten(QList *qlist, Error **errp)
+{
+    const QListEntry *entry;
+
+    for (entry = qlist_first(qlist); entry; entry = qlist_next(entry)) {
+        switch (qobject_type(entry->value)) {
+        case QTYPE_QDICT:
+            if (!qdict_unflatten(qobject_to_qdict(entry->value), errp)) {
+                return false;
+            }
+            break;
+
+        case QTYPE_QLIST:
+            if (!qlist_unflatten(qobject_to_qlist(entry->value), errp)) {
+                return false;
+            }
+            break;
+
+        default:
+            break;
+        }
+    }
+
+    return true;
+}
+
+/**
+ * qdict_unflatten(): The opposite of qdict_flatten().
+ *
+ * Every entry whose key is of the form "${prefix}.${index}" is moved to index
+ * "${index}" in a QList whose key in @qdict is "${prefix}", if
+ * qdict_array_entries(qdict, "${prefix}.") yields a positive value.
+ *
+ * Every entry whose key is of the form "${prefix}.${index}.${trailing}" is
+ * moved into a QDict at index "${index}" in a QList whose key in @qdict is
+ * "${prefix}". The moved object's key in the nested QDict is "${trailing}".
+ * This is only done if qdict_array_entries(qdict, "${prefix}.") yields a
+ * positive value.
+ *
+ * Every remaining entry whose key is of the form "${prefix}.${trailing}" is
+ * moved into a QDict whose key in @qdict is "${prefix}". The moved object's key
+ * in the nested QDict is "${trailing}".
+ *
+ * This algorithm then recurses on all QDict members (including indirect ones
+ * in QLists) of this QDict.
+ *
+ * This function will never overwrite existing members. For instance:
+ *   qdict_unflatten({ "x": 42, "x.y": 23 })
+ * is an error because there already is an "x" element which is not a QDict.
+ * However,
+ *   qdict_unflatten({ "x": { "a": 0 }, "x.y": 23 })
+ *   => { "x": { "a": 0, "y": 23 } }
+ * because the flattened "x.y" can be merged into the existing "x" QDict without
+ * overwriting any of its members. In contrast to that,
+ *   qdict_unflatten({ "x": { "y": 0 }, "x.y": 23 })
+ * is an error because "y" nested in "x" would need to be overwritten.
+ *
+ * This function returns true on success and false on error (in which case *errp
+ * is set). On error, the contents of @qdict are undefined.
+ */
+bool qdict_unflatten(QDict *qdict, Error **errp)
+{
+    const QDictEntry *entry;
+
+    /* First pass: Unflatten this level */
+    entry = qdict_first(qdict);
+    while (entry) {
+        const char *prefix_end = strchr(entry->key, '.');
+
+        if (prefix_end) {
+            size_t prefix_length = prefix_end - entry->key;
+            char *prefix, *prefix_dot;
+
+            prefix = g_malloc(prefix_length + 1);
+            strncpy(prefix, entry->key, prefix_length);
+            prefix[prefix_length] = 0;
+
+            prefix_dot = g_strdup_printf("%s.", prefix);
+
+            if (qdict_array_entries(qdict, prefix_dot) > 0) {
+                /* Move all entries with this prefix into a nested QList */
+                QDict *array_qdict;
+                QList *target_qlist;
+
+                /* We cannot merge two non-empty lists without one overwriting
+                 * members of the other */
+                if (qdict_haskey(qdict, prefix)) {
+                    if (qobject_type(qdict_get(qdict, prefix)) != QTYPE_QLIST ||
+                        !qlist_empty(qdict_get_qlist(qdict, prefix)))
+                    {
+                        error_setg(errp, "Cannot unflatten list '%s': Overlaps "
+                                   "with existing member", prefix);
+                        g_free(prefix);
+                        g_free(prefix_dot);
+                        return false;
+                    }
+
+                    /* Remove the existing empty list so we can replace it */
+                    qdict_del(qdict, prefix);
+                }
+
+                qdict_extract_subqdict(qdict, &array_qdict, prefix_dot);
+                qdict_array_split(array_qdict, &target_qlist);
+                assert(!qdict_size(array_qdict));
+                QDECREF(array_qdict);
+                qdict_put(qdict, prefix, target_qlist);
+            } else {
+                /* Move all entries with this prefix into a nested QDict */
+                QDict *target_qdict, *tmp_qdict;
+
+                if (qdict_haskey(qdict, prefix)) {
+                    if (qobject_type(qdict_get(qdict, prefix)) != QTYPE_QDICT) {
+                        error_setg(errp, "Cannot unflatten dict '%s': Overlaps "
+                                   "with non-dict member", prefix);
+                        g_free(prefix);
+                        g_free(prefix_dot);
+                        return false;
+                    }
+
+                    /* If there already is a QDict with this prefix, try to
+                     * merge the unflattened members into it */
+                    target_qdict = qdict_get_qdict(qdict, prefix);
+                } else {
+                    /* Otherwise created a new one */
+                    target_qdict = qdict_new();
+                    qdict_put(qdict, prefix, target_qdict);
+                }
+
+                qdict_extract_subqdict(qdict, &tmp_qdict, prefix_dot);
+                qdict_join(target_qdict, tmp_qdict, false);
+
+                if (qdict_size(tmp_qdict)) {
+                    error_setg(errp, "Flattened member '%s.%s' exists in "
+                               "previously existing unflattened dict",
+                               prefix, qdict_first(tmp_qdict)->key);
+                    QDECREF(tmp_qdict);
+                    g_free(prefix);
+                    g_free(prefix_dot);
+                    return false;
+                }
+
+                QDECREF(tmp_qdict);
+            }
+
+            g_free(prefix);
+            g_free(prefix_dot);
+
+            /* The QDict has changed, so we need to reiterate. This will not
+             * result in an infinite loop because every time we get here, one
+             * entry whose key contains a '.' has been removed, and in its place
+             * at most one entry whose key does not contain a '.' has been
+             * inserted. Therefore, the number of entries with '.' in their key
+             * decreases and will eventually reach 0, at which point we cannot
+             * get here anymore. */
+            entry = qdict_first(qdict);
+            continue;
+        }
+
+        entry = qdict_next(qdict, entry);
+    }
+
+    /* Second pass: Recurse to nested QDicts and QLists */
+    for (entry = qdict_first(qdict); entry; entry = qdict_next(qdict, entry)) {
+        switch (qobject_type(entry->value)) {
+        case QTYPE_QDICT:
+            if (!qdict_unflatten(qobject_to_qdict(entry->value), errp)) {
+                return false;
+            }
+            break;
+
+        case QTYPE_QLIST:
+            if (!qlist_unflatten(qobject_to_qlist(entry->value), errp)) {
+                return false;
+            }
+            break;
+
+        default:
+            break;
+        }
+    }
+
+    return true;
+}
+
+/**
  * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
  * elements from src to dest.
  *
-- 
2.7.1

[Qemu-devel] [PATCH v2 03/16] check-qdict: Add a test for qdict_unflatten()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/check-qdict.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 267 insertions(+)

diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index a43056c..f6a5cda 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -325,6 +325,272 @@ static void qdict_flatten_test(void)
     QDECREF(dict3);
 }
 
+static void qdict_unflatten_test(void)
+{
+    QDict *dict;
+    QList *list_a, *list_o;
+    QDict *dict_a1, *dict_d, *dict_df, *dict_dh, *dict_i, *dict_l;
+    const QListEntry *le;
+
+    /*
+     * Test the unflattening of
+     *
+     * {
+     *     "a.0": 0,
+     *     "a.1.b": 1,
+     *     "a.1.c": 2,
+     *     "a.2": 3,
+     *     "d.e": 4,
+     *     "d.f.g": 5,
+     *     "d.h.0": 6,
+     *     "d.h.2": 7,
+     *     "i.0": 8,
+     *     "i.j": 9,
+     *     "k": 10,
+     *     "l": {
+     *         "m": 11
+     *     },
+     *     "l.n": 12,
+     *     "o": [],
+     *     "o.0": 13
+     * }
+     *
+     * to
+     *
+     * {
+     *     "a": [
+     *         0,
+     *         {
+     *             "b": 1,
+     *             "c": 2
+     *         },
+     *         3
+     *     ],
+     *     "d": {
+     *         "e": 4,
+     *         "f": {
+     *             "g": 5
+     *         },
+     *         "h": {
+     *             "0": 6,
+     *             "2": 7
+     *         },
+     *     },
+     *     "i": {
+     *         "0": 8,
+     *         "j": 9
+     *     },
+     *     "k": 10,
+     *     "l": {
+     *         "m": 11,
+     *         "n": 12
+     *     },
+     *     "o": [
+     *         13
+     *     ]
+     * }
+     *
+     * This tests:
+     * - Unflattening in general
+     * - Conversion of "x.0", "x.2" into a dict instead of a list
+     * - Conversion of "x.0", "x.y" into a dict instead of a list
+     * - Merging of previously existing and new unflattened dicts
+     *   ({ "x": { "y": 0 }, "x.z": 1 } => { "x": { "y": 0, "z": 1 } })
+     * - Merging of previously existing and new unflattened lists; only works
+     *   if the previous list was empty
+     *   ({ "x": [], "x.0": 0 } => { "x": [ 0 ] })
+     */
+
+    dict = qdict_new();
+
+    qdict_put(dict, "a.0",   qint_from_int( 0));
+    qdict_put(dict, "a.1.b", qint_from_int( 1));
+    qdict_put(dict, "a.1.c", qint_from_int( 2));
+    qdict_put(dict, "a.2",   qint_from_int( 3));
+    qdict_put(dict, "d.e",   qint_from_int( 4));
+    qdict_put(dict, "d.f.g", qint_from_int( 5));
+    qdict_put(dict, "d.h.0", qint_from_int( 6));
+    qdict_put(dict, "d.h.2", qint_from_int( 7));
+    qdict_put(dict, "i.0",   qint_from_int( 8));
+    qdict_put(dict, "i.j",   qint_from_int( 9));
+    qdict_put(dict, "k",     qint_from_int(10));
+    qdict_put(dict, "l", qdict_new());
+    qdict_put(qdict_get_qdict(dict, "l"), "m", qint_from_int(11));
+    qdict_put(dict, "l.n",   qint_from_int(12));
+    qdict_put(dict, "o", qlist_new());
+    qdict_put(dict, "o.0",   qint_from_int(13));
+
+    qdict_unflatten(dict, &error_abort);
+
+    list_a = qdict_get_qlist(dict, "a");
+    g_assert(list_a);
+
+    /* a.0 */
+    le = qlist_first(list_a);
+    g_assert(le);
+    g_assert(qint_get_int(qobject_to_qint(le->value)) == 0);
+    /* a.1 */
+    le = qlist_next(le);
+    g_assert(le);
+    dict_a1 = qobject_to_qdict(le->value);
+    g_assert(dict_a1);
+    g_assert(qdict_get_int(dict_a1, "b") == 1);
+    g_assert(qdict_get_int(dict_a1, "c") == 2);
+    g_assert(qdict_size(dict_a1) == 2);
+    /* a.2 */
+    le = qlist_next(le);
+    g_assert(le);
+    g_assert(qint_get_int(qobject_to_qint(le->value)) == 3);
+
+    g_assert(!qlist_next(le));
+
+    dict_d = qdict_get_qdict(dict, "d");
+    g_assert(dict_d);
+    g_assert(qdict_get_int(dict_d, "e") == 4);
+
+    dict_df = qdict_get_qdict(dict_d, "f");
+    g_assert(dict_df);
+    g_assert(qdict_get_int(dict_df, "g") == 5);
+    g_assert(qdict_size(dict_df) == 1);
+
+    dict_dh = qdict_get_qdict(dict_d, "h");
+    g_assert(dict_dh);
+    g_assert(qdict_get_int(dict_dh, "0") == 6);
+    g_assert(qdict_get_int(dict_dh, "2") == 7);
+    g_assert(qdict_size(dict_dh) == 2);
+
+    g_assert(qdict_size(dict_d) == 3);
+
+    dict_i = qdict_get_qdict(dict, "i");
+    g_assert(dict_i);
+    g_assert(qdict_get_int(dict_i, "0") == 8);
+    g_assert(qdict_get_int(dict_i, "j") == 9);
+    g_assert(qdict_size(dict_i) == 2);
+
+    g_assert(qdict_get_int(dict, "k") == 10);
+
+    dict_l = qdict_get_qdict(dict, "l");
+    g_assert(dict_l);
+    g_assert(qdict_get_int(dict_l, "m") == 11);
+    g_assert(qdict_get_int(dict_l, "n") == 12);
+    g_assert(qdict_size(dict_l) == 2);
+
+    list_o = qdict_get_qlist(dict, "o");
+    g_assert(list_o);
+
+    /* o.0 */
+    le = qlist_first(list_o);
+    g_assert(le);
+    g_assert(qint_get_int(qobject_to_qint(le->value)) == 13);
+
+    g_assert(!qlist_next(le));
+
+    g_assert(qdict_size(dict) == 6);
+
+    QDECREF(dict);
+
+
+    /*
+     * Test that unflattening
+     *
+     * {
+     *     "a": 0,
+     *     "a.b": 1
+     * }
+     *
+     * fails.
+     *
+     * (Cannot create new QDict "a" because "a" is already a QString)
+     */
+
+    dict = qdict_new();
+
+    qdict_put(dict, "a", qint_from_int(0));
+    qdict_put(dict, "a.b", qint_from_int(1));
+
+    g_assert(!qdict_unflatten(dict, NULL));
+
+    QDECREF(dict);
+
+
+    /*
+     * Test that unflattening
+     *
+     * {
+     *     "a": [
+     *         0
+     *     ],
+     *     "a.0": 1
+     * }
+     *
+     * fails.
+     *
+     * (Cannot replace list member "a.0")
+     */
+
+    dict = qdict_new();
+
+    qdict_put(dict, "a", qlist_new());
+    qlist_append(qdict_get_qlist(dict, "a"), qint_from_int(0));
+    qdict_put(dict, "a.0", qint_from_int(1));
+
+    g_assert(!qdict_unflatten(dict, NULL));
+
+    QDECREF(dict);
+
+
+    /*
+     * Test that unflattening
+     *
+     * {
+     *     "a": {
+     *         "b": 0
+     *     },
+     *     "a.b": 1
+     * }
+     *
+     * fails.
+     *
+     * (Cannot replace existing nested dict member "a.b")
+     */
+
+    dict = qdict_new();
+
+    qdict_put(dict, "a", qdict_new());
+    qdict_put(qdict_get_qdict(dict, "a"), "b", qint_from_int(0));
+    qdict_put(dict, "a.b", qint_from_int(1));
+
+    g_assert(!qdict_unflatten(dict, NULL));
+
+    QDECREF(dict);
+
+
+    /*
+     * Test that unflattening
+     *
+     * {
+     *     "a": {
+     *         "b": 0
+     *     },
+     *     "a.0": 1
+     * }
+     *
+     * fails.
+     *
+     * (Cannot replace existing dict "a" by a list)
+     */
+
+    dict = qdict_new();
+
+    qdict_put(dict, "a", qdict_new());
+    qdict_put(qdict_get_qdict(dict, "a"), "b", qint_from_int(0));
+    qdict_put(dict, "a.0", qint_from_int(1));
+
+    g_assert(!qdict_unflatten(dict, NULL));
+
+    QDECREF(dict);
+}
+
 static void qdict_array_split_test(void)
 {
     QDict *test_dict = qdict_new();
@@ -736,6 +1002,7 @@ int main(int argc, char **argv)
     g_test_add_func("/public/to_qdict", qobject_to_qdict_test);
     g_test_add_func("/public/iterapi", qdict_iterapi_test);
     g_test_add_func("/public/flatten", qdict_flatten_test);
+    g_test_add_func("/public/unflatten", qdict_unflatten_test);
     g_test_add_func("/public/array_split", qdict_array_split_test);
     g_test_add_func("/public/array_entries", qdict_array_entries_test);
     g_test_add_func("/public/join", qdict_join_test);
-- 
2.7.1

[Qemu-devel] [PATCH v2 04/16] 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 db57b49..ce31119 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -194,9 +194,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 0f8a8d3..5166bea 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 85fc05d..d0bbfcb 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.7.1

[Qemu-devel] [PATCH v2 05/16] 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 ce31119..6a2fc27 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -200,6 +200,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.7.1

[Qemu-devel] [PATCH v2 06/16] 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 6a2fc27..8d9a217 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -411,6 +411,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) {
@@ -419,27 +423,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.7.1

[Qemu-devel] [PATCH v2 07/16] 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 8d9a217..145db39 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -415,7 +415,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),
@@ -432,16 +432,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.7.1

[Qemu-devel] [PATCH v2 08/16] 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 145db39..2d96dd1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -119,6 +119,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)
 {
@@ -127,12 +146,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.7.1

[Qemu-devel] [PATCH v2 09/16] 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 | 80 +++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 21 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 2d96dd1..86907bc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -419,37 +419,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 (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 (path) {
-        qdict_put(opts, "path", qstring_from_str(path));
+    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.7.1

[Qemu-devel] [PATCH v2 10/16] 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                   | 93 +++++++++++++++++++++++++++----------------
 tests/qemu-iotests/051.out    |  4 +-
 tests/qemu-iotests/051.pc.out |  4 +-
 3 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 86907bc..213ba70 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -31,6 +31,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"
@@ -127,7 +129,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);
@@ -201,44 +205,64 @@ 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;
+    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_unflatten(addr, errp)) {
+        goto fail;
     }
 
-    saddr = g_new0(SocketAddress, 1);
+    if (!qdict_size(addr)) {
+        error_setg(errp, "NBD server address missing");
+        goto fail;
+    }
 
-    if (qdict_haskey(options, "path")) {
-        saddr->type = SOCKET_ADDRESS_KIND_UNIX;
-        saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
-        saddr->u.q_unix->path = g_strdup(qdict_get_str(options, "path"));
-        qdict_del(options, "path");
-    } else {
-        saddr->type = SOCKET_ADDRESS_KIND_INET;
-        saddr->u.inet = g_new0(InetSocketAddress, 1);
-        saddr->u.inet->host = g_strdup(qdict_get_str(options, "host"));
-        if (!qdict_get_try_str(options, "port")) {
-            saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
-        } else {
-            saddr->u.inet->port = g_strdup(qdict_get_str(options, "port"));
-        }
-        qdict_del(options, "host");
-        qdict_del(options, "port");
+    iv = qmp_input_visitor_new(QOBJECT(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"));
@@ -246,7 +270,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)
@@ -425,9 +454,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")) {
@@ -471,9 +497,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 5166bea..82c1694 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 d0bbfcb..45137b7 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.7.1

[Qemu-devel] [PATCH v2 11/16] 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 213ba70..471395a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -88,9 +88,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;
@@ -105,12 +109,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:
@@ -187,7 +191,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;
 
@@ -196,8 +201,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);
     }
 
@@ -506,10 +512,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.7.1

[Qemu-devel] [PATCH v2 12/16] 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 9bf1b22..21760e0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1633,13 +1633,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', 'null-aio', 'null-co', 'parallels',
+            'http', 'https', 'nbd', 'null-aio', 'null-co', 'parallels',
             'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
             'vmdk', 'vpc', 'vvfat' ] }
 
@@ -2068,6 +2069,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.
@@ -2093,7 +2112,7 @@
       'http':       'BlockdevOptionsFile',
       'https':      'BlockdevOptionsFile',
 # TODO iscsi: Wait for structured options
-# TODO nbd: Should take InetSocketAddress for 'host'?
+      'nbd':        'BlockdevOptionsNbd',
 # TODO nfs: Wait for structured options
       'null-aio':   'BlockdevOptionsNull',
       'null-co':    'BlockdevOptionsNull',
-- 
2.7.1

[Qemu-devel] [PATCH v2 13/16] 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 0a238ec..dd8805a 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']
 
 # This will not work if arguments contain spaces but is necessary if we
@@ -41,6 +41,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(' ')
@@ -86,6 +90,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.7.1

[Qemu-devel] [PATCH v2 14/16] 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 dd8805a..fed5301 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -130,10 +130,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.7.1

[Qemu-devel] [PATCH v2 15/16] 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.7.1

[Qemu-devel] [PATCH v2 16/16] 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 47fd40c..28d2f20 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -148,3 +148,4 @@
 143 auto quick
 144 rw auto quick
 145 auto quick
+147 auto
-- 
2.7.1

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

[Eric Blake]

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

On 02/29/2016 04:19 PM, 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.
> 
> Patches 1 and 2 add one less and one more complicated QDict function,
> respectively, which I needed in later NBD patches: Patch 1 for handling
> legacy options (move "host" to "address.data.host" etc.) and patch 2
> because I'd like to use the input visitor for transforming the NBD
> options into a SocketAddress. Unfortunately, the block layer uses
> flattened QDicts everywhere, so we'll have to unflatten (erect?) them
> before we can use that visitor.

Dan had a patch proposal that called the operation "crumple"; I need to
review both proposals and see which one I like.
https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04618.html

> 
> Patch 3 adds a test for qdict_unflatten().
> 
> Patches 4, 5, 6, and 7 are minor patches with no functional relation to
> this series, other than later patches will touch the code they touch,
> too.
> 
> Patches 8 and 9 prepare the code for the addition of a new option
> prefix, which is "address.".
> 
> Patch 10 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 11 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 12, the goal of this series, is again not very complicated.
> 
> Patches 13, 14, and 15 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 16 then adds the iotest for NBD's blockdev-add interface.
> 
> 
> Note on the relation to v1: As you can see, most of this series is
> completely new. Patch 5 was patch 1 in v1, and the only thing that has
> changed is that I removed the full stop at the end of the error message;
> consequently I kept Eric's R-b.

Looks like my review queue is getting longer because I (like several
other people) are trying to post last-minute series before soft freeze.

-- 
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 v2 00/16] qapi: Allow blockdev-add for NBD

[Max Reitz]

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

On 01.03.2016 00:24, Eric Blake wrote:
> On 02/29/2016 04:19 PM, 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.
>>
>> Patches 1 and 2 add one less and one more complicated QDict function,
>> respectively, which I needed in later NBD patches: Patch 1 for handling
>> legacy options (move "host" to "address.data.host" etc.) and patch 2
>> because I'd like to use the input visitor for transforming the NBD
>> options into a SocketAddress. Unfortunately, the block layer uses
>> flattened QDicts everywhere, so we'll have to unflatten (erect?) them
>> before we can use that visitor.
> 
> Dan had a patch proposal that called the operation "crumple"; I need to
> review both proposals and see which one I like.
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04618.html

Well, here I go again, not looking at patches on the list...

Looking at the design, I like his idea of having an escape sequence;
also, his qdict_crumple() can return boths lists and dicts where my
qdict_unflatten() only returns dicts (then again, this is what
qdict_flatten() always works on). And his patch doesn't suffer from as
much indentation as mine does.

What I like more about my patch, however, is that I'm reusing
qdict_array_split() and qdict_array_entries(). That is mostly because my
function modifies the given QDict, where Dan's does not.

>>
>> Patch 3 adds a test for qdict_unflatten().
>>
>> Patches 4, 5, 6, and 7 are minor patches with no functional relation to
>> this series, other than later patches will touch the code they touch,
>> too.
>>
>> Patches 8 and 9 prepare the code for the addition of a new option
>> prefix, which is "address.".
>>
>> Patch 10 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 11 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 12, the goal of this series, is again not very complicated.
>>
>> Patches 13, 14, and 15 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 16 then adds the iotest for NBD's blockdev-add interface.
>>
>>
>> Note on the relation to v1: As you can see, most of this series is
>> completely new. Patch 5 was patch 1 in v1, and the only thing that has
>> changed is that I removed the full stop at the end of the error message;
>> consequently I kept Eric's R-b.
> 
> Looks like my review queue is getting longer because I (like several
> other people) are trying to post last-minute series before soft freeze.

So is mine. :-)

Yes, during the last week I had to prioritize patches over reviewing.

Max


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

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

[Daniel P. Berrange]

On Tue, Mar 01, 2016 at 12:37:14AM +0100, Max Reitz wrote:
> On 01.03.2016 00:24, Eric Blake wrote:
> > On 02/29/2016 04:19 PM, 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.
> >>
> >> Patches 1 and 2 add one less and one more complicated QDict function,
> >> respectively, which I needed in later NBD patches: Patch 1 for handling
> >> legacy options (move "host" to "address.data.host" etc.) and patch 2
> >> because I'd like to use the input visitor for transforming the NBD
> >> options into a SocketAddress. Unfortunately, the block layer uses
> >> flattened QDicts everywhere, so we'll have to unflatten (erect?) them
> >> before we can use that visitor.
> > 
> > Dan had a patch proposal that called the operation "crumple"; I need to
> > review both proposals and see which one I like.
> > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04618.html
> 
> Well, here I go again, not looking at patches on the list...
> 
> Looking at the design, I like his idea of having an escape sequence;
> also, his qdict_crumple() can return boths lists and dicts where my
> qdict_unflatten() only returns dicts (then again, this is what
> qdict_flatten() always works on). And his patch doesn't suffer from as
> much indentation as mine does.

The escape sequence is critical to support for my use case, because
sadly some object properties have '.' in their name :-(

> What I like more about my patch, however, is that I'm reusing
> qdict_array_split() and qdict_array_entries(). That is mostly because my
> function modifies the given QDict, where Dan's does not.

The reason for that is that the use context in which I need to call
qdict_crumple() has a const QDict, so modifying the original QDict
was not an option.

Second, for error handling, if there is a problem we can't resolve
half way through the unflattening process, then if you're modifying
the original QDict you end up with a QDict that is a hybrid between
the flat & unflat forms. I think it is pretty bad practice for API
design / behaviour to leave inputs in such a state on error. ie if
the code isn't capable of rolling back to the original state it
should not be modifying the input arg.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Kevin Wolf]

Am 01.03.2016 um 11:00 hat Daniel P. Berrange geschrieben:
> On Tue, Mar 01, 2016 at 12:37:14AM +0100, Max Reitz wrote:
> > On 01.03.2016 00:24, Eric Blake wrote:
> > > On 02/29/2016 04:19 PM, 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.
> > >>
> > >> Patches 1 and 2 add one less and one more complicated QDict function,
> > >> respectively, which I needed in later NBD patches: Patch 1 for handling
> > >> legacy options (move "host" to "address.data.host" etc.) and patch 2
> > >> because I'd like to use the input visitor for transforming the NBD
> > >> options into a SocketAddress. Unfortunately, the block layer uses
> > >> flattened QDicts everywhere, so we'll have to unflatten (erect?) them
> > >> before we can use that visitor.
> > > 
> > > Dan had a patch proposal that called the operation "crumple"; I need to
> > > review both proposals and see which one I like.
> > > https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04618.html
> > 
> > Well, here I go again, not looking at patches on the list...
> > 
> > Looking at the design, I like his idea of having an escape sequence;
> > also, his qdict_crumple() can return boths lists and dicts where my
> > qdict_unflatten() only returns dicts (then again, this is what
> > qdict_flatten() always works on). And his patch doesn't suffer from as
> > much indentation as mine does.
> 
> The escape sequence is critical to support for my use case, because
> sadly some object properties have '.' in their name :-(
> 
> > What I like more about my patch, however, is that I'm reusing
> > qdict_array_split() and qdict_array_entries(). That is mostly because my
> > function modifies the given QDict, where Dan's does not.
> 
> The reason for that is that the use context in which I need to call
> qdict_crumple() has a const QDict, so modifying the original QDict
> was not an option.

You can always clone and modify if modifying an existing QDict turns out
to be nicer to implement.

> Second, for error handling, if there is a problem we can't resolve
> half way through the unflattening process, then if you're modifying
> the original QDict you end up with a QDict that is a hybrid between
> the flat & unflat forms. I think it is pretty bad practice for API
> design / behaviour to leave inputs in such a state on error. ie if
> the code isn't capable of rolling back to the original state it
> should not be modifying the input arg.

I think we generally abort the whole action in such error cases. Then
it doesn't really matter in what state the to be freed QDict is.

Kevin