[v4,5/5] qapi: allow blockdev-add for ssh

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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

On 10/25/2016 08:04 AM, 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.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)

Sorry for not noticing this when I finally replied to v4;


> +##
> +# @BlockdevOptionsSsh
> +#
> +# @server:              host address
> +#
> +# @path:                path to the image on the host
> +#
> +# @user:                #optional user as which to connect, defaults to current
> +#                       local user name
> +#
> +# @host_key_check       #optional defines how and what to check the host
> +#                       key against, defaults to "yes"

I still have reservations about this parameter. I think we have time to
fix it as followups during soft freeze if Kevin would rather get your
initial patches in now, if that's what it takes to meet soft freeze
deadlines, but I do not want to bake it into the actual 2.8 release
without addressing those concerns.

Am 25.10.2016 um 21:58 hat Eric Blake geschrieben:
> On 10/25/2016 08:04 AM, 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.
> > 
> > Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> Sorry for not noticing this when I finally replied to v4;
> 
> > +##
> > +# @BlockdevOptionsSsh
> > +#
> > +# @server:              host address
> > +#
> > +# @path:                path to the image on the host
> > +#
> > +# @user:                #optional user as which to connect, defaults to current
> > +#                       local user name
> > +#
> > +# @host_key_check       #optional defines how and what to check the host
> > +#                       key against, defaults to "yes"
> 
> I still have reservations about this parameter.

It doesn't seem to be as easy as an enum. The real thing is structured
because some we support colon-separated mode/key, like this:

    host_key_check=md5:xx:yy:zz:...

The description for the real thing (to which we would have to
translate the existing string) seems to be something like this:

{ 'enum': 'HostKeyCheckMode', 'data': [ 'yes', 'no', 'md5', 'sha1' ] }

{ 'union': 'HostKeyCheck',
  'base': {
      'mode': 'HostKeyCheckMode',
  },
  'discriminator': 'mode',
  'data': {
      'yes': {},
      'no': {},
      'md5': { 'key': 'str' },
      'sha1': { 'key': 'str' }
  }

(Hm, are inline struct definitions even allowed for union branches, or
do we have to create a named type there?)

> I think we have time to fix it as followups during soft freeze if
> Kevin would rather get your initial patches in now, if that's what it
> takes to meet soft freeze deadlines, but I do not want to bake it into
> the actual 2.8 release without addressing those concerns.

We don't really have a soft freeze like before any more, we're rather
going into something that is still called soft freeze, but is really a
hard freeze in disguise. So I'm not sure if we can change this during
the freeze.

If we can't work it out this week, I'd drop host_key_check from the
schema and leave a TODO comment instead so that it could be added in the
2.9 timeframe.

Kevin

On 10/26/2016 03:31 AM, Kevin Wolf wrote:

>>> +# @host_key_check       #optional defines how and what to check the host
>>> +#                       key against, defaults to "yes"
>>
>> I still have reservations about this parameter.
> 
> It doesn't seem to be as easy as an enum. The real thing is structured
> because some we support colon-separated mode/key, like this:
> 
>     host_key_check=md5:xx:yy:zz:...
> 
> The description for the real thing (to which we would have to
> translate the existing string) seems to be something like this:
> 
> { 'enum': 'HostKeyCheckMode', 'data': [ 'yes', 'no', 'md5', 'sha1' ] }
> 
> { 'union': 'HostKeyCheck',
>   'base': {
>       'mode': 'HostKeyCheckMode',
>   },
>   'discriminator': 'mode',
>   'data': {
>       'yes': {},
>       'no': {},
>       'md5': { 'key': 'str' },
>       'sha1': { 'key': 'str' }
>   }

Okay, that does look like a better definition.

> 
> (Hm, are inline struct definitions even allowed for union branches, or
> do we have to create a named type there?)

I have a patch for making them inline struct definitions, but it
probably won't be in 2.8 unless it solves problems that we can't work
around in other ways, due to Markus' review backlog on other qapi
patches. Creating a named type now may be verbose, but we could always
simplify later with an inline struct without breaking back-compat once
my patch does make it in.

> 
>> I think we have time to fix it as followups during soft freeze if
>> Kevin would rather get your initial patches in now, if that's what it
>> takes to meet soft freeze deadlines, but I do not want to bake it into
>> the actual 2.8 release without addressing those concerns.
> 
> We don't really have a soft freeze like before any more, we're rather
> going into something that is still called soft freeze, but is really a
> hard freeze in disguise. So I'm not sure if we can change this during
> the freeze.
> 
> If we can't work it out this week, I'd drop host_key_check from the
> schema and leave a TODO comment instead so that it could be added in the
> 2.9 timeframe.

That's also a reasonable idea.  Better to avoid a rush that ends up
baking in an API we can't support, even if the initial release is less
powerful as a result.