[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 0/2] block/ssh: Implement .bdrv_refresh_filen
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 0/2] block/ssh: Implement .bdrv_refresh_filename() |
Date: |
Wed, 6 Feb 2019 18:16:20 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
On 06.02.19 18:00, Richard W.M. Jones wrote:
> On Wed, Feb 06, 2019 at 05:42:15PM +0100, Max Reitz wrote:
>> On 06.02.19 17:37, Richard W.M. Jones wrote:
>>> On Wed, Feb 06, 2019 at 04:29:17PM +0100, Max Reitz wrote:
>>>> This series implements .bdrv_refresh_filename() for the ssh block
>>>> driver, along with an appropriate .bdrv_dirname() so we don't chop off
>>>> query strings for backing files with relative filenames.
>>>>
>>>> This series depends on my "block: Fix some filename generation issues"
>>>> series.
>>>>
>>>> Based-on: address@hidden
>>>
>>> I have verified that this doesn't appear to break the existing driver:
>>> ssh connections to block devices still work as well as they did before
>>> (which is to say, not very well, I wish we would replace this driver
>>> with Pino Toscano's reimplementation that uses libssh1).
>>>
>>> However I wasn't sure how I could trigger the bdrv_refresh_filename
>>> code path, so I don't think I tested that.
>>
>> One test case goes like this:
>>
>> Before this series:
>>
>> $ ./qemu-img create -f qcow2 /tmp/base.qcow2 64M
>> $ ./qemu-img create -f qcow2 -b base.qcow2 /tmp/top.qcow2
>> $ ./qemu-img info ssh://localhost/tmp/top.qcow2
>> image: json:{"driver": "qcow2", "file": {"server.host": "localhost",
>> "server.port": "22", "driver": "ssh", "path": "/tmp/top.qcow2"}}
>> [...]
>> backing file: base.qcow2 (cannot determine actual path)
>> [...]
>> $ ./qemu-io ssh://localhost/tmp/top.qcow2
>> can't open device ssh://localhost/tmp/top.qcow2: Cannot generate a base
>> directory for ssh nodes
>>
>>
>> So the filename is weird and you cannot open overlays with relative
>> backing files.
>>
>> After this series:
>>
>> $ ./qemu-img info ssh://localhost/tmp/top.qcow2
>> image: ssh://address@hidden:22/tmp/top.qcow2
>> [...]
>> backing file: base.qcow2 (actual path:
>> ssh://address@hidden:22/tmp/base.qcow2)
>> $ ./qemu-io ssh://localhost/tmp/top.qcow2
>> qemu-io> quit
>>
>> The filename looks better and the image is usable.
>
> I have to use ?host_key_check=no because of my modern ssh server. I
> see this error (with your patch series applied):
>
> $ ./qemu-img info 'ssh://localhost/tmp/top.qcow2?host_key_check=no'
> image: ssh://address@hidden:22/tmp/top.qcow2?host_key_check=no
> ...
> backing file: base.qcow2 (cannot determine actual path)
>
> $ ./qemu-io 'ssh://localhost/tmp/top.qcow2?host_key_check=no'
> can't open device ssh://localhost/tmp/top.qcow2?host_key_check=no: Cannot
> generate a base directory with host_key_check set
>
> I can see the error message in block/ssh.c:
>
> static char *ssh_bdrv_dirname(BlockDriverState *bs, Error **errp)
> {
> if (qdict_haskey(bs->full_open_options, "host_key_check")) {
> /*
> * We cannot generate a simple prefix if we would have to
> * append a query string.
> */
> error_setg(errp,
> "Cannot generate a base directory with host_key_check
> set");
>
> It seems as if bdrv_dirname is mis-designed? Either it shouldn't
> assume dirname is always a strict prefix of a path, or g_strconcatdir
> [from bdrv_make_absolute_filename] should really be a bdrv_* function
> so that block devices can override it.
We can always make it more complicated, yes. :-)
> In any case I don't think this should hold up the patch, so:
>
> Tested-by: Richard W.M. Jones <address@hidden>
Thanks!
Max
signature.asc
Description: OpenPGP digital signature