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

[Ashijeet Acharya]

On Mon, Oct 10, 2016 at 5:01 PM, Kevin Wolf <address@hidden> wrote:
> Am 10.10.2016 um 12:48 hat Ashijeet Acharya geschrieben:
>> On Mon, Oct 10, 2016 at 2:45 PM, Kevin Wolf <address@hidden> 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