[Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Ashijeet Acharya]

Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
support blockdev-add for SSH network protocol driver. Use only 'struct
InetSocketAddress' since SSH only supports connection over TCP.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 qapi/block-core.json | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..237ec6b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1716,7 +1716,8 @@
             'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
             'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
             'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
-	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
+            'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
+            'vvfat' ] }
 
 ##
 # @BlockdevOptionsFile
@@ -1953,6 +1954,25 @@
             '*vport': 'int',
             '*segment': 'str' } }
 
+##
+# @BlockdevoptionsSsh
+#
+# @server:              host address and port number
+#
+# @path:                path to the image on the host
+#
+# @user:                user as which to connect
+#
+# @host_key_check       defines how and what to check the host key against
+#
+# Since 2.8
+##
+{ 'struct': 'BlockdevoptionsSsh',
+  'data': { 'server': 'InetSocketAddress',
+            'path': 'str',
+            'user': 'str',
+            'host_key_check': 'str' } }
+
 
 ##
 # @BlkdebugEvent
@@ -2281,7 +2301,7 @@
 # TODO rbd: Wait for structured options
       'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
-# TODO ssh: Should take InetSocketAddress for 'host'?
+      'ssh':        'BlockdevoptionsSsh',
       'tftp':       'BlockdevOptionsCurl',
       'vdi':        'BlockdevOptionsGenericFormat',
       'vhdx':       'BlockdevOptionsGenericFormat',
-- 
2.6.2

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Richard W.M. Jones]

On Sat, Oct 08, 2016 at 04:14:06PM +0530, Ashijeet Acharya wrote:
> Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
> support blockdev-add for SSH network protocol driver. Use only 'struct
> InetSocketAddress' since SSH only supports connection over TCP.
>  
> +##
> +# @BlockdevoptionsSsh
> +#
> +# @server:              host address and port number
> +#
> +# @path:                path to the image on the host
> +#
> +# @user:                user as which to connect
> +#
> +# @host_key_check       defines how and what to check the host key against
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'BlockdevoptionsSsh',
> +  'data': { 'server': 'InetSocketAddress',
> +            'path': 'str',
> +            'user': 'str',
> +            'host_key_check': 'str' } }
> +

This certainly reflects the current ssh settings.

If you really wanted to get into the down-and-dirty details, then
host_key_check has some structure.  "yes", "no", "sha1:..." and
others.  But probably we don't want all of that in the JSON.

Rich.

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

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Ashijeet Acharya]

On Sat, Oct 8, 2016 at 4:24 PM, Richard W.M. Jones <rjones@redhat.com> wrote:
> On Sat, Oct 08, 2016 at 04:14:06PM +0530, Ashijeet Acharya wrote:
>> Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
>> support blockdev-add for SSH network protocol driver. Use only 'struct
>> InetSocketAddress' since SSH only supports connection over TCP.
>>
>> +##
>> +# @BlockdevoptionsSsh
>> +#
>> +# @server:              host address and port number
>> +#
>> +# @path:                path to the image on the host
>> +#
>> +# @user:                user as which to connect
>> +#
>> +# @host_key_check       defines how and what to check the host key against
>> +#
>> +# Since 2.8
>> +##
>> +{ 'struct': 'BlockdevoptionsSsh',
>> +  'data': { 'server': 'InetSocketAddress',
>> +            'path': 'str',
>> +            'user': 'str',
>> +            'host_key_check': 'str' } }
>> +
>
> This certainly reflects the current ssh settings.
>
> If you really wanted to get into the down-and-dirty details, then
> host_key_check has some structure.  "yes", "no", "sha1:..." and
> others.  But probably we don't want all of that in the JSON.

Yeah, JSON only requires the 'host_key_check' option of string
datatype and the input from the user automatically gets checked using
the ssh driver later I guess.

Although, I think we might also need to put a check on condition that
port can only be used with a host in ssh.c, like:

if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) {
       error_setg(errp, "port can only be used with host.");
}

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

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Kevin Wolf]

Am 08.10.2016 um 12:44 hat Ashijeet Acharya geschrieben:
> Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
> support blockdev-add for SSH network protocol driver. Use only 'struct
> InetSocketAddress' since SSH only supports connection over TCP.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  qapi/block-core.json | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9d797b8..237ec6b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1716,7 +1716,8 @@
>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>              'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>              'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
> -	    'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
> +            'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
> +            'vvfat' ] }
>  
>  ##
>  # @BlockdevOptionsFile
> @@ -1953,6 +1954,25 @@
>              '*vport': 'int',
>              '*segment': 'str' } }
>  
> +##
> +# @BlockdevoptionsSsh
> +#
> +# @server:              host address and port number
> +#
> +# @path:                path to the image on the host
> +#
> +# @user:                user as which to connect
> +#
> +# @host_key_check       defines how and what to check the host key against
> +#
> +# Since 2.8
> +##
> +{ 'struct': 'BlockdevoptionsSsh',
> +  'data': { 'server': 'InetSocketAddress',
> +            'path': 'str',
> +            'user': 'str',
> +            'host_key_check': 'str' } }

Did you test this? The ssh driver currently takes a separate 'host'
string and 'port' integer, not a 'server' InetSocketAddress, so I think
the C code needs an update, too.

As for how to use a SocketAddress in order to establish a connection,
you can look at block/nbd.c for reference.

Kevin

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Ashijeet Acharya]

On Mon, Oct 10, 2016 at 2:45 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 08.10.2016 um 12:44 hat Ashijeet Acharya geschrieben:
>> Introduce new object 'BlockdevOptionsSsh' in qapi/block-core.json to
>> support blockdev-add for SSH network protocol driver. Use only 'struct
>> InetSocketAddress' since SSH only supports connection over TCP.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>>  qapi/block-core.json | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 9d797b8..237ec6b 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1716,7 +1716,8 @@
>>              'dmg', 'file', 'ftp', 'ftps', 'gluster', 'host_cdrom',
>>              'host_device', 'http', 'https', 'luks', 'null-aio', 'null-co',
>>              'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
>> -         'replication', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] }
>> +            'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
>> +            'vvfat' ] }
>>
>>  ##
>>  # @BlockdevOptionsFile
>> @@ -1953,6 +1954,25 @@
>>              '*vport': 'int',
>>              '*segment': 'str' } }
>>
>> +##
>> +# @BlockdevoptionsSsh
>> +#
>> +# @server:              host address and port number
>> +#
>> +# @path:                path to the image on the host
>> +#
>> +# @user:                user as which to connect
>> +#
>> +# @host_key_check       defines how and what to check the host key against
>> +#
>> +# Since 2.8
>> +##
>> +{ 'struct': 'BlockdevoptionsSsh',
>> +  'data': { 'server': 'InetSocketAddress',
>> +            'path': 'str',
>> +            'user': 'str',
>> +            'host_key_check': 'str' } }
>
> Did you test this? The ssh driver currently takes a separate 'host'
> string and 'port' integer, not a 'server' InetSocketAddress, so I think
> the C code needs an update, too.

Ohh, maybe I misunderstood. I will update the c code too.
One question though, here when we talk about server, we refer to something like
         <host>:<port>
right? And similarly, maybe not related to this but when we parse uri
using 'uri_parse(filename)', does the 'uri->server' stores a similar
kind of format I mentioned above?

>
> As for how to use a SocketAddress in order to establish a connection,
> you can look at block/nbd.c for reference.

Great! I will use that as a reference.

Ashijeet
>
> Kevin

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Kevin Wolf]

Am 10.10.2016 um 12:48 hat Ashijeet Acharya geschrieben:
> On Mon, Oct 10, 2016 at 2:45 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 08.10.2016 um 12:44 hat Ashijeet Acharya geschrieben:
> >> +{ 'struct': 'BlockdevoptionsSsh',
> >> +  'data': { 'server': 'InetSocketAddress',
> >> +            'path': 'str',
> >> +            'user': 'str',
> >> +            'host_key_check': 'str' } }

Another thing I just noticed now: I think host_key_check should be
marked optional (i.e. '*host_key_check')

> >
> > Did you test this? The ssh driver currently takes a separate 'host'
> > string and 'port' integer, not a 'server' InetSocketAddress, so I think
> > the C code needs an update, too.
> 
> Ohh, maybe I misunderstood. I will update the c code too.
> One question though, here when we talk about server, we refer to something like
>          <host>:<port>
> right?

You (correctly) defined 'server' as an InetSocketAddress. This in turn
is defined in qapi-schema.json:

{ 'struct': 'InetSocketAddress',
  'data': {
    'host': 'str',
    'port': 'str',
    '*to': 'uint16',
    '*ipv4': 'bool',
    '*ipv6': 'bool' } }

Your .bdrv_open() callback in ssh gets these options as keys in
QDict *options, using the dot syntax. options might look like this
(using "key" = "value" for the example):

    "server.host" = "localhost"
    "server.port" = "1234"
    "server.ipv4" = "on"
    "server.ipv6" = "on"
    "path" = "/tmp/test.img"
    "user" = "test"

You can manually parse the "server.*" fields with
qdict_extract_subqdict() and QemuOpts and then construct an
InetSocketAddress object in C similar to what NBD does currently.

The other option, and maybe a bit nicer, would be to use a visitor to
directly go from the (sub-)QDict to InetSocketAddress.

> And similarly, maybe not related to this but when we parse uri
> using 'uri_parse(filename)', does the 'uri->server' stores a similar
> kind of format I mentioned above?

It appears to return a struct URI, which contains 'char *server' and
'int port', but also many other fields.

> > As for how to use a SocketAddress in order to establish a connection,
> > you can look at block/nbd.c for reference.
> 
> Great! I will use that as a reference.

Another thing to have a look at might be the NBD series that Max posted
to convert it to blockdev-add. I still haven't done that myself, but I
suppose many of the things he does there apply to ssh as well.

Kevin

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Ashijeet Acharya]

On Mon, Oct 10, 2016 at 5:01 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 10.10.2016 um 12:48 hat Ashijeet Acharya geschrieben:
>> On Mon, Oct 10, 2016 at 2:45 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 08.10.2016 um 12:44 hat Ashijeet Acharya geschrieben:
>> >> +{ 'struct': 'BlockdevoptionsSsh',
>> >> +  'data': { 'server': 'InetSocketAddress',
>> >> +            'path': 'str',
>> >> +            'user': 'str',
>> >> +            'host_key_check': 'str' } }
>
> Another thing I just noticed now: I think host_key_check should be
> marked optional (i.e. '*host_key_check')

Okay, I will do that.

>
>> >
>> > Did you test this? The ssh driver currently takes a separate 'host'
>> > string and 'port' integer, not a 'server' InetSocketAddress, so I think
>> > the C code needs an update, too.
>>
>> Ohh, maybe I misunderstood. I will update the c code too.
>> One question though, here when we talk about server, we refer to something like
>>          <host>:<port>
>> right?
>
> You (correctly) defined 'server' as an InetSocketAddress. This in turn
> is defined in qapi-schema.json:
>
> { 'struct': 'InetSocketAddress',
>   'data': {
>     'host': 'str',
>     'port': 'str',
>     '*to': 'uint16',
>     '*ipv4': 'bool',
>     '*ipv6': 'bool' } }
>

Yes, by misunderstood I meant to say that I thought we were using
'InetSocketAddress' just so that the block-core.json has all the
options wrapped into a single option but still preserve the legacy
options for user. But now while going through Max's patch series for
NBD, I am getting hold of the idea.

> Your .bdrv_open() callback in ssh gets these options as keys in
> QDict *options, using the dot syntax. options might look like this
> (using "key" = "value" for the example):
>
>     "server.host" = "localhost"
>     "server.port" = "1234"
>     "server.ipv4" = "on"
>     "server.ipv6" = "on"
>     "path" = "/tmp/test.img"
>     "user" = "test"
>
> You can manually parse the "server.*" fields with
> qdict_extract_subqdict() and QemuOpts and then construct an
> InetSocketAddress object in C similar to what NBD does currently.
>
> The other option, and maybe a bit nicer, would be to use a visitor to
> directly go from the (sub-)QDict to InetSocketAddress.

If I am not wrong, this is how Max did it here to unflatten things:
    https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06709.html

But they don't seem to have been merged yet. I will rebase on top of
his work though.

>
>> And similarly, maybe not related to this but when we parse uri
>> using 'uri_parse(filename)', does the 'uri->server' stores a similar
>> kind of format I mentioned above?
>
> It appears to return a struct URI, which contains 'char *server' and
> 'int port', but also many other fields.

Okay.

>
>> > As for how to use a SocketAddress in order to establish a connection,
>> > you can look at block/nbd.c for reference.
>>
>> Great! I will use that as a reference.
>
> Another thing to have a look at might be the NBD series that Max posted
> to convert it to blockdev-add. I still haven't done that myself, but I
> suppose many of the things he does there apply to ssh as well.

Yeah, I am currently looking at those.

Ashijeet
>
> Kevin

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Kevin Wolf]

Am 10.10.2016 um 13:54 hat Ashijeet Acharya geschrieben:
> On Mon, Oct 10, 2016 at 5:01 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Your .bdrv_open() callback in ssh gets these options as keys in
> > QDict *options, using the dot syntax. options might look like this
> > (using "key" = "value" for the example):
> >
> >     "server.host" = "localhost"
> >     "server.port" = "1234"
> >     "server.ipv4" = "on"
> >     "server.ipv6" = "on"
> >     "path" = "/tmp/test.img"
> >     "user" = "test"
> >
> > You can manually parse the "server.*" fields with
> > qdict_extract_subqdict() and QemuOpts and then construct an
> > InetSocketAddress object in C similar to what NBD does currently.
> >
> > The other option, and maybe a bit nicer, would be to use a visitor to
> > directly go from the (sub-)QDict to InetSocketAddress.
> 
> If I am not wrong, this is how Max did it here to unflatten things:
>     https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06709.html
> 
> But they don't seem to have been merged yet. I will rebase on top of
> his work though.

The current version of his series is v4 (from end of September), which
has in turn rebased on top of Dan's "QAPI/QOM work for non-scalar object
properties" series, which adds a qdict_crumple() function. This is a
more complete function to "unflatten" the dict again.

So maybe you want to rebase on top of Dan's series instead of Max's, and
just refer to Max's series to see how he's integrating things in the
block driver.

Kevin

Re: [Qemu-devel] [PATCH] qapi: allow blockdev-add for ssh

[Ashijeet Acharya]

On Mon, Oct 10, 2016 at 6:23 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 10.10.2016 um 13:54 hat Ashijeet Acharya geschrieben:
>> On Mon, Oct 10, 2016 at 5:01 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Your .bdrv_open() callback in ssh gets these options as keys in
>> > QDict *options, using the dot syntax. options might look like this
>> > (using "key" = "value" for the example):
>> >
>> >     "server.host" = "localhost"
>> >     "server.port" = "1234"
>> >     "server.ipv4" = "on"
>> >     "server.ipv6" = "on"
>> >     "path" = "/tmp/test.img"
>> >     "user" = "test"
>> >
>> > You can manually parse the "server.*" fields with
>> > qdict_extract_subqdict() and QemuOpts and then construct an
>> > InetSocketAddress object in C similar to what NBD does currently.
>> >
>> > The other option, and maybe a bit nicer, would be to use a visitor to
>> > directly go from the (sub-)QDict to InetSocketAddress.
>>
>> If I am not wrong, this is how Max did it here to unflatten things:
>>     https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg06709.html
>>
>> But they don't seem to have been merged yet. I will rebase on top of
>> his work though.
>
> The current version of his series is v4 (from end of September), which
> has in turn rebased on top of Dan's "QAPI/QOM work for non-scalar object
> properties" series, which adds a qdict_crumple() function. This is a
> more complete function to "unflatten" the dict again.
>
> So maybe you want to rebase on top of Dan's series instead of Max's, and
> just refer to Max's series to see how he's integrating things in the
> block driver.

Ohh, okay I will find the latest v4 series and rebase on top of Daniel's work.

Ashijeet

>
> Kevin