Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Max Reitz]

On 2018-03-06 21:48, Stefan Hajnoczi wrote:
> This test case adds an NBD server export and then invokes
> blockdev-snapshot-sync, which changes the BlockDriverState node that the
> NBD server's BlockBackend points to.  This is an interesting scenario to
> test and exercises the code path fixed by the previous commit.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/208 | 55 
> ++
>  tests/qemu-iotests/208.out |  9 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100755 tests/qemu-iotests/208
>  create mode 100644 tests/qemu-iotests/208.out
> 
> diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
> new file mode 100755
> index 00..4e82b96c82
> --- /dev/null
> +++ b/tests/qemu-iotests/208
> @@ -0,0 +1,55 @@
> +#!/usr/bin/env python
> +#
> +# Copyright (C) 2018 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +# Creator/Owner: Stefan Hajnoczi 
> +#
> +# Check that the runtime NBD server does not crash when stopped after
> +# blockdev-snapshot-sync.
> +
> +import iotests
> +
> +with iotests.FilePath('disk.img') as disk_img_path, \
> + iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
> + iotests.FilePath('nbd.sock') as nbd_sock_path, \
> + iotests.VM() as vm:
> +
> +img_size = '10M'
> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path, 
> img_size)
> +
> +iotests.log('Launching VM...')
> +(vm.add_drive(disk_img_path, 'node-name=drive0-node', interface='none')
> +   .launch())
> +
> +iotests.log('Starting NBD server...')
> +iotests.log(vm.qmp('nbd-server-start', addr={
> +"type": "unix",
> +"data": {
> +"path": nbd_sock_path,
> +}
> +}))
> +
> +iotests.log('Adding NBD export...')
> +iotests.log(vm.qmp('nbd-server-add', device='drive0-node', 
> writable=True))
> +
> +iotests.log('Creating external snapshot...')
> +iotests.log(vm.qmp('blockdev-snapshot-sync',
> +node_name='drive0-node',
> +snapshot_node_name='drive0-snapshot-node',
> +snapshot_file=disk_snapshot_img_path))
> +
> +iotests.log('Stopping NBD server...')
> +iotests.log(vm.qmp('nbd-server-stop'))

Hm.  Tests what it's supposed to test.

However, I'm not sure whether that's what we want...  If I give a node
name to nbd-server-add, I'd probably expect the NBD server to stay at
that node.  We have BdrvChild.stay_at_node for that, so we could
implement it.  The question is whether we want to.

However, both wanting the snapshot to be below and above the NBD server
makes sense...  Perhaps it's best to leave it as-is, even if it is
surprising to some, because this way we at least don't have to switch to
RO mode (which we probably would if the NBD server is suddenly below the
snapshot).

I'm sure adding atomic graph change operations to QMP will fix all of
that.  I'm sure.

Anyway, since your test doesn't even check whether the NBD server is
above or below the snapshot (clever! :-))...

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Eric Blake]

On 03/06/2018 02:48 PM, Stefan Hajnoczi wrote:

This test case adds an NBD server export and then invokes
blockdev-snapshot-sync, which changes the BlockDriverState node that the


Do we want to test 'blockdev-snapshot' instead (or in addition), given 
the subthread discussion about blockdev-snapshot-sync being the older 
non-preferred form?



NBD server's BlockBackend points to.  This is an interesting scenario to
test and exercises the code path fixed by the previous commit.

Signed-off-by: Stefan Hajnoczi 
---
  tests/qemu-iotests/208 | 55 ++
  tests/qemu-iotests/208.out |  9 
  tests/qemu-iotests/group   |  1 +
  3 files changed, 65 insertions(+)
  create mode 100755 tests/qemu-iotests/208
  create mode 100644 tests/qemu-iotests/208.out



Switching the order of the two patches in this series makes it obvious 
that this patch does tickle the code path in question, so you definitely 
get:


Tested-by: Eric Blake 

And unless answering the question about blockdev-snapshot causes you to 
change things for more/different QMP commands, I'm also fine with:


Reviewed-by: Eric Blake 

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

Re: [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefano Panella]

I have applied this patch and when I run the following qmp commands I I do
not see the crash anymore but there is still something wrong because only
/root/a is opened from qemu. It looks like nbd-server-stop is also getting
rid of the nodes added with blockdev-snapshot-sync, therfore is than not
possible to do blockdev-del on /root/d because node-name is not found

{ "execute": "qmp_capabilities" }
{
"execute": "blockdev-add",
"arguments": {
"driver": "qcow2",
"node-name": "/root/a",
"discard": "unmap",
"cache": {
"direct": true
},
"file": {
"driver": "file",
"filename": "/root/a"
}
}
}

{
"execute": "nbd-server-start",
"arguments": {
"addr": {
"type": "unix",
"data": {
"path": "/tmp/nbd.test1"
}
}
}
}

{
"execute": "nbd-server-add",
"arguments": {
"device": "/root/a",
"writable": true
}
}


{
"execute": "blockdev-snapshot-sync",
"arguments": {
"node-name": "/root/a",
"snapshot-node-name": "/root/b",
"snapshot-file": "/root/b"
}
}
{
"execute": "blockdev-snapshot-sync",
"arguments": {
"node-name": "/root/b",
"snapshot-node-name": "/root/c",
"snapshot-file": "/root/c"
}
}
{
"execute": "blockdev-snapshot-sync",
"arguments": {
"node-name": "/root/c",
"snapshot-node-name": "/root/d",
"snapshot-file": "/root/d"
}
}

{
"execute": "nbd-server-stop"
}


On Tue, Mar 6, 2018 at 8:48 PM, Stefan Hajnoczi  wrote:
>
> This test case adds an NBD server export and then invokes
> blockdev-snapshot-sync, which changes the BlockDriverState node that the
> NBD server's BlockBackend points to.  This is an interesting scenario to
> test and exercises the code path fixed by the previous commit.
>
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qemu-iotests/208 | 55
++
>  tests/qemu-iotests/208.out |  9 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100755 tests/qemu-iotests/208
>  create mode 100644 tests/qemu-iotests/208.out
>
> diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
> new file mode 100755
> index 00..4e82b96c82
> --- /dev/null
> +++ b/tests/qemu-iotests/208
> @@ -0,0 +1,55 @@
> +#!/usr/bin/env python
> +#
> +# Copyright (C) 2018 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +# Creator/Owner: Stefan Hajnoczi 
> +#
> +# Check that the runtime NBD server does not crash when stopped after
> +# blockdev-snapshot-sync.
> +
> +import iotests
> +
> +with iotests.FilePath('disk.img') as disk_img_path, \
> + iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
> + iotests.FilePath('nbd.sock') as nbd_sock_path, \
> + iotests.VM() as vm:
> +
> +img_size = '10M'
> +iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path,
img_size)
> +
> +iotests.log('Launching VM...')
> +(vm.add_drive(disk_img_path, 'node-name=drive0-node',
interface='none')
> +   .launch())
> +
> +iotests.log('Starting NBD server...')
> +iotests.log(vm.qmp('nbd-server-start', addr={
> +"type": "unix",
> +"data": {
> +"path": nbd_sock_path,
> +}
> +}))
> +
> +iotests.log('Adding NBD export...')
> +iotests.log(vm.qmp('nbd-server-add', device='drive0-node',
writable=True))
> +
> +iotests.log('Creating external snapshot...')
> +iotests.log(vm.qmp('blockdev-snapshot-sync',
> +node_name='drive0-node',
> +snapshot_node_name='drive0-snapshot-node',
> +snapshot_file=disk_snapshot_img_path))
> +
> +iotests.log('Stopping NBD server...')
> +iotests.log(vm.qmp('nbd-server-stop'))
> diff --git a/tests/qemu-iotests/208.out b/tests/qemu-iotests/208.out
> new file mode 100644
> index 00..3687e9d0dd
> --- /dev/null
> +++ b/tests/qemu-iotests/208.out
> @@ -0,0 +1,9 @@
> +Launching VM...
> +Starting NBD server...
> +{u'return': {}}
> +Adding NBD export...
> +{u'return': {}}
> +Creating external snapshot...
> +{u'return': {}}
> +Stopping NBD server...
> +{u'return': {}}
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index a2dfe79d86..01c03019dd 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -202,3 +202,4 @@
>  203 rw auto
>  204 rw auto quick
>  205 rw auto quick
> +208 rw auto quick
> --
> 2.14.3
>

[Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Stefan Hajnoczi]

This test case adds an NBD server export and then invokes
blockdev-snapshot-sync, which changes the BlockDriverState node that the
NBD server's BlockBackend points to.  This is an interesting scenario to
test and exercises the code path fixed by the previous commit.

Signed-off-by: Stefan Hajnoczi 
---
 tests/qemu-iotests/208 | 55 ++
 tests/qemu-iotests/208.out |  9 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 65 insertions(+)
 create mode 100755 tests/qemu-iotests/208
 create mode 100644 tests/qemu-iotests/208.out

diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
new file mode 100755
index 00..4e82b96c82
--- /dev/null
+++ b/tests/qemu-iotests/208
@@ -0,0 +1,55 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+# Creator/Owner: Stefan Hajnoczi 
+#
+# Check that the runtime NBD server does not crash when stopped after
+# blockdev-snapshot-sync.
+
+import iotests
+
+with iotests.FilePath('disk.img') as disk_img_path, \
+ iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
+ iotests.FilePath('nbd.sock') as nbd_sock_path, \
+ iotests.VM() as vm:
+
+img_size = '10M'
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk_img_path, 
img_size)
+
+iotests.log('Launching VM...')
+(vm.add_drive(disk_img_path, 'node-name=drive0-node', interface='none')
+   .launch())
+
+iotests.log('Starting NBD server...')
+iotests.log(vm.qmp('nbd-server-start', addr={
+"type": "unix",
+"data": {
+"path": nbd_sock_path,
+}
+}))
+
+iotests.log('Adding NBD export...')
+iotests.log(vm.qmp('nbd-server-add', device='drive0-node', writable=True))
+
+iotests.log('Creating external snapshot...')
+iotests.log(vm.qmp('blockdev-snapshot-sync',
+node_name='drive0-node',
+snapshot_node_name='drive0-snapshot-node',
+snapshot_file=disk_snapshot_img_path))
+
+iotests.log('Stopping NBD server...')
+iotests.log(vm.qmp('nbd-server-stop'))
diff --git a/tests/qemu-iotests/208.out b/tests/qemu-iotests/208.out
new file mode 100644
index 00..3687e9d0dd
--- /dev/null
+++ b/tests/qemu-iotests/208.out
@@ -0,0 +1,9 @@
+Launching VM...
+Starting NBD server...
+{u'return': {}}
+Adding NBD export...
+{u'return': {}}
+Creating external snapshot...
+{u'return': {}}
+Stopping NBD server...
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a2dfe79d86..01c03019dd 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -202,3 +202,4 @@
 203 rw auto
 204 rw auto quick
 205 rw auto quick
+208 rw auto quick
-- 
2.14.3