Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Disable 126 for some vmdk subformats

[Max Reitz]

On 12.08.19 23:33, John Snow wrote:
> 
> 
> On 7/25/19 11:57 AM, Max Reitz wrote:
>> Several vmdk subformats do not work with iotest 126, so disable them.
>>
>> (twoGbMaxExtentSparse actually should work, but fixing that is a bit
>> difficult.  The problem is that the vmdk descriptor file will contain a
>> referenc to "image:base.vmdk", which the block layer cannot open because
> 
> reference
> 
>> it does not know the protocol "image".  This is not trivial to solve,
>> because I suppose real protocols like "http://"; should be supported.
>> Making vmdk treat all paths with a potential protocol prefix that the
>> block layer does not recognize as plain files seems a bit weird,
>> though.  Ignoring this problem does not seem too bad.)
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  tests/qemu-iotests/126 | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/126 b/tests/qemu-iotests/126
>> index 9b0dcf9255..8e55d7c843 100755
>> --- a/tests/qemu-iotests/126
>> +++ b/tests/qemu-iotests/126
>> @@ -33,6 +33,12 @@ status=1  # failure is the default!
>>  
>>  # Needs backing file support
>>  _supported_fmt qcow qcow2 qed vmdk
>> +# (1) Flat vmdk images do not support backing files
>> +# (2) Split vmdk images simply fail this test right now.  Fixing that
>> +#     is left for another day.
> 
> Which one? :)

Hmmmm?  Fixing refers to #2.  #1 is not a bug or missing feature, it’s
just how it is.  (This test needs backing files, so...)

If you mean “which are which“, then the ones with *Flat are flat images
(:-)), and the ones with twoGbMaxExtent* are split.

>> +_unsupported_imgopts "subformat=monolithicFlat" \
>> +                     "subformat=twoGbMaxExtentFlat" \
>> +                     "subformat=twoGbMaxExtentSparse"
>>  # This is the default protocol (and we want to test the difference between
>>  # colons which separate a protocol prefix from the rest and colons which are
>>  # just part of the filename, so we cannot test protocols which require a 
>> prefix)
>>
> 
> What exactly fails?

Interestingly I only now noticed that the test passes with “vmdk: Use
bdrv_dirname() for relative extent paths” (patch 2) reverted...

>                     Does the VMDK driver see `image:` and think it's a
> special filename it needs to handle and fails to do so?
No.  Whenever the block layer sees a parsee filename[1] with a colon
before a slash, it thinks everything before the colon is a protocol
prefix.  For example:

$ qemu-img info foo:bar
qemu-img: Could not open 'foo:bar': Unknown protocol 'foo'

This test is precisely for this.  How can you specify an image filename
that has a colon in it (without using -blockdev)?  One way is to prepend
it with “./”, the other is “file:”.

Now with split VMDKs, we must write something in the header file to
reference the extents.  What vmdk does for an image like
“image:foo.vmdk” is it writes “image:foo-s001.vmdk” there.

When it tries to open that extent, what happens depends on whether
“vmdk: Use bdrv_dirname() for relative extent paths” (patch 2) is applied:

--- Before that patch ---

vmdk takes the descriptor filename, which, thanks to some magic in the
block layer, is always “./image:foo.vmdk”, even when you gave it as
“file:image:foo.vmdk” (the “file:” is stripped because it does nothing,
generally, and the “./” is then prepended because of the false protocol
prefix “image:”).

It then invokes path_combine() with that path and the path given in the
descriptor file (“image:foo-s001.vmdk”).  This yields
“./image:foo-s001.vmdk”, which actually works.

--- After that patch ---

OK, what I messed up is that I just took the extent path to be an
absolute path if it has a protocol prefix.  (Because that’s how we
usually do it.)  Turns out that vmdk never did that, and path_combine()
actually completely ignores protocol prefixes in the relative filename.

I suppose I could do the same and just drop the path_has_protocol() from
patch 2.  But that’d be a bit broken, as I wrote in the commit
message...  If the descriptor file refers to an extent on
“http://example.com/extent.vmdk”, I suppose that should not be
interpreted as a relative path, but actually work...

But anyway, I guess if it’s a bit broken already, I might just keep it
that way.


tl;dr: Turns out patch 2 broke this test, because it (accidentally)
tried to fix something that I consider broken.  If I just keep it broken
(I didn’t know it was), this test will continue to work and probably
nobody will care because, well, it already is broken and nobody cares.

Max


[1] By this I mean whether it is piped through .bdrv_parse_filename().
If you specifying something with -hda or -drive file=, it will be.
These are filenames like nbd://localhost:10809 or blkdebug:conf:image.
If you pass a filename through QMP, that is, with -blockdev or
blockdev-add, it will not be parsed.  It will be given to the block
driver as is.  Protocol prefixes and other filename magic are ignored
(you need to explicitly specify the driver anyway).

signature.asc
Description: OpenPGP digital signature