[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to lib
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh |
Date: |
Fri, 7 Jun 2019 12:14:37 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
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 :)
Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh, Philippe Mathieu-Daudé, 2019/06/12
Re: [Qemu-block] [PATCH v6] ssh: switch from libssh2 to libssh, Pino Toscano, 2019/06/14