Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh

[Daniel P . Berrangé]

On Fri, Jun 07, 2019 at 12:14:37PM +0200, Philippe Mathieu-Daudé wrote:
> On 6/7/19 12:08 PM, Daniel P. Berrangé wrote:
> > On Thu, Jun 06, 2019 at 07:51:15PM +0200, Pino Toscano wrote:
> >> On Thursday, 6 June 2019 13:12:32 CEST Daniel P. Berrangé wrote:
> >>> On Wed, Jun 05, 2019 at 11:36:54PM +0200, Pino Toscano wrote:
> >>>> Rewrite the implementation of the ssh block driver to use libssh instead
> >>>> of libssh2.  The libssh library has various advantages over libssh2:
> >>>> - easier API for authentication (for example for using ssh-agent)
> >>>> - easier API for known_hosts handling
> >>>> - supports newer types of keys in known_hosts
> >>>>
> >>>> Use APIs/features available in libssh 0.8 conditionally, to support
> >>>> older versions (which are not recommended though).
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Pino Toscano <address@hidden>
> >>>> ---
> >>>>
> >>>> Changes from v5:
> >>>> - adapt to newer tracing APIs
> >>>> - disable ssh compression (mimic what libssh2 does by default)
> >>>> - use build time checks for libssh 0.8, and use newer APIs directly
> >>>>
> >>>> Changes from v4:
> >>>> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
> >>>> - fix few return code checks
> >>>> - remove now-unused parameters in few internal functions
> >>>> - allow authentication with "none" method
> >>>> - switch to unsigned int for the port number
> >>>> - enable TCP_NODELAY on the socket
> >>>> - fix one reference error message in iotest 207
> >>>>
> >>>> Changes from v3:
> >>>> - fix socket cleanup in connect_to_ssh()
> >>>> - add comments about the socket cleanup
> >>>> - improve the error reporting (closer to what was with libssh2)
> >>>> - improve EOF detection on sftp_read()
> >>>>
> >>>> Changes from v2:
> >>>> - used again an own fd
> >>>> - fixed co_yield() implementation
> >>>>
> >>>> Changes from v1:
> >>>> - fixed jumbo packets writing
> >>>> - fixed missing 'err' assignment
> >>>> - fixed commit message
> >>>>
> >>>>  block/Makefile.objs        |   6 +-
> >>>>  block/ssh.c                | 610 +++++++++++++++++++------------------
> >>>>  block/trace-events         |  14 +-
> >>>>  configure                  |  62 ++--
> >>>>  tests/qemu-iotests/207.out |   2 +-
> >>>>  5 files changed, 351 insertions(+), 343 deletions(-)
> >>>
> >>>
> >>>> diff --git a/configure b/configure
> >>>> index b091b82cb3..bfdd70c40a 100755
> >>>> --- a/configure
> >>>> +++ b/configure
> >>>
> >>>> @@ -3914,43 +3914,17 @@ EOF
> >>>>  fi
> >>>>  
> >>>>  ##########################################
> >>>> -# libssh2 probe
> >>>> -min_libssh2_version=1.2.8
> >>>
> >>> The commit message says we're conditionally using APIs from 0.8.0,
> >>> but doesn't say what minimum version we actually need and there's
> >>> no check here.
> >>
> >> When I started to work on this, the libssh version available was
> >> 0.6.x IIRC, which is very old.  This v6 uses APIs added in 0.8
> >> conditionally, so it will still build with libssh < 0.8 -- of course,
> >> using an older libssh results in a less performant ssh driver, although
> >> I would think this can be considered somehow acceptable.
> >>
> >>> In terms of our supported build platforms, the oldest libssh I
> >>> see is RHEL-7 with 0.7.1.
> >>>
> >>> So assume it does actually compile on RHEL-7, then it is desirable
> >>> to have a min_libssh_Version=0.7.1 set here & checked below.
> >>
> >> For now I do not see the need to enforce a minimum version required;
> >> it can be easily added in the future in case we need to use an API only
> >> available starting from some version, and there is no fallback way for
> >> older versions.
> > 
> > In general we aim to set a clear minimum version for all our third
> > party deps based on our platform support policy. We don't want to
> > keep backcompat code around forever even if it is posisble to add
> > fallback with #ifdefs. So even if we might still work with 0.6.x,
> > we should declare 0.7.1 our min version IMHO.
> 
> With our CI setup we use:
> 
> Trusty (Ubuntu 14.04.5 LTS)
> Source: libssh
> Version: 0.6.1-0ubuntu3
> Replaces: libssh-2-dev
> 
> Xenial
> Source: libssh
> Version: 0.6.3-4.3
> Replaces: libssh-2-dev
> 
> The distrib packages do not allow dual use.

I missed that Ubuntu, unusually, had older versions than RHEL.

We don't use Trusty though - our Travis config requests Xenial
these days. So 0.6.3 is sufficient as a min version

> > Incidentally that reminds me that it is desirable to modify the
> > various native arch tests/docker/dockerfiles/*docker files to
> > list libssh as a package to install so that we get compile testing
> > coverage.
> 
> I'm testing Pino patch and already did that :)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|