Re: [Qemu-devel] [PATCH v2 0/2] block/ssh: Implement .bdrv_refresh_filename()

[Richard W.M. Jones]

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.

In any case I don't think this should hold up the patch, so:

Tested-by: Richard W.M. Jones <address@hidden>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW