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(-)

-- 
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.

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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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.

-- 
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

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