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

[Philippe Mathieu-Daudé]

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.

> 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 :)