Re: [Qemu-block] [PATCH] ssh: switch from libssh2 to libssh
On Thu, Oct 20, 2016 at 05:44:41PM +0200, Pino Toscano wrote: > On Thursday, 20 October 2016 16:32:50 CEST Richard W.M. Jones wrote: > > On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote: > > > Rewrite the implementation of the ssh block driver to use libssh instead > > > of libssh. The libssh library has various advantages over libssh: > > > - easier API for authentication (for example for using ssh-agent) > > > - easier API for known_hosts handling > > > - supports newer types of keys in known_hosts > > > > > > Kerberos authentication can be enabled once the libssh bug for it [1] is > > > fixed. > > > > > > The development version of libssh (i.e. the future 0.8.x) supports > > > fsync, so reuse the build time check for this. > > > > > > [1] https://red.libssh.org/issues/242 > > > > > > Signed-off-by: Pino Toscano > > > > > > When I applied this patch and compiled it with warnings enabled: > > > > block/ssh.c: In function ‘connect_to_ssh’: > > block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this > > function [-Wmaybe-uninitialized] > > return ret; > > ^~~ > > Interesting, there was no warning for me. Anyway, I think this: > > diff --git a/block/ssh.c b/block/ssh.c > index 7c316db..7ff376e 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -519,6 +519,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, > /* Create SSH session. */ > s->session = ssh_new(); > if (!s->session) { > +ret = -EINVAL; > goto err; > } > > should fix it (added already locally). Yes, this fixes the warning. > > To test the patch, I used virt-builder to create a virtual machine > > disk image on another machine (accessible over ssh). Then from my > > laptop I ran: > > > > ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \ > > -M accel=kvm -cpu host -m 2048 \ > > -drive > > file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio > > \ > > -serial stdio > > > > Unfortunately this failed with a large number of sftp errors: > > > > read failed: (sftp error code: 0) > > > > and subsequently hung. So I'm afraid I couldn't test the patch at all :-( > > Can you please enable the logging of the ssh driver, and libssh own > logging too? Basically (see lines 45-46) set: > > #define DEBUG_SSH 1 > #define TRACE_LIBSSH 4 I'll send you the full trace privately since it's enormous. Rich. > > Also fsync was not supported for me, but I'm using 0.7.3 and the code > > says I need 0.8.0. > > Yes, this is correct. > > Thanks, > -- > Pino Toscano -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Re: [Qemu-block] [PATCH] ssh: switch from libssh2 to libssh
On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote: > Rewrite the implementation of the ssh block driver to use libssh instead > of libssh. The libssh library has various advantages over libssh: > - easier API for authentication (for example for using ssh-agent) > - easier API for known_hosts handling > - supports newer types of keys in known_hosts > > Kerberos authentication can be enabled once the libssh bug for it [1] is > fixed. > > The development version of libssh (i.e. the future 0.8.x) supports > fsync, so reuse the build time check for this. > > [1] https://red.libssh.org/issues/242 > > Signed-off-by: Pino Toscano When I applied this patch and compiled it with warnings enabled: block/ssh.c: In function ‘connect_to_ssh’: block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized] return ret; ^~~ To test the patch, I used virt-builder to create a virtual machine disk image on another machine (accessible over ssh). Then from my laptop I ran: ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \ -M accel=kvm -cpu host -m 2048 \ -drive file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio \ -serial stdio Unfortunately this failed with a large number of sftp errors: read failed: (sftp error code: 0) and subsequently hung. So I'm afraid I couldn't test the patch at all :-( One slightly peculiar thing is that qemu ends up being linked to both libssh and libssh2. I believe the libssh2 dependency comes indirectly from libcurl. It's a slightly surprising situation but I suppose nothing to worry about. Also fsync was not supported for me, but I'm using 0.7.3 and the code says I need 0.8.0. I'll do a more detailed review when the above are fixed. Rich. > block/Makefile.objs | 6 +- > block/ssh.c | 543 > +--- > configure | 65 --- > 3 files changed, 249 insertions(+), 365 deletions(-) > > diff --git a/block/Makefile.objs b/block/Makefile.objs > index 67a036a..602a182 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -19,7 +19,7 @@ block-obj-$(CONFIG_CURL) += curl.o > block-obj-$(CONFIG_RBD) += rbd.o > block-obj-$(CONFIG_GLUSTERFS) += gluster.o > block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o > -block-obj-$(CONFIG_LIBSSH2) += ssh.o > +block-obj-$(CONFIG_LIBSSH) += ssh.o > block-obj-y += accounting.o dirty-bitmap.o > block-obj-y += write-threshold.o > block-obj-y += backup.o > @@ -38,8 +38,8 @@ rbd.o-cflags := $(RBD_CFLAGS) > rbd.o-libs := $(RBD_LIBS) > gluster.o-cflags := $(GLUSTERFS_CFLAGS) > gluster.o-libs := $(GLUSTERFS_LIBS) > -ssh.o-cflags := $(LIBSSH2_CFLAGS) > -ssh.o-libs := $(LIBSSH2_LIBS) > +ssh.o-cflags := $(LIBSSH_CFLAGS) > +ssh.o-libs := $(LIBSSH_LIBS) > archipelago.o-libs := $(ARCHIPELAGO_LIBS) > block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o > dmg-bz2.o-libs := $(BZIP2_LIBS) > diff --git a/block/ssh.c b/block/ssh.c > index 5ce12b6..7c316db 100644 > --- a/block/ssh.c > +++ b/block/ssh.c > @@ -24,8 +24,8 @@ > > #include "qemu/osdep.h" > > -#include > -#include > +#include > +#include > > #include "block/block_int.h" > #include "qapi/error.h" > @@ -38,14 +38,12 @@ > /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in > * this block driver code. > * > - * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note > - * that this requires that libssh2 was specially compiled with the > - * `./configure --enable-debug' option, so most likely you will have > - * to compile it yourself. The meaning of is described > - * here: http://www.libssh2.org/libssh2_trace.html > + * TRACE_LIBSSH= enables tracing in libssh itself. > + * The meaning of is described here: > + * http://api.libssh.org/master/group__libssh__log.html > */ > #define DEBUG_SSH 0 > -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */ > +#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */ > > #define DPRINTF(fmt, ...) \ > do {\ > @@ -60,50 +58,39 @@ typedef struct BDRVSSHState { > CoMutex lock; > > /* SSH connection. */ > -int sock; /* socket */ > -LIBSSH2_SESSION *session; /* ssh session */ > -LIBSSH2_SFTP *sftp; /* sftp session */ > -LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */ > +ssh_session session; /* ssh session */ > +sftp_session sftp;/* sftp session */ > +sftp_file sftp_handle;/* sftp remote file handle */ > > -/* See ssh_seek() function below. */ > -int64_t offset; > -bool offset_op_read; > - > -/* File attributes at open. We try to keep the .filesize field > +/* File attributes at open. We try to keep the .size field > * updat
Re: [Qemu-block] [PATCH] ssh: switch from libssh2 to libssh
On Thursday, 20 October 2016 16:32:50 CEST Richard W.M. Jones wrote: > On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote: > > Rewrite the implementation of the ssh block driver to use libssh instead > > of libssh. The libssh library has various advantages over libssh: > > - easier API for authentication (for example for using ssh-agent) > > - easier API for known_hosts handling > > - supports newer types of keys in known_hosts > > > > Kerberos authentication can be enabled once the libssh bug for it [1] is > > fixed. > > > > The development version of libssh (i.e. the future 0.8.x) supports > > fsync, so reuse the build time check for this. > > > > [1] https://red.libssh.org/issues/242 > > > > Signed-off-by: Pino Toscano > > > When I applied this patch and compiled it with warnings enabled: > > block/ssh.c: In function ‘connect_to_ssh’: > block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > return ret; > ^~~ Interesting, there was no warning for me. Anyway, I think this: diff --git a/block/ssh.c b/block/ssh.c index 7c316db..7ff376e 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -519,6 +519,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, /* Create SSH session. */ s->session = ssh_new(); if (!s->session) { +ret = -EINVAL; goto err; } should fix it (added already locally). > To test the patch, I used virt-builder to create a virtual machine > disk image on another machine (accessible over ssh). Then from my > laptop I ran: > > ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \ > -M accel=kvm -cpu host -m 2048 \ > -drive > file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio > \ > -serial stdio > > Unfortunately this failed with a large number of sftp errors: > > read failed: (sftp error code: 0) > > and subsequently hung. So I'm afraid I couldn't test the patch at all :-( Can you please enable the logging of the ssh driver, and libssh own logging too? Basically (see lines 45-46) set: #define DEBUG_SSH 1 #define TRACE_LIBSSH 4 > Also fsync was not supported for me, but I'm using 0.7.3 and the code > says I need 0.8.0. Yes, this is correct. Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
[Qemu-block] [PATCH] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead of libssh. The libssh library has various advantages over libssh: - easier API for authentication (for example for using ssh-agent) - easier API for known_hosts handling - supports newer types of keys in known_hosts Kerberos authentication can be enabled once the libssh bug for it [1] is fixed. The development version of libssh (i.e. the future 0.8.x) supports fsync, so reuse the build time check for this. [1] https://red.libssh.org/issues/242 Signed-off-by: Pino Toscano --- block/Makefile.objs | 6 +- block/ssh.c | 543 +--- configure | 65 --- 3 files changed, 249 insertions(+), 365 deletions(-) diff --git a/block/Makefile.objs b/block/Makefile.objs index 67a036a..602a182 100644 --- a/block/Makefile.objs +++ b/block/Makefile.objs @@ -19,7 +19,7 @@ block-obj-$(CONFIG_CURL) += curl.o block-obj-$(CONFIG_RBD) += rbd.o block-obj-$(CONFIG_GLUSTERFS) += gluster.o block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o -block-obj-$(CONFIG_LIBSSH2) += ssh.o +block-obj-$(CONFIG_LIBSSH) += ssh.o block-obj-y += accounting.o dirty-bitmap.o block-obj-y += write-threshold.o block-obj-y += backup.o @@ -38,8 +38,8 @@ rbd.o-cflags := $(RBD_CFLAGS) rbd.o-libs := $(RBD_LIBS) gluster.o-cflags := $(GLUSTERFS_CFLAGS) gluster.o-libs := $(GLUSTERFS_LIBS) -ssh.o-cflags := $(LIBSSH2_CFLAGS) -ssh.o-libs := $(LIBSSH2_LIBS) +ssh.o-cflags := $(LIBSSH_CFLAGS) +ssh.o-libs := $(LIBSSH_LIBS) archipelago.o-libs := $(ARCHIPELAGO_LIBS) block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o dmg-bz2.o-libs := $(BZIP2_LIBS) diff --git a/block/ssh.c b/block/ssh.c index 5ce12b6..7c316db 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -24,8 +24,8 @@ #include "qemu/osdep.h" -#include -#include +#include +#include #include "block/block_int.h" #include "qapi/error.h" @@ -38,14 +38,12 @@ /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in * this block driver code. * - * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note - * that this requires that libssh2 was specially compiled with the - * `./configure --enable-debug' option, so most likely you will have - * to compile it yourself. The meaning of is described - * here: http://www.libssh2.org/libssh2_trace.html + * TRACE_LIBSSH= enables tracing in libssh itself. + * The meaning of is described here: + * http://api.libssh.org/master/group__libssh__log.html */ #define DEBUG_SSH 0 -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */ +#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */ #define DPRINTF(fmt, ...) \ do {\ @@ -60,50 +58,39 @@ typedef struct BDRVSSHState { CoMutex lock; /* SSH connection. */ -int sock; /* socket */ -LIBSSH2_SESSION *session; /* ssh session */ -LIBSSH2_SFTP *sftp; /* sftp session */ -LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */ +ssh_session session; /* ssh session */ +sftp_session sftp;/* sftp session */ +sftp_file sftp_handle;/* sftp remote file handle */ -/* See ssh_seek() function below. */ -int64_t offset; -bool offset_op_read; - -/* File attributes at open. We try to keep the .filesize field +/* File attributes at open. We try to keep the .size field * updated if it changes (eg by writing at the end of the file). */ -LIBSSH2_SFTP_ATTRIBUTES attrs; +sftp_attributes attrs; /* Used to warn if 'flush' is not supported. */ -char *hostport; bool unsafe_flush_warning; } BDRVSSHState; static void ssh_state_init(BDRVSSHState *s) { memset(s, 0, sizeof *s); -s->sock = -1; -s->offset = -1; qemu_co_mutex_init(&s->lock); } static void ssh_state_free(BDRVSSHState *s) { -g_free(s->hostport); +if (s->attrs) { +sftp_attributes_free(s->attrs); +} if (s->sftp_handle) { -libssh2_sftp_close(s->sftp_handle); +sftp_close(s->sftp_handle); } if (s->sftp) { -libssh2_sftp_shutdown(s->sftp); +sftp_free(s->sftp); } if (s->session) { -libssh2_session_disconnect(s->session, - "from qemu ssh client: " - "user closed the connection"); -libssh2_session_free(s->session); -} -if (s->sock >= 0) { -close(s->sock); +ssh_disconnect(s->session); +ssh_free(s->session); } } @@ -118,13 +105,13 @@ session_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...) va_end(args); if (s->session) { -char *ssh_err; +const char *ssh_err; int ssh_err_code; -/* This is not an errno. See . */ -ssh_err_code = libssh2_se