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 <[email protected]> >>>> --- >>>> >>>> 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 :)
