| Message ID | 1477400641-7750-6-git-send-email-ashijeetacharya@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
diff --git a/qapi/block-core.json b/qapi/block-core.json index 9d797b8..689dc0a 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,27 @@ '*vport': 'int', '*segment': 'str' } } +## +# @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" +# +# Since 2.8 +## +{ 'struct': 'BlockdevOptionsSsh', + 'data': { 'server': 'InetSocketAddress', + 'path': 'str', + '*user': 'str', + '*host_key_check': 'str' } } + ## # @BlkdebugEvent @@ -2281,7 +2303,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',