On 13.11.18 00:08, Eric Blake wrote:
> On 8/9/18 5:31 PM, Max Reitz wrote:
>> Parts of the block layer treat BDS.backing_file as if it were whatever
>> the image header says (i.e., if it is a relative path, it is relative to
>> the overlay), other parts treat it like a cache for
>> bs->backing->bs->filename (relative paths are relative to the CWD).
>> Considering bs->backing->bs->filename exists, let us make it mean the
>> former.
>>
>> Among other things, this now allows the user to specify a base when
>> using qemu-img to commit an image file in a directory that is not the
>> CWD (assuming, everything uses relative filenames).
>>
>> Before this patch:
>>
>> $ ./qemu-img create -f qcow2 foo/bot.qcow2 1M
>> $ ./qemu-img create -f qcow2 -b bot.qcow2 foo/mid.qcow2
>> $ ./qemu-img create -f qcow2 -b mid.qcow2 foo/top.qcow2
>> $ ./qemu-img commit -b mid.qcow2 foo/top.qcow2
>> qemu-img: Did not find 'mid.qcow2' in the backing chain of
>> 'foo/top.qcow2'
>> $ ./qemu-img commit -b foo/mid.qcow2 foo/top.qcow2
>> qemu-img: Did not find 'foo/mid.qcow2' in the backing chain of
>> 'foo/top.qcow2'
>> $ ./qemu-img commit -b $PWD/foo/mid.qcow2 foo/top.qcow2
>> qemu-img: Did not find '[...]/foo/mid.qcow2' in the backing chain of
>> 'foo/top.qcow2'
>
> Three failures in a row - no way to commit short of changing your
> working directory.
>
>>
>> After this patch:
>>
>> $ ./qemu-img commit -b mid.qcow2 foo/top.qcow2
>> Image committed.
>> $ ./qemu-img commit -b foo/mid.qcow2 foo/top.qcow2
>> qemu-img: Did not find 'foo/mid.qcow2' in the backing chain of
>> 'foo/top.qcow2'
>> $ ./qemu-img commit -b $PWD/foo/mid.qcow2 foo/top.qcow2
>> Image committed.
>
> Yay, that looks saner.
>
>>
>> With this change, bdrv_find_backing_image() must look at whether the
>> user has overridden a BDS's backing file. If so, it can no longer use
>> bs->backing_file, but must instead compare the given filename against
>> the backing node's filename directly.
>>
>> Note that this changes the QAPI output for a node's backing_file. We
>> had very inconsistent output there (sometimes what the image header
>> said, sometimes the actual filename of the backing image). This
>> inconsistent output was effectively useless, so we have to decide one
>> way or the other. Considering that bs->backing_file usually at runtime
>> contained the path to the image relative to qemu's CWD (or absolute),
>> this patch changes QAPI's backing_file to always report the
>> bs->backing->bs->filename from now on. If you want to receive the image
>> header information, you have to refer to full-backing-filename.
>>
>> This necessitates a change to iotest 228. The interesting information
>> it really wanted is the image header, and it can get that now, but it
>> has to use full-backing-filename instead of backing_file. Because of
>> this patch's changes to bs->backing_file's behavior, we also need some
>> reference output changes.
>>
>> Along with the changes to bs->backing_file, stop updating
>> BDS.backing_format in bdrv_backing_attach() as well. This necessitates
>> a change to the reference output of iotest 191.
>
> Good explanations for the test changes.
>
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> ---
>> include/block/block_int.h | 14 +++++++++-----
>> block.c | 29 ++++++++++++++++++++++-------
>> block/qapi.c | 7 ++++---
>> qemu-img.c | 12 ++++++++++--
>> tests/qemu-iotests/191.out | 1 -
>> tests/qemu-iotests/228 | 6 +++---
>> tests/qemu-iotests/228.out | 6 +++---
>> 7 files changed, 51 insertions(+), 24 deletions(-)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index d3d8b22155..8f2c515ec1 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -737,11 +737,15 @@ struct BlockDriverState {
>> bool walking_aio_notifiers; /* to make removal during iteration
>> safe */
>> char filename[PATH_MAX];
>> - char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
>> - this file image */
>> - /* The backing filename indicated by the image header; if we ever
>> - * open this file, then this is replaced by the resulting BDS's
>> - * filename (i.e. after a bdrv_refresh_filename() run). */
>> + /* If non-zero, the image is a diff of this image file. Note that
>
> Pre-existing, but that sentence might read nicer as:
>
> If not empty, this image is a diff in relation to backing_file.
>
>> + * this the name given in the image header and may therefore not
>
> "this the name" is wrong; did you mean "this is the name" or "this name"
> or "the name"?Would any of the latter two make more grammatical sense? O:-) Will fix. I'll also fix the "may" wording. Like this it sounds as if this is not allowed to be equal to .backing->bs->filename, which of course is not true. It may or may not be equal. So, I'll reword to: "If not empty, this image is a diff in relation to backing_file. Note that this is the name given in the image header and therefore may or may not be equal to .backing->bs->filename. If this field contains a relative path, it is to be resolved relatively to the overlay's location." Thanks for reviewing! Max >> + * be equal to .backing->bs->filename, and relative paths are >> + * resolved relatively to their overlay. */ >> + char backing_file[PATH_MAX]; >> + /* The backing filename indicated by the image header. Contrary >> + * to backing_file, if we ever open this file, auto_backing_file >> + * is replaced by the resulting BDS's filename (i.e. after a >> + * bdrv_refresh_filename() run). */ >> char auto_backing_file[PATH_MAX]; >> char backing_format[16]; /* if non-zero and backing_file exists */ >> > > Reviewed-by: Eric Blake <ebl...@redhat.com> >
signature.asc
Description: OpenPGP digital signature
