QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] block: Fix multiple blockdev-snapshot calls
@ 2019-11-08  8:53 Kevin Wolf
  2019-11-08  8:53 ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options Kevin Wolf
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Kevin Wolf @ 2019-11-08  8:53 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz

Kevin Wolf (2):
  block: Remove 'backing': null from bs->{explicit_,}options
  iotests: Test multiple blockdev-snapshot calls

 block.c                    |   2 +
 tests/qemu-iotests/273     |  76 +++++++++
 tests/qemu-iotests/273.out | 337 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 4 files changed, 416 insertions(+)
 create mode 100755 tests/qemu-iotests/273
 create mode 100644 tests/qemu-iotests/273.out

-- 
2.20.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options
  2019-11-08  8:53 [PATCH 0/2] block: Fix multiple blockdev-snapshot calls Kevin Wolf
@ 2019-11-08  8:53 ` Kevin Wolf
  2019-11-08 14:30   ` Alberto Garcia
  2019-11-12 16:01   ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_,}options Peter Krempa
  2019-11-08  8:53 ` [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls Kevin Wolf
  2019-11-18 16:26 ` [PATCH 0/2] block: Fix " Peter Krempa
  2 siblings, 2 replies; 12+ messages in thread
From: Kevin Wolf @ 2019-11-08  8:53 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz

bs->options and bs->explicit_options shouldn't contain any options for
child nodes. bdrv_open_inherited() takes care to remove any options that
match a child name after opening the image and the same is done when
reopening.

However, we miss the case of 'backing': null, which is a child option,
but results in no child being created. This means that a 'backing': null
remains in bs->options and bs->explicit_options.

A typical use for 'backing': null is in live snapshots: blockdev-add for
the qcow2 overlay makes sure not to open the backing file (because it is
already opened and blockdev-snapshot will attach it). After doing a
blockdev-snapshot, bs->options and bs->explicit_options become
inconsistent with the actual state (bs has a backing file now, but the
options still say null). On the next occasion that the image is
reopened, e.g. switching it from read-write to read-only when another
snapshot is taken, the option will take effect again and the node
incorrectly loses its backing file.

Fix bdrv_open_inherited() to remove the 'backing' option from
bs->options and bs->explicit_options even for the case where it
specifies that no backing file is wanted.

Reported-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index dad5a3d8e0..74ba9acb08 100644
--- a/block.c
+++ b/block.c
@@ -3019,6 +3019,8 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
                         "use \"backing\": null instead");
         }
         flags |= BDRV_O_NO_BACKING;
+        qdict_del(bs->explicit_options, "backing");
+        qdict_del(bs->options, "backing");
         qdict_del(options, "backing");
     }
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls
  2019-11-08  8:53 [PATCH 0/2] block: Fix multiple blockdev-snapshot calls Kevin Wolf
  2019-11-08  8:53 ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options Kevin Wolf
@ 2019-11-08  8:53 ` Kevin Wolf
  2019-11-08 14:32   ` Alberto Garcia
                     ` (2 more replies)
  2019-11-18 16:26 ` [PATCH 0/2] block: Fix " Peter Krempa
  2 siblings, 3 replies; 12+ messages in thread
From: Kevin Wolf @ 2019-11-08  8:53 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pkrempa, qemu-devel, mreitz

Test that doing a second blockdev-snapshot doesn't make the first
overlay's backing file go away.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/273     |  76 +++++++++
 tests/qemu-iotests/273.out | 337 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 414 insertions(+)
 create mode 100755 tests/qemu-iotests/273
 create mode 100644 tests/qemu-iotests/273.out

diff --git a/tests/qemu-iotests/273 b/tests/qemu-iotests/273
new file mode 100755
index 0000000000..60076de7ce
--- /dev/null
+++ b/tests/qemu-iotests/273
@@ -0,0 +1,76 @@
+#!/usr/bin/env bash
+#
+# Test large write to a qcow2 image
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=$(basename "$0")
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This is a qcow2 regression test
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp-pretty stdio -nodefaults "$@"
+    echo
+}
+
+run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp |
+        _filter_generated_node_ids | _filter_imgfmt | _filter_actual_image_size
+}
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 64M
+TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
+_make_test_img -b "$TEST_IMG.mid"
+
+run_qemu \
+    -blockdev file,node-name=base,filename="$TEST_IMG.base" \
+     -blockdev file,node-name=midf,filename="$TEST_IMG.mid" \
+     -blockdev '{"driver":"qcow2","node-name":"mid","file":"midf","backing":null}' \
+     -blockdev file,node-name=topf,filename="$TEST_IMG" \
+     -blockdev '{"driver":"qcow2","file":"topf","node-name":"top","backing":null}' \
+<<EOF
+{"execute":"qmp_capabilities"}
+{"execute":"blockdev-snapshot","arguments":{"node":"base","overlay":"mid"}}
+{"execute":"blockdev-snapshot","arguments":{"node":"mid","overlay":"top"}}
+{"execute":"query-named-block-nodes"}
+{"execute":"x-debug-query-block-graph"}
+{"execute":"quit"}
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/273.out b/tests/qemu-iotests/273.out
new file mode 100644
index 0000000000..c410fee5c4
--- /dev/null
+++ b/tests/qemu-iotests/273.out
@@ -0,0 +1,337 @@
+QA output created by 273
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
+Testing: -blockdev file,node-name=base,filename=TEST_DIR/t.IMGFMT.base -blockdev file,node-name=midf,filename=TEST_DIR/t.IMGFMT.mid -blockdev {"driver":"IMGFMT","node-name":"mid","file":"midf","backing":null} -blockdev file,node-name=topf,filename=TEST_DIR/t.IMGFMT -blockdev {"driver":"IMGFMT","file":"topf","node-name":"top","backing":null}
+{
+    QMP_VERSION
+}
+{
+    "return": {
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "return": [
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "backing-image": {
+                        "virtual-size": 197120,
+                        "filename": "TEST_DIR/t.IMGFMT.base",
+                        "format": "file",
+                        "actual-size": SIZE,
+                        "dirty-flag": false
+                    },
+                    "backing-filename-format": "file",
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.IMGFMT.mid",
+                    "cluster-size": 65536,
+                    "format": "IMGFMT",
+                    "actual-size": SIZE,
+                    "format-specific": {
+                        "type": "IMGFMT",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "full-backing-filename": "TEST_DIR/t.IMGFMT.base",
+                    "backing-filename": "TEST_DIR/t.IMGFMT.base",
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "IMGFMT",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.IMGFMT",
+                "cluster-size": 65536,
+                "format": "IMGFMT",
+                "actual-size": SIZE,
+                "format-specific": {
+                    "type": "IMGFMT",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.IMGFMT.mid",
+                "backing-filename": "TEST_DIR/t.IMGFMT.mid",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "top",
+            "backing_file_depth": 2,
+            "drv": "IMGFMT",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.IMGFMT.mid",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.IMGFMT",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.IMGFMT",
+                "format": "file",
+                "actual-size": SIZE,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "topf",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.IMGFMT",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "virtual-size": 197120,
+                    "filename": "TEST_DIR/t.IMGFMT.base",
+                    "format": "file",
+                    "actual-size": SIZE,
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "file",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.IMGFMT.mid",
+                "cluster-size": 65536,
+                "format": "IMGFMT",
+                "actual-size": SIZE,
+                "format-specific": {
+                    "type": "IMGFMT",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.IMGFMT.base",
+                "backing-filename": "TEST_DIR/t.IMGFMT.base",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": true,
+            "node-name": "mid",
+            "backing_file_depth": 1,
+            "drv": "IMGFMT",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.IMGFMT.base",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.IMGFMT.mid",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.IMGFMT.mid",
+                "format": "file",
+                "actual-size": SIZE,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "midf",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.IMGFMT.mid",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.IMGFMT.base",
+                "format": "file",
+                "actual-size": SIZE,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": true,
+            "node-name": "base",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.IMGFMT.base",
+            "encryption_key_missing": false
+        }
+    ]
+}
+{
+    "return": {
+        "edges": [
+            {
+                "name": "file",
+                "parent": 5,
+                "shared-perm": [
+                    "graph-mod",
+                    "write-unchanged",
+                    "consistent-read"
+                ],
+                "perm": [
+                    "resize",
+                    "write",
+                    "consistent-read"
+                ],
+                "child": 4
+            },
+            {
+                "name": "backing",
+                "parent": 5,
+                "shared-perm": [
+                    "graph-mod",
+                    "resize",
+                    "write-unchanged",
+                    "write",
+                    "consistent-read"
+                ],
+                "perm": [
+                ],
+                "child": 3
+            },
+            {
+                "name": "file",
+                "parent": 3,
+                "shared-perm": [
+                    "graph-mod",
+                    "write-unchanged",
+                    "consistent-read"
+                ],
+                "perm": [
+                    "consistent-read"
+                ],
+                "child": 2
+            },
+            {
+                "name": "backing",
+                "parent": 3,
+                "shared-perm": [
+                    "graph-mod",
+                    "resize",
+                    "write-unchanged",
+                    "write",
+                    "consistent-read"
+                ],
+                "perm": [
+                ],
+                "child": 1
+            }
+        ],
+        "nodes": [
+            {
+                "name": "top",
+                "type": "block-driver",
+                "id": 5
+            },
+            {
+                "name": "topf",
+                "type": "block-driver",
+                "id": 4
+            },
+            {
+                "name": "mid",
+                "type": "block-driver",
+                "id": 3
+            },
+            {
+                "name": "midf",
+                "type": "block-driver",
+                "id": 2
+            },
+            {
+                "name": "base",
+                "type": "block-driver",
+                "id": 1
+            }
+        ]
+    }
+}
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "SHUTDOWN",
+    "data": {
+        "guest": false,
+        "reason": "host-qmp-quit"
+    }
+}
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index af322af756..5aabdfefde 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -282,3 +282,4 @@
 267 rw auto quick snapshot
 268 rw auto quick
 270 rw backing quick
+273 backing quick
-- 
2.20.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options
  2019-11-08  8:53 ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options Kevin Wolf
@ 2019-11-08 14:30   ` Alberto Garcia
  2019-11-12 16:01   ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_,}options Peter Krempa
  1 sibling, 0 replies; 12+ messages in thread
From: Alberto Garcia @ 2019-11-08 14:30 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: kwolf, qemu-devel, mreitz

On Fri 08 Nov 2019 09:53:11 AM CET, Kevin Wolf wrote:
> bs->options and bs->explicit_options shouldn't contain any options for
> child nodes. bdrv_open_inherited() takes care to remove any options that
> match a child name after opening the image and the same is done when
> reopening.
>
> However, we miss the case of 'backing': null, which is a child option,
> but results in no child being created. This means that a 'backing': null
> remains in bs->options and bs->explicit_options.
>
> A typical use for 'backing': null is in live snapshots: blockdev-add for
> the qcow2 overlay makes sure not to open the backing file (because it is
> already opened and blockdev-snapshot will attach it). After doing a
> blockdev-snapshot, bs->options and bs->explicit_options become
> inconsistent with the actual state (bs has a backing file now, but the
> options still say null). On the next occasion that the image is
> reopened, e.g. switching it from read-write to read-only when another
> snapshot is taken, the option will take effect again and the node
> incorrectly loses its backing file.
>
> Fix bdrv_open_inherited() to remove the 'backing' option from
> bs->options and bs->explicit_options even for the case where it
> specifies that no backing file is wanted.
>
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls
  2019-11-08  8:53 ` [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls Kevin Wolf
@ 2019-11-08 14:32   ` Alberto Garcia
  2019-11-13  9:28     ` Kevin Wolf
  2019-11-12 16:07   ` Peter Krempa
  2019-11-18 18:03   ` Max Reitz
  2 siblings, 1 reply; 12+ messages in thread
From: Alberto Garcia @ 2019-11-08 14:32 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: kwolf, qemu-devel, mreitz

On Fri 08 Nov 2019 09:53:12 AM CET, Kevin Wolf wrote:
> +# Test large write to a qcow2 image

This doesn't belong here I guess :)

I wonder if this test could go in 245 instead.

Berto


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_,}options
  2019-11-08  8:53 ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options Kevin Wolf
  2019-11-08 14:30   ` Alberto Garcia
@ 2019-11-12 16:01   ` Peter Krempa
  1 sibling, 0 replies; 12+ messages in thread
From: Peter Krempa @ 2019-11-12 16:01 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz

On Fri, Nov 08, 2019 at 09:53:11 +0100, Kevin Wolf wrote:
> bs->options and bs->explicit_options shouldn't contain any options for
> child nodes. bdrv_open_inherited() takes care to remove any options that
> match a child name after opening the image and the same is done when
> reopening.
> 
> However, we miss the case of 'backing': null, which is a child option,
> but results in no child being created. This means that a 'backing': null
> remains in bs->options and bs->explicit_options.
> 
> A typical use for 'backing': null is in live snapshots: blockdev-add for
> the qcow2 overlay makes sure not to open the backing file (because it is

Note that we also use '"backing": null' as a terminator for the last
image in the chain if the user configures the chain manually.

This is kind-of a protection from opening the backing file from the
header if it was misconfigured somehow. I think this functionality
should be kept despite probably not making practical sense.

In my testing this scenario worked properly.

> already opened and blockdev-snapshot will attach it). After doing a
> blockdev-snapshot, bs->options and bs->explicit_options become
> inconsistent with the actual state (bs has a backing file now, but the
> options still say null). On the next occasion that the image is
> reopened, e.g. switching it from read-write to read-only when another
> snapshot is taken, the option will take effect again and the node
> incorrectly loses its backing file.
> 
> Fix bdrv_open_inherited() to remove the 'backing' option from
> bs->options and bs->explicit_options even for the case where it
> specifies that no backing file is wanted.
> 
> Reported-by: Peter Krempa <pkrempa@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

The fix looks sane-enough to me and works as expected, but since I'm not
familiar enough with this code I'm comfortable only with a:

Tested-by: Peter Krempa <pkrempa@redhat.com>

> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls
  2019-11-08  8:53 ` [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls Kevin Wolf
  2019-11-08 14:32   ` Alberto Garcia
@ 2019-11-12 16:07   ` Peter Krempa
  2019-11-13  9:25     ` Kevin Wolf
  2019-11-18 18:03   ` Max Reitz
  2 siblings, 1 reply; 12+ messages in thread
From: Peter Krempa @ 2019-11-12 16:07 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz

On Fri, Nov 08, 2019 at 09:53:12 +0100, Kevin Wolf wrote:
> Test that doing a second blockdev-snapshot doesn't make the first
> overlay's backing file go away.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/273     |  76 +++++++++
>  tests/qemu-iotests/273.out | 337 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 414 insertions(+)
>  create mode 100755 tests/qemu-iotests/273
>  create mode 100644 tests/qemu-iotests/273.out

Didn't apply cleanly for me.

> 
> diff --git a/tests/qemu-iotests/273 b/tests/qemu-iotests/273
> new file mode 100755
> index 0000000000..60076de7ce
> --- /dev/null
> +++ b/tests/qemu-iotests/273
> @@ -0,0 +1,76 @@
> +#!/usr/bin/env bash
> +#
> +# Test large write to a qcow2 image

Cut&paste?


Rest looks good

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

> +#
> +# Copyright (C) 2019 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +seq=$(basename "$0")
> +echo "QA output created by $seq"
> +
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +# This is a qcow2 regression test
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +
> +do_run_qemu()
> +{
> +    echo Testing: "$@"
> +    $QEMU -nographic -qmp-pretty stdio -nodefaults "$@"

-qmp-pretty, that's useful

> +    echo
> +}
> +
> +run_qemu()
> +{
> +    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp |
> +        _filter_generated_node_ids | _filter_imgfmt | _filter_actual_image_size
> +}
> +
> +TEST_IMG="$TEST_IMG.base" _make_test_img 64M
> +TEST_IMG="$TEST_IMG.mid" _make_test_img -b "$TEST_IMG.base"
> +_make_test_img -b "$TEST_IMG.mid"
> +
> +run_qemu \
> +    -blockdev file,node-name=base,filename="$TEST_IMG.base" \
> +     -blockdev file,node-name=midf,filename="$TEST_IMG.mid" \
> +     -blockdev '{"driver":"qcow2","node-name":"mid","file":"midf","backing":null}' \
> +     -blockdev file,node-name=topf,filename="$TEST_IMG" \
> +     -blockdev '{"driver":"qcow2","file":"topf","node-name":"top","backing":null}' \
> +<<EOF
> +{"execute":"qmp_capabilities"}
> +{"execute":"blockdev-snapshot","arguments":{"node":"base","overlay":"mid"}}
> +{"execute":"blockdev-snapshot","arguments":{"node":"mid","overlay":"top"}}
> +{"execute":"query-named-block-nodes"}
> +{"execute":"x-debug-query-block-graph"}

Oh, this too!

> +{"execute":"quit"}
> +EOF
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls
  2019-11-12 16:07   ` Peter Krempa
@ 2019-11-13  9:25     ` Kevin Wolf
  0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2019-11-13  9:25 UTC (permalink / raw)
  To: Peter Krempa; +Cc: qemu-devel, qemu-block, mreitz

Am 12.11.2019 um 17:07 hat Peter Krempa geschrieben:
> On Fri, Nov 08, 2019 at 09:53:12 +0100, Kevin Wolf wrote:
> > Test that doing a second blockdev-snapshot doesn't make the first
> > overlay's backing file go away.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/273     |  76 +++++++++
> >  tests/qemu-iotests/273.out | 337 +++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 414 insertions(+)
> >  create mode 100755 tests/qemu-iotests/273
> >  create mode 100644 tests/qemu-iotests/273.out
> 
> Didn't apply cleanly for me.
> 
> > 
> > diff --git a/tests/qemu-iotests/273 b/tests/qemu-iotests/273
> > new file mode 100755
> > index 0000000000..60076de7ce
> > --- /dev/null
> > +++ b/tests/qemu-iotests/273
> > @@ -0,0 +1,76 @@
> > +#!/usr/bin/env bash
> > +#
> > +# Test large write to a qcow2 image
> 
> Cut&paste?

Indeed. I'll change it like this:

# Test multiple blockdev-snapshot calls with 'backing': null

> Rest looks good
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Thanks!

Kevin



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls
  2019-11-08 14:32   ` Alberto Garcia
@ 2019-11-13  9:28     ` Kevin Wolf
  0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2019-11-13  9:28 UTC (permalink / raw)
  To: Alberto Garcia; +Cc: qemu-devel, qemu-block, mreitz

Am 08.11.2019 um 15:32 hat Alberto Garcia geschrieben:
> On Fri 08 Nov 2019 09:53:12 AM CET, Kevin Wolf wrote:
> > +# Test large write to a qcow2 image
> 
> This doesn't belong here I guess :)

Yes, fixed.

> I wonder if this test could go in 245 instead.

The headline for 245 is "Test cases for the QMP 'x-blockdev-reopen'
command", so I don't think so?

We end up testing the reopen code, but only as a side effect of
snapshotting, not through x-blockdev-reopen.

Kevin



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] block: Fix multiple blockdev-snapshot calls
  2019-11-08  8:53 [PATCH 0/2] block: Fix multiple blockdev-snapshot calls Kevin Wolf
  2019-11-08  8:53 ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options Kevin Wolf
  2019-11-08  8:53 ` [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls Kevin Wolf
@ 2019-11-18 16:26 ` " Peter Krempa
  2019-11-18 16:49   ` Kevin Wolf
  2 siblings, 1 reply; 12+ messages in thread
From: Peter Krempa @ 2019-11-18 16:26 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mreitz

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

On Fri, Nov 08, 2019 at 09:53:10 +0100, Kevin Wolf wrote:
> Kevin Wolf (2):
>   block: Remove 'backing': null from bs->{explicit_,}options
>   iotests: Test multiple blockdev-snapshot calls

Hi,

I'm not sure how the freeze rules for qemu are at this point thus:

Will this patchset make it into 4.2. I argue it's a pretty important fix
as it ends up in image corruption.

If it won't make into we will need to add a QMP feature flag to detect
presence of the fix so that I can gate libvirt's support of blockdev
with it. (This can also be done if we make it into 4.2 but I'll be okay
infering it from other fixes present in 4.2).

Thanks

Peter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] block: Fix multiple blockdev-snapshot calls
  2019-11-18 16:26 ` [PATCH 0/2] block: Fix " Peter Krempa
@ 2019-11-18 16:49   ` Kevin Wolf
  0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2019-11-18 16:49 UTC (permalink / raw)
  To: Peter Krempa; +Cc: qemu-devel, qemu-block, mreitz

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

Am 18.11.2019 um 17:26 hat Peter Krempa geschrieben:
> On Fri, Nov 08, 2019 at 09:53:10 +0100, Kevin Wolf wrote:
> > Kevin Wolf (2):
> >   block: Remove 'backing': null from bs->{explicit_,}options
> >   iotests: Test multiple blockdev-snapshot calls
> 
> Hi,
> 
> I'm not sure how the freeze rules for qemu are at this point thus:
> 
> Will this patchset make it into 4.2. I argue it's a pretty important fix
> as it ends up in image corruption.
> 
> If it won't make into we will need to add a QMP feature flag to detect
> presence of the fix so that I can gate libvirt's support of blockdev
> with it. (This can also be done if we make it into 4.2 but I'll be okay
> infering it from other fixes present in 4.2).

Yes, this is planned for 4.2. I'm in the process of preparing a pull
request right now.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls
  2019-11-08  8:53 ` [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls Kevin Wolf
  2019-11-08 14:32   ` Alberto Garcia
  2019-11-12 16:07   ` Peter Krempa
@ 2019-11-18 18:03   ` Max Reitz
  2 siblings, 0 replies; 12+ messages in thread
From: Max Reitz @ 2019-11-18 18:03 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: pkrempa, qemu-devel

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

On 08.11.19 09:53, Kevin Wolf wrote:
> Test that doing a second blockdev-snapshot doesn't make the first
> overlay's backing file go away.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/273     |  76 +++++++++
>  tests/qemu-iotests/273.out | 337 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 414 insertions(+)
>  create mode 100755 tests/qemu-iotests/273
>  create mode 100644 tests/qemu-iotests/273.out

[...]

> +                "format-specific": {
> +                    "type": "IMGFMT",
> +                    "data": {
> +                        "compat": "1.1",
> +                        "lazy-refcounts": false,
> +                        "refcount-bits": 16,
> +                        "corrupt": false
> +                    }

I get a mismatch here with -o compat=0.10 or -o refcount_bits=1.

Max


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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08  8:53 [PATCH 0/2] block: Fix multiple blockdev-snapshot calls Kevin Wolf
2019-11-08  8:53 ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_, }options Kevin Wolf
2019-11-08 14:30   ` Alberto Garcia
2019-11-12 16:01   ` [PATCH 1/2] block: Remove 'backing': null from bs->{explicit_,}options Peter Krempa
2019-11-08  8:53 ` [PATCH 2/2] iotests: Test multiple blockdev-snapshot calls Kevin Wolf
2019-11-08 14:32   ` Alberto Garcia
2019-11-13  9:28     ` Kevin Wolf
2019-11-12 16:07   ` Peter Krempa
2019-11-13  9:25     ` Kevin Wolf
2019-11-18 18:03   ` Max Reitz
2019-11-18 16:26 ` [PATCH 0/2] block: Fix " Peter Krempa
2019-11-18 16:49   ` Kevin Wolf

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git