Testing success paths is important, but it's also nice to highlight
expected failure handling, to show that we don't crash, and so that
upcoming tests that change behavior can demonstrate the resulting
effects on error paths.

Add the following errors:
Attempting to export without a running server
Attempting to start a second server
Attempting to export a bad node name
Attempting to export a name that is already exported
Attempting to export an enabled bitmap
Attempting to clean an already cleaned export
Attempting to quit server a second time

All of these properly complain except for a second server-stop,
which will be fixed next.

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

---
v3: split out unrelated error path testing and expand [Vladimir]
---
 tests/qemu-iotests/223     | 19 +++++++++++++++++--
 tests/qemu-iotests/223.out |  7 +++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 5513dc62159..61b46a2f066 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -107,6 +107,7 @@ echo

 _launch_qemu 2> >(_filter_nbd)

+# Intentionally provoke some errors as well, to check error handling
 silent=
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
@@ -114,17 +115,28 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
-  "arguments":{"node":"n", "name":"b2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "error" # Attempt add without server
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
     "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "error" # Attempt to export same name twice
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
   "arguments":{"name":"n", "bitmap":"b"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
+  "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Attempt enabled bitmap
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
+  "arguments":{"node":"n", "name":"b2"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap",
   "arguments":{"name":"n2", "bitmap":"b2"}}' "return"

@@ -157,7 +169,10 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
   "arguments":{"name":"n"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
   "arguments":{"name":"n2"}}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" # Oops
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"

 # success, all done
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 99ca172fbb8..e6ede0591cd 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -27,10 +27,15 @@ wrote 2097152/2097152 bytes at offset 2097152
 {"return": {}}
 {"return": {}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "NBD server not running"}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "NBD server already running"}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
+{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
 {"return": {}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}}
 {"return": {}}
 {"return": {}}

@@ -62,6 +67,8 @@ read 2097152/2097152 bytes at offset 2097152

 {"return": {}}
 {"return": {}}
+{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
+{"return": {}}
 {"return": {}}
 {"return": {}}
 *** done
-- 
2.20.1

11.01.2019 22:47, Eric Blake wrote:
> Testing success paths is important, but it's also nice to highlight
> expected failure handling, to show that we don't crash, and so that
> upcoming tests that change behavior can demonstrate the resulting
> effects on error paths.
> 
> Add the following errors:
> Attempting to export without a running server
> Attempting to start a second server
> Attempting to export a bad node name
> Attempting to export a name that is already exported
> Attempting to export an enabled bitmap
> Attempting to clean an already cleaned export

For me "clean" sounds not the same as "remove" (I used to the thought that
remove means to remove object from larger object, and clean is clean the
object internally (remove its content?), but it continues to exist and all
its relations with parents are not touched), but I'm really far from being
an expert in English.

> Attempting to quit server a second time
> 
> All of these properly complain except for a second server-stop,
> which will be fixed next.

hm, looks a bit strange to add test first and then fix bug, but on the
other hand it's more demonstrative, OK for me.

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


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


-- 
Best regards,
Vladimir

On 1/14/19 2:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 11.01.2019 22:47, Eric Blake wrote:
>> Testing success paths is important, but it's also nice to highlight
>> expected failure handling, to show that we don't crash, and so that
>> upcoming tests that change behavior can demonstrate the resulting
>> effects on error paths.
>>
>> Add the following errors:
>> Attempting to export without a running server
>> Attempting to start a second server
>> Attempting to export a bad node name
>> Attempting to export a name that is already exported
>> Attempting to export an enabled bitmap
>> Attempting to clean an already cleaned export
> 
> For me "clean" sounds not the same as "remove" (I used to the thought that
> remove means to remove object from larger object, and clean is clean the
> object internally (remove its content?), but it continues to exist and all
> its relations with parents are not touched), but I'm really far from being
> an expert in English.

Sure, I'll s/clean/remove/ in that text.

> 
>> Attempting to quit server a second time
>>
>> All of these properly complain except for a second server-stop,
>> which will be fixed next.
> 
> hm, looks a bit strange to add test first and then fix bug, but on the
> other hand it's more demonstrative, OK for me.
> 
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

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