| Message ID | 20190206152919.5532-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | block/ssh: Implement .bdrv_refresh_filename() | expand |
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. Hey Max, I couldn't find a version of "block: Fix some filename generation issues" that applies to qemu master. Do you have a git repo I could pull it from? Rich. > Based-on: 20190201192935.18394-1-mreitz@redhat.com > > > v2: > - No longer based on the libssh2 -> libssh patches > - Put /* and */ on their own lines to make checkpatch happy > > > Max Reitz (2): > block/ssh: Implement .bdrv_refresh_filename() > block/ssh: Implement .bdrv_dirname() > > block/ssh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 68 insertions(+), 5 deletions(-) > > -- > 2.20.1
On 06.02.19 16:45, 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. > > Hey Max, I couldn't find a version of "block: Fix some filename > generation issues" that applies to qemu master. Do you have a git > repo I could pull it from? Ah, great, and I only sent it on Friday... I pushed it here: https://git.xanclic.moe/XanClic/qemu bds-base-filename-v13 Thanks, Max >> Based-on: 20190201192935.18394-1-mreitz@redhat.com >> >> >> v2: >> - No longer based on the libssh2 -> libssh patches >> - Put /* and */ on their own lines to make checkpatch happy >> >> >> Max Reitz (2): >> block/ssh: Implement .bdrv_refresh_filename() >> block/ssh: Implement .bdrv_dirname() >> >> block/ssh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 68 insertions(+), 5 deletions(-) >> >> -- >> 2.20.1 >
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-mreitz@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. Rich. > > v2: > - No longer based on the libssh2 -> libssh patches > - Put /* and */ on their own lines to make checkpatch happy > > > Max Reitz (2): > block/ssh: Implement .bdrv_refresh_filename() > block/ssh: Implement .bdrv_dirname() > > block/ssh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 68 insertions(+), 5 deletions(-) > > -- > 2.20.1
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-mreitz@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. Max
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-mreitz@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. In any case I don't think this should hold up the patch, so: Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich.
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-mreitz@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 <rjones@redhat.com> Thanks! Max
On 06.02.19 16:29, 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-mreitz@redhat.com > > > v2: > - No longer based on the libssh2 -> libssh patches > - Put /* and */ on their own lines to make checkpatch happy > > > Max Reitz (2): > block/ssh: Implement .bdrv_refresh_filename() > block/ssh: Implement .bdrv_dirname() > > block/ssh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 68 insertions(+), 5 deletions(-) Thanks for having had a look at this series, Rich; I've applied it to my block branch. Max
On 25.02.19 14:39, Max Reitz wrote: > On 06.02.19 16:29, 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-mreitz@redhat.com >> >> >> v2: >> - No longer based on the libssh2 -> libssh patches >> - Put /* and */ on their own lines to make checkpatch happy >> >> >> Max Reitz (2): >> block/ssh: Implement .bdrv_refresh_filename() >> block/ssh: Implement .bdrv_dirname() >> >> block/ssh.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 68 insertions(+), 5 deletions(-) > > Thanks for having had a look at this series, Rich; I've applied it to my > block branch. Or maybe not. It breaks iotests 104 (because the filename now includes your username) and 207 (because the json:{} filename is a nice filename now). So I'll drop it and send a v3. Max