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: 20190201192935.18394-1-mre...@redhat.com >>> >>> 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://maxx@localhost:22/tmp/top.qcow2 >> [...] >> backing file: base.qcow2 (actual path: >> ssh://maxx@localhost: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://rjones@localhost: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 <rjo...@redhat.com> Thanks! Max
signature.asc
Description: OpenPGP digital signature