subject:"\[Qemu\-block\] \[PATCH\] ssh\: switch from libssh2 to libssh"

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

2016-10-20 Thread Richard W.M. Jones

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

2016-10-20 Thread Richard W.M. Jones

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

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

2016-10-20 Thread Pino Toscano

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

2016-10-20 Thread Pino Toscano

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(>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 . */
-

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

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

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

[Qemu-block] [PATCH] ssh: switch from libssh2 to libssh

4 matches

Site Navigation

Mail list logo

Footer information