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

[Kevin Wolf]

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 <address@hidden>
> > Reviewed-by: Kevin Wolf <address@hidden>
> > ---
> >  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

pgp4LNryUNVNZ.pgp
Description: PGP signature