[Qemu-devel] [PATCH] Add support for Secure Shell (ssh) block device.

[Qemu-devel] [PATCH] Add support for Secure Shell (ssh) block device.

[Richard W.M. Jones]

This is a fun little patch which adds an ssh-based block device.
Nearly every Unix/Linux server runs an ssh daemon.  This new block
device lets qemu ssh into those servers and use files on those servers
as disks:

 qemu-system-x86_64 -m 512 \
    -drive file=ssh://rjones@onuma/mnt/scratch/f15x32.img,if=virtio

If you want to test this, note that you will need to have ssh-agent
set up so there is passwordless access from your local machine to the
remote account.  Krb5 and other authentication methods (probably)
won't work.

I have tested with a couple of Windows and Linux guest images,
successfully booting and using those disks which are located on a
remote RHEL server over 100 Mbps ethernet.  Disk speed is reasonable
though not exactly fast.

It's not ready to be applied as it would be nice to fix the "easy to
fix" problems noted in the commit message.  Nevertheless I'd
appreciate an initial review.

Also: Is there any documentation on how coroutines / AIO work? (apart
from reading the code, which I've been doing)

Rich.

[Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Richard W.M. Jones]

From: "Richard W.M. Jones" <rjones@redhat.com>

  qemu-system-x86_64 -drive file=ssh://hostname/some/image

QEMU will ssh into 'hostname' and open '/some/image' which is made
available as a standard block device.

You can specify a username (ssh://user@host/...) and/or a port number
(ssh://host:port/...).

Current limitations:

- Authentication must be done without passwords or passphrases, using
  ssh-agent.  Other authentication methods are not supported. (*)

- Does not check host key. (*)

- New remote files cannot be created. (*)

- Uses coroutine read/write, instead of true AIO.  (libssh2 supports
  non-blocking access, so this could be fixed with some effort).

- Blocks during connection and authentication.

(*) = potentially easy fix

This is implemented using libssh2 on the client side.  The server just
requires a regular ssh daemon with sftp-server support.  Most ssh
daemons on Unix/Linux systems will work out of the box.
---
 block/Makefile.objs |   1 +
 block/ssh.c         | 514 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure           |  47 +++++
 qemu-doc.texi       |  28 +++
 4 files changed, 590 insertions(+)
 create mode 100644 block/ssh.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index c067f38..6c4b5bc 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -13,6 +13,7 @@ block-obj-$(CONFIG_LIBISCSI) += iscsi.o
 block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
+block-obj-$(CONFIG_LIBSSH2) += ssh.o
 endif
 
 common-obj-y += stream.o
diff --git a/block/ssh.c b/block/ssh.c
new file mode 100644
index 0000000..7071cfd
--- /dev/null
+++ b/block/ssh.c
@@ -0,0 +1,514 @@
+/*
+ * Secure Shell (ssh) backend for QEMU.
+ *
+ * Copyright (C) 2013 Red Hat Inc., Richard W.M. Jones <rjones@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdarg.h>
+
+#ifndef _WIN32
+#include <pwd.h>
+#endif
+
+#include <libssh2.h>
+#include <libssh2_sftp.h>
+
+#include "block/block_int.h"
+#include "qemu/sockets.h"
+#include "qemu/uri.h"
+
+#define SECTOR_SIZE 512
+
+static void __attribute__((constructor)) ssh_init(void)
+{
+    int r;
+
+    r = libssh2_init(0);
+    if (r != 0) {
+        fprintf(stderr, "libssh2 initialization failed, %d\n", r);
+        exit(EXIT_FAILURE);
+    }
+}
+
+typedef struct BDRVSSHState {
+    CoMutex lock;               /* coroutine lock */
+
+    /* configuration */
+    char *user;                 /* remote username */
+    char *host;                 /* hostname and port */
+    char *path;                 /* path to remote disk image */
+
+    /* ssh connection */
+    int sock;                         /* socket */
+    LIBSSH2_SESSION *session;         /* ssh session */
+    LIBSSH2_SFTP *sftp;               /* sftp session */
+    LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */
+
+    /* file attributes (at open) */
+    LIBSSH2_SFTP_ATTRIBUTES attrs;
+} BDRVSSHState;
+
+/* Wrappers around error_report which make sure to dump as much
+ * information from libssh2 as possible.
+ */
+static void
+session_error_report(BDRVSSHState *s, const char *fs, ...)
+{
+    va_list args;
+
+    va_start(args, fs);
+
+    if ((s)->session) {
+        char *ssh_err;
+        int ssh_err_len = sizeof ssh_err;
+        int ssh_err_code;
+
+        libssh2_session_last_error((s)->session,
+                                   &ssh_err, &ssh_err_len, 0);
+        /* This is not an errno.  See <libssh2.h>. */
+        ssh_err_code = libssh2_session_last_errno((s)->session);
+
+        error_vprintf(fs, args);
+        error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
+    }
+    else {
+        error_vprintf(fs, args);
+    }
+
+    va_end(args);
+    error_printf("\n");
+}
+
+static void
+sftp_error_report(BDRVSSHState *s, const char *fs, ...)
+{
+    va_list args;
+
+    va_start(args, fs);
+
+    if ((s)->sftp) {
+        char *ssh_err;
+        int ssh_err_len = sizeof ssh_err;
+        int ssh_err_code;
+        unsigned long sftp_err_code;
+
+        libssh2_session_last_error((s)->session,
+                                   &ssh_err, &ssh_err_len, 0);
+        /* This is not an errno.  See <libssh2.h>. */
+        ssh_err_code = libssh2_session_last_errno((s)->session);
+        /* See <libssh2_sftp.h>. */
+        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
+
+        error_vprintf(fs, args);
+        error_printf(": %s (libssh2 error code: %d, sftp error code: %lu)",
+                     ssh_err, ssh_err_code, sftp_err_code);
+    }
+    else {
+        error_vprintf(fs, args);
+    }
+
+    va_end(args);
+    error_printf("\n");
+}
+
+#ifndef _WIN32
+static char *get_username(void)
+{
+    struct passwd *pwd;
+
+    pwd = getpwuid(geteuid ());
+    if (!pwd) {
+        error_report("failed to get current user name");
+        return NULL;
+    }
+    return g_strdup(pwd->pw_name);
+}
+#else
+static char *get_username(void)
+{
+    error_report("on Windows, specify a remote user name in the URI");
+    return NULL;
+}
+#endif
+
+static int parse_uri(BDRVSSHState *s, const char *filename)
+{
+    URI *uri;
+
+    uri = uri_parse(filename);
+    if (!uri)
+        return -EINVAL;
+
+    if (strcmp(uri->scheme, "ssh") != 0) {
+        error_report("URI scheme must be 'ssh'");
+        goto err;
+    }
+
+    if (!uri->server || strcmp(uri->server, "") == 0) {
+        error_report("missing hostname in URI");
+        goto err;
+    }
+
+    if (!uri->path || strcmp(uri->path, "") == 0) {
+        error_report("missing remote path in URI");
+        goto err;
+    }
+
+    /* If user is defined in the URI, use it.  Else use local username. */
+    if(uri->user && strcmp(uri->user, "") != 0)
+        s->user = g_strdup(uri->user);
+    else {
+        s->user = get_username();
+        if (!s->user)
+            goto err;
+    }
+
+    /* Construct the hostname:port string for inet_connect. */
+    if(uri->port == 0)
+        uri->port = 22;
+    s->host = g_strdup_printf("%s:%d", uri->server, uri->port);
+
+    s->path = g_strdup(uri->path);
+
+    return 0;
+
+ err:
+    uri_free(uri);
+    return -EINVAL;
+}
+
+static int connect_to_ssh(BDRVSSHState *s)
+{
+    int r, ret;
+    const char *userauthlist;
+    LIBSSH2_AGENT *agent = NULL;
+    struct libssh2_agent_publickey *identity;
+    struct libssh2_agent_publickey *prev_identity = NULL;
+
+    s->session = libssh2_session_init();
+    if (!s->session) {
+        ret = -EINVAL;
+        session_error_report(s, "failed to initialize libssh2 session");
+        goto err;
+    }
+
+    r = libssh2_session_handshake(s->session, s->sock);
+    if (r != 0) {
+        ret = -EINVAL;
+        session_error_report(s, "failed to establish SSH session");
+        goto err;
+    }
+
+#if 0
+    /* Check the remote host's key against known_hosts. */
+    ret = check_host_key (s);
+    if (ret < 0)
+        goto err;
+#endif
+
+    /* Authenticate. */
+    userauthlist = libssh2_userauth_list(s->session, s->user, strlen(s->user));
+    if (strstr(userauthlist, "publickey") == NULL) {
+        ret = -EPERM;
+        error_report("remote server does not support \"publickey\" authentication");
+        goto err;
+    }
+
+    /* Connect to ssh-agent and try each identity in turn. */
+    agent = libssh2_agent_init(s->session);
+    if (!agent) {
+        ret = -EINVAL;
+        session_error_report(s, "failed to initialize ssh-agent support");
+        goto err;
+    }
+    if (libssh2_agent_connect(agent)) {
+        ret = -ECONNREFUSED;
+        session_error_report(s, "failed to connect to ssh-agent");
+        goto err;
+    }
+    if (libssh2_agent_list_identities(agent)) {
+        ret = -EINVAL;
+        session_error_report(s, "failed requesting identities from ssh-agent");
+        goto err;
+    }
+
+    for(;;) {
+        r = libssh2_agent_get_identity(agent, &identity, prev_identity);
+        if (r == 1)             /* end of list */
+            break;
+        if (r < 0) {
+            ret = -EINVAL;
+            session_error_report(s, "failed to obtain identity from ssh-agent");
+            goto err;
+        }
+        r = libssh2_agent_userauth(agent, s->user, identity);
+        if (r == 0)
+            goto authenticated;
+        /* Failed to authenticate with this identity, try the next one. */
+        prev_identity = identity;
+    }
+
+    ret = -EPERM;
+    error_report("failed to authenticate using publickey authentication "
+                 "and the identities held by your ssh-agent");
+    goto err;
+
+ authenticated:
+    libssh2_agent_disconnect(agent);
+    libssh2_agent_free(agent);
+    agent = NULL;
+
+    /* Start SFTP. */
+    s->sftp = libssh2_sftp_init(s->session);
+    if (!s->sftp) {
+        session_error_report(s, "failed to initialize sftp handle");
+        ret = -EINVAL;
+        goto err;
+    }
+
+    s->sftp_handle = libssh2_sftp_open(s->sftp, s->path,
+                                       LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE,
+                                       0);
+    if (!s->sftp_handle) {
+        session_error_report(s, "failed to open remote file '%s'", s->path);
+        ret = -EINVAL;
+        goto err;
+    }
+
+    r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
+    if (r < 0) {
+        sftp_error_report(s, "failed to read file attributes of '%s'",
+                          s->path);
+        ret = -EINVAL;
+        goto err;
+    }
+
+    return 0;
+
+ err:
+    if (s->sftp_handle)
+        libssh2_sftp_close(s->sftp_handle);
+    s->sftp_handle = NULL;
+    if (s->sftp)
+        libssh2_sftp_shutdown(s->sftp);
+    s->sftp = NULL;
+    if (s->session) {
+        libssh2_session_disconnect(s->session,
+                                   "from qemu ssh client: "
+                                   "error opening connection");
+        libssh2_session_free(s->session);
+    }
+    s->session = NULL;
+
+    if (agent) {
+        libssh2_agent_disconnect(agent);
+        libssh2_agent_free(agent);
+    }
+
+    return ret;
+}
+
+static int ssh_file_open(BlockDriverState *bs, const char *filename,
+                         int bdrv_flags)
+{
+    BDRVSSHState *s = bs->opaque;
+    int ret;
+    Error *err = NULL;
+
+    s->sock = -1;
+
+    ret = parse_uri(s, filename);
+    if (ret < 0)
+        goto err;
+
+    /* Open the socket and connect. */
+    s->sock = inet_connect(s->host, &err);
+    if (err != NULL) {
+        ret = -errno;
+        qerror_report_err(err);
+        error_free(err);
+        goto err;
+    }
+
+    /* Start up SSH. */
+    ret = connect_to_ssh(s);
+    if (ret < 0)
+        goto err;
+
+#if 0 /* Enable this when AIO callbacks are implemented. */
+    /* Go non-blocking. */
+    libssh2_session_set_blocking(s->session, 0);
+#endif
+
+    qemu_co_mutex_init(&s->lock);
+
+    return 0;
+
+ err:
+    if (s->sock >= 0)
+        close(s->sock);
+    s->sock = -1;
+
+    g_free(s->path);
+    s->path = NULL;
+    g_free(s->host);
+    s->host = NULL;
+    g_free(s->user);
+    s->user = NULL;
+
+    return ret;
+}
+
+static void ssh_close(BlockDriverState *bs)
+{
+    BDRVSSHState *s = bs->opaque;
+
+    if (s->sftp_handle)
+        libssh2_sftp_close(s->sftp_handle);
+    if (s->sftp)
+        libssh2_sftp_shutdown(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);
+
+    g_free(s->path);
+    g_free(s->host);
+    g_free(s->user);
+}
+
+static coroutine_fn int ssh_read(BDRVSSHState *s,
+                                int64_t offset, char *buf, size_t size)
+{
+    ssize_t r;
+    size_t got;
+
+    /* According to the docs, this just updates a field in the
+     * sftp_handle structure, so there is no network traffic and it
+     * cannot fail.
+     */
+    libssh2_sftp_seek64(s->sftp_handle, offset);
+
+    /* libssh2 has a hard-coded limit of 2000 bytes per request,
+     * although it will also do readahead behind our backs.  Therefore
+     * we may have to do repeated reads here until we have read 'size'
+     * bytes.
+     */
+    for (got = 0; got < size; ) {
+    again:
+        r = libssh2_sftp_read(s->sftp_handle, buf + got, size - got);
+
+        if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT)
+            goto again;
+        if (r < 0) {
+            sftp_error_report(s, "%s: read failed", s->path);
+            return -EIO;
+        }
+        if (r == 0) {
+            error_report("%s: read failed (EOF)", s->path);
+            return -EIO;
+        }
+
+        got += r;
+    }
+
+    return 0;
+}
+
+static coroutine_fn int ssh_co_read(BlockDriverState *bs,
+                                    int64_t sector_num,
+                                    uint8_t *buf, int nb_sectors)
+{
+    BDRVSSHState *s = bs->opaque;
+    int ret;
+
+    qemu_co_mutex_lock(&s->lock);
+    ret = ssh_read(s, sector_num * SECTOR_SIZE, (char *) buf,
+                   nb_sectors * SECTOR_SIZE);
+    qemu_co_mutex_unlock(&s->lock);
+
+    return ret;
+}
+
+static int ssh_write(BDRVSSHState *s,
+                     int64_t offset, const char *buf, size_t size)
+{
+    ssize_t r;
+    size_t written;
+
+    /* According to the docs, this just updates a field in the
+     * sftp_handle structure, so there is no network traffic and it
+     * cannot fail.
+     */
+    libssh2_sftp_seek64(s->sftp_handle, offset);
+
+    for (written = 0; written < size; ) {
+    again:
+        r = libssh2_sftp_write(s->sftp_handle, buf + written, size - written);
+
+        if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT)
+            goto again;
+        if (r < 0) {
+            sftp_error_report(s, "%s: write failed", s->path);
+            return -EIO;
+        }
+
+        written += r;
+    }
+
+    return 0;
+}
+
+static coroutine_fn int ssh_co_write(BlockDriverState *bs,
+                                     int64_t sector_num,
+                                     const uint8_t *buf, int nb_sectors)
+{
+    BDRVSSHState *s = bs->opaque;
+    int ret;
+
+    qemu_co_mutex_lock(&s->lock);
+    ret = ssh_write(s, sector_num * SECTOR_SIZE, (const char *)buf,
+                    nb_sectors * SECTOR_SIZE);
+
+    qemu_co_mutex_unlock(&s->lock);
+
+    return ret;
+}
+
+static int64_t ssh_getlength(BlockDriverState *bs)
+{
+    BDRVSSHState *s = bs->opaque;
+    return (int64_t) s->attrs.filesize;
+}
+
+static BlockDriver bdrv_ssh = {
+    .format_name                  = "ssh",
+    .protocol_name                = "ssh",
+    .instance_size                = sizeof(BDRVSSHState),
+    .bdrv_file_open               = ssh_file_open,
+    .bdrv_close                   = ssh_close,
+    .bdrv_read                    = ssh_co_read,
+    .bdrv_write                   = ssh_co_write,
+    .bdrv_getlength               = ssh_getlength,
+#if 0
+    .bdrv_aio_readv               = ssh_aio_readv,
+    .bdrv_aio_writev              = ssh_aio_writev,
+    .bdrv_aio_flush               = ssh_aio_flush,
+#endif
+};
+
+static void bdrv_ssh_init(void)
+{
+    bdrv_register(&bdrv_ssh);
+}
+
+block_init(bdrv_ssh_init);
diff --git a/configure b/configure
index 46a7594..4ea1b39 100755
--- a/configure
+++ b/configure
@@ -229,6 +229,7 @@ virtio_blk_data_plane=""
 gtk=""
 gtkabi="2.0"
 tpm="no"
+libssh2=""
 
 # parse CC options first
 for opt do
@@ -908,6 +909,10 @@ for opt do
   ;;
   --enable-tpm) tpm="yes"
   ;;
+  --disable-libssh2) libssh2="no"
+  ;;
+  --enable-libssh2) libssh2="yes"
+  ;;
   *) echo "ERROR: unknown option $opt"; show_help="yes"
   ;;
   esac
@@ -1164,6 +1169,7 @@ echo "  --disable-glusterfs      disable GlusterFS backend"
 echo "  --enable-gcov            enable test coverage analysis with gcov"
 echo "  --gcov=GCOV              use specified gcov [$gcov_tool]"
 echo "  --enable-tpm             enable TPM support"
+echo "  --enable-libssh2         enable ssh block device support"
 echo ""
 echo "NOTE: The object files are built at the place where configure is launched"
 exit 1
@@ -2328,6 +2334,42 @@ EOF
 fi
 
 ##########################################
+# libssh2 probe
+if test "$libssh2" != "no" ; then
+  cat > $TMPC <<EOF
+#include <stdio.h>
+#include <libssh2.h>
+#include <libssh2_sftp.h>
+int main(void) {
+    LIBSSH2_SESSION *session;
+    session = libssh2_session_init ();
+    (void) libssh2_sftp_init (session);
+    return 0;
+}
+EOF
+
+  if $pkg_config libssh2 --modversion >/dev/null 2>&1; then
+    libssh2_cflags=`$pkg_config libssh2 --cflags`
+    libssh2_libs=`$pkg_config libssh2 --libs`
+  else
+    libssh2_cflags=
+    libssh2_libs="-lssh2"
+  fi
+
+  if compile_prog "$libssh2_cflags" "$libssh2_libs" ; then
+    libssh2=yes
+    libs_tools="$libssh2_libs $libs_tools"
+    libs_softmmu="$libssh2_libs $libs_softmmu"
+    QEMU_CFLAGS="$QEMU_CFLAGS $libssh2_cflags"
+  else
+    if test "$libssh2" = "yes" ; then
+      feature_not_found "libssh2"
+    fi
+    libssh2=no
+  fi
+fi
+
+##########################################
 # linux-aio probe
 
 if test "$linux_aio" != "no" ; then
@@ -3439,6 +3481,7 @@ echo "virtio-blk-data-plane $virtio_blk_data_plane"
 echo "gcov              $gcov_tool"
 echo "gcov enabled      $gcov"
 echo "TPM support       $tpm"
+echo "libssh2 support   $libssh2"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -3804,6 +3847,10 @@ if test "$glusterfs" = "yes" ; then
   echo "CONFIG_GLUSTERFS=y" >> $config_host_mak
 fi
 
+if test "$libssh2" = "yes" ; then
+  echo "CONFIG_LIBSSH2=y" >> $config_host_mak
+fi
+
 if test "$virtio_blk_data_plane" = "yes" ; then
   echo "CONFIG_VIRTIO_BLK_DATA_PLANE=y" >> $config_host_mak
 fi
diff --git a/qemu-doc.texi b/qemu-doc.texi
index af84bef..004b2ff 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -423,6 +423,7 @@ snapshots.
 * disk_images_sheepdog::      Sheepdog disk images
 * disk_images_iscsi::         iSCSI LUNs
 * disk_images_gluster::       GlusterFS disk images
+* disk_images_ssh::           Secure Shell (ssh) disk images
 @end menu
 
 @node disk_images_quickstart
@@ -1038,6 +1039,33 @@ qemu-system-x86_64 -drive file=gluster+unix:///testvol/dir/a.img?socket=/tmp/glu
 qemu-system-x86_64 -drive file=gluster+rdma://1.2.3.4:24007/testvol/a.img
 @end example
 
+@node disk_images_ssh
+@subsection Secure Shell (ssh) disk images
+
+You can access disk images located on a remote ssh server
+by using the ssh protocol:
+@example
+qemu-system-x86_64 -drive file=ssh://[@var{user}@@]@var{server}[:@var{port}]/@var{path}
+@end example
+
+@var{ssh} is the protocol.
+
+@var{user} is the remote user.  If not specified, then the local
+username is tried.
+
+@var{server} specifies the remote ssh server.  Any ssh server can be
+used, but it must implement the sftp-server protocol.  Most Unix/Linux
+systems should work without requiring any extra configuration.
+
+@var{port} is the port number on which sshd is listening.  By default
+the standard ssh port (22) is used.
+
+@var{path} is the path to the disk image.
+
+Authentication must currently be done without requiring a password or
+passphrase.  Set up ssh-agent with authorized keys, or Kerberos or
+similar.
+
 @node pcsys_network
 @section Network emulation
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Stefan Hajnoczi]

On Thu, Mar 21, 2013 at 01:38:58PM +0000, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
>   qemu-system-x86_64 -drive file=ssh://hostname/some/image
> 
> QEMU will ssh into 'hostname' and open '/some/image' which is made
> available as a standard block device.
> 
> You can specify a username (ssh://user@host/...) and/or a port number
> (ssh://host:port/...).

I can see this being handy for qemu-img since it gives you the ability
to work with remote image files.

> Current limitations:
> 
> - Authentication must be done without passwords or passphrases, using
>   ssh-agent.  Other authentication methods are not supported. (*)
> 
> - Does not check host key. (*)
> 
> - New remote files cannot be created. (*)

Would be important to fix these limitations.  Authentication methods to
make this more usable.  Host key check for security.  File creation for
qemu-img.

> - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
>   non-blocking access, so this could be fixed with some effort).

This patch does not really use coroutines - the SSH I/O is blocking!

Coroutines must submit the SSH I/O and then yield so the QEMU event loop
can get on with other work.  When SSH I/O finishes the request's
coroutine is re-entered and the request gets completed.

> - Blocks during connection and authentication.

Right now the code also blocks while SSH I/O takes place.

> (*) = potentially easy fix
> 
> This is implemented using libssh2 on the client side.  The server just
> requires a regular ssh daemon with sftp-server support.  Most ssh
> daemons on Unix/Linux systems will work out of the box.

How much of a win over sshfs is this?

sshfs can be mounted by unprivileged users and QEMU accesses it like a
regular file.  So the sshfs approach already works today.

Stefan

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Richard W.M. Jones]

On Thu, Mar 21, 2013 at 04:26:23PM +0100, Stefan Hajnoczi wrote:
> On Thu, Mar 21, 2013 at 01:38:58PM +0000, Richard W.M. Jones wrote:
> > From: "Richard W.M. Jones" <rjones@redhat.com>
> > 
> >   qemu-system-x86_64 -drive file=ssh://hostname/some/image
> > 
> > QEMU will ssh into 'hostname' and open '/some/image' which is made
> > available as a standard block device.
> > 
> > You can specify a username (ssh://user@host/...) and/or a port number
> > (ssh://host:port/...).
> 
> I can see this being handy for qemu-img since it gives you the ability
> to work with remote image files.
> 
> > Current limitations:
> > 
> > - Authentication must be done without passwords or passphrases, using
> >   ssh-agent.  Other authentication methods are not supported. (*)
> > 
> > - Does not check host key. (*)
> > 
> > - New remote files cannot be created. (*)
> 
> Would be important to fix these limitations.  Authentication methods to
> make this more usable.  Host key check for security.  File creation for
> qemu-img.

I agree.

> > - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
> >   non-blocking access, so this could be fixed with some effort).
> 
> This patch does not really use coroutines - the SSH I/O is blocking!
> 
> Coroutines must submit the SSH I/O and then yield so the QEMU event loop
> can get on with other work.  When SSH I/O finishes the request's
> coroutine is re-entered and the request gets completed.

Hmm OK.  Is there any documentation at all on how coroutines are
supposed to work?  Or AIO for that matter?  For example, do coroutines
really replace all the read/write syscalls deep inside a library
(libssh2) so that these calls context switch, or am I missing the
point of how this works entirely?

[...]
> > This is implemented using libssh2 on the client side.  The server just
> > requires a regular ssh daemon with sftp-server support.  Most ssh
> > daemons on Unix/Linux systems will work out of the box.
> 
> How much of a win over sshfs is this?
> 
> sshfs can be mounted by unprivileged users and QEMU accesses it like a
> regular file.  So the sshfs approach already works today.

Sure, but compared to having to install and set up sshfs and FUSE,
using this is a lot simpler.  It's also potentially faster since it
cuts out FUSE and context switching to the sshfs process.

BTW I looked into the implementation of sshfs before starting this,
and what it does is to run an 'ssh' client subprocess, then implements
the sftp protocol by hand on top.  So using libssh2 cuts out *two*
external processes (plus FUSE).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Stefan Hajnoczi]

On Thu, Mar 21, 2013 at 4:39 PM, Richard W.M. Jones <rjones@redhat.com> wrote:
> On Thu, Mar 21, 2013 at 04:26:23PM +0100, Stefan Hajnoczi wrote:
>> On Thu, Mar 21, 2013 at 01:38:58PM +0000, Richard W.M. Jones wrote:
>> > From: "Richard W.M. Jones" <rjones@redhat.com>
>> >
>> >   qemu-system-x86_64 -drive file=ssh://hostname/some/image
>> >
>> > QEMU will ssh into 'hostname' and open '/some/image' which is made
>> > available as a standard block device.
>> >
>> > You can specify a username (ssh://user@host/...) and/or a port number
>> > (ssh://host:port/...).
>>
>> I can see this being handy for qemu-img since it gives you the ability
>> to work with remote image files.
>>
>> > Current limitations:
>> >
>> > - Authentication must be done without passwords or passphrases, using
>> >   ssh-agent.  Other authentication methods are not supported. (*)
>> >
>> > - Does not check host key. (*)
>> >
>> > - New remote files cannot be created. (*)
>>
>> Would be important to fix these limitations.  Authentication methods to
>> make this more usable.  Host key check for security.  File creation for
>> qemu-img.
>
> I agree.
>
>> > - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
>> >   non-blocking access, so this could be fixed with some effort).
>>
>> This patch does not really use coroutines - the SSH I/O is blocking!
>>
>> Coroutines must submit the SSH I/O and then yield so the QEMU event loop
>> can get on with other work.  When SSH I/O finishes the request's
>> coroutine is re-entered and the request gets completed.
>
> Hmm OK.  Is there any documentation at all on how coroutines are
> supposed to work?  Or AIO for that matter?  For example, do coroutines
> really replace all the read/write syscalls deep inside a library
> (libssh2) so that these calls context switch, or am I missing the
> point of how this works entirely?

Before we go into coroutines, take a look at AIO since it may fit
libssh2's non-blocking mode of operation better.

Doing AIO means implementing .bdrv_aio_readv()/.bdrv_aio_writev().
These functions cannot block so you need to use non-blocking libssh2
interfaces and let QEMU's event loop notify us of
readability/writability (see qemu_aio_set_fd_handler()).

Look at block/iscsi.c:iscsi_aio_readv() for an example of how to
interface with an asynchronous library.

Back to coroutines.  Coroutines are just a primitive, like threads,
that your code can use.  They aren't a framework for how to do block
I/O.

If you want an example, take a look at block/sheepdog.c:

static coroutine_fn void do_co_req(void *opaque)
{
    int ret;
    Coroutine *co;
[...]
    co = qemu_coroutine_self();
    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, have_co_req, co);

    ret = send_co_req(sockfd, hdr, data, wlen);

Using qemu_aio_set_fd_handler() we tell the event loop to call
restart_co_req() when the file descriptor becomes writable.

Then we call send_co_req() which yields if send(2) returns EAGAIN.

This is an example of setting up an event handler, invoking a
non-blocking syscall, and yielding on EAGAIN.  Here is what
restart_co_req() looks like:

static void restart_co_req(void *opaque)
{
    Coroutine *co = opaque;

    qemu_coroutine_enter(co, NULL);
}

If you want to make the code clean and reusable it's best to put the
event handler, non-blocking I/O, and yield/re-enter into a function
that can be reused.  That way each callers don't duplicate this
low-level code.

Hope this gives you an idea of how to use coroutines for block drivers.

>> > This is implemented using libssh2 on the client side.  The server just
>> > requires a regular ssh daemon with sftp-server support.  Most ssh
>> > daemons on Unix/Linux systems will work out of the box.
>>
>> How much of a win over sshfs is this?
>>
>> sshfs can be mounted by unprivileged users and QEMU accesses it like a
>> regular file.  So the sshfs approach already works today.
>
> Sure, but compared to having to install and set up sshfs and FUSE,
> using this is a lot simpler.  It's also potentially faster since it
> cuts out FUSE and context switching to the sshfs process.
>
> BTW I looked into the implementation of sshfs before starting this,
> and what it does is to run an 'ssh' client subprocess, then implements
> the sftp protocol by hand on top.  So using libssh2 cuts out *two*
> external processes (plus FUSE).

I see.  Benchmarks would be interesting once AIO or coroutines is implemented.

Stefan

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Stefan Hajnoczi]

On Thu, Mar 21, 2013 at 2:38 PM, Richard W.M. Jones <rjones@redhat.com> wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
>
>   qemu-system-x86_64 -drive file=ssh://hostname/some/image
>
> QEMU will ssh into 'hostname' and open '/some/image' which is made
> available as a standard block device.
>
> You can specify a username (ssh://user@host/...) and/or a port number
> (ssh://host:port/...).
>
> Current limitations:
>
> - Authentication must be done without passwords or passphrases, using
>   ssh-agent.  Other authentication methods are not supported. (*)
>
> - Does not check host key. (*)
>
> - New remote files cannot be created. (*)
>
> - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
>   non-blocking access, so this could be fixed with some effort).
>
> - Blocks during connection and authentication.
>
> (*) = potentially easy fix
>
> This is implemented using libssh2 on the client side.  The server just
> requires a regular ssh daemon with sftp-server support.  Most ssh
> daemons on Unix/Linux systems will work out of the box.
> ---
>  block/Makefile.objs |   1 +
>  block/ssh.c         | 514 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  configure           |  47 +++++
>  qemu-doc.texi       |  28 +++
>  4 files changed, 590 insertions(+)
>  create mode 100644 block/ssh.c

Just noticed that libcurl supports sftp.

Did you try enabling sftp support in block/curl.c?  I think you just
need to add CURLPROTO_SFTP to #define PROTOCOLS.

Stefan

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Richard W.M. Jones]

On Thu, Mar 21, 2013 at 08:35:29PM +0100, Stefan Hajnoczi wrote:
> On Thu, Mar 21, 2013 at 2:38 PM, Richard W.M. Jones <rjones@redhat.com> wrote:
> > From: "Richard W.M. Jones" <rjones@redhat.com>
> >
> >   qemu-system-x86_64 -drive file=ssh://hostname/some/image
> >
> > QEMU will ssh into 'hostname' and open '/some/image' which is made
> > available as a standard block device.
> >
> > You can specify a username (ssh://user@host/...) and/or a port number
> > (ssh://host:port/...).
> >
> > Current limitations:
> >
> > - Authentication must be done without passwords or passphrases, using
> >   ssh-agent.  Other authentication methods are not supported. (*)
> >
> > - Does not check host key. (*)
> >
> > - New remote files cannot be created. (*)
> >
> > - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
> >   non-blocking access, so this could be fixed with some effort).
> >
> > - Blocks during connection and authentication.
> >
> > (*) = potentially easy fix
> >
> > This is implemented using libssh2 on the client side.  The server just
> > requires a regular ssh daemon with sftp-server support.  Most ssh
> > daemons on Unix/Linux systems will work out of the box.
> > ---
> >  block/Makefile.objs |   1 +
> >  block/ssh.c         | 514 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  configure           |  47 +++++
> >  qemu-doc.texi       |  28 +++
> >  4 files changed, 590 insertions(+)
> >  create mode 100644 block/ssh.c
> 
> Just noticed that libcurl supports sftp.
> 
> Did you try enabling sftp support in block/curl.c?  I think you just
> need to add CURLPROTO_SFTP to #define PROTOCOLS.

Interestingly curl's sftp support is implemented using libssh2.  I'll
take a look at how easy this will be.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

[Qemu-devel] [PATCH] block/curl: Add support for Secure Shell (ssh/sftp) block device

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]


I got it working with Curl, patch attached.

However there are multiple issues (these are mainly notes for myself):

(1) libcurl cannot read the size of the file.  I had to hard-code
this.  This is probably just a shortcoming of libcurl (libssh2/sftp
itself can read the size of files).  Will try to work on a patch for
upstream.

(2) Fedora's curl (which is heavily patched) is broken in some way and
deadlocks itself.  Upstream curl from git works better.  I haven't yet
identified which patch/commit is responsible.

(3) ssh-agent authentication doesn't work.  It appears that either
ssh-agent itself doesn't like multiple connections from a single
process (qemu), or libcurl/libssh2 is having a problem with making
multiple connections out to ssh-agent.  If I disable ssh-agent auth,
it works.  Still investigating this.

(4) You must specify a user@ in the URL, else libcurl tries to
authenticate as user "".  I will see if I can send a fix for this
upstream.

(5) Although it gets much of the way through a boot of a guest, it
eventually segfaults.  Still investigating this.

(6) There are several more issues marked by XXX's in the code.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

[-- Attachment #2: 0001-block-curl-Add-support-for-Secure-Shell-ssh-sftp-blo.patch --]
[-- Type: text/plain, Size: 3280 bytes --]

>From c2ae4abf29973b8b8bc490b84fb761a8a4053fbf Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Thu, 21 Mar 2013 21:38:10 +0000
Subject: [PATCH] block/curl: Add support for Secure Shell (ssh/sftp) block
 device.

 qemu-system-x86_64 -drive file=sftp://user@hostname/some/image

QEMU will ssh into 'hostname' as 'user' and open '/some/image' which
is made available as a standard block device.

You must specify a 'user@' in the URL.

The server just requires a regular ssh daemon with sftp-server
support.  Most ssh daemons on Unix/Linux systems will work out of the
box.

This is implemented using curl (using libssh2 underneath).

Thanks: Stefan Hajnoczi for pointing out that this could be done much
more easily using curl instead of using libssh2 directly.
---
 block/curl.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 98947da..b236f7f 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -36,7 +36,7 @@
 
 #define PROTOCOLS (CURLPROTO_HTTP | CURLPROTO_HTTPS | \
                    CURLPROTO_FTP | CURLPROTO_FTPS | \
-                   CURLPROTO_TFTP)
+                   CURLPROTO_TFTP | CURLPROTO_SFTP)
 
 #define CURL_NUM_STATES 8
 #define CURL_NUM_ACB    8
@@ -305,6 +305,17 @@ static CURLState *curl_init_state(BDRVCURLState *s)
     curl_easy_setopt(state->curl, CURLOPT_NOSIGNAL, 1);
     curl_easy_setopt(state->curl, CURLOPT_ERRORBUFFER, state->errmsg);
     curl_easy_setopt(state->curl, CURLOPT_FAILONERROR, 1);
+#if defined(CURLSSH_AUTH_ANY)
+    curl_easy_setopt(state->curl, CURLOPT_SSH_AUTH_TYPES, CURLSSH_AUTH_ANY);
+#endif
+
+    /* XXX */
+    curl_easy_setopt (state->curl, CURLOPT_SSH_PRIVATE_KEYFILE,
+                      "/home/rjones/.ssh/id_rsa"); /* XXX */
+    curl_easy_setopt (state->curl, CURLOPT_SSH_PUBLIC_KEYFILE,
+                      "/home/rjones/.ssh/id_rsa.pub"); /* XXX */
+    curl_easy_setopt (state->curl, CURLOPT_KEYPASSWD,
+                      "XXX PUT YOUR PASSPHRASE HERE XXX"); /* XXX */
 
     /* Restrict supported protocols to avoid security issues in the more
      * obscure protocols.  For example, do not allow POP3/SMTP/IMAP see
@@ -406,8 +417,12 @@ static int curl_open(BlockDriverState *bs, const char *filename, int flags)
     curl_easy_setopt(state->curl, CURLOPT_NOBODY, 0);
     if (d)
         s->len = (size_t)d;
+#if 0
     else if(!s->len)
         goto out;
+#else
+    s->len = 8589934592; /* XXX */
+#endif
     DPRINTF("CURL: Size = %zd\n", s->len);
 
     curl_clean_state(state);
@@ -626,6 +641,18 @@ static BlockDriver bdrv_tftp = {
     .bdrv_aio_readv  = curl_aio_readv,
 };
 
+static BlockDriver bdrv_sftp = {
+    .format_name     = "sftp",
+    .protocol_name   = "sftp",
+
+    .instance_size   = sizeof(BDRVCURLState),
+    .bdrv_file_open  = curl_open,
+    .bdrv_close      = curl_close,
+    .bdrv_getlength  = curl_getlength,
+
+    .bdrv_aio_readv  = curl_aio_readv,
+};
+
 static void curl_block_init(void)
 {
     bdrv_register(&bdrv_http);
@@ -633,6 +660,7 @@ static void curl_block_init(void)
     bdrv_register(&bdrv_ftp);
     bdrv_register(&bdrv_ftps);
     bdrv_register(&bdrv_tftp);
+    bdrv_register(&bdrv_sftp);
 }
 
 block_init(curl_block_init);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] block/curl: Add support for Secure Shell (ssh/sftp) block device

[Stefan Hajnoczi]

On Fri, Mar 22, 2013 at 01:04:55PM +0000, Richard W.M. Jones wrote:
> 
> I got it working with Curl, patch attached.
> 
> However there are multiple issues (these are mainly notes for myself):
> 
> (1) libcurl cannot read the size of the file.  I had to hard-code
> this.  This is probably just a shortcoming of libcurl (libssh2/sftp
> itself can read the size of files).  Will try to work on a patch for
> upstream.
> 
> (2) Fedora's curl (which is heavily patched) is broken in some way and
> deadlocks itself.  Upstream curl from git works better.  I haven't yet
> identified which patch/commit is responsible.
> 
> (3) ssh-agent authentication doesn't work.  It appears that either
> ssh-agent itself doesn't like multiple connections from a single
> process (qemu), or libcurl/libssh2 is having a problem with making
> multiple connections out to ssh-agent.  If I disable ssh-agent auth,
> it works.  Still investigating this.
> 
> (4) You must specify a user@ in the URL, else libcurl tries to
> authenticate as user "".  I will see if I can send a fix for this
> upstream.
> 
> (5) Although it gets much of the way through a boot of a guest, it
> eventually segfaults.  Still investigating this.
> 
> (6) There are several more issues marked by XXX's in the code.

Thank you for improving libcurl!  You're making it better for everybody.

A lot of people go back to NIH when they hit limitations in existing
software.

Stefan

Re: [Qemu-devel] [PATCH] block/curl: Add support for Secure Shell (ssh/sftp) block device

[Richard W.M. Jones]

On Fri, Mar 22, 2013 at 01:04:55PM +0000, Richard W.M. Jones wrote:
> I got it working with Curl, patch attached.
> 
> However there are multiple issues (these are mainly notes for myself):
> 
> (1) libcurl cannot read the size of the file.  I had to hard-code
> this.  This is probably just a shortcoming of libcurl (libssh2/sftp
> itself can read the size of files).  Will try to work on a patch for
> upstream.

After my holiday and in the cold of day I've had a long look at the
SFTP implementation in libcurl.

  https://github.com/bagder/curl/blob/master/lib/ssh.c

It's implemented as a huge state machine and simply implementing (1)
above is problematic (I believe we would have to reopen the connection
after our call to curl_easy_perform).

The larger issue is that the qemu curl block driver doesn't support
writes.  Now these could in theory be added.  Indeed curl does support
"random access" writes, although AFAICT you have to open a new
connection each time you want to seek backwards, and you can't read
and write over the same connection (so you'd need >= 1 connections for
reading and another >= 1 connections for writing).

I think I will continue with the pure libssh2-based block driver,
making it support all the missing features discussed earlier:

  http://www.mail-archive.com/qemu-devel@nongnu.org/msg161997.html

plus of course non-blocking AIO.

I think this way we'll end up with a much more robust, reliable and
easier to debug ssh implementation in qemu.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH] block/curl: Add support for Secure Shell (ssh/sftp) block device

[Stefan Hajnoczi]

On Mon, Mar 25, 2013 at 1:32 PM, Richard W.M. Jones <rjones@redhat.com> wrote:
> On Fri, Mar 22, 2013 at 01:04:55PM +0000, Richard W.M. Jones wrote:
>> I got it working with Curl, patch attached.
>>
>> However there are multiple issues (these are mainly notes for myself):
>>
>> (1) libcurl cannot read the size of the file.  I had to hard-code
>> this.  This is probably just a shortcoming of libcurl (libssh2/sftp
>> itself can read the size of files).  Will try to work on a patch for
>> upstream.
>
> After my holiday and in the cold of day I've had a long look at the
> SFTP implementation in libcurl.
>
>   https://github.com/bagder/curl/blob/master/lib/ssh.c
>
> It's implemented as a huge state machine and simply implementing (1)
> above is problematic (I believe we would have to reopen the connection
> after our call to curl_easy_perform).
>
> The larger issue is that the qemu curl block driver doesn't support
> writes.  Now these could in theory be added.  Indeed curl does support
> "random access" writes, although AFAICT you have to open a new
> connection each time you want to seek backwards, and you can't read
> and write over the same connection (so you'd need >= 1 connections for
> reading and another >= 1 connections for writing).
>
> I think I will continue with the pure libssh2-based block driver,
> making it support all the missing features discussed earlier:
>
>   http://www.mail-archive.com/qemu-devel@nongnu.org/msg161997.html
>
> plus of course non-blocking AIO.
>
> I think this way we'll end up with a much more robust, reliable and
> easier to debug ssh implementation in qemu.

Fair enough.

Stefan

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Kevin Wolf]

Am 21.03.2013 um 14:38 hat Richard W.M. Jones geschrieben:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
>   qemu-system-x86_64 -drive file=ssh://hostname/some/image
> 
> QEMU will ssh into 'hostname' and open '/some/image' which is made
> available as a standard block device.
> 
> You can specify a username (ssh://user@host/...) and/or a port number
> (ssh://host:port/...).
> 
> Current limitations:
> 
> - Authentication must be done without passwords or passphrases, using
>   ssh-agent.  Other authentication methods are not supported. (*)

Maybe you can just expose it as an encrypted image in order to allow
password authentication?

> - Does not check host key. (*)
> 
> - New remote files cannot be created. (*)
> 
> - Uses coroutine read/write, instead of true AIO.  (libssh2 supports
>   non-blocking access, so this could be fixed with some effort).
> 
> - Blocks during connection and authentication.
> 
> (*) = potentially easy fix
> 
> This is implemented using libssh2 on the client side.  The server just
> requires a regular ssh daemon with sftp-server support.  Most ssh
> daemons on Unix/Linux systems will work out of the box.

> --- /dev/null
> +++ b/block/ssh.c
> @@ -0,0 +1,514 @@
> +/*
> + * Secure Shell (ssh) backend for QEMU.
> + *
> + * Copyright (C) 2013 Red Hat Inc., Richard W.M. Jones <rjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */

Please consider the MIT license or LGPL for block layer code; otherwise
we may have to compile the driver out for a future libqblock.

> +
> +#define SECTOR_SIZE 512

BDRV_SECTOR_SIZE exists.

> +
> +static void __attribute__((constructor)) ssh_init(void)
> +{
> +    int r;
> +
> +    r = libssh2_init(0);
> +    if (r != 0) {
> +        fprintf(stderr, "libssh2 initialization failed, %d\n", r);
> +        exit(EXIT_FAILURE);
> +    }
> +}

Why don't you include this in bdrv_ssh_init()?

> +static int parse_uri(BDRVSSHState *s, const char *filename)
> +{
> +    URI *uri;
> +
> +    uri = uri_parse(filename);
> +    if (!uri)
> +        return -EINVAL;
> +
> +    if (strcmp(uri->scheme, "ssh") != 0) {
> +        error_report("URI scheme must be 'ssh'");
> +        goto err;
> +    }
> +
> +    if (!uri->server || strcmp(uri->server, "") == 0) {
> +        error_report("missing hostname in URI");
> +        goto err;
> +    }
> +
> +    if (!uri->path || strcmp(uri->path, "") == 0) {
> +        error_report("missing remote path in URI");
> +        goto err;
> +    }
> +
> +    /* If user is defined in the URI, use it.  Else use local username. */
> +    if(uri->user && strcmp(uri->user, "") != 0)
> +        s->user = g_strdup(uri->user);
> +    else {
> +        s->user = get_username();
> +        if (!s->user)
> +            goto err;
> +    }
> +
> +    /* Construct the hostname:port string for inet_connect. */
> +    if(uri->port == 0)
> +        uri->port = 22;
> +    s->host = g_strdup_printf("%s:%d", uri->server, uri->port);
> +
> +    s->path = g_strdup(uri->path);
> +
> +    return 0;
> +
> + err:
> +    uri_free(uri);
> +    return -EINVAL;
> +}

You could implement this similar to what I just changed in NBD, using
the new .bdrv_parse_filename callback and driver-specific options, so
that instead of specifying a URL you can use something like:

    -drive file.driver=ssh,file.host=localhost,file.user=test

This should turn out much more elegant than management tools encoding
everything in a single filename string and qemu parsing it again,
especially once QMP support passing this as JSON objects.

> +static int ssh_file_open(BlockDriverState *bs, const char *filename,
> +                         int bdrv_flags)

After rebasing, this will have a new parameter for driver-specific
options.

> +    ret = parse_uri(s, filename);
> +    if (ret < 0)
> +        goto err;

I only see it now, but it seems to be repeated all over the place: The
qemu coding style requires braces here.

> +
> +    /* Open the socket and connect. */
> +    s->sock = inet_connect(s->host, &err);
> +    if (err != NULL) {
> +        ret = -errno;
> +        qerror_report_err(err);
> +        error_free(err);
> +        goto err;
> +    }
> +
> +    /* Start up SSH. */
> +    ret = connect_to_ssh(s);
> +    if (ret < 0)
> +        goto err;
> +
> +#if 0 /* Enable this when AIO callbacks are implemented. */
> +    /* Go non-blocking. */
> +    libssh2_session_set_blocking(s->session, 0);
> +#endif
> +
> +    qemu_co_mutex_init(&s->lock);
> +
> +    return 0;
> +
> + err:
> +    if (s->sock >= 0)
> +        close(s->sock);
> +    s->sock = -1;
> +
> +    g_free(s->path);
> +    s->path = NULL;
> +    g_free(s->host);
> +    s->host = NULL;
> +    g_free(s->user);
> +    s->user = NULL;

Resetting the fields isn't really necessary. s will be freed anyway.

> +static BlockDriver bdrv_ssh = {
> +    .format_name                  = "ssh",
> +    .protocol_name                = "ssh",
> +    .instance_size                = sizeof(BDRVSSHState),
> +    .bdrv_file_open               = ssh_file_open,
> +    .bdrv_close                   = ssh_close,
> +    .bdrv_read                    = ssh_co_read,
> +    .bdrv_write                   = ssh_co_write,
> +    .bdrv_getlength               = ssh_getlength,
> +#if 0
> +    .bdrv_aio_readv               = ssh_aio_readv,
> +    .bdrv_aio_writev              = ssh_aio_writev,
> +    .bdrv_aio_flush               = ssh_aio_flush,
> +#endif
> +};

You don't have a bdrv_co_flush_to_disk, what does this mean? Is every
write immediately flushed to the disk, is the driver unsafe by design,
or is this just a missing implementation detail?

Kevin

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Richard W.M. Jones]

On Mon, Mar 25, 2013 at 03:36:22PM +0100, Kevin Wolf wrote:
> You don't have a bdrv_co_flush_to_disk, what does this mean? Is every
> write immediately flushed to the disk, is the driver unsafe by design,
> or is this just a missing implementation detail?

Initially there is no .bdrv_co_flush_to_disk because this patch is
just a proof of concept.

However it does seem likely that a true flush-to-disk operation is not
possible with sftp.  Assuming this is supposed to guarantee that the
bits have been committed to disk, I don't see anything in the sftp
protocol that supports that guarantee.  It seems to be left up to the
implementation to do whatever it wants.

Here's the protocol v3 as implemented by OpenSSH:

  http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02

and here's the latest version v6 (not implemented by anyone AFAIK):

  http://tools.ietf.org/html/draft-ietf-secsh-filexfer-13

if you think you can see anything that I've missed ...

Also I grepped over the source of OpenSSH and there is no call to
sync(2), fsync(2) or fdatasync(2).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] [PATCH] block: Add support for Secure Shell (ssh) block device.

[Stefan Hajnoczi]

On Mon, Mar 25, 2013 at 03:11:37PM +0000, Richard W.M. Jones wrote:
> On Mon, Mar 25, 2013 at 03:36:22PM +0100, Kevin Wolf wrote:
> > You don't have a bdrv_co_flush_to_disk, what does this mean? Is every
> > write immediately flushed to the disk, is the driver unsafe by design,
> > or is this just a missing implementation detail?
> 
> Initially there is no .bdrv_co_flush_to_disk because this patch is
> just a proof of concept.
> 
> However it does seem likely that a true flush-to-disk operation is not
> possible with sftp.  Assuming this is supposed to guarantee that the
> bits have been committed to disk, I don't see anything in the sftp
> protocol that supports that guarantee.  It seems to be left up to the
> implementation to do whatever it wants.
> 
> Here's the protocol v3 as implemented by OpenSSH:
> 
>   http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02
> 
> and here's the latest version v6 (not implemented by anyone AFAIK):
> 
>   http://tools.ietf.org/html/draft-ietf-secsh-filexfer-13
> 
> if you think you can see anything that I've missed ...
> 
> Also I grepped over the source of OpenSSH and there is no call to
> sync(2), fsync(2) or fdatasync(2).

I looked at OpenSSH sftp-server.c to see if open, close, or write do
anything special.  They don't.

There's also no 'sync' command in the SFTP protocol.

So unless the underlying filesystem is mounted -o sync, there is no
guarantee that data is safely on disk.

Stefan