Re: [PATCH v7 41/47] block: Leave BDS.backing_file constant
On 24.08.20 15:14, Kevin Wolf wrote: > Am 25.06.2020 um 17:22 hat Max Reitz geschrieben: >> 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' >> >> 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. >> >> 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. In order not to >> change our externally visible behavior (incompatibly), we have to let >> bdrv_query_image_info() try to get the image format from bs->backing if >> bs->backing_format is unset. (The QAPI schema describes >> backing-filename-format as "the format of the backing file", so it is >> not necessarily what the image header says, but just the format of the >> file referenced by backing-filename (if known).) > > Why is it okay to change backing-filename incompatibly, but not > backing-filename-format? I hope you’re asking the reverse, i.e. why I don’t change backing-filename-format, too. The answer to that is yeah, why not. :) > I would find it much more consistent if > ImageInfo reported the value from the header in both fields, and > BlockDeviceInfo reported the values actually in use. > > The QAPI schema described ImageInfo as "Information about a QEMU image > file" and runtime state really isn't information about an image file. > > If you want to know the probed image format, you can still look at > backing-image.format. I don't think this change is much different from > what you described above for BlockDeviceInfo.backing_file. Well, OK then. Max signature.asc Description: OpenPGP digital signature
Re: [PATCH v7 41/47] block: Leave BDS.backing_file constant
Am 25.06.2020 um 17:22 hat Max Reitz geschrieben: > 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' > > 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. > > 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. In order not to > change our externally visible behavior (incompatibly), we have to let > bdrv_query_image_info() try to get the image format from bs->backing if > bs->backing_format is unset. (The QAPI schema describes > backing-filename-format as "the format of the backing file", so it is > not necessarily what the image header says, but just the format of the > file referenced by backing-filename (if known).) Why is it okay to change backing-filename incompatibly, but not backing-filename-format? I would find it much more consistent if ImageInfo reported the value from the header in both fields, and BlockDeviceInfo reported the values actually in use. The QAPI schema described ImageInfo as "Information about a QEMU image file" and runtime state really isn't information about an image file. If you want to know the probed image format, you can still look at backing-image.format. I don't think this change is much different from what you described above for BlockDeviceInfo.backing_file. > iotest 245 changes in behavior: With the backing node no longer > overriding the parent node's backing_file string, you can now omit the > @backing option when reopening a node with neither a default nor a > current backing file even if it used to have a backing node at some > point. > > Signed-off-by: Max Reitz Kevin
Re: [PATCH v7 41/47] block: Leave BDS.backing_file constant
On 27.07.20 14:27, Andrey Shinkevich wrote:
> On 25.06.2020 18:22, 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'
>>
>> 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.
>>
>> 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. In order not to
>> change our externally visible behavior (incompatibly), we have to let
>> bdrv_query_image_info() try to get the image format from bs->backing if
>> bs->backing_format is unset. (The QAPI schema describes
>> backing-filename-format as "the format of the backing file", so it is
>> not necessarily what the image header says, but just the format of the
>> file referenced by backing-filename (if known).)
>>
>> iotest 245 changes in behavior: With the backing node no longer
>> overriding the parent node's backing_file string, you can now omit the
>> @backing option when reopening a node with neither a default nor a
>> current backing file even if it used to have a backing node at some
>> point.
>>
>> Signed-off-by: Max Reitz
>> ---
>> include/block/block_int.h | 21 -
>> block.c | 35 +++
>> block/qapi.c | 17 +
>> tests/qemu-iotests/228 | 6 +++---
>> tests/qemu-iotests/228.out | 6 +++---
>> tests/qemu-iotests/245 | 4 +++-
>> 6 files changed, 65 insertions(+), 24 deletions(-)
>>
> ...
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 2628323b63..5da6d7e6e0 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -47,7 +47,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend
>> *blk,
>> Error **errp)
>> {
>> ImageInfo **p_image_info;
>> - BlockDriverState *bs0;
>> + BlockDriverState *bs0, *backing;
>> BlockDeviceInfo *info;
>> if (!bs->drv) {
>> @@ -76,9 +76,10 @@ BlockDeviceInfo
>> *bdrv_block_device_info(BlockBackend *blk,
>> info->node_name = g_strdup(bs->node_name);
>> }
>> - if (bs->backing_file[0]) {
>> + backing = bdrv_cow_bs(bs);
>> + if (backing) {
>> info->has_backing_file = true;
>> - info->backing_file = g_strdup(bs->backing_file);
>> + info->backing_file = g_strdup(backing->filename);
>> }
>> if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>> @@ -314,6 +3
Re: [PATCH v7 41/47] block: Leave BDS.backing_file constant
On 25.06.2020 18:22, 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'
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.
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. In order not to
change our externally visible behavior (incompatibly), we have to let
bdrv_query_image_info() try to get the image format from bs->backing if
bs->backing_format is unset. (The QAPI schema describes
backing-filename-format as "the format of the backing file", so it is
not necessarily what the image header says, but just the format of the
file referenced by backing-filename (if known).)
iotest 245 changes in behavior: With the backing node no longer
overriding the parent node's backing_file string, you can now omit the
@backing option when reopening a node with neither a default nor a
current backing file even if it used to have a backing node at some
point.
Signed-off-by: Max Reitz
---
include/block/block_int.h | 21 -
block.c| 35 +++
block/qapi.c | 17 +
tests/qemu-iotests/228 | 6 +++---
tests/qemu-iotests/228.out | 6 +++---
tests/qemu-iotests/245 | 4 +++-
6 files changed, 65 insertions(+), 24 deletions(-)
...
diff --git a/block/qapi.c b/block/qapi.c
index 2628323b63..5da6d7e6e0 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -47,7 +47,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
Error **errp)
{
ImageInfo **p_image_info;
-BlockDriverState *bs0;
+BlockDriverState *bs0, *backing;
BlockDeviceInfo *info;
if (!bs->drv) {
@@ -76,9 +76,10 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info->node_name = g_strdup(bs->node_name);
}
-if (bs->backing_file[0]) {
+backing = bdrv_cow_bs(bs);
+if (backing) {
info->has_backing_file = true;
-info->backing_file = g_strdup(bs->backing_file);
+info->backing_file = g_strdup(backing->filename);
}
if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
@@ -314,6 +315,8 @@ void bdrv_query_image_info(BlockDriverState *bs,
backing_filename = bs->backing_file;
if (backing_filename[0] != '\0') {
char *backing_filename2;
+const char *backing_format = NULL;
+
info->backing_filename = g_strdup(backing_filename);
info->has_backing_filename = true;
backing_f
[PATCH v7 41/47] block: Leave BDS.backing_file constant
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'
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.
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. In order not to
change our externally visible behavior (incompatibly), we have to let
bdrv_query_image_info() try to get the image format from bs->backing if
bs->backing_format is unset. (The QAPI schema describes
backing-filename-format as "the format of the backing file", so it is
not necessarily what the image header says, but just the format of the
file referenced by backing-filename (if known).)
iotest 245 changes in behavior: With the backing node no longer
overriding the parent node's backing_file string, you can now omit the
@backing option when reopening a node with neither a default nor a
current backing file even if it used to have a backing node at some
point.
Signed-off-by: Max Reitz
---
include/block/block_int.h | 21 -
block.c| 35 +++
block/qapi.c | 17 +
tests/qemu-iotests/228 | 6 +++---
tests/qemu-iotests/228.out | 6 +++---
tests/qemu-iotests/245 | 4 +++-
6 files changed, 65 insertions(+), 24 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e5a328c389..465a601955 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -835,11 +835,20 @@ 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 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.
+ */
+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_refres
