From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cAohn-0001fN-Ij for qemu-devel@nongnu.org; Sat, 26 Nov 2016 20:56:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cAohm-0006kn-6a for qemu-devel@nongnu.org; Sat, 26 Nov 2016 20:56:35 -0500 From: Max Reitz Date: Sun, 27 Nov 2016 02:55:58 +0100 Message-Id: <20161127015622.24105-1-mreitz@redhat.com> Subject: [Qemu-devel] [PATCH v2 00/24] block: Fix some filename generation issues List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, Max Reitz , Kevin Wolf , Alberto Garcia , Eric Blake Finally, a year after I (allegedly) started working on this, here is v2! Rejoice! There are some issues regarding filename generation right now: - You always get a JSON filename if you set even a single qcow2-specific runtime options (as long as it does not have a dot in it, which is a bug, too, but here it is working in our favor...). That is not nice and actually breaks the usage of backing files with relative filenames with such qcow2 BDS. - As hinted above, you cannot use relative backing filenames with BDS that have a JSON filename only, even though qemu might be able to obtain the directory name by walking through the BDS graph to the protocol level. - Overriding the backing file at runtime should invalidate the filename because it actually changes the BDS's data. Therefore, we need to force a JSON filename in that case, containing the backing file override. - Much of our code assumes paths never to exceed PATH_MAX in length. This is wrong, at least because of JSON filenames. This should be fixed wherever the opportunity arises. - If a driver decides to implement bdrv_refresh_filename(), that implementation has to not only refresh the filename (as one would think) but it must also refresh the runtime options (bs->full_open_options). That is stupid. (I'm allowed to say that because I'm to blame for it.) This series is enclosed by four patches (two at the front, two at the back) that fix more or less general issues. They are included because: - Patch 1 is required so that in patch 3 it's obvious why we don't need to set backing_overriden there or call bdrv_refresh_filename() - Patch 2 is already reviewed, so I might just as well keep it. - Patches 23 and 24 are basically general bug fixes. Their connection to this series is obvious, however, I think, and they depend on the rest of the series, so I decided to just put them in. Patches 3 and 4 address the third issue above. Patches 5 to 9 address the fourth issue above, and are also necessary preparation for the following patches. Patches 10 to 16 address the second issue above, patch 17 adds a test case. They implement a bdrv_dirname() function that returns the base directory of a BDS by walking through the BDS graph to the protocol layer and then trying to obtain a path based on that BDS's exact_filename. This obviously fails if exact_filename on the protocol layer is not set. This behavior can be overriden either by any block driver along the way implementing bdrv_dirname() itself or by the user through the new 'base-directory' node option. This may allow us to resolve relative filenames even if the reference BDS only has a JSON filename. Patches 18 to 22 address both the first and last issues above. They add a field called "sgfnt_runtime_opts" to the BlockDriver structure. Block drivers may point this to an array containing all of the runtime options they accept that may change their BDS's data (i.e. that are "significant"). bdrv_refresh_filename() will use this list to generate bs->full_open_options itself (with only a little help by the block driver, if necessary, through the .bdrv_gather_child_options() function). This not only simplifies the process significantly, but also results in the default implementation generating JSON filenames only when really necessary. v2: Much has been rewritten, so I cannot say much about differences to v1. Anyway, here is the backport-diff: Key: [----] : patches are identical [####] : number of functional differences between v1/v2 patch [down] : patch is v2-only The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively 001/24:[down] 'block/mirror: Small absolute-paths simplification' 002/24:[----] [--] 'block: Use children list in bdrv_refresh_filename' 003/24:[0032] [FC] 'block: Add BDS.backing_overridden' 004/24:[0006] [FC] 'block: Respect backing bs in bdrv_refresh_filename' 005/24:[----] [-C] 'block: Make path_combine() return the path' 006/24:[----] [-C] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.' 007/24:[0008] [FC] 'block: bdrv_get_full_backing_filename's ret. val.' 008/24:[----] [-C] 'block: Add bdrv_make_absolute_filename()' 009/24:[----] [--] 'block: Fix bdrv_find_backing_image()' 010/24:[0002] [FC] 'block: Add bdrv_dirname()' 011/24:[----] [--] 'blkverify: Make bdrv_dirname() return NULL' 012/24:[----] [--] 'quorum: Make bdrv_dirname() return NULL' 013/24:[0034] [FC] 'block/nbd: Implement bdrv_dirname()' 014/24:[down] 'block/nfs: Implement bdrv_dirname()' 015/24:[----] [--] 'block: Use bdrv_dirname() for relative filenames' 016/24:[0005] [FC] 'block: Add 'base-directory' BDS option' 017/24:[0009] [FC] 'iotests: Add quorum case to test 110' 018/24:[down] 'block: Add sgfnt_runtime_opts to BlockDriver' 019/24:[down] 'block: Add BlockDriver.bdrv_gather_child_options' 020/24:[down] 'block: Generically refresh runtime options' 021/24:[down] 'block: Purify .bdrv_refresh_filename()' 022/24:[down] 'block: Do not copy exact_filename from format file' 023/24:[down] 'block/curl: Implement bdrv_refresh_filename()' 024/24:[down] 'block/null: Generate filename even with latency-ns' Now, at least comments for the functionally differing patches: - Patch 3: Basically rewritten. - Patch 4: Now uses qdict_put() instead of qdict_put_obj(); doesn't really matter since that portion of code will be rewritten later in the series anyway (patches 20+21). - Patch 7: *Very* old rebase conflict. - Patch 10: Not really a difference there. - Patch 13: More or less rewritten (not least due to rebase conflicts). - Patch 16: Now actually adhere to 80 characters/line, and s/2\.7/2.9/ - Patch 17: I think at some point in time John fixed the issue where you couldn't use qemu-img info on images whose absolute backing filename could not be determined. Anyway, it was broken in v1 and it is fixed now, so the reference output changed. Max Reitz (24): block/mirror: Small absolute-paths simplification block: Use children list in bdrv_refresh_filename block: Add BDS.backing_overridden block: Respect backing bs in bdrv_refresh_filename 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: Implement bdrv_dirname() block/nfs: Implement bdrv_dirname() block: Use bdrv_dirname() for relative filenames block: Add 'base-directory' BDS option iotests: Add quorum case to test 110 block: Add sgfnt_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/curl: Implement bdrv_refresh_filename() block/null: Generate filename even with latency-ns block.c | 421 ++++++++++++++++++++++++++---------------- block/blkdebug.c | 50 ++--- block/blkverify.c | 30 ++- block/curl.c | 38 ++++ block/gluster.c | 19 ++ block/mirror.c | 10 +- block/nbd.c | 54 ++++-- block/nfs.c | 50 ++--- block/null.c | 33 +++- block/qapi.c | 12 +- block/quorum.c | 62 ++++--- block/rbd.c | 2 + block/vmdk.c | 24 ++- block/vvfat.c | 4 + blockdev.c | 16 ++ include/block/block.h | 15 +- include/block/block_int.h | 14 +- qapi/block-core.json | 9 + tests/qemu-iotests/051.out | 8 +- tests/qemu-iotests/051.pc.out | 8 +- tests/qemu-iotests/110 | 51 ++++- tests/qemu-iotests/110.out | 14 +- 22 files changed, 609 insertions(+), 335 deletions(-) -- 2.10.2