[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 07/11] block: Leave BDS.backing_file constant
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 07/11] block: Leave BDS.backing_file constant |
Date: |
Wed, 14 Nov 2018 21:01:09 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
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 <address@hidden>
>> ---
>> 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 <address@hidden>
>
signature.asc
Description: OpenPGP digital signature