| Message ID | 20180205151835.20812-5-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix some filename generation issues | expand |
On Mon 05 Feb 2018 04:18:13 PM CET, Max Reitz wrote: > Overriding the backing image should result in a json:{} pseudo-filename. > Then, you can no longer use the commit block job with filename > parameters. Therefore, do not explicitly add the base and override the > middle image in iotest 191, since we do not need to anyway. This will > allow us to continue to use the middle image's filename to identify it. > > In the long run, we want block-commit to accept node names for base and > top (just like block-stream does). > > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: > Overriding the backing image should result in a json:{} pseudo-filename. > Then, you can no longer use the commit block job with filename > parameters. Therefore, do not explicitly add the base and override the > middle image in iotest 191, since we do not need to anyway. This will > allow us to continue to use the middle image's filename to identify it. > > In the long run, we want block-commit to accept node names for base and > top (just like block-stream does). > > Signed-off-by: Max Reitz <mreitz@redhat.com> Is this actually needed once the FIXME in patch 3 is addressed and only significant fields are considered, or can we revert the patch at the end of the series? Once we implement a node-named based blockdev-commit, we'll want to test both versions here, so it would be good to be able to set a node-name and still be able to use the filename for the older interface. Kevin
On 2018-02-22 15:34, Kevin Wolf wrote: > Am 05.02.2018 um 16:18 hat Max Reitz geschrieben: >> Overriding the backing image should result in a json:{} pseudo-filename. >> Then, you can no longer use the commit block job with filename >> parameters. Therefore, do not explicitly add the base and override the >> middle image in iotest 191, since we do not need to anyway. This will >> allow us to continue to use the middle image's filename to identify it. >> >> In the long run, we want block-commit to accept node names for base and >> top (just like block-stream does). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > Is this actually needed once the FIXME in patch 3 is addressed and only > significant fields are considered, or can we revert the patch at the end > of the series? The empiric result is "no". :-) In the series's current state, overriding the backing file always means that it is being treated as overridden -- even if you override it with itself and thus the resulting graph is basically the same. I'm still open to suggestions whether we can do anything about this, but the only thing that comes to my mind is: (1) Open bs->backing_file as a BDS. (2) Run bdrv_refresh_filename() on it (done automatically). (3) Compare the result with bs->backing->bs->filename. But you can't just open bs->backing_file twice (that is, have to BDS that refer to the same file), except maybe with BDRV_O_NO_IO. Still, that would be a huge hack. (And isn't guaranteed to work, even with BDRV_O_NO_IO.) So what we'd need is an off-line bdrv_refresh_filename() that constructs a filename from filename+options, without a BDS. But honestly, I don't want to write that just so that we can continue to do blockdev-commit with filenames. I'd rather break it so we get node names there. Max > Once we implement a node-named based blockdev-commit, we'll want to test > both versions here, so it would be good to be able to set a node-name > and still be able to use the filename for the older interface. > > Kevin >
diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191 index dfad6555e4..af93e932bf 100755 --- a/tests/qemu-iotests/191 +++ b/tests/qemu-iotests/191 @@ -66,8 +66,7 @@ qemu_comm_method="qmp" qmp_pretty="y" _launch_qemu \ - -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.base,node-name=base" \ - -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.mid,node-name=mid,backing=base" \ + -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.mid,node-name=mid" \ -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG},node-name=top,backing=mid" \ -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.ovl2,node-name=top2,backing=mid" h=$QEMU_HANDLE diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out index 190c5f049a..575cd3ea2d 100644 --- a/tests/qemu-iotests/191.out +++ b/tests/qemu-iotests/191.out @@ -184,33 +184,21 @@ wrote 65536/65536 bytes at offset 1048576 "iops_rd": 0, "detect_zeroes": "off", "image": { - "backing-image": { - "virtual-size": 67108864, - "filename": "TEST_DIR/t.IMGFMT.base", - "cluster-size": 65536, - "format": "IMGFMT", - "actual-size": SIZE, - "dirty-flag": false - }, - "backing-filename-format": "IMGFMT", "virtual-size": 67108864, - "filename": "TEST_DIR/t.IMGFMT.mid", + "filename": "TEST_DIR/t.IMGFMT.base", "cluster-size": 65536, "format": "IMGFMT", "actual-size": SIZE, - "full-backing-filename": "TEST_DIR/t.IMGFMT.base", - "backing-filename": "TEST_DIR/t.IMGFMT.base", "dirty-flag": false }, "iops_wr": 0, - "ro": false, - "node-name": "mid", - "backing_file_depth": 1, + "ro": true, + "node-name": "NODE_NAME", + "backing_file_depth": 0, "drv": "IMGFMT", "iops": 0, "bps_wr": 0, "write_threshold": 0, - "backing_file": "TEST_DIR/t.IMGFMT.base", "encrypted": false, "bps": 0, "bps_rd": 0, @@ -219,7 +207,7 @@ wrote 65536/65536 bytes at offset 1048576 "direct": false, "writeback": true }, - "file": "TEST_DIR/t.IMGFMT.mid", + "file": "TEST_DIR/t.IMGFMT.base", "encryption_key_missing": false }, { @@ -227,13 +215,13 @@ wrote 65536/65536 bytes at offset 1048576 "detect_zeroes": "off", "image": { "virtual-size": 393216, - "filename": "TEST_DIR/t.IMGFMT.mid", + "filename": "TEST_DIR/t.IMGFMT.base", "format": "file", "actual-size": SIZE, "dirty-flag": false }, "iops_wr": 0, - "ro": false, + "ro": true, "node-name": "NODE_NAME", "backing_file_depth": 0, "drv": "file", @@ -248,28 +236,40 @@ wrote 65536/65536 bytes at offset 1048576 "direct": false, "writeback": true }, - "file": "TEST_DIR/t.IMGFMT.mid", + "file": "TEST_DIR/t.IMGFMT.base", "encryption_key_missing": false }, { "iops_rd": 0, "detect_zeroes": "off", "image": { + "backing-image": { + "virtual-size": 67108864, + "filename": "TEST_DIR/t.IMGFMT.base", + "cluster-size": 65536, + "format": "IMGFMT", + "actual-size": SIZE, + "dirty-flag": false + }, + "backing-filename-format": "IMGFMT", "virtual-size": 67108864, - "filename": "TEST_DIR/t.IMGFMT.base", + "filename": "TEST_DIR/t.IMGFMT.mid", "cluster-size": 65536, "format": "IMGFMT", "actual-size": SIZE, + "full-backing-filename": "TEST_DIR/t.IMGFMT.base", + "backing-filename": "TEST_DIR/t.IMGFMT.base", "dirty-flag": false }, "iops_wr": 0, "ro": false, - "node-name": "base", - "backing_file_depth": 0, + "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, @@ -278,7 +278,7 @@ wrote 65536/65536 bytes at offset 1048576 "direct": false, "writeback": true }, - "file": "TEST_DIR/t.IMGFMT.base", + "file": "TEST_DIR/t.IMGFMT.mid", "encryption_key_missing": false }, { @@ -286,7 +286,7 @@ wrote 65536/65536 bytes at offset 1048576 "detect_zeroes": "off", "image": { "virtual-size": 393216, - "filename": "TEST_DIR/t.IMGFMT.base", + "filename": "TEST_DIR/t.IMGFMT.mid", "format": "file", "actual-size": SIZE, "dirty-flag": false @@ -307,7 +307,7 @@ wrote 65536/65536 bytes at offset 1048576 "direct": false, "writeback": true }, - "file": "TEST_DIR/t.IMGFMT.base", + "file": "TEST_DIR/t.IMGFMT.mid", "encryption_key_missing": false } ]
Overriding the backing image should result in a json:{} pseudo-filename. Then, you can no longer use the commit block job with filename parameters. Therefore, do not explicitly add the base and override the middle image in iotest 191, since we do not need to anyway. This will allow us to continue to use the middle image's filename to identify it. In the long run, we want block-commit to accept node names for base and top (just like block-stream does). Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/191 | 3 +-- tests/qemu-iotests/191.out | 52 +++++++++++++++++++++++----------------------- 2 files changed, 27 insertions(+), 28 deletions(-)