Re: [Qemu-devel] [Qemu-block] [PATCH v9 00/31] block: Fix some filename generation issues

[Ari Sundholm <ari@tuxera.com>]

On 07/13/2018 12:49 PM, Max Reitz wrote:
> Ping – any thoughts on the design, Kevin?
> 
> 
> (Continuing to see how much of a mess our backing filename handling is
> (half of the time, bs->backing_file is seen as a value in the image
> header (so relative paths are interpreted relatively to the overlay),
> half of the time it is seen as a cache of bs->backing->bs->filename (so
> relative paths are given relatively to qemu)), I am nearly inclined to
> move off the first part of this series that deals with detecting changed
> backing files until later, when I try to tackle that bs->backing_file
> issue.)
> 

I'm not Kevin, but it seems this series needs updating to accommodate 
the recent addition of the blklogwrites block driver.

Best regards,
Ari Sundholm
ari@tuxera.com

> 
> On 2018-06-28 02:07, Max Reitz wrote:
>> Once more, I’ll spare both me and you another iteration of the cover
>> letter, so see here:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
>>
>> (Although this series no longer includes a @base-directory option.)
>>
>> In regards to the last version, the biggest change is that I dropped
>> backing_overridden and instead try to compare the filename from the
>> image header with the filename of the actual backing BDS to find out
>> whether the backing file has been overridden.
>>
>> In order that this doesn’t break whenever the header contains a slightly
>> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
>> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
>> different from what bdrv_refresh_filename() would generate), when the
>> reference filename in the BDS (auto_backing_file) is used to open the
>> backing file, it is updated from the backing BDS's resulting filename.
>>
>>
>> All (I hope) changes in v9:
>> - Rebase (warranting an NVME patch)
>>
>> - Dropped backing_overridden, switched to the comparison described above
>>    (and dealt with the fallout, for example test 191 can now stay
>>    unchanged)
>>
>> - Removed all existing bdrv_refresh_filename() calls and moved them to
>>    the users of BDS.filename (first patch) -- otherwise, I got iotest
>>    failure (because it's hard to reflect all graph changes properly with
>>    a “pushing” bdrv_refresh_filename()), and I don't even want to
>>    remember how 191 broke without this change.
>>
>> - Renamed “significant options” to “strong options”
>>
>> - Implicit nodes should be completely skipped in
>>    bdrv_refresh_filename(), including setting of bs->full_open_options
>>    (patch 3)
>>
>> - vmdk_gather_child_options() failed to set backing=null when the image
>>    header asked for a backing file, but the user forbid its use
>>
>> - Added the penultimate patch which makes json:{} filenames for explicit
>>    internal nodes kind of working?
>>    (When you specify @filter-node e.g. for block-commit, the node should
>>    be visible, so it may appear in a json:{} filename; but creating that
>>    node did not take a real options QDict, so it was lacking the @driver
>>    option, and that was a bit sad.)
>>
>> - Dropped a superfluous “suspend.” in blkdebug
>>
>> - Dropped the first patch of v8
>>
>> - Restyled some comments (“/*” on its own line where “*/” is on its own
>>    line)
>>
>> - Fixed mention of "NBD" in the NFS patch (18)
>>
>>
>> git-backport-diff against v8:
>>
>> Key:
>> [----] : patches are identical
>> [####] : number of functional differences between upstream/downstream patch
>> [down] : patch is downstream-only
>> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>>
>> 001/31:[down] 'block: Use bdrv_refresh_filename() to pull'
>> 002/31:[----] [--] 'block: Use children list in bdrv_refresh_filename'
>> 003/31:[down] 'block: Skip implicit nodes for filename info'
>> 004/31:[down] 'block: Add BDS.auto_backing_file'
>> 005/31:[0052] [FC] 'block: Respect backing bs in bdrv_refresh_filename'
>> 006/31:[down] 'iotests.py: Add filter_imgfmt()'
>> 007/31:[down] 'iotests.py: Add node_info()'
>> 008/31:[down] 'iotests: Add test for backing file overrides'
>> 009/31:[----] [--] 'block: Make path_combine() return the path'
>> 010/31:[0016] [FC] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.'
>> 011/31:[0001] [FC] 'block: bdrv_get_full_backing_filename's ret. val.'
>> 012/31:[0022] [FC] 'block: Add bdrv_make_absolute_filename()'
>> 013/31:[0003] [FC] 'block: Fix bdrv_find_backing_image()'
>> 014/31:[0001] [FC] 'block: Add bdrv_dirname()'
>> 015/31:[----] [--] 'blkverify: Make bdrv_dirname() return NULL'
>> 016/31:[----] [-C] 'quorum: Make bdrv_dirname() return NULL'
>> 017/31:[----] [-C] 'block/nbd: Make bdrv_dirname() return NULL'
>> 018/31:[0003] [FC] 'block/nfs: Implement bdrv_dirname()'
>> 019/31:[0014] [FC] 'block: Use bdrv_dirname() for relative filenames'
>> 020/31:[----] [--] 'iotests: Add quorum case to test 110'
>> 021/31:[down] 'block: Add strong_runtime_opts to BlockDriver'
>> 022/31:[0037] [FC] 'block: Add BlockDriver.bdrv_gather_child_options'
>> 023/31:[0084] [FC] 'block: Generically refresh runtime options'
>> 024/31:[0076] [FC] 'block: Purify .bdrv_refresh_filename()'
>> 025/31:[0005] [FC] 'block: Do not copy exact_filename from format file'
>> 026/31:[down] 'block/nvme: Fix bdrv_refresh_filename()'
>> 027/31:[----] [--] 'block/curl: Harmonize option defaults'
>> 028/31:[----] [-C] 'block/curl: Implement bdrv_refresh_filename()'
>> 029/31:[----] [--] 'block/null: Generate filename even with latency-ns'
>> 030/31:[down] 'block: BDS options may lack the "driver" option'
>> 031/31:[down] 'iotests: Test json:{} filenames of internal BDSs'
>>
>>
>> Max Reitz (31):
>>    block: Use bdrv_refresh_filename() to pull
>>    block: Use children list in bdrv_refresh_filename
>>    block: Skip implicit nodes for filename info
>>    block: Add BDS.auto_backing_file
>>    block: Respect backing bs in bdrv_refresh_filename
>>    iotests.py: Add filter_imgfmt()
>>    iotests.py: Add node_info()
>>    iotests: Add test for backing file overrides
>>    block: Make path_combine() return the path
>>    block: bdrv_get_full_backing_filename_from_...'s ret. val.
>>    block: bdrv_get_full_backing_filename's ret. val.
>>    block: Add bdrv_make_absolute_filename()
>>    block: Fix bdrv_find_backing_image()
>>    block: Add bdrv_dirname()
>>    blkverify: Make bdrv_dirname() return NULL
>>    quorum: Make bdrv_dirname() return NULL
>>    block/nbd: Make bdrv_dirname() return NULL
>>    block/nfs: Implement bdrv_dirname()
>>    block: Use bdrv_dirname() for relative filenames
>>    iotests: Add quorum case to test 110
>>    block: Add strong_runtime_opts to BlockDriver
>>    block: Add BlockDriver.bdrv_gather_child_options
>>    block: Generically refresh runtime options
>>    block: Purify .bdrv_refresh_filename()
>>    block: Do not copy exact_filename from format file
>>    block/nvme: Fix bdrv_refresh_filename()
>>    block/curl: Harmonize option defaults
>>    block/curl: Implement bdrv_refresh_filename()
>>    block/null: Generate filename even with latency-ns
>>    block: BDS options may lack the "driver" option
>>    iotests: Test json:{} filenames of internal BDSs
>>
>>   include/block/block.h         |  16 +-
>>   include/block/block_int.h     |  48 +++-
>>   block.c                       | 505 ++++++++++++++++++++++------------
>>   block/blkdebug.c              |  70 ++---
>>   block/blkverify.c             |  29 +-
>>   block/commit.c                |   3 +-
>>   block/crypto.c                |   8 +
>>   block/curl.c                  |  55 +++-
>>   block/gluster.c               |  19 ++
>>   block/iscsi.c                 |  18 ++
>>   block/mirror.c                |   3 +-
>>   block/nbd.c                   |  46 ++--
>>   block/nfs.c                   |  54 ++--
>>   block/null.c                  |  32 ++-
>>   block/nvme.c                  |  27 +-
>>   block/qapi.c                  |  16 +-
>>   block/qcow.c                  |  14 +-
>>   block/qcow2.c                 |  17 +-
>>   block/qed.c                   |   7 +-
>>   block/quorum.c                |  71 +++--
>>   block/raw-format.c            |  11 +-
>>   block/rbd.c                   |  14 +
>>   block/replication.c           |  10 +-
>>   block/sheepdog.c              |  12 +
>>   block/ssh.c                   |  12 +
>>   block/throttle.c              |   7 +
>>   block/vhdx-log.c              |   1 +
>>   block/vmdk.c                  |  43 ++-
>>   block/vpc.c                   |  11 +-
>>   block/vvfat.c                 |  12 +
>>   block/vxhs.c                  |  11 +
>>   blockdev.c                    |   8 +
>>   qemu-img.c                    |  12 +-
>>   tests/qemu-iotests/051.out    |   8 +-
>>   tests/qemu-iotests/051.pc.out |   8 +-
>>   tests/qemu-iotests/110        |  29 +-
>>   tests/qemu-iotests/110.out    |   9 +-
>>   tests/qemu-iotests/223        | 235 ++++++++++++++++
>>   tests/qemu-iotests/223.out    |  84 ++++++
>>   tests/qemu-iotests/224        | 139 ++++++++++
>>   tests/qemu-iotests/224.out    |  18 ++
>>   tests/qemu-iotests/group      |   2 +
>>   tests/qemu-iotests/iotests.py |  10 +
>>   43 files changed, 1374 insertions(+), 390 deletions(-)
>>   create mode 100755 tests/qemu-iotests/223
>>   create mode 100644 tests/qemu-iotests/223.out
>>   create mode 100755 tests/qemu-iotests/224
>>   create mode 100644 tests/qemu-iotests/224.out
>>
> 
>