[Qemu-devel] [PATCH v2 0/6] nbd export qmp interface

[Qemu-devel] [PATCH v2 0/6] nbd export qmp interface

[Vladimir Sementsov-Ogievskiy]

Here:
    1. separate export name from device name
    1.1 several exports per device possible
    2. add nbd-server-remove

v2:
01: add r-bs by Max and Eric, add comment to code (hope you don't mind)
03: address export by its name, not by device name
    make it possible to force-remove export, which is already
    non-force-removed (thourh new "hidden" field)
other patches are new

v1:
Add command to remove nbd export, pair to nbd-server-add.
The whole thing and description are in patch 02.


Vladimir Sementsov-Ogievskiy (6):
  nbd/server: add additional assert to nbd_export_put
  qapi: add name parameter to nbd-server-add
  qapi: add nbd-server-remove
  iotest 147: add cases to test new @name parameter of nbd-server-add
  iotests: implement QemuIoInteractive class
  iotest 201: new test for qmp nbd-server-remove

 qapi/block.json               |  27 ++++++++-
 include/block/nbd.h           |   3 +-
 blockdev-nbd.c                |  41 +++++++++++--
 hmp.c                         |   5 +-
 nbd/server.c                  |  31 +++++++++-
 tests/qemu-iotests/147        |  68 +++++++++++++++++-----
 tests/qemu-iotests/147.out    |   4 +-
 tests/qemu-iotests/201        | 130 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/201.out    |   5 ++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  38 ++++++++++++
 11 files changed, 325 insertions(+), 28 deletions(-)
 create mode 100644 tests/qemu-iotests/201
 create mode 100644 tests/qemu-iotests/201.out

-- 
2.11.1

[Qemu-devel] [PATCH v2 1/6] nbd/server: add additional assert to nbd_export_put

[Vladimir Sementsov-Ogievskiy]

This place is not obvious, nbd_export_close may theoretically reduce
refcount to 0. It may happen if someone calls nbd_export_put on named
export not through nbd_export_set_name when refcount is 1.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 nbd/server.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 92c0fdd03b..e817c48087 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1190,6 +1190,12 @@ void nbd_export_put(NBDExport *exp)
         nbd_export_close(exp);
     }
 
+    /* nbd_export_close() may theoretically reduce refcount to 0. It may happen
+     * if someone calls nbd_export_put() on named export not through
+     * nbd_export_set_name() when refcount is 1. So, let's assert that
+     * it is > 0.
+     */
+    assert(exp->refcount > 0);
     if (--exp->refcount == 0) {
         assert(exp->name == NULL);
         assert(exp->description == NULL);
-- 
2.11.1

[Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Vladimir Sementsov-Ogievskiy]

Allow user to specify name for new export, to not reuse internal
node name and to not show it to clients.

This also allows creating several exports per device.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json |  9 +++++++--
 blockdev-nbd.c  | 14 +++++++++-----
 hmp.c           |  5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index f093fa3f27..503d4b287b 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -213,14 +213,19 @@
 #
 # @device: The device name or node name of the node to be exported
 #
+# @name: Export name. If unspecified @device parameter used as export name.
+#        (Since 2.12)
+#
 # @writable: Whether clients should be able to write to the device via the
 #     NBD connection (default false).
 #
-# Returns: error if the device is already marked for export.
+# Returns: error if the server is not running, or export with the same name
+#          already exists.
 #
 # Since: 1.3.0
 ##
-{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
+{ 'command': 'nbd-server-add',
+  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
 
 ##
 # @nbd-server-stop:
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 28f551a7b0..46c885aa35 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -158,8 +158,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
     qapi_free_SocketAddress(addr_flat);
 }
 
-void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
-                        Error **errp)
+void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
+                        bool has_writable, bool writable, Error **errp)
 {
     BlockDriverState *bs = NULL;
     BlockBackend *on_eject_blk;
@@ -170,8 +170,12 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
-    if (nbd_export_find(device)) {
-        error_setg(errp, "NBD server already exporting device '%s'", device);
+    if (!has_name) {
+        name = device;
+    }
+
+    if (nbd_export_find(name)) {
+        error_setg(errp, "NBD server already has export named '%s'", name);
         return;
     }
 
@@ -195,7 +199,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         return;
     }
 
-    nbd_export_set_name(exp, device);
+    nbd_export_set_name(exp, name);
 
     /* The list of named exports has a strong reference to this export now and
      * our only way of accessing it is through nbd_export_find(), so we can drop
diff --git a/hmp.c b/hmp.c
index 35a7041824..0ea9c09b58 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2203,7 +2203,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
             continue;
         }
 
-        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
+        qmp_nbd_server_add(info->value->device, false, NULL,
+                           true, writable, &local_err);
 
         if (local_err != NULL) {
             qmp_nbd_server_stop(NULL);
@@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;
 
-    qmp_nbd_server_add(device, true, writable, &local_err);
+    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
 
     if (local_err != NULL) {
         hmp_handle_error(mon, &local_err);
-- 
2.11.1

[Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

Add command for export removing. It is needed for cases when we
don't want to keep export after the operation on it was completed.
The other example is temporary node, created with blockdev-add.
If we want to delete it we should firstly remove corresponding
NBD export.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block.json     | 18 ++++++++++++++++++
 include/block/nbd.h |  3 ++-
 blockdev-nbd.c      | 29 ++++++++++++++++++++++++++++-
 nbd/server.c        | 25 ++++++++++++++++++++++---
 4 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 503d4b287b..f83485c325 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -228,6 +228,24 @@
   'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
 
 ##
+# @nbd-server-remove:
+#
+# Remove NBD export by name.
+#
+# @name: Export name.
+#
+# @force: Whether active connections to the export should be closed. If this
+#         parameter is false the export only becomes hidden from clients (new
+#         connections are impossible), and would be automatically freed after
+#         all clients are disconnected (default false).
+#
+# Returns: error if the server is not running or export is not found.
+#
+# Since: 2.12
+##
+{ 'command': 'nbd-server-remove', 'data': {'name': 'str', '*force': 'bool'} }
+
+##
 # @nbd-server-stop:
 #
 # Stop QEMU's embedded NBD server, and unregister all devices previously
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 113c707a5e..b89d116597 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -261,12 +261,13 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
 void nbd_export_close(NBDExport *exp);
+void nbd_export_hide(NBDExport *exp);
 void nbd_export_get(NBDExport *exp);
 void nbd_export_put(NBDExport *exp);
 
 BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
 
-NBDExport *nbd_export_find(const char *name);
+NBDExport *nbd_export_find(const char *name, bool include_hidden);
 void nbd_export_set_name(NBDExport *exp, const char *name);
 void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 46c885aa35..2495d38f2c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -174,7 +174,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         name = device;
     }
 
-    if (nbd_export_find(name)) {
+    if (nbd_export_find(name, true)) {
         error_setg(errp, "NBD server already has export named '%s'", name);
         return;
     }
@@ -207,6 +207,33 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
     nbd_export_put(exp);
 }
 
+void qmp_nbd_server_remove(const char *name, bool has_force, bool force,
+                           Error **errp)
+{
+    NBDExport *exp;
+
+    if (!nbd_server) {
+        error_setg(errp, "NBD server not running");
+        return;
+    }
+
+    exp = nbd_export_find(name, true);
+    if (exp == NULL) {
+        error_setg(errp, "Export '%s' is not found", name);
+        return;
+    }
+
+    if (!has_force) {
+        force = false;
+    }
+
+    if (force) {
+        nbd_export_close(exp);
+    } else {
+        nbd_export_hide(exp);
+    }
+}
+
 void qmp_nbd_server_stop(Error **errp)
 {
     nbd_export_close_all();
diff --git a/nbd/server.c b/nbd/server.c
index e817c48087..d85f2dc747 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -78,6 +78,10 @@ struct NBDExport {
 
     BlockBackend *eject_notifier_blk;
     Notifier eject_notifier;
+
+    /* hidden export is not available for new connections and should be removed
+     * after last client disconnect */
+    bool hidden;
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -300,7 +304,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length,
 
     trace_nbd_negotiate_handle_export_name_request(name);
 
-    client->exp = nbd_export_find(name);
+    client->exp = nbd_export_find(name, false);
     if (!client->exp) {
         error_setg(errp, "export not found");
         return -EINVAL;
@@ -429,7 +433,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,
     }
     assert(length == 0);
 
-    exp = nbd_export_find(name);
+    exp = nbd_export_find(name, false);
     if (!exp) {
         return nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_UNKNOWN,
                                           opt, errp, "export '%s' not present",
@@ -966,6 +970,9 @@ void nbd_client_put(NBDClient *client)
         if (client->exp) {
             QTAILQ_REMOVE(&client->exp->clients, client, next);
             nbd_export_put(client->exp);
+            if (client->exp->hidden && QTAILQ_EMPTY(&client->exp->clients)) {
+                nbd_export_close(client->exp);
+            }
         }
         g_free(client);
     }
@@ -1125,10 +1132,14 @@ fail:
     return NULL;
 }
 
-NBDExport *nbd_export_find(const char *name)
+NBDExport *nbd_export_find(const char *name, bool include_hidden)
 {
     NBDExport *exp;
     QTAILQ_FOREACH(exp, &exports, next) {
+        if (!include_hidden && exp->hidden) {
+            continue;
+        }
+
         if (strcmp(name, exp->name) == 0) {
             return exp;
         }
@@ -1177,6 +1188,14 @@ void nbd_export_close(NBDExport *exp)
     nbd_export_put(exp);
 }
 
+void nbd_export_hide(NBDExport *exp)
+{
+    exp->hidden = true;
+    if (QTAILQ_EMPTY(&exp->clients)) {
+        nbd_export_close(exp);
+    }
+}
+
 void nbd_export_get(NBDExport *exp)
 {
     assert(exp->refcount > 0);
-- 
2.11.1

[Qemu-devel] [PATCH v2 4/6] iotest 147: add cases to test new @name parameter of nbd-server-add

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/147     | 68 +++++++++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/147.out |  4 +--
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 90f40ed245..d2081df84b 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -38,8 +38,8 @@ def flatten_sock_addr(crumpled_address):
 
 
 class NBDBlockdevAddBase(iotests.QMPTestCase):
-    def blockdev_add_options(self, address, export=None):
-        options = { 'node-name': 'nbd-blockdev',
+    def blockdev_add_options(self, address, export, node_name):
+        options = { 'node-name': node_name,
                     'driver': 'raw',
                     'file': {
                         'driver': 'nbd',
@@ -50,23 +50,28 @@ class NBDBlockdevAddBase(iotests.QMPTestCase):
             options['file']['export'] = export
         return options
 
-    def client_test(self, filename, address, export=None):
-        bao = self.blockdev_add_options(address, export)
+    def client_test(self, filename, address, export=None,
+                    node_name='nbd-blockdev', delete=True):
+        bao = self.blockdev_add_options(address, export, node_name)
         result = self.vm.qmp('blockdev-add', **bao)
         self.assert_qmp(result, 'return', {})
 
+        found = False
         result = self.vm.qmp('query-named-block-nodes')
         for node in result['return']:
-            if node['node-name'] == 'nbd-blockdev':
+            if node['node-name'] == node_name:
+                found = True
                 if isinstance(filename, str):
                     self.assert_qmp(node, 'image/filename', filename)
                 else:
                     self.assert_json_filename_equal(node['image']['filename'],
                                                     filename)
                 break
+        self.assertTrue(found)
 
-        result = self.vm.qmp('blockdev-del', node_name='nbd-blockdev')
-        self.assert_qmp(result, 'return', {})
+        if delete:
+            result = self.vm.qmp('blockdev-del', node_name=node_name)
+            self.assert_qmp(result, 'return', {})
 
 
 class QemuNBD(NBDBlockdevAddBase):
@@ -125,26 +130,63 @@ class BuiltinNBD(NBDBlockdevAddBase):
         except OSError:
             pass
 
-    def _server_up(self, address):
+    def _server_up(self, address, export_name=None, export_name2=None):
         result = self.server.qmp('nbd-server-start', addr=address)
         self.assert_qmp(result, 'return', {})
 
-        result = self.server.qmp('nbd-server-add', device='nbd-export')
+        if export_name is None:
+            result = self.server.qmp('nbd-server-add', device='nbd-export')
+        else:
+            result = self.server.qmp('nbd-server-add', device='nbd-export',
+                                     name=export_name)
         self.assert_qmp(result, 'return', {})
 
+        if export_name2 is not None:
+            result = self.server.qmp('nbd-server-add', device='nbd-export',
+                                     name=export_name2)
+            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):
+    def do_test_inet(self, export_name=None):
         address = { 'type': 'inet',
                     'data': {
                         'host': 'localhost',
                         'port': str(NBD_PORT)
                     } }
-        self._server_up(address)
-        self.client_test('nbd://localhost:%i/nbd-export' % NBD_PORT,
-                         flatten_sock_addr(address), 'nbd-export')
+        self._server_up(address, export_name)
+        export_name = export_name or 'nbd-export'
+        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, export_name),
+                         flatten_sock_addr(address), export_name)
+        self._server_down()
+
+    def test_inet_default_export_name(self):
+        self.do_test_inet()
+
+    def test_inet_same_export_name(self):
+        self.do_test_inet('nbd-export')
+
+    def test_inet_different_export_name(self):
+        self.do_test_inet('shadow')
+
+    def test_inet_two_exports(self):
+        address = { 'type': 'inet',
+                    'data': {
+                        'host': 'localhost',
+                        'port': str(NBD_PORT)
+                    } }
+        self._server_up(address, 'exp1', 'exp2')
+        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp1'),
+                         flatten_sock_addr(address), 'exp1', 'node1', False)
+        self.client_test('nbd://localhost:%i/%s' % (NBD_PORT, 'exp2'),
+                         flatten_sock_addr(address), 'exp2', 'node2', False)
+        result = self.vm.qmp('blockdev-del', node_name='node1')
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp('blockdev-del', node_name='node2')
+        self.assert_qmp(result, 'return', {})
         self._server_down()
 
     def test_inet6(self):
diff --git a/tests/qemu-iotests/147.out b/tests/qemu-iotests/147.out
index 3f8a935a08..dae404e278 100644
--- a/tests/qemu-iotests/147.out
+++ b/tests/qemu-iotests/147.out
@@ -1,5 +1,5 @@
-......
+.........
 ----------------------------------------------------------------------
-Ran 6 tests
+Ran 9 tests
 
 OK
-- 
2.11.1

[Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive class

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6f057904a9..a9f312db83 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -93,6 +93,44 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
+
+class QemuIoInteractive:
+    def __init__(self, *args):
+        self.args = qemu_io_args + list(args)
+        self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
+                                   stdout=subprocess.PIPE,
+                                   stderr=subprocess.STDOUT)
+        assert self._p.stdout.read(9) == 'qemu-io> '
+
+    def close(self):
+        self._p.communicate('q\n')
+
+    def _read_output(self):
+        pattern = 'qemu-io> '
+        n = len(pattern)
+        pos = 0
+        s = []
+        while pos != n:
+            c = self._p.stdout.read(1)
+            #check unexpected EOF
+            assert c != ''
+            s.append(c)
+            if c == pattern[pos]:
+                pos += 1
+            else:
+                pos = 0
+
+        return ''.join(s[:-n])
+
+    def cmd(self, cmd):
+        # quit command is in close(), '\n' is added automatically
+        assert '\n' not in cmd
+        cmd = cmd.strip()
+        assert cmd != 'q' and cmd != 'quit'
+        self._p.stdin.write(cmd + '\n')
+        return self._read_output()
+
+
 def qemu_nbd(*args):
     '''Run qemu-nbd in daemon mode and return the parent's exit code'''
     return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
-- 
2.11.1

[Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/201     | 130 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/201.out |   5 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 136 insertions(+)
 create mode 100644 tests/qemu-iotests/201
 create mode 100644 tests/qemu-iotests/201.out

diff --git a/tests/qemu-iotests/201 b/tests/qemu-iotests/201
new file mode 100644
index 0000000000..28d0324195
--- /dev/null
+++ b/tests/qemu-iotests/201
@@ -0,0 +1,130 @@
+#!/usr/bin/env python
+#
+# Tests for qmp command nbd-server-remove.
+#
+# Copyright (c) 2017 Virtuozzo International GmbH
+#
+# 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 sys
+import iotests
+import time
+from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive
+
+nbd_port = '10900'
+nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp'
+disk = os.path.join(iotests.test_dir, 'disk')
+
+class TestNbdServerRemove(iotests.QMPTestCase):
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
+
+        self.vm = iotests.VM().add_drive(disk)
+        self.vm.launch()
+
+        address = {
+            'type': 'inet',
+            'data': {
+                'host': 'localhost',
+                'port': nbd_port
+            }
+        }
+
+        result = self.vm.qmp('nbd-server-start', addr=address)
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp('nbd-server-add', device='drive0', name='exp')
+        self.assert_qmp(result, 'return', {})
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(disk)
+
+    def remove_export(self, name, force=None):
+        if force is None:
+            result = self.vm.qmp('nbd-server-remove', name=name)
+        else:
+            result = self.vm.qmp('nbd-server-remove', name=name, force=force)
+        self.assert_qmp(result, 'return', {})
+
+    def assertExportNotFound(self, name):
+        result = self.vm.qmp('nbd-server-remove', name=name)
+        self.assert_qmp(result, 'error/desc', "Export 'exp' is not found")
+
+    def assertReadOk(self, qemu_io_output):
+        self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
+                'read 512/512 bytes at offset 0\n' +
+                '512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)')
+
+    def assertReadFailed(self, qemu_io_output):
+        self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
+                         'read failed: Input/output error')
+
+    def assertConnectFailed(self, qemu_io_output):
+        self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
+                         "can't open device nbd+tcp://localhost:10900/exp: " +
+                         "Requested export not available\nserver reported: " +
+                         "export 'exp' not present")
+
+    def do_test_connect_after_remove(self, force=None):
+        args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)
+        self.assertReadOk(qemu_io(*args))
+        self.remove_export('exp', force)
+        self.assertExportNotFound('exp')
+        self.assertConnectFailed(qemu_io(*args))
+
+    def test_connect_after_remove_default(self):
+        self.do_test_connect_after_remove()
+
+    def test_connect_after_remove_safe(self):
+        self.do_test_connect_after_remove(False)
+
+    def test_connect_after_remove_force(self):
+        self.do_test_connect_after_remove(True)
+
+    def do_test_remove_during_connect(self, force=None):
+        qio = QemuIoInteractive('-r', '-f', 'raw', nbd_uri)
+        self.assertReadOk(qio.cmd('read 0 512'))
+        self.remove_export('exp', force)
+        if force:
+            self.assertReadFailed(qio.cmd('read 0 512'))
+            self.assertExportNotFound('exp')
+        else:
+            self.assertReadOk(qio.cmd('read 0 512'))
+        qio.close()
+        self.assertExportNotFound('exp')
+
+    def test_remove_during_connect_default(self):
+        self.do_test_remove_during_connect()
+
+    def test_remove_during_connect_safe(self):
+        self.do_test_remove_during_connect(False)
+
+    def test_remove_during_connect_force(self):
+        self.do_test_remove_during_connect(True)
+
+    def test_remove_during_connect_safe_force(self):
+        qio = QemuIoInteractive('-r', '-f', 'raw', nbd_uri)
+        self.assertReadOk(qio.cmd('read 0 512'))
+        self.remove_export('exp', False)
+        self.assertReadOk(qio.cmd('read 0 512'))
+        self.remove_export('exp', True)
+        self.assertExportNotFound('exp')
+        self.assertReadFailed(qio.cmd('read 0 512'))
+        qio.close()
+
+
+if __name__ == '__main__':
+    iotests.main()
diff --git a/tests/qemu-iotests/201.out b/tests/qemu-iotests/201.out
new file mode 100644
index 0000000000..2f7d3902f2
--- /dev/null
+++ b/tests/qemu-iotests/201.out
@@ -0,0 +1,5 @@
+.......
+----------------------------------------------------------------------
+Ran 7 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3e688678dd..15df7678b3 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -197,3 +197,4 @@
 197 rw auto quick
 198 rw auto
 200 rw auto
+201 rw auto quick
-- 
2.11.1

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> Allow user to specify name for new export, to not reuse internal
> node name and to not show it to clients.
> 
> This also allows creating several exports per device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json |  9 +++++++--
>  blockdev-nbd.c  | 14 +++++++++-----
>  hmp.c           |  5 +++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..503d4b287b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -213,14 +213,19 @@
>  #
>  # @device: The device name or node name of the node to be exported
>  #
> +# @name: Export name. If unspecified @device parameter used as export name.
> +#        (Since 2.12)
> +#
>  # @writable: Whether clients should be able to write to the device via the
>  #     NBD connection (default false).
>  #
> -# Returns: error if the device is already marked for export.
> +# Returns: error if the server is not running, or export with the same name
> +#          already exists.
>  #
>  # Since: 1.3.0
>  ##
> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
> +{ 'command': 'nbd-server-add',
> +  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>  
>  ##
>  # @nbd-server-stop:
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 28f551a7b0..46c885aa35 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -158,8 +158,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
>      qapi_free_SocketAddress(addr_flat);
>  }
>  
> -void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
> -                        Error **errp)
> +void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
> +                        bool has_writable, bool writable, Error **errp)
>  {
>      BlockDriverState *bs = NULL;
>      BlockBackend *on_eject_blk;
> @@ -170,8 +170,12 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>          return;
>      }
>  
> -    if (nbd_export_find(device)) {
> -        error_setg(errp, "NBD server already exporting device '%s'", device);
> +    if (!has_name) {
> +        name = device;
> +    }
> +
> +    if (nbd_export_find(name)) {
> +        error_setg(errp, "NBD server already has export named '%s'", name);
>          return;
>      }
>  
> @@ -195,7 +199,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>          return;
>      }
>  
> -    nbd_export_set_name(exp, device);
> +    nbd_export_set_name(exp, name);
>  
>      /* The list of named exports has a strong reference to this export now and
>       * our only way of accessing it is through nbd_export_find(), so we can drop
> diff --git a/hmp.c b/hmp.c
> index 35a7041824..0ea9c09b58 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2203,7 +2203,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>              continue;
>          }
>  
> -        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
> +        qmp_nbd_server_add(info->value->device, false, NULL,
> +                           true, writable, &local_err);
>  
>          if (local_err != NULL) {
>              qmp_nbd_server_stop(NULL);
> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>      bool writable = qdict_get_try_bool(qdict, "writable", false);
>      Error *local_err = NULL;
>  
> -    qmp_nbd_server_add(device, true, writable, &local_err);
> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);

I wont insist, but it would be nice if you wired up an optional
parameter on HMP as well.

Dave

>      if (local_err != NULL) {
>          hmp_handle_error(mon, &local_err);
> -- 
> 2.11.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Vladimir Sementsov-Ogievskiy]

08.12.2017 20:33, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> Allow user to specify name for new export, to not reuse internal
>> node name and to not show it to clients.
>>
>> This also allows creating several exports per device.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block.json |  9 +++++++--
>>   blockdev-nbd.c  | 14 +++++++++-----
>>   hmp.c           |  5 +++--
>>   3 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index f093fa3f27..503d4b287b 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -213,14 +213,19 @@
>>   #
>>   # @device: The device name or node name of the node to be exported
>>   #
>> +# @name: Export name. If unspecified @device parameter used as export name.
>> +#        (Since 2.12)
>> +#
>>   # @writable: Whether clients should be able to write to the device via the
>>   #     NBD connection (default false).
>>   #
>> -# Returns: error if the device is already marked for export.
>> +# Returns: error if the server is not running, or export with the same name
>> +#          already exists.
>>   #
>>   # Since: 1.3.0
>>   ##
>> -{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
>> +{ 'command': 'nbd-server-add',
>> +  'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>>   
>>   ##
>>   # @nbd-server-stop:
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 28f551a7b0..46c885aa35 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -158,8 +158,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
>>       qapi_free_SocketAddress(addr_flat);
>>   }
>>   
>> -void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>> -                        Error **errp)
>> +void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>> +                        bool has_writable, bool writable, Error **errp)
>>   {
>>       BlockDriverState *bs = NULL;
>>       BlockBackend *on_eject_blk;
>> @@ -170,8 +170,12 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>>           return;
>>       }
>>   
>> -    if (nbd_export_find(device)) {
>> -        error_setg(errp, "NBD server already exporting device '%s'", device);
>> +    if (!has_name) {
>> +        name = device;
>> +    }
>> +
>> +    if (nbd_export_find(name)) {
>> +        error_setg(errp, "NBD server already has export named '%s'", name);
>>           return;
>>       }
>>   
>> @@ -195,7 +199,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>>           return;
>>       }
>>   
>> -    nbd_export_set_name(exp, device);
>> +    nbd_export_set_name(exp, name);
>>   
>>       /* The list of named exports has a strong reference to this export now and
>>        * our only way of accessing it is through nbd_export_find(), so we can drop
>> diff --git a/hmp.c b/hmp.c
>> index 35a7041824..0ea9c09b58 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -2203,7 +2203,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>>               continue;
>>           }
>>   
>> -        qmp_nbd_server_add(info->value->device, true, writable, &local_err);
>> +        qmp_nbd_server_add(info->value->device, false, NULL,
>> +                           true, writable, &local_err);
>>   
>>           if (local_err != NULL) {
>>               qmp_nbd_server_stop(NULL);
>> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>>       bool writable = qdict_get_try_bool(qdict, "writable", false);
>>       Error *local_err = NULL;
>>   
>> -    qmp_nbd_server_add(device, true, writable, &local_err);
>> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> I wont insist, but it would be nice if you wired up an optional
> parameter on HMP as well.
>
> Dave

Hmm.. Honestly, I don't see real usage of it.

>
>>       if (local_err != NULL) {
>>           hmp_handle_error(mon, &local_err);
>> -- 
>> 2.11.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


-- 
Best regards,
Vladimir

[Qemu-devel] ping Re: [PATCH v2 0/6] nbd export qmp interface

[Vladimir Sementsov-Ogievskiy]

Hi, what about this?

07.12.2017 18:50, Vladimir Sementsov-Ogievskiy wrote:
> Here:
>      1. separate export name from device name
>      1.1 several exports per device possible
>      2. add nbd-server-remove
>
> v2:
> 01: add r-bs by Max and Eric, add comment to code (hope you don't mind)
> 03: address export by its name, not by device name
>      make it possible to force-remove export, which is already
>      non-force-removed (thourh new "hidden" field)
> other patches are new
>
> v1:
> Add command to remove nbd export, pair to nbd-server-add.
> The whole thing and description are in patch 02.
>
>
> Vladimir Sementsov-Ogievskiy (6):
>    nbd/server: add additional assert to nbd_export_put
>    qapi: add name parameter to nbd-server-add
>    qapi: add nbd-server-remove
>    iotest 147: add cases to test new @name parameter of nbd-server-add
>    iotests: implement QemuIoInteractive class
>    iotest 201: new test for qmp nbd-server-remove
>
>   qapi/block.json               |  27 ++++++++-
>   include/block/nbd.h           |   3 +-
>   blockdev-nbd.c                |  41 +++++++++++--
>   hmp.c                         |   5 +-
>   nbd/server.c                  |  31 +++++++++-
>   tests/qemu-iotests/147        |  68 +++++++++++++++++-----
>   tests/qemu-iotests/147.out    |   4 +-
>   tests/qemu-iotests/201        | 130 ++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/201.out    |   5 ++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |  38 ++++++++++++
>   11 files changed, 325 insertions(+), 28 deletions(-)
>   create mode 100644 tests/qemu-iotests/201
>   create mode 100644 tests/qemu-iotests/201.out
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/6] nbd export qmp interface

[Markus Armbruster]

Looks like you forgot to cc: Eric.  Fixing...

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Here:
>     1. separate export name from device name
>     1.1 several exports per device possible
>     2. add nbd-server-remove
>
> v2:
> 01: add r-bs by Max and Eric, add comment to code (hope you don't mind)
> 03: address export by its name, not by device name
>     make it possible to force-remove export, which is already
>     non-force-removed (thourh new "hidden" field)
> other patches are new
>
> v1:
> Add command to remove nbd export, pair to nbd-server-add.
> The whole thing and description are in patch 02.
>
>
> Vladimir Sementsov-Ogievskiy (6):
>   nbd/server: add additional assert to nbd_export_put
>   qapi: add name parameter to nbd-server-add
>   qapi: add nbd-server-remove
>   iotest 147: add cases to test new @name parameter of nbd-server-add
>   iotests: implement QemuIoInteractive class
>   iotest 201: new test for qmp nbd-server-remove
>
>  qapi/block.json               |  27 ++++++++-
>  include/block/nbd.h           |   3 +-
>  blockdev-nbd.c                |  41 +++++++++++--
>  hmp.c                         |   5 +-
>  nbd/server.c                  |  31 +++++++++-
>  tests/qemu-iotests/147        |  68 +++++++++++++++++-----
>  tests/qemu-iotests/147.out    |   4 +-
>  tests/qemu-iotests/201        | 130 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/201.out    |   5 ++
>  tests/qemu-iotests/group      |   1 +
>  tests/qemu-iotests/iotests.py |  38 ++++++++++++
>  11 files changed, 325 insertions(+), 28 deletions(-)
>  create mode 100644 tests/qemu-iotests/201
>  create mode 100644 tests/qemu-iotests/201.out

Re: [Qemu-devel] [PATCH v2 0/6] nbd export qmp interface

[Eric Blake]

On 12/21/2017 09:28 AM, Markus Armbruster wrote:
> Looks like you forgot to cc: Eric.  Fixing...

Or that he did cc me, and the mailman bug ate the cc line because of my 
list subscription settings.  (Have I mentioned that I hate that mailman 
2 bug, but don't know whether mailman 3 will make it any better?)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 0/6] nbd export qmp interface

[Vladimir Sementsov-Ogievskiy]

21.12.2017 21:32, Eric Blake wrote:
> On 12/21/2017 09:28 AM, Markus Armbruster wrote:
>> Looks like you forgot to cc: Eric. Fixing...
>
> Or that he did cc me, and the mailman bug ate the cc line because of 
> my list subscription settings.  (Have I mentioned that I hate that 
> mailman 2 bug, but don't know whether mailman 3 will make it any better?)
>

Hmm, when did it eat it? I've send this with git send-email, and in my 
thunderbird I see (through view->source)

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: <qemu-devel@nongnu.org>, <qemu-block@nongnu.org>
CC: <armbru@redhat.com>, <dgilbert@redhat.com>, <pbonzini@redhat.com>,
	<eblake@redhat.com>, <mreitz@redhat.com>, <kwolf@redhat.com>,
	<vsementsov@virtuozzo.com>, <den@openvz.org>
Subject: [PATCH v2 0/6] nbd export qmp interface
Date: Thu, 7 Dec 2017 18:50:56 +0300
Message-ID: <20171207155102.66622-1-vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 0/6] nbd export qmp interface

[Eric Blake]

On 12/22/2017 02:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 21.12.2017 21:32, Eric Blake wrote:
>> On 12/21/2017 09:28 AM, Markus Armbruster wrote:
>>> Looks like you forgot to cc: Eric. Fixing...
>>
>> Or that he did cc me, and the mailman bug ate the cc line because of 
>> my list subscription settings.  (Have I mentioned that I hate that 
>> mailman 2 bug, but don't know whether mailman 3 will make it any better?)
>>
> 
> Hmm, when did it eat it? I've send this with git send-email, and in my 
> thunderbird I see (through view->source)

Mailman eats the cc line of anyone that sets the 'avoid duplicate emails 
to me' checkbox, but only for people that get the mail through mailman. 
If you get the email directly (as is true in this case), then you see 
the original cc's.  It's a highly annoying behavior of mailman 2; I wish 
there were two separate knobs (one for avoiding duplicates, one for 
controlling whether your name appears in cc), rather than one knob that 
does both things at once.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Eric Blake]

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

On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> Allow user to specify name for new export, to not reuse internal
> node name and to not show it to clients.
> 
> This also allows creating several exports per device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json |  9 +++++++--
>  blockdev-nbd.c  | 14 +++++++++-----
>  hmp.c           |  5 +++--
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index f093fa3f27..503d4b287b 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -213,14 +213,19 @@
>  #
>  # @device: The device name or node name of the node to be exported
>  #
> +# @name: Export name. If unspecified @device parameter used as export name.

s/unspecified/unspecified, the/
s/used as/is used as the/

Will tweak as part of applying to my NBD queue.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Eric Blake]

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

On 12/08/2017 11:33 AM, Dr. David Alan Gilbert wrote:

>> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>>      bool writable = qdict_get_try_bool(qdict, "writable", false);
>>      Error *local_err = NULL;
>>  
>> -    qmp_nbd_server_add(device, true, writable, &local_err);
>> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> 
> I wont insist, but it would be nice if you wired up an optional
> parameter on HMP as well.

Can be done as a followup patch; I'm not sure how many people are
setting up NBD exports via HMP, and I'm also okay with just stating that
the full power requires use of QMP.  But I'll give such a followup patch
a quick try, to see whether it is easy after all.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH v2 2.5/6] hmp: Add name parameter to nbd_server_add

[Eric Blake]

Extend the flexibility of the previous QMP patch to also work
in HMP.

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

In response to Dave's request.
I could also squash this into Vladimir's 2/6 if desired.

 hmp.c           | 3 ++-
 hmp-commands.hx | 9 +++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hmp.c b/hmp.c
index 80c5b01540..c9fcdc8593 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2221,10 +2221,11 @@ exit:
 void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_try_str(qdict, "name");
     bool writable = qdict_get_try_bool(qdict, "writable", false);
     Error *local_err = NULL;

-    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
+    qmp_nbd_server_add(device, !!name, name, true, writable, &local_err);

     if (local_err != NULL) {
         hmp_handle_error(mon, &local_err);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 6d5ebdf6ab..b8b6fb9184 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1553,17 +1553,18 @@ ETEXI

     {
         .name       = "nbd_server_add",
-        .args_type  = "writable:-w,device:B",
-        .params     = "nbd_server_add [-w] device",
+        .args_type  = "writable:-w,device:B,name:s?",
+        .params     = "nbd_server_add [-w] device [name]",
         .help       = "export a block device via NBD",
         .cmd        = hmp_nbd_server_add,
     },
 STEXI
-@item nbd_server_add @var{device}
+@item nbd_server_add @var{device} [ @var{name} ]
 @findex nbd_server_add
 Export a block device through QEMU's NBD server, which must be started
 beforehand with @command{nbd_server_start}.  The @option{-w} option makes the
-exported device writable too.
+exported device writable too.  The export name is controlled by @var{name},
+defaulting to @var{device}.
 ETEXI

     {
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

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

On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add command for export removing. It is needed for cases when we
> don't want to keep export after the operation on it was completed.
> The other example is temporary node, created with blockdev-add.
> If we want to delete it we should firstly remove corresponding
> NBD export.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block.json     | 18 ++++++++++++++++++
>  include/block/nbd.h |  3 ++-
>  blockdev-nbd.c      | 29 ++++++++++++++++++++++++++++-
>  nbd/server.c        | 25 ++++++++++++++++++++++---
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block.json b/qapi/block.json
> index 503d4b287b..f83485c325 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -228,6 +228,24 @@
>    'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>  
>  ##
> +# @nbd-server-remove:
> +#
> +# Remove NBD export by name.
> +#
> +# @name: Export name.
> +#
> +# @force: Whether active connections to the export should be closed. If this
> +#         parameter is false the export only becomes hidden from clients (new
> +#         connections are impossible), and would be automatically freed after
> +#         all clients are disconnected (default false).

Unstated, but if the parameter is true, existing clients are forcefully
disconnected, possibly losing pending transactions.

Do we want a third mode in the middle, where the server starts replying
to all existing clients with ESHUTDOWN errors for all new requests
rather than abruptly disconnecting (no new I/O, but no forced disconnect
and pending in-flight transactions can still complete gracefully)?

> +#
> +# Returns: error if the server is not running or export is not found.
> +#
> +# Since: 2.12
> +##
> +{ 'command': 'nbd-server-remove', 'data': {'name': 'str', '*force': 'bool'} }
> +

If we're okay with just the bool parameter, then this patch looks good;
but if we want a third mode, then we want '*force' to be an enum type.
So tentative:

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 4/6] iotest 147: add cases to test new @name parameter of nbd-server-add

[Eric Blake]

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

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/147     | 68 +++++++++++++++++++++++++++++++++++++---------
>  tests/qemu-iotests/147.out |  4 +--
>  2 files changed, 57 insertions(+), 15 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive class

[Eric Blake]

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

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:

The subject says what, but there is no commit body that says why.

Presumably this makes writing the test in the next patch easier, but
some details in the commit message would make this obvious.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/iotests.py | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 

My python is not strong; it looks good overall, although I have a few
questions that may warrant a v3 before I give R-b.

> +class QemuIoInteractive:
> +    def __init__(self, *args):
> +        self.args = qemu_io_args + list(args)
> +        self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
> +                                   stdout=subprocess.PIPE,
> +                                   stderr=subprocess.STDOUT)

Why STDOUT instead of STDERR?  Is the redirection intentional?


> +    def _read_output(self):
> +        pattern = 'qemu-io> '
> +        n = len(pattern)
> +        pos = 0
> +        s = []
> +        while pos != n:
> +            c = self._p.stdout.read(1)
> +            #check unexpected EOF

pep8 doesn't like this comment style (it wants space after #).  The file
is already unclean under pep8, but you shouldn't make it worse.

> +            assert c != ''

Is assert really the nicest control flow when early EOF is present? Or
is it because we are only using this in tests, where we don't expect
early EOF, so asserting is just fine for our usage?

> +            s.append(c)
> +            if c == pattern[pos]:
> +                pos += 1
> +            else:
> +                pos = 0
> +
> +        return ''.join(s[:-n])

Is it necessary to use join() on the empty string here, compared to just
returning the array slice directly?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Eric Blake]

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

On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/201     | 130 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/201.out |   5 ++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 136 insertions(+)
>  create mode 100644 tests/qemu-iotests/201
>  create mode 100644 tests/qemu-iotests/201.out
> 

pep8 complains:

$ pep8 tests/qemu-iotests/201
tests/qemu-iotests/201:31:1: E302 expected 2 blank lines, found 1
tests/qemu-iotests/201:68:17: E128 continuation line under-indented for
visual indent
tests/qemu-iotests/201:69:17: E128 continuation line under-indented for
visual indent

I don't mind cleaning those up (if you don't have to respin because of
my comments on 5/6).

> +import os
> +import sys
> +import iotests
> +import time
> +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive
> +
> +nbd_port = '10900'
> +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp'

Is this still going to be safe when we improve the testsuite to run
multiple tests in parallel?  Wouldn't it be safer to dynamically choose
the port, instead of hard-coding one?

> +disk = os.path.join(iotests.test_dir, 'disk')
> +
> +class TestNbdServerRemove(iotests.QMPTestCase):
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
> +
> +        self.vm = iotests.VM().add_drive(disk)
> +        self.vm.launch()
> +
> +        address = {
> +            'type': 'inet',
> +            'data': {
> +                'host': 'localhost',
> +                'port': nbd_port

Again, for a one-shot test, this works; but it doesn't lend itself to
parallel testing.  Maybe do a loop with incrementing port numbers until
one succeeds, if we can reliably detect when a port is already in use?

> +    def assertConnectFailed(self, qemu_io_output):
> +        self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
> +                         "can't open device nbd+tcp://localhost:10900/exp: " +

Worth parameterizing or filtering this assertion to match the rest of
the parameterization of nbd_port?

> +                         "Requested export not available\nserver reported: " +
> +                         "export 'exp' not present")
> +
> +    def do_test_connect_after_remove(self, force=None):
> +        args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)
> +        self.assertReadOk(qemu_io(*args))
> +        self.remove_export('exp', force)
> +        self.assertExportNotFound('exp')
> +        self.assertConnectFailed(qemu_io(*args))
> +
> +    def test_connect_after_remove_default(self):
> +        self.do_test_connect_after_remove()
> +
> +    def test_connect_after_remove_safe(self):
> +        self.do_test_connect_after_remove(False)
> +
> +    def test_connect_after_remove_force(self):
> +        self.do_test_connect_after_remove(True)

May need updates if my comments on 3/6 result in us having three states
rather than just 2 (the difference being whether there is a knob for
choosing to let existing clients gracefully disconnect with all pending
I/O completed, and/or failing the nbd-server-remove if a client is still
connected).

> +++ b/tests/qemu-iotests/201.out
> @@ -0,0 +1,5 @@
> +.......
> +----------------------------------------------------------------------
> +Ran 7 tests

I'm not a fan of python tests that are difficult to debug.  Your
additions to 147 in patch 4/6 are okay (hard to debug, but an
incremental addition); but is it possible to rewrite this test in a bit
more verbose manner?  See test 194 and this message for more details:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html

> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 3e688678dd..15df7678b3 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -197,3 +197,4 @@
>  197 rw auto quick
>  198 rw auto
>  200 rw auto
> +201 rw auto quick
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2/6] qapi: add name parameter to nbd-server-add

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 12/08/2017 11:33 AM, Dr. David Alan Gilbert wrote:
> 
> >> @@ -2223,7 +2224,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
> >>      bool writable = qdict_get_try_bool(qdict, "writable", false);
> >>      Error *local_err = NULL;
> >>  
> >> -    qmp_nbd_server_add(device, true, writable, &local_err);
> >> +    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> > 
> > I wont insist, but it would be nice if you wired up an optional
> > parameter on HMP as well.
> 
> Can be done as a followup patch

Yes, agreed.

> I'm not sure how many people are
> setting up NBD exports via HMP, and I'm also okay with just stating that
> the full power requires use of QMP.  But I'll give such a followup patch
> a quick try, to see whether it is easy after all.

Thanks; I have set up NBD exports up via HMP a few times.

Dave

> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 1/6] nbd/server: add additional assert to nbd_export_put

[Eric Blake]

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

On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> This place is not obvious, nbd_export_close may theoretically reduce
> refcount to 0. It may happen if someone calls nbd_export_put on named
> export not through nbd_export_set_name when refcount is 1.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  nbd/server.c | 6 ++++++
>  1 file changed, 6 insertions(+)

I'm going ahead and staging this one in my NBD queue now, while still
waiting for resolution on my comments later in the series.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2.5/6] hmp: Add name parameter to nbd_server_add

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> Extend the flexibility of the previous QMP patch to also work
> in HMP.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Thanks, that looks good to me from the HMP side; let me
know which way you want to take it; I can take it via
HMP once the code that uses it is in; whichever is easiest.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
> 
> In response to Dave's request.
> I could also squash this into Vladimir's 2/6 if desired.
> 
>  hmp.c           | 3 ++-
>  hmp-commands.hx | 9 +++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 80c5b01540..c9fcdc8593 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2221,10 +2221,11 @@ exit:
>  void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
>  {
>      const char *device = qdict_get_str(qdict, "device");
> +    const char *name = qdict_get_try_str(qdict, "name");
>      bool writable = qdict_get_try_bool(qdict, "writable", false);
>      Error *local_err = NULL;
> 
> -    qmp_nbd_server_add(device, false, NULL, true, writable, &local_err);
> +    qmp_nbd_server_add(device, !!name, name, true, writable, &local_err);
> 
>      if (local_err != NULL) {
>          hmp_handle_error(mon, &local_err);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 6d5ebdf6ab..b8b6fb9184 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1553,17 +1553,18 @@ ETEXI
> 
>      {
>          .name       = "nbd_server_add",
> -        .args_type  = "writable:-w,device:B",
> -        .params     = "nbd_server_add [-w] device",
> +        .args_type  = "writable:-w,device:B,name:s?",
> +        .params     = "nbd_server_add [-w] device [name]",
>          .help       = "export a block device via NBD",
>          .cmd        = hmp_nbd_server_add,
>      },
>  STEXI
> -@item nbd_server_add @var{device}
> +@item nbd_server_add @var{device} [ @var{name} ]
>  @findex nbd_server_add
>  Export a block device through QEMU's NBD server, which must be started
>  beforehand with @command{nbd_server_start}.  The @option{-w} option makes the
> -exported device writable too.
> +exported device writable too.  The export name is controlled by @var{name},
> +defaulting to @var{device}.
>  ETEXI
> 
>      {
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 2.5/6] hmp: Add name parameter to nbd_server_add

[Eric Blake]

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

On 01/11/2018 11:59 AM, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> Extend the flexibility of the previous QMP patch to also work
>> in HMP.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Thanks, that looks good to me from the HMP side; let me
> know which way you want to take it; I can take it via
> HMP once the code that uses it is in; whichever is easiest.
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I'll take it through the NBD tree, along with the rest of the series,
once I get more answers on my open questions on the second half of the
series.

> 
>> ---
>>
>> In response to Dave's request.
>> I could also squash this into Vladimir's 2/6 if desired.

At this point, I'm thinking it's better to leave it separate from
Vladimir's QMP patch, since you've only reviewed this one.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 2.5/6] hmp: Add name parameter to nbd_server_add

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 01/11/2018 11:59 AM, Dr. David Alan Gilbert wrote:
> > * Eric Blake (eblake@redhat.com) wrote:
> >> Extend the flexibility of the previous QMP patch to also work
> >> in HMP.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> > Thanks, that looks good to me from the HMP side; let me
> > know which way you want to take it; I can take it via
> > HMP once the code that uses it is in; whichever is easiest.
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> I'll take it through the NBD tree, along with the rest of the series,
> once I get more answers on my open questions on the second half of the
> series.

Sure.

> > 
> >> ---
> >>
> >> In response to Dave's request.
> >> I could also squash this into Vladimir's 2/6 if desired.
> 
> At this point, I'm thinking it's better to leave it separate from
> Vladimir's QMP patch, since you've only reviewed this one.

Feel free to do either way.

Dave

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

09.01.2018 22:52, Eric Blake wrote:
> On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Add command for export removing. It is needed for cases when we
>> don't want to keep export after the operation on it was completed.
>> The other example is temporary node, created with blockdev-add.
>> If we want to delete it we should firstly remove corresponding
>> NBD export.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block.json     | 18 ++++++++++++++++++
>>   include/block/nbd.h |  3 ++-
>>   blockdev-nbd.c      | 29 ++++++++++++++++++++++++++++-
>>   nbd/server.c        | 25 ++++++++++++++++++++++---
>>   4 files changed, 70 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block.json b/qapi/block.json
>> index 503d4b287b..f83485c325 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -228,6 +228,24 @@
>>     'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
>>   
>>   ##
>> +# @nbd-server-remove:
>> +#
>> +# Remove NBD export by name.
>> +#
>> +# @name: Export name.
>> +#
>> +# @force: Whether active connections to the export should be closed. If this
>> +#         parameter is false the export only becomes hidden from clients (new
>> +#         connections are impossible), and would be automatically freed after
>> +#         all clients are disconnected (default false).
> Unstated, but if the parameter is true, existing clients are forcefully
> disconnected, possibly losing pending transactions.
>
> Do we want a third mode in the middle, where the server starts replying
> to all existing clients with ESHUTDOWN errors for all new requests
> rather than abruptly disconnecting (no new I/O, but no forced disconnect
> and pending in-flight transactions can still complete gracefully)?

looks interesting. what about the following naming?

@mode: possible values:
               hide - just hide server from new clients, maintain 
existing connections,
                           remove after all clients disconnected
               soft - like hide, but answer with ESHUTDOWN for all 
further requests from
                           existing connections
               hard - hard disconnect all clients and remove server
               (default: soft)

new corresponding states of nbd export:
hidden, shutting_down

and we allow transitions:

normal_execution -> hidden
normal_execution -> shutting_down
normal_execution -> exit
hidden -> shutting_down
hidden -> exit
shutting_down -> exit


>
>> +#
>> +# Returns: error if the server is not running or export is not found.
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'command': 'nbd-server-remove', 'data': {'name': 'str', '*force': 'bool'} }
>> +
> If we're okay with just the bool parameter, then this patch looks good;
> but if we want a third mode, then we want '*force' to be an enum type.
> So tentative:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

09.01.2018 23:49, Eric Blake wrote:
> On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/201     | 130 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/201.out |   5 ++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 136 insertions(+)
>>   create mode 100644 tests/qemu-iotests/201
>>   create mode 100644 tests/qemu-iotests/201.out
>>
> pep8 complains:
>
> $ pep8 tests/qemu-iotests/201
> tests/qemu-iotests/201:31:1: E302 expected 2 blank lines, found 1
> tests/qemu-iotests/201:68:17: E128 continuation line under-indented for
> visual indent
> tests/qemu-iotests/201:69:17: E128 continuation line under-indented for
> visual indent
>
> I don't mind cleaning those up (if you don't have to respin because of
> my comments on 5/6).
>
>> +import os
>> +import sys
>> +import iotests
>> +import time
>> +from iotests import qemu_img, qemu_io, filter_qemu_io, QemuIoInteractive
>> +
>> +nbd_port = '10900'
>> +nbd_uri = 'nbd+tcp://localhost:' + nbd_port + '/exp'
> Is this still going to be safe when we improve the testsuite to run
> multiple tests in parallel?  Wouldn't it be safer to dynamically choose
> the port, instead of hard-coding one?
>
>> +disk = os.path.join(iotests.test_dir, 'disk')
>> +
>> +class TestNbdServerRemove(iotests.QMPTestCase):
>> +    def setUp(self):
>> +        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
>> +
>> +        self.vm = iotests.VM().add_drive(disk)
>> +        self.vm.launch()
>> +
>> +        address = {
>> +            'type': 'inet',
>> +            'data': {
>> +                'host': 'localhost',
>> +                'port': nbd_port
> Again, for a one-shot test, this works; but it doesn't lend itself to
> parallel testing.  Maybe do a loop with incrementing port numbers until
> one succeeds, if we can reliably detect when a port is already in use?
>
>> +    def assertConnectFailed(self, qemu_io_output):
>> +        self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
>> +                         "can't open device nbd+tcp://localhost:10900/exp: " +
> Worth parameterizing or filtering this assertion to match the rest of
> the parameterization of nbd_port?
>
>> +                         "Requested export not available\nserver reported: " +
>> +                         "export 'exp' not present")
>> +
>> +    def do_test_connect_after_remove(self, force=None):
>> +        args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)
>> +        self.assertReadOk(qemu_io(*args))
>> +        self.remove_export('exp', force)
>> +        self.assertExportNotFound('exp')
>> +        self.assertConnectFailed(qemu_io(*args))
>> +
>> +    def test_connect_after_remove_default(self):
>> +        self.do_test_connect_after_remove()
>> +
>> +    def test_connect_after_remove_safe(self):
>> +        self.do_test_connect_after_remove(False)
>> +
>> +    def test_connect_after_remove_force(self):
>> +        self.do_test_connect_after_remove(True)
> May need updates if my comments on 3/6 result in us having three states
> rather than just 2 (the difference being whether there is a knob for
> choosing to let existing clients gracefully disconnect with all pending
> I/O completed, and/or failing the nbd-server-remove if a client is still
> connected).
>
>> +++ b/tests/qemu-iotests/201.out
>> @@ -0,0 +1,5 @@
>> +.......
>> +----------------------------------------------------------------------
>> +Ran 7 tests
> I'm not a fan of python tests that are difficult to debug.  Your
> additions to 147 in patch 4/6 are okay (hard to debug, but an
> incremental addition); but is it possible to rewrite this test in a bit
> more verbose manner?  See test 194 and this message for more details:
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html

hmm, what do you mean by "difficult to debug"? This is a usual python 
unittest based test.
And there 3 test cases, sharing same setUp. Do not you say that unittest 
becomes
deprecated in qemu? I think, if we have only one testcase, we may use 
194-like approach,
but if we have more, it's better to use unittest.

>
>> +
>> +OK
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index 3e688678dd..15df7678b3 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -197,3 +197,4 @@
>>   197 rw auto quick
>>   198 rw auto
>>   200 rw auto
>> +201 rw auto quick
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive class

[Vladimir Sementsov-Ogievskiy]

09.01.2018 23:34, Eric Blake wrote:
> On 12/07/2017 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>
> The subject says what, but there is no commit body that says why.
>
> Presumably this makes writing the test in the next patch easier, but
> some details in the commit message would make this obvious.
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
> My python is not strong; it looks good overall, although I have a few
> questions that may warrant a v3 before I give R-b.
>
>> +class QemuIoInteractive:
>> +    def __init__(self, *args):
>> +        self.args = qemu_io_args + list(args)
>> +        self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
>> +                                   stdout=subprocess.PIPE,
>> +                                   stderr=subprocess.STDOUT)
> Why STDOUT instead of STDERR?  Is the redirection intentional?

this special flag means: "send subprocess stderr to the same place as 
stdout", so,
I'll have both stdout and stderr in one .PIPE. I don't print these 
outputs, but return
them to the user.

>
>
>> +    def _read_output(self):
>> +        pattern = 'qemu-io> '
>> +        n = len(pattern)
>> +        pos = 0
>> +        s = []
>> +        while pos != n:
>> +            c = self._p.stdout.read(1)
>> +            #check unexpected EOF
> pep8 doesn't like this comment style (it wants space after #).  The file
> is already unclean under pep8, but you shouldn't make it worse.
>
>> +            assert c != ''
> Is assert really the nicest control flow when early EOF is present? Or
> is it because we are only using this in tests, where we don't expect
> early EOF, so asserting is just fine for our usage?

The only usage for now is a test in the following patch. I think assert 
is OK for now,
we can change it in future if needed.

>
>> +            s.append(c)
>> +            if c == pattern[pos]:
>> +                pos += 1
>> +            else:
>> +                pos = 0
>> +
>> +        return ''.join(s[:-n])
> Is it necessary to use join() on the empty string here, compared to just
> returning the array slice directly?

It seems more usual for such function to return string, not a list. 
Without join here,
I'll have to join in caller.

>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/6] iotests: implement QemuIoInteractive class

[Eric Blake]

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

On 01/12/2018 05:56 AM, Vladimir Sementsov-Ogievskiy wrote:

>> My python is not strong; it looks good overall, although I have a few
>> questions that may warrant a v3 before I give R-b.
>>
>>> +class QemuIoInteractive:
>>> +    def __init__(self, *args):
>>> +        self.args = qemu_io_args + list(args)
>>> +        self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
>>> +                                   stdout=subprocess.PIPE,
>>> +                                   stderr=subprocess.STDOUT)
>> Why STDOUT instead of STDERR?  Is the redirection intentional?
> 
> this special flag means: "send subprocess stderr to the same place as
> stdout", so,
> I'll have both stdout and stderr in one .PIPE. I don't print these
> outputs, but return
> them to the user.

Okay, makes sense (my python inexperience is showing ;)

I'd welcome a review from anyone else, but since the resulting test
passes, I'm not opposed to taking the patch as-is.  If it goes through
my NBD queue, it will get my S-o-b (because I'll have to touch it); but
if it goes through anyone else's queue, that maintainer can add this
(weaker than R-b, but at least says I'm okay with it):

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Eric Blake]

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

On 01/12/2018 05:43 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> +++ b/tests/qemu-iotests/201.out
>>> @@ -0,0 +1,5 @@
>>> +.......
>>> +----------------------------------------------------------------------
>>> +Ran 7 tests
>> I'm not a fan of python tests that are difficult to debug.  Your
>> additions to 147 in patch 4/6 are okay (hard to debug, but an
>> incremental addition); but is it possible to rewrite this test in a bit
>> more verbose manner?  See test 194 and this message for more details:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html
> 
> hmm, what do you mean by "difficult to debug"? This is a usual python
> unittest based test.

And the list archives show several threads of people complaining that
./check failing with a diff that merely shows:

-.....
+..E..

makes it rather hard to see WHAT test 2 was doing that caused an error
instead of a pass, let alone set up a reproduction scenario on JUST the
failing test.  Yes, a lot of existing iotests use this unittest layout,
and on that grounds, I'm not opposed to adding another one; but test 194
really IS easier to debug when something goes wrong.

> And there 3 test cases, sharing same setUp. Do not you say that unittest
> becomes
> deprecated in qemu? I think, if we have only one testcase, we may use
> 194-like approach,
> but if we have more, it's better to use unittest.

Yes, I think a nice goal for improved testing is to write more
python-based iotests in the style that uses actual output, and not just
the unittest framework, in the test log.  It's not a hard requirement as
long as no one has converted existing tests, but is food for thought.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

12.01.2018 19:54, Eric Blake wrote:
> On 01/12/2018 05:43 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> +++ b/tests/qemu-iotests/201.out
>>>> @@ -0,0 +1,5 @@
>>>> +.......
>>>> +----------------------------------------------------------------------
>>>> +Ran 7 tests
>>> I'm not a fan of python tests that are difficult to debug.  Your
>>> additions to 147 in patch 4/6 are okay (hard to debug, but an
>>> incremental addition); but is it possible to rewrite this test in a bit
>>> more verbose manner?  See test 194 and this message for more details:
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg00234.html
>> hmm, what do you mean by "difficult to debug"? This is a usual python
>> unittest based test.
> And the list archives show several threads of people complaining that
> ./check failing with a diff that merely shows:
>
> -.....
> +..E..

didn't see that. usually, for failed iotests I see

-.....
+..E..

+ some kind of assert-fail in one of testcases

so we know in which testcase and in which line it was failed.

>
> makes it rather hard to see WHAT test 2 was doing that caused an error
> instead of a pass, let alone set up a reproduction scenario on JUST the
> failing test.  Yes, a lot of existing iotests use this unittest layout,
> and on that grounds, I'm not opposed to adding another one; but test 194
> really IS easier to debug when something goes wrong.
>
>> And there 3 test cases, sharing same setUp. Do not you say that unittest
>> becomes
>> deprecated in qemu? I think, if we have only one testcase, we may use
>> 194-like approach,
>> but if we have more, it's better to use unittest.
> Yes, I think a nice goal for improved testing is to write more
> python-based iotests in the style that uses actual output, and not just
> the unittest framework, in the test log.  It's not a hard requirement as
> long as no one has converted existing tests, but is food for thought.
>

I think, it doesn't mean that we should not use unittest at all, we just 
need more output with
it.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Eric Blake]

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

On 01/15/2018 08:40 AM, Vladimir Sementsov-Ogievskiy wrote:

>> And the list archives show several threads of people complaining that
>> ./check failing with a diff that merely shows:
>>
>> -.....
>> +..E..
> 
> didn't see that. usually, for failed iotests I see
> 
> -.....
> +..E..
> 
> + some kind of assert-fail in one of testcases

Although deciphering the assert-fail is not always trivial, and it is
still sorely underdocumented on how to manually reproduce the situation
that got to the stackdump.

> 
> so we know in which testcase and in which line it was failed.
> 
>>
>> makes it rather hard to see WHAT test 2 was doing that caused an error
>> instead of a pass, let alone set up a reproduction scenario on JUST the
>> failing test.  Yes, a lot of existing iotests use this unittest layout,
>> and on that grounds, I'm not opposed to adding another one; but test 194
>> really IS easier to debug when something goes wrong.
>>
>>> And there 3 test cases, sharing same setUp. Do not you say that unittest
>>> becomes
>>> deprecated in qemu? I think, if we have only one testcase, we may use
>>> 194-like approach,
>>> but if we have more, it's better to use unittest.
>> Yes, I think a nice goal for improved testing is to write more
>> python-based iotests in the style that uses actual output, and not just
>> the unittest framework, in the test log.  It's not a hard requirement as
>> long as no one has converted existing tests, but is food for thought.
>>
> 
> I think, it doesn't mean that we should not use unittest at all, we just
> need more output with
> it.

Yes, that's also a potentially viable option.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

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

On 01/12/2018 03:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 09.01.2018 22:52, Eric Blake wrote:
>> On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add command for export removing. It is needed for cases when we
>>> don't want to keep export after the operation on it was completed.
>>> The other example is temporary node, created with blockdev-add.
>>> If we want to delete it we should firstly remove corresponding
>>> NBD export.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---

>> Do we want a third mode in the middle, where the server starts replying
>> to all existing clients with ESHUTDOWN errors for all new requests
>> rather than abruptly disconnecting (no new I/O, but no forced disconnect
>> and pending in-flight transactions can still complete gracefully)?
> 
> looks interesting. what about the following naming?
> 
> @mode: possible values:
>               hide - just hide server from new clients, maintain
> existing connections,
>                           remove after all clients disconnected
>               soft - like hide, but answer with ESHUTDOWN for all
> further requests from
>                           existing connections
>               hard - hard disconnect all clients and remove server
>               (default: soft)

Or even a fourth mode that causes an immediate error return without
state change if there are any connected clients, but otherwise removes
the server.

> 
> new corresponding states of nbd export:
> hidden, shutting_down
> 
> and we allow transitions:
> 
> normal_execution -> hidden
> normal_execution -> shutting_down
> normal_execution -> exit
> hidden -> shutting_down
> hidden -> exit
> shutting_down -> exit

Seems reasonable.  Are you planning on tackling a respin of this series
incorporating that idea?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

15.01.2018 18:09, Eric Blake wrote:
> On 01/12/2018 03:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 09.01.2018 22:52, Eric Blake wrote:
>>> On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add command for export removing. It is needed for cases when we
>>>> don't want to keep export after the operation on it was completed.
>>>> The other example is temporary node, created with blockdev-add.
>>>> If we want to delete it we should firstly remove corresponding
>>>> NBD export.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>> Do we want a third mode in the middle, where the server starts replying
>>> to all existing clients with ESHUTDOWN errors for all new requests
>>> rather than abruptly disconnecting (no new I/O, but no forced disconnect
>>> and pending in-flight transactions can still complete gracefully)?
>> looks interesting. what about the following naming?
>>
>> @mode: possible values:
>>                hide - just hide server from new clients, maintain
>> existing connections,
>>                            remove after all clients disconnected
>>                soft - like hide, but answer with ESHUTDOWN for all
>> further requests from
>>                            existing connections
>>                hard - hard disconnect all clients and remove server
>>                (default: soft)
> Or even a fourth mode that causes an immediate error return without
> state change if there are any connected clients, but otherwise removes
> the server.
>
>> new corresponding states of nbd export:
>> hidden, shutting_down
>>
>> and we allow transitions:
>>
>> normal_execution -> hidden
>> normal_execution -> shutting_down
>> normal_execution -> exit
>> hidden -> shutting_down
>> hidden -> exit
>> shutting_down -> exit
> Seems reasonable.  Are you planning on tackling a respin of this series
> incorporating that idea?
>

yes, will do.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/6] iotest 201: new test for qmp nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

15.01.2018 18:05, Eric Blake wrote:
> On 01/15/2018 08:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>> And the list archives show several threads of people complaining that
>>> ./check failing with a diff that merely shows:
>>>
>>> -.....
>>> +..E..
>> didn't see that. usually, for failed iotests I see
>>
>> -.....
>> +..E..
>>
>> + some kind of assert-fail in one of testcases
> Although deciphering the assert-fail is not always trivial, and it is
> still sorely underdocumented on how to manually reproduce the situation
> that got to the stackdump.

hmm, restart test? is it documented for 194?

I don't see an option to run only one testcase, but unittest supports it,
so we just need to add this option to ./check


Is it really not trivial with my test?

example, inject bug:
--- a/tests/qemu-iotests/201
+++ b/tests/qemu-iotests/201
@@ -76,7 +76,7 @@ class TestNbdServerRemove(iotests.QMPTestCase):
          self.assertEqual(filter_qemu_io(qemu_io_output).strip(),
                           "can't open device 
nbd+tcp://localhost:10900/exp: " +
                           "Requested export not available\nserver 
reported: " +
-                         "export 'exp' not present")
+                         "1export 'exp' not present")

      def do_test_connect_after_remove(self, force=None):
          args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)



output:

-.......
+FFF....
+======================================================================
+FAIL: test_connect_after_remove_default (__main__.TestNbdServerRemove)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "201", line 89, in test_connect_after_remove_default
+    self.do_test_connect_after_remove()
+  File "201", line 86, in do_test_connect_after_remove
+    self.assertConnectFailed(qemu_io(*args))
+  File "201", line 79, in assertConnectFailed
+    "1export 'exp' not present")
+AssertionError: "can't open device nbd+tcp://localhost:10900/exp: 
Requested export not available\nserver reported: export 'exp' not 
present" != "can't open device nbd+tcp://localhost:10900/exp: Requested 
export not available\nserver reported: 1export 'exp' not present"
+
[...]


- all obvious. We see what happened and where. And the name of the 
broken testcase.


----
I remember the following problems with iotests, but I do not think that 
this is a reason to deprecate unittest and go
in some custom way. Better is to fix them. It's all are problems of our 
unittest wrapping, not of python unittest.

- asserts and prints are not naturally mixed in final output (isn't it a 
reason for never using print in these tests?)
- no progress, to see the output we should wait until test finished
- if qemu crashed, it is hard to understand, in which testcase

so, my point is: use unittest. It is a standard library and common way 
of doing this. And it is already in Qemu iotests.
It gives good organization of test code.

Maybe, a "plain executable python test" is good for complicated tests, 
which are actually cant be called "unit test", but
which are more like "system wide test", when we actually need only one 
testcase, but it needs several pages of code.

>
>> so we know in which testcase and in which line it was failed.
>>
>>> makes it rather hard to see WHAT test 2 was doing that caused an error
>>> instead of a pass, let alone set up a reproduction scenario on JUST the
>>> failing test.  Yes, a lot of existing iotests use this unittest layout,
>>> and on that grounds, I'm not opposed to adding another one; but test 194
>>> really IS easier to debug when something goes wrong.
>>>
>>>> And there 3 test cases, sharing same setUp. Do not you say that unittest
>>>> becomes
>>>> deprecated in qemu? I think, if we have only one testcase, we may use
>>>> 194-like approach,
>>>> but if we have more, it's better to use unittest.
>>> Yes, I think a nice goal for improved testing is to write more
>>> python-based iotests in the style that uses actual output, and not just
>>> the unittest framework, in the test log.  It's not a hard requirement as
>>> long as no one has converted existing tests, but is food for thought.
>>>
>> I think, it doesn't mean that we should not use unittest at all, we just
>> need more output with
>> it.
> Yes, that's also a potentially viable option.
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

15.01.2018 20:47, Vladimir Sementsov-Ogievskiy wrote:
> 15.01.2018 18:09, Eric Blake wrote:
>> On 01/12/2018 03:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 09.01.2018 22:52, Eric Blake wrote:
>>>> On 12/07/2017 09:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Add command for export removing. It is needed for cases when we
>>>>> don't want to keep export after the operation on it was completed.
>>>>> The other example is temporary node, created with blockdev-add.
>>>>> If we want to delete it we should firstly remove corresponding
>>>>> NBD export.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>>>> <vsementsov@virtuozzo.com>
>>>>> ---
>>>> Do we want a third mode in the middle, where the server starts 
>>>> replying
>>>> to all existing clients with ESHUTDOWN errors for all new requests
>>>> rather than abruptly disconnecting (no new I/O, but no forced 
>>>> disconnect
>>>> and pending in-flight transactions can still complete gracefully)?
>>> looks interesting. what about the following naming?
>>>
>>> @mode: possible values:
>>>                hide - just hide server from new clients, maintain
>>> existing connections,
>>>                            remove after all clients disconnected
>>>                soft - like hide, but answer with ESHUTDOWN for all
>>> further requests from
>>>                            existing connections
>>>                hard - hard disconnect all clients and remove server
>>>                (default: soft)
>> Or even a fourth mode that causes an immediate error return without
>> state change if there are any connected clients, but otherwise removes
>> the server.
>>
>>> new corresponding states of nbd export:
>>> hidden, shutting_down
>>>
>>> and we allow transitions:
>>>
>>> normal_execution -> hidden
>>> normal_execution -> shutting_down
>>> normal_execution -> exit
>>> hidden -> shutting_down
>>> hidden -> exit
>>> shutting_down -> exit
>> Seems reasonable.  Are you planning on tackling a respin of this series
>> incorporating that idea?
>>
>
> yes, will do.
>
>

Discussed with Nikolay.
For now we actually need only one mode: hard.
In near future we _may be_ will need your proposed fourth mode (what 
about "safe" name for it ?)

I was going to implement all 4 modes, but now I doubt, isn't it too 
hastily, to introduce 3 new modes to the
interface, which we (personally) do not need. May be it is better to 
start from one or two modes.

Finally what do you think, Eric? Which modes do you need?

ps: I've created hmp version for 2/6, it will be in v2.
  also, I'm going to add query-nbd-server, which should list all exports

also, about HMP: If I understand correctly, people use it because 
writing qmp command by hand is not very comfortable.
I have a script (for managing libvirt guest, but it can be adopted for 
qemu or even used for qemu monitor), which allows
me run qmp commands on vms as easy as:

|qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1 
mode hard or even |

|qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true 
direct true} aio native discard unmap file {driver file filename 
/tmp/somedisk} |||

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

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

On 01/17/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>> looks interesting. what about the following naming?
>>>>
>>>> @mode: possible values:
>>>>                hide - just hide server from new clients, maintain
>>>> existing connections,
>>>>                            remove after all clients disconnected
>>>>                soft - like hide, but answer with ESHUTDOWN for all
>>>> further requests from
>>>>                            existing connections
>>>>                hard - hard disconnect all clients and remove server
>>>>                (default: soft)
>>> Or even a fourth mode that causes an immediate error return without
>>> state change if there are any connected clients, but otherwise removes
>>> the server.
>>>
>>>> new corresponding states of nbd export:
>>>> hidden, shutting_down
>>>>
>>>> and we allow transitions:
>>>>
>>>> normal_execution -> hidden
>>>> normal_execution -> shutting_down
>>>> normal_execution -> exit
>>>> hidden -> shutting_down
>>>> hidden -> exit
>>>> shutting_down -> exit
>>> Seems reasonable.  Are you planning on tackling a respin of this series
>>> incorporating that idea?
>>>
>>
>> yes, will do.
>>
>>
> 
> Discussed with Nikolay.
> For now we actually need only one mode: hard.
> In near future we _may be_ will need your proposed fourth mode (what
> about "safe" name for it ?)

'safe' sounds reasonable.

Of course, if we only have two modes at front ('safe' which returns an
error if a client is connected, and 'hard' which disconnects all clients
immediately; leaving 'hide' and 'soft' for the future), then we don't
have to worry about a state transition or any hidden exports.

A QAPI enum with only two values now is at least extensible in the
future if someone has a need for another mode, and introspectible to
learn which modes are currently supported.

> 
> I was going to implement all 4 modes, but now I doubt, isn't it too
> hastily, to introduce 3 new modes to the
> interface, which we (personally) do not need. May be it is better to
> start from one or two modes.

Starting with just two modes is fine as well.

> 
> Finally what do you think, Eric? Which modes do you need?

'hide' may be interesting for the purpose of connecting a single client,
then hiding the export so no other clients can connect, while waiting
for the first client to take its time.  But right now, I don't have
actual use cases in mind so much as making sure we aren't limiting
ourself from future expansion as needs are identified, so a conservative
choice of just 'safe' and 'hard' for now is reasonable.

> 
> ps: I've created hmp version for 2/6, it will be in v2.
>  also, I'm going to add query-nbd-server, which should list all exports

Sounds good.

> 
> also, about HMP: If I understand correctly, people use it because
> writing qmp command by hand is not very comfortable.
> I have a script (for managing libvirt guest, but it can be adopted for
> qemu or even used for qemu monitor), which allows
> me run qmp commands on vms as easy as:
> 
> |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
> mode hard or even |
> 
> |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
> direct true} aio native discard unmap file {driver file filename
> /tmp/somedisk} |||

Yeah, there are various scripting solutions around QMP that can make it
easier; but HMP is often still an easy front-line interface for experiments.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

17.01.2018 18:23, Eric Blake wrote:
> On 01/17/2018 07:36 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>>> looks interesting. what about the following naming?
>>>>>
>>>>> @mode: possible values:
>>>>>                 hide - just hide server from new clients, maintain
>>>>> existing connections,
>>>>>                             remove after all clients disconnected
>>>>>                 soft - like hide, but answer with ESHUTDOWN for all
>>>>> further requests from
>>>>>                             existing connections
>>>>>                 hard - hard disconnect all clients and remove server
>>>>>                 (default: soft)
>>>> Or even a fourth mode that causes an immediate error return without
>>>> state change if there are any connected clients, but otherwise removes
>>>> the server.
>>>>
>>>>> new corresponding states of nbd export:
>>>>> hidden, shutting_down
>>>>>
>>>>> and we allow transitions:
>>>>>
>>>>> normal_execution -> hidden
>>>>> normal_execution -> shutting_down
>>>>> normal_execution -> exit
>>>>> hidden -> shutting_down
>>>>> hidden -> exit
>>>>> shutting_down -> exit
>>>> Seems reasonable.  Are you planning on tackling a respin of this series
>>>> incorporating that idea?
>>>>
>>> yes, will do.
>>>
>>>
>> Discussed with Nikolay.
>> For now we actually need only one mode: hard.
>> In near future we _may be_ will need your proposed fourth mode (what
>> about "safe" name for it ?)
> 'safe' sounds reasonable.
>
> Of course, if we only have two modes at front ('safe' which returns an
> error if a client is connected, and 'hard' which disconnects all clients
> immediately; leaving 'hide' and 'soft' for the future), then we don't
> have to worry about a state transition or any hidden exports.
>
> A QAPI enum with only two values now is at least extensible in the
> future if someone has a need for another mode, and introspectible to
> learn which modes are currently supported.
>
>> I was going to implement all 4 modes, but now I doubt, isn't it too
>> hastily, to introduce 3 new modes to the
>> interface, which we (personally) do not need. May be it is better to
>> start from one or two modes.
> Starting with just two modes is fine as well.
>
>> Finally what do you think, Eric? Which modes do you need?
> 'hide' may be interesting for the purpose of connecting a single client,
> then hiding the export so no other clients can connect, while waiting
> for the first client to take its time.  But right now, I don't have
> actual use cases in mind so much as making sure we aren't limiting
> ourself from future expansion as needs are identified, so a conservative
> choice of just 'safe' and 'hard' for now is reasonable.

ok, I agree. 'safe' looks like better option for default behavior. So 
I'll post these two options in v3.

>
>> ps: I've created hmp version for 2/6, it will be in v2.
>>   also, I'm going to add query-nbd-server, which should list all exports
> Sounds good.
>
>> also, about HMP: If I understand correctly, people use it because
>> writing qmp command by hand is not very comfortable.
>> I have a script (for managing libvirt guest, but it can be adopted for
>> qemu or even used for qemu monitor), which allows
>> me run qmp commands on vms as easy as:
>>
>> |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
>> mode hard or even |
>>
>> |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
>> direct true} aio native discard unmap file {driver file filename
>> /tmp/somedisk} |||
> Yeah, there are various scripting solutions around QMP that can make it
> easier; but HMP is often still an easy front-line interface for experiments.
>

isn't it because these solutions are not available directly in monitor, 
when HMP is?
may be, we need third type of monitor HQMP which is QMP with simplified 
syntax? Or
allow qmp commands in simplified syntax directly in HMP?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

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

On 01/17/2018 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> I have a script (for managing libvirt guest, but it can be adopted for
>>> qemu or even used for qemu monitor), which allows
>>> me run qmp commands on vms as easy as:
>>>
>>> |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
>>> mode hard or even |
>>>
>>> |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
>>> direct true} aio native discard unmap file {driver file filename
>>> /tmp/somedisk} |||
>> Yeah, there are various scripting solutions around QMP that can make it
>> easier; but HMP is often still an easy front-line interface for
>> experiments.
>>
> 
> isn't it because these solutions are not available directly in monitor,
> when HMP is?

QMP can be directly accessed in a monitor; it just requires more typing.
 If you are developing QMP commands, it may be easier to use
./scripts/qmp/qmp-shell (couple it with a readline wrapper, and you can
even get tab-completion and history across sessions).  There's also
things like libvirt's 'virsh qmp-monitor-command' for shell-scripting
access to arbitrary QMP commands, provided your guest is run by libvirt.

> may be, we need third type of monitor HQMP which is QMP with simplified
> syntax? Or
> allow qmp commands in simplified syntax directly in HMP?

No, I don't think we need either thing.  Wrappers around existing
monitors is better than bloating qemu proper with a third flavor of
monitor.  And HMP is for humans, with no restrictions on back-compat
changes, so if it doesn't do something you want for quick-and-dirty
testing, you can either add a new HMP command, or just use QMP (or one
of its wrappers, like qmp-shell) in the first place.  Ultimately, our
long-term concern is only about the QMP interface; HMP is supposed to be
convenient.  So if it starts costing too much time to port a QMP
interface to HMP, then don't worry about it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

17.01.2018 19:03, Eric Blake wrote:
> On 01/17/2018 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> I have a script (for managing libvirt guest, but it can be adopted for
>>>> qemu or even used for qemu monitor), which allows
>>>> me run qmp commands on vms as easy as:
>>>>
>>>> |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
>>>> mode hard or even |
>>>>
>>>> |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
>>>> direct true} aio native discard unmap file {driver file filename
>>>> /tmp/somedisk} |||
>>> Yeah, there are various scripting solutions around QMP that can make it
>>> easier; but HMP is often still an easy front-line interface for
>>> experiments.
>>>
>> isn't it because these solutions are not available directly in monitor,
>> when HMP is?
> QMP can be directly accessed in a monitor; it just requires more typing.
>   If you are developing QMP commands, it may be easier to use
> ./scripts/qmp/qmp-shell (couple it with a readline wrapper, and you can
> even get tab-completion and history across sessions).  There's also
> things like libvirt's 'virsh qmp-monitor-command' for shell-scripting
> access to arbitrary QMP commands, provided your guest is run by libvirt.
>
>> may be, we need third type of monitor HQMP which is QMP with simplified
>> syntax? Or
>> allow qmp commands in simplified syntax directly in HMP?
> No, I don't think we need either thing.  Wrappers around existing
> monitors is better than bloating qemu proper with a third flavor of
> monitor.  And HMP is for humans, with no restrictions on back-compat
> changes, so if it doesn't do something you want for quick-and-dirty
> testing, you can either add a new HMP command, or just use QMP (or one
> of its wrappers, like qmp-shell) in the first place.  Ultimately, our
> long-term concern is only about the QMP interface; HMP is supposed to be
> convenient.  So if it starts costing too much time to port a QMP
> interface to HMP, then don't worry about it.
>

most of commands, ported to hmp are done in same style: they just call 
corresponding qmp command.
Isn't it better to provide common interface for calling qmp commands 
through HMP monitor, to never
create hmp versions of new commands? they will be available automatically.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> 17.01.2018 19:03, Eric Blake wrote:
> > On 01/17/2018 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 
> > > > > I have a script (for managing libvirt guest, but it can be adopted for
> > > > > qemu or even used for qemu monitor), which allows
> > > > > me run qmp commands on vms as easy as:
> > > > > 
> > > > > |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
> > > > > mode hard or even |
> > > > > 
> > > > > |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
> > > > > direct true} aio native discard unmap file {driver file filename
> > > > > /tmp/somedisk} |||
> > > > Yeah, there are various scripting solutions around QMP that can make it
> > > > easier; but HMP is often still an easy front-line interface for
> > > > experiments.
> > > > 
> > > isn't it because these solutions are not available directly in monitor,
> > > when HMP is?
> > QMP can be directly accessed in a monitor; it just requires more typing.
> >   If you are developing QMP commands, it may be easier to use
> > ./scripts/qmp/qmp-shell (couple it with a readline wrapper, and you can
> > even get tab-completion and history across sessions).  There's also
> > things like libvirt's 'virsh qmp-monitor-command' for shell-scripting
> > access to arbitrary QMP commands, provided your guest is run by libvirt.
> > 
> > > may be, we need third type of monitor HQMP which is QMP with simplified
> > > syntax? Or
> > > allow qmp commands in simplified syntax directly in HMP?
> > No, I don't think we need either thing.  Wrappers around existing
> > monitors is better than bloating qemu proper with a third flavor of
> > monitor.  And HMP is for humans, with no restrictions on back-compat
> > changes, so if it doesn't do something you want for quick-and-dirty
> > testing, you can either add a new HMP command, or just use QMP (or one
> > of its wrappers, like qmp-shell) in the first place.  Ultimately, our
> > long-term concern is only about the QMP interface; HMP is supposed to be
> > convenient.  So if it starts costing too much time to port a QMP
> > interface to HMP, then don't worry about it.
> > 
> 
> most of commands, ported to hmp are done in same style: they just call
> corresponding qmp command.
> Isn't it better to provide common interface for calling qmp commands through
> HMP monitor, to never
> create hmp versions of new commands? they will be available automatically.

It would be nice to do that, but they're not that consistent in how they
convert parameters and options, but I occasionally wonder if we could
automate more of it.

Dave

> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

26.01.2018 18:05, Dr. David Alan Gilbert wrote:
> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> 17.01.2018 19:03, Eric Blake wrote:
>>> On 01/17/2018 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>>>> I have a script (for managing libvirt guest, but it can be adopted for
>>>>>> qemu or even used for qemu monitor), which allows
>>>>>> me run qmp commands on vms as easy as:
>>>>>>
>>>>>> |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
>>>>>> mode hard or even |
>>>>>>
>>>>>> |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
>>>>>> direct true} aio native discard unmap file {driver file filename
>>>>>> /tmp/somedisk} |||
>>>>> Yeah, there are various scripting solutions around QMP that can make it
>>>>> easier; but HMP is often still an easy front-line interface for
>>>>> experiments.
>>>>>
>>>> isn't it because these solutions are not available directly in monitor,
>>>> when HMP is?
>>> QMP can be directly accessed in a monitor; it just requires more typing.
>>>    If you are developing QMP commands, it may be easier to use
>>> ./scripts/qmp/qmp-shell (couple it with a readline wrapper, and you can
>>> even get tab-completion and history across sessions).  There's also
>>> things like libvirt's 'virsh qmp-monitor-command' for shell-scripting
>>> access to arbitrary QMP commands, provided your guest is run by libvirt.
>>>
>>>> may be, we need third type of monitor HQMP which is QMP with simplified
>>>> syntax? Or
>>>> allow qmp commands in simplified syntax directly in HMP?
>>> No, I don't think we need either thing.  Wrappers around existing
>>> monitors is better than bloating qemu proper with a third flavor of
>>> monitor.  And HMP is for humans, with no restrictions on back-compat
>>> changes, so if it doesn't do something you want for quick-and-dirty
>>> testing, you can either add a new HMP command, or just use QMP (or one
>>> of its wrappers, like qmp-shell) in the first place.  Ultimately, our
>>> long-term concern is only about the QMP interface; HMP is supposed to be
>>> convenient.  So if it starts costing too much time to port a QMP
>>> interface to HMP, then don't worry about it.
>>>
>> most of commands, ported to hmp are done in same style: they just call
>> corresponding qmp command.
>> Isn't it better to provide common interface for calling qmp commands through
>> HMP monitor, to never
>> create hmp versions of new commands? they will be available automatically.
> It would be nice to do that, but they're not that consistent in how they
> convert parameters and options, but I occasionally wonder if we could
> automate more of it.


What about allowing some new syntax in hmp, directly mapped to qmp?

something like

 >>> blockdev-add id disk driver qcow2 cache {writeback true direct 
true} aio native discard unmap file {driver file filename /tmp/somedisk}

?

Or it may be realized as a separate hmp command "qmp" (looks more safe 
as a first step, however, I think previous variant (direct call) is better):

 >>> qmp blockdev-add id disk driver qcow2 cache {writeback true direct 
true} aio native discard unmap file {driver file filename /tmp/somedisk}

what do think? This looks simple to implement and should be useful.

>
> Dave
>
>> -- 
>> Best regards,
>> Vladimir
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Eric Blake]

On 02/06/2018 09:29 AM, Vladimir Sementsov-Ogievskiy wrote:

>>> most of commands, ported to hmp are done in same style: they just call
>>> corresponding qmp command.
>>> Isn't it better to provide common interface for calling qmp commands 
>>> through
>>> HMP monitor, to never
>>> create hmp versions of new commands? they will be available 
>>> automatically.
>> It would be nice to do that, but they're not that consistent in how they
>> convert parameters and options, but I occasionally wonder if we could
>> automate more of it.
> 
> 
> What about allowing some new syntax in hmp, directly mapped to qmp?
> 
> something like
> 
>  >>> blockdev-add id disk driver qcow2 cache {writeback true direct 
> true} aio native discard unmap file {driver file filename /tmp/somedisk}
> 

Personally, if I'm testing blockdev-add, I'll use QMP directly (or even 
scripts/qmp/qmp-shell or virsh qemu-monitor-command), not an HMP wrapper 
where I have to learn a new syntax of how to write something that will 
convert to QMP.  We already have enough different ways to write things 
that I don't need to learn yet another syntax wrapper.  Or maybe what 
I'm saying is that instead of inventing a new syntax, that if you DO add 
an HMP command that forwards to QMP, please reuse an existing syntax 
(whether direct JSON as used by QMP, or the syntax used by qmp-shell).

If you think writing a new HMP command is worth it, I won't stop you 
from writing it.  But at this point, our current approach of writing a 
manual wrapper per command as we have interest, rather than a generic 
wrap-anything, has worked for the cases that HMP users have cared about. 
  Remember, QMP is the interface that MUST work, while HMP is only for 
convenience, and if it is not trivial to make HMP do everything that QMP 
can do, it is no real loss.

> ?
> 
> Or it may be realized as a separate hmp command "qmp" (looks more safe 
> as a first step, however, I think previous variant (direct call) is 
> better):
> 
>  >>> qmp blockdev-add id disk driver qcow2 cache {writeback true direct 
> true} aio native discard unmap file {driver file filename /tmp/somedisk}
> 
> what do think? This looks simple to implement and should be useful.

Up to you if you want to tackle anything like that, but it would be a 
new thread (a generic way to invoke QMP from HMP is independent of 
nbd-server-remove).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Vladimir Sementsov-Ogievskiy]

06.02.2018 19:06, Eric Blake wrote:
> On 02/06/2018 09:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> most of commands, ported to hmp are done in same style: they just call
>>>> corresponding qmp command.
>>>> Isn't it better to provide common interface for calling qmp 
>>>> commands through
>>>> HMP monitor, to never
>>>> create hmp versions of new commands? they will be available 
>>>> automatically.
>>> It would be nice to do that, but they're not that consistent in how 
>>> they
>>> convert parameters and options, but I occasionally wonder if we could
>>> automate more of it.
>>
>>
>> What about allowing some new syntax in hmp, directly mapped to qmp?
>>
>> something like
>>
>>  >>> blockdev-add id disk driver qcow2 cache {writeback true direct 
>> true} aio native discard unmap file {driver file filename /tmp/somedisk}
>>
>
> Personally, if I'm testing blockdev-add, I'll use QMP directly (or 
> even scripts/qmp/qmp-shell or virsh qemu-monitor-command), not an HMP 
> wrapper where I have to learn a new syntax of how to write something 
> that will convert to QMP.  We already have enough different ways to 
> write things that I don't need to learn yet another syntax wrapper.  
> Or maybe what I'm saying is that instead of inventing a new syntax, 
> that if you DO add an HMP command that forwards to QMP, please reuse 
> an existing syntax (whether direct JSON as used by QMP, or the syntax 
> used by qmp-shell).

I'm afraid, that JSON is too hard to use in human monitor. And this will 
make the whole feature useless.

>
> If you think writing a new HMP command is worth it, I won't stop you 
> from writing it.  But at this point, our current approach of writing a 
> manual wrapper per command as we have interest, rather than a generic 
> wrap-anything, has worked for the cases that HMP users have cared 
> about.  Remember, QMP is the interface that MUST work, while HMP is 
> only for convenience, and if it is not trivial to make HMP do 
> everything that QMP can do, it is no real loss.
>

But we create hmp wrappers on demand, and for each case, we actually 
invent new syntax. I just search for the way to avoid creating new and 
new hmp wrappers, by introducing new syntax only once.
And, here is almost nothing to learn:

command := command-name parameters
parameters = [key value ]...
value = simple-value | array | map
map = '{' parameters '}'
array = '[' [value ]... ']'

another variant is to use yaml - like json, but we do not need put all 
keys into quotes.

On the other hand, implementing new parser in qemu is not trivial task 
(hmm, I don't want do it=), it should be simpler to create direct JSON 
wrapper in HMP monitor, and use some python wrapper around the monitor. 
And this looks useless, as with same result I can use wrapper around QMP 
monitor. So, may be the most interesting solution would be to make some 
easy-to-use python-based wrapper, which will give a simple way to use 
both qmp and hmp commands.. I'll think about it. However it doesn't 
solve initial problem of creating new and new hmp wrappers by hand.

>> ?
>>
>> Or it may be realized as a separate hmp command "qmp" (looks more 
>> safe as a first step, however, I think previous variant (direct call) 
>> is better):
>>
>>  >>> qmp blockdev-add id disk driver qcow2 cache {writeback true 
>> direct true} aio native discard unmap file {driver file filename 
>> /tmp/somedisk}
>>
>> what do think? This looks simple to implement and should be useful.
>
> Up to you if you want to tackle anything like that, but it would be a 
> new thread (a generic way to invoke QMP from HMP is independent of 
> nbd-server-remove).
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> 26.01.2018 18:05, Dr. David Alan Gilbert wrote:
> > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
> > > 17.01.2018 19:03, Eric Blake wrote:
> > > > On 01/17/2018 09:51 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > > 
> > > > > > > I have a script (for managing libvirt guest, but it can be adopted for
> > > > > > > qemu or even used for qemu monitor), which allows
> > > > > > > me run qmp commands on vms as easy as:
> > > > > > > 
> > > > > > > |qmp VMNAME query-block-jobs or qmp VMNAME nbd-server-remove name exp1
> > > > > > > mode hard or even |
> > > > > > > 
> > > > > > > |qmp VMNAME blockdev-add id disk driver qcow2 cache {writeback true
> > > > > > > direct true} aio native discard unmap file {driver file filename
> > > > > > > /tmp/somedisk} |||
> > > > > > Yeah, there are various scripting solutions around QMP that can make it
> > > > > > easier; but HMP is often still an easy front-line interface for
> > > > > > experiments.
> > > > > > 
> > > > > isn't it because these solutions are not available directly in monitor,
> > > > > when HMP is?
> > > > QMP can be directly accessed in a monitor; it just requires more typing.
> > > >    If you are developing QMP commands, it may be easier to use
> > > > ./scripts/qmp/qmp-shell (couple it with a readline wrapper, and you can
> > > > even get tab-completion and history across sessions).  There's also
> > > > things like libvirt's 'virsh qmp-monitor-command' for shell-scripting
> > > > access to arbitrary QMP commands, provided your guest is run by libvirt.
> > > > 
> > > > > may be, we need third type of monitor HQMP which is QMP with simplified
> > > > > syntax? Or
> > > > > allow qmp commands in simplified syntax directly in HMP?
> > > > No, I don't think we need either thing.  Wrappers around existing
> > > > monitors is better than bloating qemu proper with a third flavor of
> > > > monitor.  And HMP is for humans, with no restrictions on back-compat
> > > > changes, so if it doesn't do something you want for quick-and-dirty
> > > > testing, you can either add a new HMP command, or just use QMP (or one
> > > > of its wrappers, like qmp-shell) in the first place.  Ultimately, our
> > > > long-term concern is only about the QMP interface; HMP is supposed to be
> > > > convenient.  So if it starts costing too much time to port a QMP
> > > > interface to HMP, then don't worry about it.
> > > > 
> > > most of commands, ported to hmp are done in same style: they just call
> > > corresponding qmp command.
> > > Isn't it better to provide common interface for calling qmp commands through
> > > HMP monitor, to never
> > > create hmp versions of new commands? they will be available automatically.
> > It would be nice to do that, but they're not that consistent in how they
> > convert parameters and options, but I occasionally wonder if we could
> > automate more of it.
> 
> 
> What about allowing some new syntax in hmp, directly mapped to qmp?
> 
> something like
> 
> >>> blockdev-add id disk driver qcow2 cache {writeback true direct true} aio
> native discard unmap file {driver file filename /tmp/somedisk}
> 
> ?

Hmm, I don't particularly find that easy to read either; however the
actual block device specification for HMP should be the same as what we
pass on the command line, so we only have to worry about any extra
things that are part of blockdev_add.
(I'm sure we can find a way of making the one we pass on the commandline
more readable as well, there's so much duplication).

Dave

> Or it may be realized as a separate hmp command "qmp" (looks more safe as a
> first step, however, I think previous variant (direct call) is better):
> 
> >>> qmp blockdev-add id disk driver qcow2 cache {writeback true direct true}
> aio native discard unmap file {driver file filename /tmp/somedisk}
> 
> what do think? This looks simple to implement and should be useful.
> 
> > 
> > Dave
> > 
> > > -- 
> > > Best regards,
> > > Vladimir
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add nbd-server-remove

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
>> 26.01.2018 18:05, Dr. David Alan Gilbert wrote:
>> > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote:
[...]
>> > > most of commands, ported to hmp are done in same style: they just call
>> > > corresponding qmp command.

HMP commands *should* call the QMP command to do the actual work.  That
way, we *know* all the functionality is available in QMP, and HMP is
consistent with it.

Sometimes, calling helpers shared with QMP is more convenient, and
that's okay, but then you have to think about QMP completeness and
HMP/QMP consistency.

The only exception are HMP commands that don't make sense in QMP, such
as @cpu.

>> > > Isn't it better to provide common interface for calling qmp commands through
>> > > HMP monitor, to never
>> > > create hmp versions of new commands? they will be available automatically.
>> > It would be nice to do that, but they're not that consistent in how they
>> > convert parameters and options, but I occasionally wonder if we could
>> > automate more of it.
>> 
>> 
>> What about allowing some new syntax in hmp, directly mapped to qmp?
>> 
>> something like
>> 
>> >>> blockdev-add id disk driver qcow2 cache {writeback true direct true} aio
>> native discard unmap file {driver file filename /tmp/somedisk}
>> 
>> ?
>
> Hmm, I don't particularly find that easy to read either; however the
> actual block device specification for HMP should be the same as what we
> pass on the command line, so we only have to worry about any extra
> things that are part of blockdev_add.
> (I'm sure we can find a way of making the one we pass on the commandline
> more readable as well, there's so much duplication).

Good points.

QMP syntax is different for a good reason: it serves machines rather
than humans.

Both HMP and command line serve the same humans, yet the syntax they
wrap around common functionality is different.  Sad waste of developer
time, sad waste of user brain power.  The former could perhaps be
reduced with better tooling, say having QAPI generate the details.

If you have QAPI generate HMP and command line from the exact same
definitions as QMP, you get what Vladimir wants: different interfaces to
the exact same functionality, without additional coding.

Note that the needs of humans and machines differ in more ways than just
syntax.  For instance, humans appreciate convenience features to save
typing.  In a machine interface, they'd add unnecessary and
inappropriate complexity.  Adding convenience is a good reason for
actually designing the HMP interface, rather than copying the QMP one
blindly.