nbd export qmp interface

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 <[email protected]>

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Max Reitz <[email protected]>

---
 nbd/server.c | 6 ++++++
 1 file changed, 6 insertions(+)

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 <[email protected]>

---
 qapi/block.json |  9 +++++++--
 blockdev-nbd.c  | 14 +++++++++-----
 hmp.c           |  5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)

qapi/block.json +7 -2

index f093fa3f27..503d4b287b 100644
@@ -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:

blockdev-nbd.c +9 -5

index 28f551a7b0..46c885aa35 100644
@@ -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

hmp.c +2 -1

index 35a7041824..0ea9c09b58 100644
@@ -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);

hmp.c +0 -0

     if (local_err != NULL) {
         hmp_handle_error(mon, &local_err);

Signature · 1 line

2.11.1

Quoted 95 lines · * Vladimir Sementsov-Ogievskiy ([email protected]) 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 <[email protected]>

---
  qapi/block.json |  9 +++++++--
  blockdev-nbd.c  | 14 +++++++++-----
  hmp.c           |  5 +++--
  3 files changed, 19 insertions(+), 9 deletions(-)

qapi/block.json +7 -2

index f093fa3f27..503d4b287b 100644
@@ -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:

blockdev-nbd.c +9 -5

index 28f551a7b0..46c885aa35 100644
@@ -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

hmp.c +2 -1

index 35a7041824..0ea9c09b58 100644
@@ -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;
 

hmp.c void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) · line 2227

Quoted 2 lines

hmp.c +1 -1

-    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

Quoted 6 lines

hmp.c +0 -0

      if (local_err != NULL) {
          hmp_handle_error(mon, &local_err);

Signature · 1 line

2.11.1

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

hmp.c void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) · line 2227

Quoted 6 lines

hmp.c +1 -1

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

Quoted 4 lines · Dr. David Alan Gilbert

Quoted 4 lines

hmp.c +0 -0

@@ -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;
 

Quoted 5 lines

hmp.c void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) · line 2227

Quoted 2 lines

hmp.c +1 -1

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

Signature · 3 lines

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

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

Signed-off-by Eric Blake <[email protected]>

---

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(-)

hmp.c +2 -1

index 80c5b01540..c9fcdc8593 100644
@@ -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);

hmp-commands.hx +5 -4

index 6d5ebdf6ab..b8b6fb9184 100644
@@ -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
 
     {

Signature · 1 line

2.14.3

Quoted 3 lines · * Eric Blake ([email protected]) wrote:

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

Quoted 1 line

Signed-off-by Eric Blake <[email protected]>

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 <[email protected]>

Quoted 5 lines

---

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

Quoted 11 lines · Dr. David Alan Gilbert

Quoted 4 lines · * Eric Blake ([email protected]) wrote:

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

Signed-off-by Eric Blake <[email protected]>

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.

Quoted 1 line

Reviewed-by Dr. David Alan Gilbert <[email protected]>

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.

Quoted 5 lines

Quoted 5 lines

---

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.

Signature · 4 lines

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

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 <[email protected]>

---
 qapi/block.json |  9 +++++++--
 blockdev-nbd.c  | 14 +++++++++-----
 hmp.c           |  5 +++--
 3 files changed, 19 insertions(+), 9 deletions(-)

qapi/block.json +0 -0

index f093fa3f27..503d4b287b 100644

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 <[email protected]>

---
 qapi/block.json     | 18 ++++++++++++++++++
 include/block/nbd.h |  3 ++-
 blockdev-nbd.c      | 29 ++++++++++++++++++++++++++++-
 nbd/server.c        | 25 ++++++++++++++++++++++---
 4 files changed, 70 insertions(+), 5 deletions(-)

qapi/block.json +0 -0

index 503d4b287b..f83485c325 100644
@@ -228,6 +228,24 @@
   'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
 
 ##

Quoted 22 lines · 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 <[email protected]>

---
  qapi/block.json     | 18 ++++++++++++++++++
  include/block/nbd.h |  3 ++-
  blockdev-nbd.c      | 29 ++++++++++++++++++++++++++++-
  nbd/server.c        | 25 ++++++++++++++++++++++---
  4 files changed, 70 insertions(+), 5 deletions(-)

qapi/block.json +0 -0

index 503d4b287b..f83485c325 100644
@@ -228,6 +228,24 @@
    'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} }
 
  ##

qapi/block.json line 240

Quoted 10 lines

qapi/block.json +10 -0

+# @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)?

qapi/block.json line 247

Quoted 8 lines

qapi/block.json +7 -0

+#
+# 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 <[email protected]>

Quoted 7 lines · Eric Blake

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---

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

Quoted 16 lines · Vladimir Sementsov-Ogievskiy

Quoted 7 lines · Eric Blake

Quoted 6 lines · 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.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---

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?

Quoted 9 lines

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

Quoted 5 lines

new corresponding states of nbd export:
hidden, shutting_down

and we allow transitions:

Quoted 6 lines

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?

Quoted 41 lines · Eric Blake

Quoted 16 lines · Vladimir Sementsov-Ogievskiy

Quoted 7 lines · Eric Blake

Quoted 6 lines · 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.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---

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?

Quoted 9 lines

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

Quoted 11 lines

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

Quoted 3 lines

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

yes, will do.

Quoted 30 lines

Quoted 29 lines

Quoted 11 lines

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.

Quoted 11 lines

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?

Quoted 3 lines

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} |||

Quoted 29 lines · Vladimir Sementsov-Ogievskiy

Quoted 29 lines

Quoted 29 lines

Quoted 11 lines

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.

Quoted 11 lines

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?

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 6 lines

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:

Quoted 6 lines

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

Quoted 10 lines

Quoted 10 lines

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} |||

Quoted 4 lines

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?

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 10 lines

Quoted 10 lines

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} |||

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 28 lines · Eric Blake

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 10 lines

Quoted 10 lines

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} |||

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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?

Quoted 10 lines

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.

Signature · 2 lines

Best regards,
Vladimir

Quoted 39 lines · * Vladimir Sementsov-Ogievskiy ([email protected]) wrote:

Quoted 38 lines · Eric Blake

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 10 lines

Quoted 10 lines

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} |||

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 1 line

Quoted 1 line

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

Quoted 1 line

Quoted 1 line

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

Dave

Quoted 4 lines

Signature · 2 lines

Best regards,
Vladimir

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 8 lines

Quoted 5 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

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):

Quoted 1 line

Quoted 1 line

Quoted 1 line

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.

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

?

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

Quoted 40 lines · Dr. David Alan Gilbert

Quoted 39 lines · * Vladimir Sementsov-Ogievskiy ([email protected]) wrote:

Quoted 38 lines · Eric Blake

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 10 lines

Quoted 10 lines

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} |||

Quoted 6 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

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):

Quoted 1 line

Quoted 1 line

Quoted 1 line

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.

Quoted 1 line

Quoted 8 lines

Dave

Quoted 4 lines

Signature · 2 lines

Best regards,
Vladimir

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Signature · 2 lines

Best regards,
Vladimir

Quoted 2 lines · * Vladimir Sementsov-Ogievskiy ([email protected]) wrote:

Quoted 1 line · Dr. David Alan Gilbert

* Vladimir Sementsov-Ogievskiy ([email protected]) wrote:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

most of commands, ported to hmp are done in same style: they just call corresponding qmp command.

Quoted 15 lines

Quoted 6 lines

Quoted 3 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

blockdev-add id disk driver qcow2 cache {writeback true direct true} aio

native discard unmap file {driver file filename /tmp/somedisk}

Quoted 1 line

?

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

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---
 tests/qemu-iotests/147     | 68 +++++++++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/147.out |  4 +--
 2 files changed, 57 insertions(+), 15 deletions(-)

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

---
 tests/qemu-iotests/iotests.py | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

+ assert c != ''

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.

Quoted 5 lines

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

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

tests/qemu-iotests/iotests.py def qemu_io(*args): · line 102

Quoted 6 lines

tests/qemu-iotests/iotests.py +6 -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)

Why STDOUT instead of STDERR? Is the redirection intentional?

tests/qemu-iotests/iotests.py def qemu_io(*args): · line 115

Quoted 10 lines

tests/qemu-iotests/iotests.py +8 -0

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

Quoted 1 line

+ 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?

tests/qemu-iotests/iotests.py def qemu_io(*args): · line 123

Quoted 8 lines

tests/qemu-iotests/iotests.py +7 -0

+            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?

Quoted 9 lines

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.

Quoted 6 lines

+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)

Quoted 1 line

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.

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

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

tests/qemu-iotests/201 new file +4 -0

+disk = os.path.join(iotests.test_dir, 'disk')
+
+class TestNbdServerRemove(iotests.QMPTestCase):
+    def setUp(self):

tests/qemu-iotests/201 new file +8 -0

+                         "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')

tests/qemu-iotests/201.out new file +2 -0

+
+OK

tests/qemu-iotests/group +1 -0

index 3e688678dd..15df7678b3 100644
@@ -197,3 +197,4 @@
 197 rw auto quick
 198 rw auto
 200 rw auto
+201 rw auto quick

Quoted 9 lines · Vladimir Sementsov-Ogievskiy

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>

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

tests/qemu-iotests/201 line 28

Quoted 8 lines

tests/qemu-iotests/201 new file +8 -0

+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?

Quoted 4 lines

tests/qemu-iotests/201 new file +4 -0

+disk = os.path.join(iotests.test_dir, 'disk')
+
+class TestNbdServerRemove(iotests.QMPTestCase):
+    def setUp(self):

tests/qemu-iotests/201 line 42

Quoted 10 lines

tests/qemu-iotests/201 new file +10 -0

+        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?

tests/qemu-iotests/201 line 77

Quoted 3 lines

tests/qemu-iotests/201 new file +3 -0

+    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?

Quoted 8 lines

tests/qemu-iotests/201 new file +8 -0

+                         "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')

tests/qemu-iotests/201 line 95

Quoted 10 lines

tests/qemu-iotests/201 new file +10 -0

+        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).

tests/qemu-iotests/201.out line 3

Quoted 5 lines

+++ b/tests/qemu-iotests/201.out

tests/qemu-iotests/201.out new file +3 -0

@@ -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

Quoted 13 lines

tests/qemu-iotests/201.out new file +2 -0

+
+OK

tests/qemu-iotests/group +1 -0

index 3e688678dd..15df7678b3 100644
@@ -197,3 +197,4 @@
  197 rw auto quick
  198 rw auto
  200 rw auto
+201 rw auto quick

Quoted 10 lines

tests/qemu-iotests/201.out line 3

Quoted 5 lines

+++ b/tests/qemu-iotests/201.out

tests/qemu-iotests/201.out new file +3 -0

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

Quoted 10 lines · Vladimir Sementsov-Ogievskiy

Quoted 10 lines

tests/qemu-iotests/201.out line 3

Quoted 5 lines

+++ b/tests/qemu-iotests/201.out

tests/qemu-iotests/201.out new file +3 -0

@@ -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

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 12 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 7 lines · Vladimir Sementsov-Ogievskiy

Quoted 5 lines

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

Quoted 4 lines

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

Quoted 14 lines

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

Quoted 11 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

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

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

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

Quoted 1 line · Markus Armbruster

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?)

Quoted 5 lines · Eric Blake

Quoted 1 line · Markus Armbruster

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)