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

[Philippe Mathieu-Daudé]

 On Thu, Jun 20, 2019 at 10:36 PM Philippe Mathieu-Daudé
<address@hidden> wrote:
> On 6/20/19 10:08 PM, 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).
> >
> > Adjust the iotest 207 according to the different error message, and to
> > find the default key type for localhost (to properly compare the
> > fingerprint with).
> > Contributed-by: Max Reitz <address@hidden>
> >
> > Adjust the various Docker/Travis scripts to use libssh when available
> > instead of libssh2. The mingw/mxe testing is dropped for now, as there
> > are no packages for it.
> >
> > Signed-off-by: Pino Toscano <address@hidden>
> > Tested-by: Philippe Mathieu-Daudé <address@hidden>
> > Acked-by: Alex Bennée <address@hidden>
>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
> > ---
> >
> > Changes from v10:
> > - improve error message for key mismatch
> > - integrate Max Reitz' fix to iotest 207 to detect the key type used by
> >   localhost
> >
> > Changes from v9:
> > - restored "default" case in the server status switch for libssh < 0.8.0
> > - print the host key type & fingerprint on mismatch with known_hosts
> > - improve/fix message for failed socket_set_nodelay()
> > - reset s->sock properly
> >
> > Changes from v8:
> > - use a newer key type in iotest 207
> > - improve the commit message
> >
> > Changes from v7:
> > - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
> > - ptrdiff_t -> size_t
> >
> > Changes from v6:
> > - fixed few checkpatch style issues
> > - detect libssh 0.8 via symbol detection
> > - adjust travis/docker test material
> > - remove dead "default" case in a switch
> > - use variables for storing MIN() results
> > - adapt a documentation bit
> >
> > 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
> >
> >  .travis.yml                                   |   4 +-
> >  block/Makefile.objs                           |   6 +-
> >  block/ssh.c                                   | 669 ++++++++++--------
> >  block/trace-events                            |  14 +-
> >  configure                                     |  65 +-
> >  docs/qemu-block-drivers.texi                  |   2 +-
> >  .../dockerfiles/debian-win32-cross.docker     |   1 -
> >  .../dockerfiles/debian-win64-cross.docker     |   1 -
> >  tests/docker/dockerfiles/fedora.docker        |   4 +-
> >  tests/docker/dockerfiles/ubuntu.docker        |   2 +-
> >  tests/docker/dockerfiles/ubuntu1804.docker    |   2 +-
> >  tests/qemu-iotests/207                        |  54 +-
> >  tests/qemu-iotests/207.out                    |   2 +-
> >  13 files changed, 468 insertions(+), 358 deletions(-)
> >
[...]
> > diff --git a/configure b/configure
> > index b091b82cb3..5c7914570e 100755
> > --- a/configure
> > +++ b/configure
> > @@ -472,7 +472,7 @@ auth_pam=""
> >  vte=""
> >  virglrenderer=""
> >  tpm=""
> > -libssh2=""
> > +libssh=""
> >  live_block_migration="yes"
> >  numa=""
> >  tcmalloc="no"
> > @@ -1439,9 +1439,9 @@ for opt do
> >    ;;
> >    --enable-tpm) tpm="yes"
> >    ;;
> > -  --disable-libssh2) libssh2="no"

Now than I'm bisecting over this commit, I realize removing this
option was not a good idea, we should have done like commit
cb6414dfec8 or 315d3184525:

  @@ -886,10 +885,6 @@ for opt do
  -  --disable-uuid) uuid="no"
  -  ;;
  -  --enable-uuid) uuid="yes"
  -  ;;
  ...
  +  --enable-uuid|--disable-uuid)
  +      echo "$0: $opt is obsolete, UUID support is always built" >&2
  +  ;;

> > +  --disable-libssh) libssh="no"
> >    ;;
> > -  --enable-libssh2) libssh2="yes"
> > +  --enable-libssh) libssh="yes"
> >    ;;
> >    --disable-live-block-migration) live_block_migration="no"
> >    ;;
> > @@ -1810,7 +1810,7 @@ disabled with --disable-FEATURE, default is enabled 
> > if available:
> >    coroutine-pool  coroutine freelist (better performance)
> >    glusterfs       GlusterFS backend
> >    tpm             TPM support
> > -  libssh2         ssh block device support
> > +  libssh          ssh block device support
> >    numa            libnuma support
> >    libxml2         for Parallels image format
> >    tcmalloc        tcmalloc support
> > @@ -3914,43 +3914,34 @@ EOF
> >  fi
> >
> >  ##########################################
> > -# libssh2 probe
> > -min_libssh2_version=1.2.8
> > -if test "$libssh2" != "no" ; then
> > -  if $pkg_config --atleast-version=$min_libssh2_version libssh2; then
> > -    libssh2_cflags=$($pkg_config libssh2 --cflags)
> > -    libssh2_libs=$($pkg_config libssh2 --libs)
> > -    libssh2=yes
> > +# libssh probe
> > +if test "$libssh" != "no" ; then
> > +  if $pkg_config --exists libssh; then
> > +    libssh_cflags=$($pkg_config libssh --cflags)
> > +    libssh_libs=$($pkg_config libssh --libs)
> > +    libssh=yes
> >    else
> > -    if test "$libssh2" = "yes" ; then
> > -      error_exit "libssh2 >= $min_libssh2_version required for 
> > --enable-libssh2"
> > +    if test "$libssh" = "yes" ; then
> > +      error_exit "libssh required for --enable-libssh"
> >      fi
> > -    libssh2=no
> > +    libssh=no
> >    fi
> >  fi
> >
> >  ##########################################
> > -# libssh2_sftp_fsync probe
> > +# Check for libssh 0.8
> > +# This is done like this instead of using the LIBSSH_VERSION_* and
> > +# SSH_VERSION_* macros because some distributions in the past shipped
> > +# snapshots of the future 0.8 from Git, and those snapshots did not
> > +# have updated version numbers (still referring to 0.7.0).
> >
> > -if test "$libssh2" = "yes"; then
> > +if test "$libssh" = "yes"; then
> >    cat > $TMPC <<EOF
> > -#include <stdio.h>
> > -#include <libssh2.h>
> > -#include <libssh2_sftp.h>
> > -int main(void) {
> > -    LIBSSH2_SESSION *session;
> > -    LIBSSH2_SFTP *sftp;
> > -    LIBSSH2_SFTP_HANDLE *sftp_handle;
> > -    session = libssh2_session_init ();
> > -    sftp = libssh2_sftp_init (session);
> > -    sftp_handle = libssh2_sftp_open (sftp, "/", 0, 0);
> > -    libssh2_sftp_fsync (sftp_handle);
> > -    return 0;
> > -}
> > +#include <libssh/libssh.h>
> > +int main(void) { return ssh_get_server_publickey(NULL, NULL); }
> >  EOF
> > -  # libssh2_cflags/libssh2_libs defined in previous test.
> > -  if compile_prog "$libssh2_cflags" "$libssh2_libs" ; then
> > -    QEMU_CFLAGS="-DHAS_LIBSSH2_SFTP_FSYNC $QEMU_CFLAGS"
> > +  if compile_prog "$libssh_cflags" "$libssh_libs"; then
> > +    libssh_cflags="-DHAVE_LIBSSH_0_8 $libssh_cflags"
> >    fi
> >  fi
> >
> > @@ -6451,7 +6442,7 @@ echo "GlusterFS support $glusterfs"
> >  echo "gcov              $gcov_tool"
> >  echo "gcov enabled      $gcov"
> >  echo "TPM support       $tpm"
> > -echo "libssh2 support   $libssh2"
> > +echo "libssh support    $libssh"
> >  echo "QOM debugging     $qom_cast_debug"
> >  echo "Live block migration $live_block_migration"
> >  echo "lzo support       $lzo"
> > @@ -7144,10 +7135,10 @@ if test "$glusterfs_iocb_has_stat" = "yes" ; then
> >    echo "CONFIG_GLUSTERFS_IOCB_HAS_STAT=y" >> $config_host_mak
> >  fi
> >
> > -if test "$libssh2" = "yes" ; then
> > -  echo "CONFIG_LIBSSH2=m" >> $config_host_mak
> > -  echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
> > -  echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
> > +if test "$libssh" = "yes" ; then
> > +  echo "CONFIG_LIBSSH=m" >> $config_host_mak
> > +  echo "LIBSSH_CFLAGS=$libssh_cflags" >> $config_host_mak
> > +  echo "LIBSSH_LIBS=$libssh_libs" >> $config_host_mak
> >  fi
> >
> >  if test "$live_block_migration" = "yes" ; then
[...]