| Message ID | 20190725155735.11872-8-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vmdk: Misc fixes | expand |
On 7/25/19 10: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 <mreitz@redhat.com> > --- > tests/qemu-iotests/126 | 6 ++++++ > 1 file changed, 6 insertions(+) Are you aiming to get any of this series in -rc3, or is it firmly 4.2 material?
On 25.07.19 19:00, Eric Blake wrote: > On 7/25/19 10: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 <mreitz@redhat.com> >> --- >> tests/qemu-iotests/126 | 6 ++++++ >> 1 file changed, 6 insertions(+) > > Are you aiming to get any of this series in -rc3, or is it firmly 4.2 > material? No bug fix is ever firmly -next material. However, the bugs aren’t regressions and not dramatic, so we don’t need to force it into 4.1. If I had a strong opinion myself on where to place this series, I’d have given it a for-4.x tag. I didn’t, because I don’t. Max
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 <mreitz@redhat.com> > --- > 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? :) > +_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? Does the VMDK driver see `image:` and think it's a special filename it needs to handle and fails to do so?
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 <mreitz@redhat.com> >> --- >> 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).
On 8/13/19 10:00 AM, Max Reitz wrote: > 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 <mreitz@redhat.com> >>> --- >>> 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. > "Which day" ;) >>> +_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: > Actually, I think we're on the same page here. I maybe meant to type "block layer" instead of "VMDK driver", but it does look like it does special processing on this sort of filename that breaks in this case. > $ 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. > So which kinda-broken thing are you making the case for? Are you re-spinning in light of your discovery or... are we fine with the state of things as they land here? (Sorry, it wasn't clear to me which way you were leaning.) --js > 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). >
On 14.08.19 00:26, John Snow wrote: > > > On 8/13/19 10:00 AM, Max Reitz wrote: >> 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 <mreitz@redhat.com> >>>> --- >>>> 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. >> > > "Which day" ;) > >>>> +_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: >> > > Actually, I think we're on the same page here. I maybe meant to type > "block layer" instead of "VMDK driver", but it does look like it does > special processing on this sort of filename that breaks in this case. > >> $ 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. >> > > So which kinda-broken thing are you making the case for? Are you > re-spinning in light of your discovery or... are we fine with the state > of things as they land here? > > (Sorry, it wasn't clear to me which way you were leaning.) I’m going to respin and drop the “path_has_protocol(fname)” condition from patch 2. Max
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. +_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)
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 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 <mreitz@redhat.com> --- tests/qemu-iotests/126 | 6 ++++++ 1 file changed, 6 insertions(+)