Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command

[Max Reitz]

On 14.05.19 13:30, Alberto Garcia wrote:
> On Sat 13 Apr 2019 02:53:42 AM CEST, Max Reitz wrote:
>>> Calling x-blockdev-reopen without 'backing' should only fail if
>>>
>>>  a) the image has a backing file attached to it.
>>>     In this case it doesn't: we just detached it in the previous line.
>>>
>>>  b) there's a default backing file written on the image header.
>>>     In this case there isn't (hd0 is created without one in setUp()).
>>
>> That’s what I thought, too, hence me applying effectively the same
>> change to the test in v4 of my series as you in your "Fix check for
>> default backing files" series:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html
>>
>>> So it should not fail. I think the bug is that the test for condition
>>> (b) in bdrv_reopen_prepare() that returns "backing is missing..." is
>>> using backing_file but it should use (correct me if I'm wrong)
>>> auto_backing_file.
>>
>> Well, I think both should be fine, because...
> 
> Why would both be fine? backing_file refers to the backing file
> currently attached, and auto_backing_file refers to the one written on
> the image metadata, or am I wrong?

After my series, backing_file refers to the image metadata.

>>> Not directly related to this, but should bdrv_backing_detach() also
>>> clear backing_file ?
>>
>> ...I don’t think it should.  That’s what that my patch addresses. The
>> real problem is that bs->backing_file is not a cache for
>> bs->backing->bs->filename, so it shouldn’t be treated as such.
> 
> But what's the point of having backing_file set if no backing file is
> attached?

What’s the point of having backing_file set to the same value as
bs->backing->bs->filename?

We need some storage for “What does the image header say”.  Currently,
that sometimes is BDS.backing_file.  Sometimes it isn’t.  That’s broken.
 See
http://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00308.html.
BDS.auto_backing_file does not refer to the image metadata.  It refers
to something like bdrv_refresh_filename(bdrv_open(bs->backing_file)).
We need this just to calculate @backing_overridden in
bdrv_refresh_filename().

Max

signature.asc
Description: OpenPGP digital signature