* [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
@ 2019-06-05 21:36 Pino Toscano
2019-06-05 22:57 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Pino Toscano @ 2019-06-05 21:36 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: kwolf, ptoscano, philmd, rjones, mreitz
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 <ptoscano@redhat.com>
---
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/block/Makefile.objs b/block/Makefile.objs
index ae11605c9f..bf01429dd5 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
block-obj-$(CONFIG_RBD) += rbd.o
block-obj-$(CONFIG_GLUSTERFS) += gluster.o
block-obj-$(CONFIG_VXHS) += vxhs.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
@@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
gluster.o-cflags := $(GLUSTERFS_CFLAGS)
gluster.o-libs := $(GLUSTERFS_LIBS)
vxhs.o-libs := $(VXHS_LIBS)
-ssh.o-cflags := $(LIBSSH2_CFLAGS)
-ssh.o-libs := $(LIBSSH2_LIBS)
+ssh.o-cflags := $(LIBSSH_CFLAGS)
+ssh.o-libs := $(LIBSSH_LIBS)
block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
dmg-bz2.o-libs := $(BZIP2_LIBS)
diff --git a/block/ssh.c b/block/ssh.c
index 12fd4f39e8..ce2363a471 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include <libssh2.h>
-#include <libssh2_sftp.h>
+#include <libssh/libssh.h>
+#include <libssh/sftp.h>
#include "block/block_int.h"
#include "block/qdict.h"
@@ -43,14 +43,13 @@
#include "qapi/qobject-output-visitor.h"
#include "trace.h"
-/*
- * TRACE_LIBSSH2=<bitmask> 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 <bitmask> is described
- * here: http://www.libssh2.org/libssh2_trace.html
+/* TRACE_LIBSSH=<level> enables tracing in libssh itself.
+ * The meaning of <level> is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
+
+#define HAVE_LIBSSH_0_8 (LIBSSH_VERSION_INT >= SSH_VERSION_INT(0, 8, 0))
typedef struct BDRVSSHState {
/* Coroutine. */
@@ -58,18 +57,14 @@ typedef struct BDRVSSHState {
/* 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;
InetSocketAddress *inet;
@@ -89,7 +84,6 @@ static void ssh_state_init(BDRVSSHState *s)
{
memset(s, 0, sizeof *s);
s->sock = -1;
- s->offset = -1;
qemu_co_mutex_init(&s->lock);
}
@@ -97,21 +91,20 @@ static void ssh_state_free(BDRVSSHState *s)
{
g_free(s->user);
+ 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);
}
+ /* s->sock is owned by the ssh_session, which frees it. */
}
static void GCC_FMT_ATTR(3, 4)
@@ -125,13 +118,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 <libssh2.h>. */
- ssh_err_code = libssh2_session_last_error(s->session,
- &ssh_err, NULL, 0);
- error_setg(errp, "%s: %s (libssh2 error code: %d)",
+ /* This is not an errno. See <libssh/libssh.h>. */
+ ssh_err = ssh_get_error(s->session);
+ ssh_err_code = ssh_get_error_code(s->session);
+ error_setg(errp, "%s: %s (libssh error code: %d)",
msg, ssh_err, ssh_err_code);
} else {
error_setg(errp, "%s", msg);
@@ -150,18 +143,18 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
va_end(args);
if (s->sftp) {
- char *ssh_err;
+ const char *ssh_err;
int ssh_err_code;
- unsigned long sftp_err_code;
+ int sftp_err_code;
- /* This is not an errno. See <libssh2.h>. */
- ssh_err_code = libssh2_session_last_error(s->session,
- &ssh_err, NULL, 0);
- /* See <libssh2_sftp.h>. */
- sftp_err_code = libssh2_sftp_last_error((s)->sftp);
+ /* This is not an errno. See <libssh/libssh.h>. */
+ ssh_err = ssh_get_error(s->session);
+ ssh_err_code = ssh_get_error_code(s->session);
+ /* See <libssh/sftp.h>. */
+ sftp_err_code = sftp_get_error(s->sftp);
error_setg(errp,
- "%s: %s (libssh2 error code: %d, sftp error code: %lu)",
+ "%s: %s (libssh error code: %d, sftp error code: %d)",
msg, ssh_err, ssh_err_code, sftp_err_code);
} else {
error_setg(errp, "%s", msg);
@@ -171,15 +164,15 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
static void sftp_error_trace(BDRVSSHState *s, const char *op)
{
- char *ssh_err;
+ const char *ssh_err;
int ssh_err_code;
- unsigned long sftp_err_code;
+ int sftp_err_code;
- /* This is not an errno. See <libssh2.h>. */
- ssh_err_code = libssh2_session_last_error(s->session,
- &ssh_err, NULL, 0);
- /* See <libssh2_sftp.h>. */
- sftp_err_code = libssh2_sftp_last_error((s)->sftp);
+ /* This is not an errno. See <libssh/libssh.h>. */
+ ssh_err = ssh_get_error(s->session);
+ ssh_err_code = ssh_get_error_code(s->session);
+ /* See <libssh/sftp.h>. */
+ sftp_err_code = sftp_get_error(s->sftp);
trace_sftp_error(op, ssh_err, ssh_err_code, sftp_err_code);
}
@@ -280,82 +273,89 @@ static void ssh_parse_filename(const char *filename, QDict *options,
parse_uri(filename, options, errp);
}
-static int check_host_key_knownhosts(BDRVSSHState *s,
- const char *host, int port, Error **errp)
+static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
{
- const char *home;
- char *knh_file = NULL;
- LIBSSH2_KNOWNHOSTS *knh = NULL;
- struct libssh2_knownhost *found;
- int ret, r;
- const char *hostkey;
- size_t len;
- int type;
+ int ret;
+#if HAVE_LIBSSH_0_8
+ enum ssh_known_hosts_e state;
- hostkey = libssh2_session_hostkey(s->session, &len, &type);
- if (!hostkey) {
+ state = ssh_session_is_known_server(s->session);
+ trace_ssh_server_status(state);
+
+ switch (state) {
+ case SSH_KNOWN_HOSTS_OK:
+ /* OK */
+ trace_ssh_check_host_key_knownhosts();
+ break;
+ case SSH_KNOWN_HOSTS_CHANGED:
ret = -EINVAL;
- session_error_setg(errp, s, "failed to read remote host key");
+ error_setg(errp, "host key does not match the one in known_hosts");
goto out;
- }
-
- knh = libssh2_knownhost_init(s->session);
- if (!knh) {
+ case SSH_KNOWN_HOSTS_OTHER:
ret = -EINVAL;
- session_error_setg(errp, s,
- "failed to initialize known hosts support");
+ error_setg(errp,
+ "host key for this server not found, another type exists");
+ goto out;
+ case SSH_KNOWN_HOSTS_UNKNOWN:
+ ret = -ENOENT;
+ error_setg(errp, "no host key was found in known_hosts");
+ goto out;
+ case SSH_KNOWN_HOSTS_NOT_FOUND:
+ ret = -EINVAL;
+ error_setg(errp, "known_hosts file not found");
+ goto out;
+ case SSH_KNOWN_HOSTS_ERROR:
+ ret = -EINVAL;
+ error_setg(errp, "error while checking the host");
+ goto out;
+ default:
+ ret = -EINVAL;
+ error_setg(errp, "error while checking for known server");
goto out;
}
+#else /* !HAVE_LIBSSH_0_8 */
+ int state;
- home = getenv("HOME");
- if (home) {
- knh_file = g_strdup_printf("%s/.ssh/known_hosts", home);
- } else {
- knh_file = g_strdup_printf("/root/.ssh/known_hosts");
- }
-
- /* Read all known hosts from OpenSSH-style known_hosts file. */
- libssh2_knownhost_readfile(knh, knh_file, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
+ state = ssh_is_server_known(s->session);
+ trace_ssh_server_status(state);
- r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
- LIBSSH2_KNOWNHOST_TYPE_PLAIN|
- LIBSSH2_KNOWNHOST_KEYENC_RAW,
- &found);
- switch (r) {
- case LIBSSH2_KNOWNHOST_CHECK_MATCH:
+ switch (state) {
+ case SSH_SERVER_KNOWN_OK:
/* OK */
- trace_ssh_check_host_key_knownhosts(found->key);
+ trace_ssh_check_host_key_knownhosts();
break;
- case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
+ case SSH_SERVER_KNOWN_CHANGED:
+ ret = -EINVAL;
+ error_setg(errp, "host key does not match the one in known_hosts");
+ goto out;
+ case SSH_SERVER_FOUND_OTHER:
ret = -EINVAL;
- session_error_setg(errp, s,
- "host key does not match the one in known_hosts"
- " (found key %s)", found->key);
+ error_setg(errp,
+ "host key for this server not found, another type exists");
+ goto out;
+ case SSH_SERVER_FILE_NOT_FOUND:
+ ret = -ENOENT;
+ error_setg(errp, "known_hosts file not found");
goto out;
- case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
+ case SSH_SERVER_NOT_KNOWN:
ret = -EINVAL;
- session_error_setg(errp, s, "no host key was found in known_hosts");
+ error_setg(errp, "no host key was found in known_hosts");
goto out;
- case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
+ case SSH_SERVER_ERROR:
ret = -EINVAL;
- session_error_setg(errp, s,
- "failure matching the host key with known_hosts");
+ error_setg(errp, "server error");
goto out;
default:
ret = -EINVAL;
- session_error_setg(errp, s, "unknown error matching the host key"
- " with known_hosts (%d)", r);
+ error_setg(errp, "error while checking for known server");
goto out;
}
+#endif /* !HAVE_LIBSSH_0_8 */
/* known_hosts checking successful. */
ret = 0;
out:
- if (knh != NULL) {
- libssh2_knownhost_free(knh);
- }
- g_free(knh_file);
return ret;
}
@@ -399,18 +399,34 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
static int
check_host_key_hash(BDRVSSHState *s, const char *hash,
- int hash_type, size_t fingerprint_len, Error **errp)
+ enum ssh_publickey_hash_type type, Error **errp)
{
- const char *fingerprint;
+ int r;
+ ssh_key pubkey;
+ unsigned char *server_hash;
+ size_t server_hash_len;
- fingerprint = libssh2_hostkey_hash(s->session, hash_type);
- if (!fingerprint) {
+#if HAVE_LIBSSH_0_8
+ r = ssh_get_server_publickey(s->session, &pubkey);
+#else
+ r = ssh_get_publickey(s->session, &pubkey);
+#endif
+ if (r != SSH_OK) {
session_error_setg(errp, s, "failed to read remote host key");
return -EINVAL;
}
- if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
- hash) != 0) {
+ r = ssh_get_publickey_hash(pubkey, type, &server_hash, &server_hash_len);
+ ssh_key_free(pubkey);
+ if (r != 0) {
+ session_error_setg(errp, s,
+ "failed reading the hash of the server SSH key");
+ return -EINVAL;
+ }
+
+ r = compare_fingerprint(server_hash, server_hash_len, hash);
+ ssh_clean_pubkey_hash(&server_hash);
+ if (r != 0) {
error_setg(errp, "remote host key does not match host_key_check '%s'",
hash);
return -EPERM;
@@ -419,8 +435,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
return 0;
}
-static int check_host_key(BDRVSSHState *s, const char *host, int port,
- SshHostKeyCheck *hkc, Error **errp)
+static int check_host_key(BDRVSSHState *s, SshHostKeyCheck *hkc, Error **errp)
{
SshHostKeyCheckMode mode;
@@ -436,15 +451,15 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
case SSH_HOST_KEY_CHECK_MODE_HASH:
if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_MD5) {
return check_host_key_hash(s, hkc->u.hash.hash,
- LIBSSH2_HOSTKEY_HASH_MD5, 16, errp);
+ SSH_PUBLICKEY_HASH_MD5, errp);
} else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA1) {
return check_host_key_hash(s, hkc->u.hash.hash,
- LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp);
+ SSH_PUBLICKEY_HASH_SHA1, errp);
}
g_assert_not_reached();
break;
case SSH_HOST_KEY_CHECK_MODE_KNOWN_HOSTS:
- return check_host_key_knownhosts(s, host, port, errp);
+ return check_host_key_knownhosts(s, errp);
default:
g_assert_not_reached();
}
@@ -452,60 +467,42 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
return -EINVAL;
}
-static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
+static int authenticate(BDRVSSHState *s, Error **errp)
{
int r, ret;
- const char *userauthlist;
- LIBSSH2_AGENT *agent = NULL;
- struct libssh2_agent_publickey *identity;
- struct libssh2_agent_publickey *prev_identity = NULL;
+ int method;
- userauthlist = libssh2_userauth_list(s->session, user, strlen(user));
- if (strstr(userauthlist, "publickey") == NULL) {
+ /* Try to authenticate with the "none" method. */
+ r = ssh_userauth_none(s->session, NULL);
+ if (r == SSH_AUTH_ERROR) {
ret = -EPERM;
- error_setg(errp,
- "remote server does not support \"publickey\" authentication");
+ session_error_setg(errp, s, "failed to authenticate using none "
+ "authentication");
goto out;
- }
-
- /* Connect to ssh-agent and try each identity in turn. */
- agent = libssh2_agent_init(s->session);
- if (!agent) {
- ret = -EINVAL;
- session_error_setg(errp, s, "failed to initialize ssh-agent support");
- goto out;
- }
- if (libssh2_agent_connect(agent)) {
- ret = -ECONNREFUSED;
- session_error_setg(errp, s, "failed to connect to ssh-agent");
- goto out;
- }
- if (libssh2_agent_list_identities(agent)) {
- ret = -EINVAL;
- session_error_setg(errp, s,
- "failed requesting identities from ssh-agent");
+ } else if (r == SSH_AUTH_SUCCESS) {
+ /* Authenticated! */
+ ret = 0;
goto out;
}
- for(;;) {
- r = libssh2_agent_get_identity(agent, &identity, prev_identity);
- if (r == 1) { /* end of list */
- break;
- }
- if (r < 0) {
+ method = ssh_userauth_list(s->session, NULL);
+ trace_ssh_auth_methods(method);
+
+ /* Try to authenticate with publickey, using the ssh-agent
+ * if available.
+ */
+ if (method & SSH_AUTH_METHOD_PUBLICKEY) {
+ r = ssh_userauth_publickey_auto(s->session, NULL, NULL);
+ if (r == SSH_AUTH_ERROR) {
ret = -EINVAL;
- session_error_setg(errp, s,
- "failed to obtain identity from ssh-agent");
+ session_error_setg(errp, s, "failed to authenticate using "
+ "publickey authentication");
goto out;
- }
- r = libssh2_agent_userauth(agent, user, identity);
- if (r == 0) {
+ } else if (r == SSH_AUTH_SUCCESS) {
/* Authenticated! */
ret = 0;
goto out;
}
- /* Failed to authenticate with this identity, try the next one. */
- prev_identity = identity;
}
ret = -EPERM;
@@ -513,13 +510,6 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
"and the identities held by your ssh-agent");
out:
- if (agent != NULL) {
- /* Note: libssh2 implementation implicitly calls
- * libssh2_agent_disconnect if necessary.
- */
- libssh2_agent_free(agent);
- }
-
return ret;
}
@@ -638,7 +628,8 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
int ssh_flags, int creat_mode, Error **errp)
{
int r, ret;
- long port = 0;
+ unsigned int port = 0;
+ int new_sock = -1;
if (opts->has_user) {
s->user = g_strdup(opts->user);
@@ -655,71 +646,145 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
s->inet = opts->server;
opts->server = NULL;
- if (qemu_strtol(s->inet->port, NULL, 10, &port) < 0) {
+ if (qemu_strtoui(s->inet->port, NULL, 10, &port) < 0) {
error_setg(errp, "Use only numeric port value");
ret = -EINVAL;
goto err;
}
/* Open the socket and connect. */
- s->sock = inet_connect_saddr(s->inet, errp);
- if (s->sock < 0) {
+ new_sock = inet_connect_saddr(s->inet, errp);
+ if (new_sock < 0) {
ret = -EIO;
goto err;
}
+ /* Try to disable the Nagle algorithm on TCP sockets to reduce latency,
+ * but do not fail if it cannot be disabled.
+ */
+ r = socket_set_nodelay(new_sock);
+ if (r < 0) {
+ warn_report("setting NODELAY for the ssh server %s failed: %m",
+ s->inet->host);
+ }
+
/* Create SSH session. */
- s->session = libssh2_session_init();
+ s->session = ssh_new();
if (!s->session) {
ret = -EINVAL;
- session_error_setg(errp, s, "failed to initialize libssh2 session");
+ session_error_setg(errp, s, "failed to initialize libssh session");
goto err;
}
-#if TRACE_LIBSSH2 != 0
- libssh2_trace(s->session, TRACE_LIBSSH2);
-#endif
+ /* Make sure we are in blocking mode during the connection and
+ * authentication phases.
+ */
+ ssh_set_blocking(s->session, 1);
- r = libssh2_session_handshake(s->session, s->sock);
- if (r != 0) {
+ r = ssh_options_set(s->session, SSH_OPTIONS_USER, s->user);
+ if (r < 0) {
+ ret = -EINVAL;
+ session_error_setg(errp, s,
+ "failed to set the user in the libssh session");
+ goto err;
+ }
+
+ r = ssh_options_set(s->session, SSH_OPTIONS_HOST, s->inet->host);
+ if (r < 0) {
+ ret = -EINVAL;
+ session_error_setg(errp, s,
+ "failed to set the host in the libssh session");
+ goto err;
+ }
+
+ if (port > 0) {
+ r = ssh_options_set(s->session, SSH_OPTIONS_PORT, &port);
+ if (r < 0) {
+ ret = -EINVAL;
+ session_error_setg(errp, s,
+ "failed to set the port in the libssh session");
+ goto err;
+ }
+ }
+
+ r = ssh_options_set(s->session, SSH_OPTIONS_COMPRESSION, "none");
+ if (r < 0) {
+ ret = -EINVAL;
+ session_error_setg(errp, s,
+ "failed to disable the compression in the libssh "
+ "session");
+ goto err;
+ }
+
+ /* Read ~/.ssh/config. */
+ r = ssh_options_parse_config(s->session, NULL);
+ if (r < 0) {
+ ret = -EINVAL;
+ session_error_setg(errp, s, "failed to parse ~/.ssh/config");
+ goto err;
+ }
+
+ r = ssh_options_set(s->session, SSH_OPTIONS_FD, &new_sock);
+ if (r < 0) {
+ ret = -EINVAL;
+ session_error_setg(errp, s,
+ "failed to set the socket in the libssh session");
+ goto err;
+ }
+ /* libssh took ownership of the socket. */
+ s->sock = new_sock;
+ new_sock = -1;
+
+ /* Connect. */
+ r = ssh_connect(s->session);
+ if (r != SSH_OK) {
ret = -EINVAL;
session_error_setg(errp, s, "failed to establish SSH session");
goto err;
}
/* Check the remote host's key against known_hosts. */
- ret = check_host_key(s, s->inet->host, port, opts->host_key_check, errp);
+ ret = check_host_key(s, opts->host_key_check, errp);
if (ret < 0) {
goto err;
}
/* Authenticate. */
- ret = authenticate(s, s->user, errp);
+ ret = authenticate(s, errp);
if (ret < 0) {
goto err;
}
/* Start SFTP. */
- s->sftp = libssh2_sftp_init(s->session);
+ s->sftp = sftp_new(s->session);
if (!s->sftp) {
- session_error_setg(errp, s, "failed to initialize sftp handle");
+ session_error_setg(errp, s, "failed to create sftp handle");
+ ret = -EINVAL;
+ goto err;
+ }
+
+ r = sftp_init(s->sftp);
+ if (r < 0) {
+ sftp_error_setg(errp, s, "failed to initialize sftp handle");
ret = -EINVAL;
goto err;
}
/* Open the remote file. */
trace_ssh_connect_to_ssh(opts->path, ssh_flags, creat_mode);
- s->sftp_handle = libssh2_sftp_open(s->sftp, opts->path, ssh_flags,
- creat_mode);
+ s->sftp_handle = sftp_open(s->sftp, opts->path, ssh_flags, creat_mode);
if (!s->sftp_handle) {
- session_error_setg(errp, s, "failed to open remote file '%s'",
- opts->path);
+ sftp_error_setg(errp, s, "failed to open remote file '%s'",
+ opts->path);
ret = -EINVAL;
goto err;
}
- r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
- if (r < 0) {
+ /* Make sure the SFTP file is handled in blocking mode. */
+ sftp_file_set_blocking(s->sftp_handle);
+
+ s->attrs = sftp_fstat(s->sftp_handle);
+ if (!s->attrs) {
sftp_error_setg(errp, s, "failed to read file attributes");
return -EINVAL;
}
@@ -727,21 +792,26 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
return 0;
err:
+ if (s->attrs) {
+ sftp_attributes_free(s->attrs);
+ }
+ s->attrs = NULL;
if (s->sftp_handle) {
- libssh2_sftp_close(s->sftp_handle);
+ sftp_close(s->sftp_handle);
}
s->sftp_handle = NULL;
if (s->sftp) {
- libssh2_sftp_shutdown(s->sftp);
+ sftp_free(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);
+ ssh_disconnect(s->session);
+ ssh_free(s->session);
}
s->session = NULL;
+ if (new_sock >= 0) {
+ close(new_sock);
+ }
return ret;
}
@@ -756,9 +826,11 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
ssh_state_init(s);
- ssh_flags = LIBSSH2_FXF_READ;
+ ssh_flags = 0;
if (bdrv_flags & BDRV_O_RDWR) {
- ssh_flags |= LIBSSH2_FXF_WRITE;
+ ssh_flags |= O_RDWR;
+ } else {
+ ssh_flags |= O_RDONLY;
}
opts = ssh_parse_options(options, errp);
@@ -773,16 +845,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
}
/* Go non-blocking. */
- libssh2_session_set_blocking(s->session, 0);
+ ssh_set_blocking(s->session, 0);
qapi_free_BlockdevOptionsSsh(opts);
return 0;
err:
- if (s->sock >= 0) {
- close(s->sock);
- }
s->sock = -1;
qapi_free_BlockdevOptionsSsh(opts);
@@ -795,25 +864,25 @@ static int ssh_grow_file(BDRVSSHState *s, int64_t offset, Error **errp)
{
ssize_t ret;
char c[1] = { '\0' };
- int was_blocking = libssh2_session_get_blocking(s->session);
+ int was_blocking = ssh_is_blocking(s->session);
/* offset must be strictly greater than the current size so we do
* not overwrite anything */
- assert(offset > 0 && offset > s->attrs.filesize);
+ assert(offset > 0 && offset > s->attrs->size);
- libssh2_session_set_blocking(s->session, 1);
+ ssh_set_blocking(s->session, 1);
- libssh2_sftp_seek64(s->sftp_handle, offset - 1);
- ret = libssh2_sftp_write(s->sftp_handle, c, 1);
+ sftp_seek64(s->sftp_handle, offset - 1);
+ ret = sftp_write(s->sftp_handle, c, 1);
- libssh2_session_set_blocking(s->session, was_blocking);
+ ssh_set_blocking(s->session, was_blocking);
if (ret < 0) {
sftp_error_setg(errp, s, "Failed to grow file");
return -EIO;
}
- s->attrs.filesize = offset;
+ s->attrs->size = offset;
return 0;
}
@@ -841,8 +910,7 @@ static int ssh_co_create(BlockdevCreateOptions *options, Error **errp)
ssh_state_init(&s);
ret = connect_to_ssh(&s, opts->location,
- LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
- LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
+ O_RDWR | O_CREAT | O_TRUNC,
0644, errp);
if (ret < 0) {
goto fail;
@@ -911,10 +979,8 @@ static int ssh_has_zero_init(BlockDriverState *bs)
/* Assume false, unless we can positively prove it's true. */
int has_zero_init = 0;
- if (s->attrs.flags & LIBSSH2_SFTP_ATTR_PERMISSIONS) {
- if (s->attrs.permissions & LIBSSH2_SFTP_S_IFREG) {
- has_zero_init = 1;
- }
+ if (s->attrs->type == SSH_FILEXFER_TYPE_REGULAR) {
+ has_zero_init = 1;
}
return has_zero_init;
@@ -951,12 +1017,12 @@ static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
.co = qemu_coroutine_self()
};
- r = libssh2_session_block_directions(s->session);
+ r = ssh_get_poll_flags(s->session);
- if (r & LIBSSH2_SESSION_BLOCK_INBOUND) {
+ if (r & SSH_READ_PENDING) {
rd_handler = restart_coroutine;
}
- if (r & LIBSSH2_SESSION_BLOCK_OUTBOUND) {
+ if (r & SSH_WRITE_PENDING) {
wr_handler = restart_coroutine;
}
@@ -968,33 +1034,6 @@ static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
trace_ssh_co_yield_back(s->sock);
}
-/* SFTP has a function `libssh2_sftp_seek64' which seeks to a position
- * in the remote file. Notice that it just updates a field in the
- * sftp_handle structure, so there is no network traffic and it cannot
- * fail.
- *
- * However, `libssh2_sftp_seek64' does have a catastrophic effect on
- * performance since it causes the handle to throw away all in-flight
- * reads and buffered readahead data. Therefore this function tries
- * to be intelligent about when to call the underlying libssh2 function.
- */
-#define SSH_SEEK_WRITE 0
-#define SSH_SEEK_READ 1
-#define SSH_SEEK_FORCE 2
-
-static void ssh_seek(BDRVSSHState *s, int64_t offset, int flags)
-{
- bool op_read = (flags & SSH_SEEK_READ) != 0;
- bool force = (flags & SSH_SEEK_FORCE) != 0;
-
- if (force || op_read != s->offset_op_read || offset != s->offset) {
- trace_ssh_seek(offset);
- libssh2_sftp_seek64(s->sftp_handle, offset);
- s->offset = offset;
- s->offset_op_read = op_read;
- }
-}
-
static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
int64_t offset, size_t size,
QEMUIOVector *qiov)
@@ -1006,7 +1045,8 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
trace_ssh_read(offset, size);
- ssh_seek(s, offset, SSH_SEEK_READ);
+ trace_ssh_seek(offset);
+ sftp_seek64(s->sftp_handle, offset);
/* This keeps track of the current iovec element ('i'), where we
* will write to next ('buf'), and the end of the current iovec
@@ -1016,35 +1056,33 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
buf = i->iov_base;
end_of_vec = i->iov_base + i->iov_len;
- /* 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:
- trace_ssh_read_buf(buf, end_of_vec - buf);
- r = libssh2_sftp_read(s->sftp_handle, buf, end_of_vec - buf);
- trace_ssh_read_return(r);
+ trace_ssh_read_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 16384));
+ /* The size of SFTP packets is limited to 32K bytes, so limit
+ * the amount of data requested to 16K, as libssh currently
+ * does not handle multiple requests on its own:
+ * https://red.libssh.org/issues/58
+ */
+ r = sftp_read(s->sftp_handle, buf, MIN(end_of_vec - buf, 16384));
+ trace_ssh_read_return(r, sftp_get_error(s->sftp));
- if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
+ if (r == SSH_AGAIN) {
co_yield(s, bs);
goto again;
}
- if (r < 0) {
- sftp_error_trace(s, "read");
- s->offset = -1;
- return -EIO;
- }
- if (r == 0) {
+ if (r == SSH_EOF || (r == 0 && sftp_get_error(s->sftp) == SSH_FX_EOF)) {
/* EOF: Short read so pad the buffer with zeroes and return it. */
qemu_iovec_memset(qiov, got, 0, size - got);
return 0;
}
+ if (r <= 0) {
+ sftp_error_trace(s, "read");
+ return -EIO;
+ }
got += r;
buf += r;
- s->offset += r;
if (buf >= end_of_vec && got < size) {
i++;
buf = i->iov_base;
@@ -1081,7 +1119,8 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
trace_ssh_write(offset, size);
- ssh_seek(s, offset, SSH_SEEK_WRITE);
+ trace_ssh_seek(offset);
+ sftp_seek64(s->sftp_handle, offset);
/* This keeps track of the current iovec element ('i'), where we
* will read from next ('buf'), and the end of the current iovec
@@ -1093,45 +1132,34 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
for (written = 0; written < size; ) {
again:
- trace_ssh_write_buf(buf, end_of_vec - buf);
- r = libssh2_sftp_write(s->sftp_handle, buf, end_of_vec - buf);
- trace_ssh_write_return(r);
+ trace_ssh_write_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 131072));
+ /* Avoid too large data packets, as libssh currently does not
+ * handle multiple requests on its own:
+ * https://red.libssh.org/issues/58
+ */
+ r = sftp_write(s->sftp_handle, buf, MIN(end_of_vec - buf, 131072));
+ trace_ssh_write_return(r, sftp_get_error(s->sftp));
- if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
+ if (r == SSH_AGAIN) {
co_yield(s, bs);
goto again;
}
if (r < 0) {
sftp_error_trace(s, "write");
- s->offset = -1;
return -EIO;
}
- /* The libssh2 API is very unclear about this. A comment in
- * the code says "nothing was acked, and no EAGAIN was
- * received!" which apparently means that no data got sent
- * out, and the underlying channel didn't return any EAGAIN
- * indication. I think this is a bug in either libssh2 or
- * OpenSSH (server-side). In any case, forcing a seek (to
- * discard libssh2 internal buffers), and then trying again
- * works for me.
- */
- if (r == 0) {
- ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
- co_yield(s, bs);
- goto again;
- }
written += r;
buf += r;
- s->offset += r;
if (buf >= end_of_vec && written < size) {
i++;
buf = i->iov_base;
end_of_vec = i->iov_base + i->iov_len;
}
- if (offset + written > s->attrs.filesize)
- s->attrs.filesize = offset + written;
+ if (offset + written > s->attrs->size) {
+ s->attrs->size = offset + written;
+ }
}
return 0;
@@ -1166,24 +1194,24 @@ static void unsafe_flush_warning(BDRVSSHState *s, const char *what)
}
}
-#ifdef HAS_LIBSSH2_SFTP_FSYNC
+#if HAVE_LIBSSH_0_8
static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
{
int r;
trace_ssh_flush();
+
+ if (!sftp_extension_supported(s->sftp, "fsync@openssh.com", "1")) {
+ unsafe_flush_warning(s, "OpenSSH >= 6.3");
+ return 0;
+ }
again:
- r = libssh2_sftp_fsync(s->sftp_handle);
- if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
+ r = sftp_fsync(s->sftp_handle);
+ if (r == SSH_AGAIN) {
co_yield(s, bs);
goto again;
}
- if (r == LIBSSH2_ERROR_SFTP_PROTOCOL &&
- libssh2_sftp_last_error(s->sftp) == LIBSSH2_FX_OP_UNSUPPORTED) {
- unsafe_flush_warning(s, "OpenSSH >= 6.3");
- return 0;
- }
if (r < 0) {
sftp_error_trace(s, "fsync");
return -EIO;
@@ -1204,25 +1232,25 @@ static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
return ret;
}
-#else /* !HAS_LIBSSH2_SFTP_FSYNC */
+#else /* !HAVE_LIBSSH_0_8 */
static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
{
BDRVSSHState *s = bs->opaque;
- unsafe_flush_warning(s, "libssh2 >= 1.4.4");
+ unsafe_flush_warning(s, "libssh >= 0.8.0");
return 0;
}
-#endif /* !HAS_LIBSSH2_SFTP_FSYNC */
+#endif /* !HAVE_LIBSSH_0_8 */
static int64_t ssh_getlength(BlockDriverState *bs)
{
BDRVSSHState *s = bs->opaque;
int64_t length;
- /* Note we cannot make a libssh2 call here. */
- length = (int64_t) s->attrs.filesize;
+ /* Note we cannot make a libssh call here. */
+ length = (int64_t) s->attrs->size;
trace_ssh_getlength(length);
return length;
@@ -1239,12 +1267,12 @@ static int coroutine_fn ssh_co_truncate(BlockDriverState *bs, int64_t offset,
return -ENOTSUP;
}
- if (offset < s->attrs.filesize) {
+ if (offset < s->attrs->size) {
error_setg(errp, "ssh driver does not support shrinking files");
return -ENOTSUP;
}
- if (offset == s->attrs.filesize) {
+ if (offset == s->attrs->size) {
return 0;
}
@@ -1339,12 +1367,16 @@ static void bdrv_ssh_init(void)
{
int r;
- r = libssh2_init(0);
+ r = ssh_init();
if (r != 0) {
- fprintf(stderr, "libssh2 initialization failed, %d\n", r);
+ fprintf(stderr, "libssh initialization failed, %d\n", r);
exit(EXIT_FAILURE);
}
+#if TRACE_LIBSSH != 0
+ ssh_set_log_level(TRACE_LIBSSH);
+#endif
+
bdrv_register(&bdrv_ssh);
}
diff --git a/block/trace-events b/block/trace-events
index eab51497fc..0d2126e840 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -169,19 +169,21 @@ nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags
# ssh.c
ssh_restart_coroutine(void *co) "co=%p"
ssh_flush(void) "fsync"
-ssh_check_host_key_knownhosts(const char *key) "host key OK: %s"
+ssh_check_host_key_knownhosts(void) "host key OK"
ssh_connect_to_ssh(char *path, int flags, int mode) "opening file %s flags=0x%x creat_mode=0%o"
ssh_co_yield(int sock, void *rd_handler, void *wr_handler) "s->sock=%d rd_handler=%p wr_handler=%p"
ssh_co_yield_back(int sock) "s->sock=%d - back"
ssh_getlength(int64_t length) "length=%" PRIi64
ssh_co_create_opts(uint64_t size) "total_size=%" PRIu64
ssh_read(int64_t offset, size_t size) "offset=%" PRIi64 " size=%zu"
-ssh_read_buf(void *buf, size_t size) "sftp_read buf=%p size=%zu"
-ssh_read_return(ssize_t ret) "sftp_read returned %zd"
+ssh_read_buf(void *buf, size_t size, size_t actual_size) "sftp_read buf=%p size=%zu (actual size=%zu)"
+ssh_read_return(ssize_t ret, int sftp_err) "sftp_read returned %zd (sftp error=%d)"
ssh_write(int64_t offset, size_t size) "offset=%" PRIi64 " size=%zu"
-ssh_write_buf(void *buf, size_t size) "sftp_write buf=%p size=%zu"
-ssh_write_return(ssize_t ret) "sftp_write returned %zd"
+ssh_write_buf(void *buf, size_t size, size_t actual_size) "sftp_write buf=%p size=%zu (actual size=%zu)"
+ssh_write_return(ssize_t ret, int sftp_err) "sftp_write returned %zd (sftp error=%d)"
ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
+ssh_auth_methods(int methods) "auth methods=%x"
+ssh_server_status(int status) "server status=%d"
# curl.c
curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
@@ -214,4 +216,4 @@ sheepdog_snapshot_create(const char *sn_name, const char *id) "%s %s"
sheepdog_snapshot_create_inode(const char *name, uint32_t snap, uint32_t vdi) "s->inode: name %s snap_id 0x%" PRIx32 " vdi 0x%" PRIx32
# ssh.c
-sftp_error(const char *op, const char *ssh_err, int ssh_err_code, unsigned long sftp_err_code) "%s failed: %s (libssh2 error code: %d, sftp error code: %lu)"
+sftp_error(const char *op, const char *ssh_err, int ssh_err_code, int sftp_err_code) "%s failed: %s (libssh error code: %d, sftp error code: %d)"
diff --git a/configure b/configure
index b091b82cb3..bfdd70c40a 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"
+ --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,17 @@ 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
- fi
-fi
-
-##########################################
-# libssh2_sftp_fsync probe
-
-if test "$libssh2" = "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;
-}
-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"
+ libssh=no
fi
fi
@@ -6451,7 +6425,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 +7118,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
diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
index ec9823793a..1239d9d648 100644
--- a/tests/qemu-iotests/207.out
+++ b/tests/qemu-iotests/207.out
@@ -68,7 +68,7 @@ virtual size: 4 MiB (4194304 bytes)
{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"mode": "none"}, "path": "/this/is/not/an/existing/path", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 4194304}}}
{"return": {}}
-Job failed: failed to open remote file '/this/is/not/an/existing/path': Failed opening remote file (libssh2 error code: -31)
+Job failed: failed to open remote file '/this/is/not/an/existing/path': SFTP server: No such file (libssh error code: 1, sftp error code: 2)
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
--
2.21.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-05 21:36 [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh Pino Toscano
@ 2019-06-05 22:57 ` no-reply
2019-06-06 11:12 ` Daniel P. Berrangé
2019-06-12 13:27 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2019-06-05 22:57 UTC (permalink / raw)
To: ptoscano; +Cc: kwolf, qemu-block, rjones, qemu-devel, mreitz, ptoscano, philmd
Patchew URL: https://patchew.org/QEMU/20190605213654.9785-1-ptoscano@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190605213654.9785-1-ptoscano@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
abef817 ssh: switch from libssh2 to libssh
=== OUTPUT BEGIN ===
WARNING: Block comments use a leading /* on a separate line
#68: FILE: block/ssh.c:46:
+/* TRACE_LIBSSH=<level> enables tracing in libssh itself.
WARNING: Block comments use a leading /* on a separate line
#95: FILE: block/ssh.c:64:
+ /* File attributes at open. We try to keep the .size field
WARNING: Block comments use a leading /* on a separate line
#484: FILE: block/ssh.c:491:
+ /* Try to authenticate with publickey, using the ssh-agent
WARNING: Block comments use a leading /* on a separate line
#553: FILE: block/ssh.c:662:
+ /* Try to disable the Nagle algorithm on TCP sockets to reduce latency,
WARNING: Block comments use a leading /* on a separate line
#575: FILE: block/ssh.c:679:
+ /* Make sure we are in blocking mode during the connection and
WARNING: Block comments use a leading /* on a separate line
#895: FILE: block/ssh.c:1062:
+ /* The size of SFTP packets is limited to 32K bytes, so limit
WARNING: line over 80 characters
#947: FILE: block/ssh.c:1135:
+ trace_ssh_write_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 131072));
WARNING: Block comments use a leading /* on a separate line
#948: FILE: block/ssh.c:1136:
+ /* Avoid too large data packets, as libssh currently does not
ERROR: Hex numbers must be prefixed with '0x'
#1122: FILE: block/trace-events:185:
+ssh_auth_methods(int methods) "auth methods=%x"
total: 1 errors, 8 warnings, 1175 lines checked
Commit abef817e4e8f (ssh: switch from libssh2 to libssh) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190605213654.9785-1-ptoscano@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-05 21:36 [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh Pino Toscano
2019-06-05 22:57 ` no-reply
@ 2019-06-06 11:12 ` Daniel P. Berrangé
2019-06-06 17:51 ` Pino Toscano
2019-06-12 13:27 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 15+ messages in thread
From: Daniel P. Berrangé @ 2019-06-06 11:12 UTC (permalink / raw)
To: Pino Toscano; +Cc: kwolf, qemu-block, rjones, qemu-devel, mreitz, philmd
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 <ptoscano@redhat.com>
> ---
>
> 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.
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.
At some future date when support platforms changes, we'll then be
able to tweak it to 0.8.0 and drop the conditional compilation.
> -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
> - fi
> -fi
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-06 11:12 ` Daniel P. Berrangé
@ 2019-06-06 17:51 ` Pino Toscano
2019-06-07 10:08 ` Daniel P. Berrangé
0 siblings, 1 reply; 15+ messages in thread
From: Pino Toscano @ 2019-06-06 17:51 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: kwolf, qemu-block, rjones, qemu-devel, mreitz, philmd
[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]
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 <ptoscano@redhat.com>
> > ---
> >
> > 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.
--
Pino Toscano
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-06 17:51 ` Pino Toscano
@ 2019-06-07 10:08 ` Daniel P. Berrangé
2019-06-07 10:14 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: Daniel P. Berrangé @ 2019-06-07 10:08 UTC (permalink / raw)
To: Pino Toscano; +Cc: kwolf, qemu-block, rjones, qemu-devel, mreitz, philmd
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 <ptoscano@redhat.com>
> > > ---
> > >
> > > 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.
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.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-07 10:08 ` Daniel P. Berrangé
@ 2019-06-07 10:14 ` Philippe Mathieu-Daudé
2019-06-07 10:24 ` Daniel P. Berrangé
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-07 10:14 UTC (permalink / raw)
To: Daniel P. Berrangé, Pino Toscano
Cc: kwolf, mreitz, qemu-devel, qemu-block, rjones
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 <ptoscano@redhat.com>
>>>> ---
>>>>
>>>> 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 :)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-07 10:14 ` Philippe Mathieu-Daudé
@ 2019-06-07 10:24 ` Daniel P. Berrangé
0 siblings, 0 replies; 15+ messages in thread
From: Daniel P. Berrangé @ 2019-06-07 10:24 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: kwolf, qemu-block, qemu-devel, rjones, mreitz, Pino Toscano
On Fri, Jun 07, 2019 at 12:14:37PM +0200, Philippe Mathieu-Daudé wrote:
> 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 <ptoscano@redhat.com>
> >>>> ---
> >>>>
> >>>> 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.
I missed that Ubuntu, unusually, had older versions than RHEL.
We don't use Trusty though - our Travis config requests Xenial
these days. So 0.6.3 is sufficient as a min version
> > 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 :)
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-05 21:36 [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh Pino Toscano
2019-06-05 22:57 ` no-reply
2019-06-06 11:12 ` Daniel P. Berrangé
@ 2019-06-12 13:27 ` Philippe Mathieu-Daudé
2019-06-13 13:02 ` Alex Bennée
2019-06-13 19:41 ` Stefan Weil
2 siblings, 2 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-12 13:27 UTC (permalink / raw)
To: Pino Toscano, qemu-devel, qemu-block
Cc: kwolf, Daniel P. Berrange, Stefan Weil, rjones, mreitz, Alex Bennée
Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
On 6/5/19 11:36 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).
>
> Signed-off-by: Pino Toscano <ptoscano@redhat.com>
> ---
>
> 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/block/Makefile.objs b/block/Makefile.objs
> index ae11605c9f..bf01429dd5 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
> block-obj-$(CONFIG_RBD) += rbd.o
> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> block-obj-$(CONFIG_VXHS) += vxhs.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
> @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
> gluster.o-cflags := $(GLUSTERFS_CFLAGS)
> gluster.o-libs := $(GLUSTERFS_LIBS)
> vxhs.o-libs := $(VXHS_LIBS)
> -ssh.o-cflags := $(LIBSSH2_CFLAGS)
> -ssh.o-libs := $(LIBSSH2_LIBS)
> +ssh.o-cflags := $(LIBSSH_CFLAGS)
> +ssh.o-libs := $(LIBSSH_LIBS)
> block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
> block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
> dmg-bz2.o-libs := $(BZIP2_LIBS)
> diff --git a/block/ssh.c b/block/ssh.c
> index 12fd4f39e8..ce2363a471 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -24,8 +24,8 @@
>
> #include "qemu/osdep.h"
>
> -#include <libssh2.h>
> -#include <libssh2_sftp.h>
> +#include <libssh/libssh.h>
> +#include <libssh/sftp.h>
>
> #include "block/block_int.h"
> #include "block/qdict.h"
> @@ -43,14 +43,13 @@
> #include "qapi/qobject-output-visitor.h"
> #include "trace.h"
>
> -/*
> - * TRACE_LIBSSH2=<bitmask> 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 <bitmask> is described
> - * here: http://www.libssh2.org/libssh2_trace.html
> +/* TRACE_LIBSSH=<level> enables tracing in libssh itself.
> + * The meaning of <level> is described here:
> + * http://api.libssh.org/master/group__libssh__log.html
> */
> -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
> +#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
> +
> +#define HAVE_LIBSSH_0_8 (LIBSSH_VERSION_INT >= SSH_VERSION_INT(0, 8, 0))
As I noticed with ssh_get_publickey() and reading
https://www.redhat.com/archives/libvir-list/2018-May/msg00597.html, I'm
not convinced this definition is accurate. Used in [1].
>
> typedef struct BDRVSSHState {
> /* Coroutine. */
> @@ -58,18 +57,14 @@ typedef struct BDRVSSHState {
>
> /* 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;
>
> InetSocketAddress *inet;
>
> @@ -89,7 +84,6 @@ static void ssh_state_init(BDRVSSHState *s)
> {
> memset(s, 0, sizeof *s);
> s->sock = -1;
> - s->offset = -1;
> qemu_co_mutex_init(&s->lock);
> }
>
> @@ -97,21 +91,20 @@ static void ssh_state_free(BDRVSSHState *s)
> {
> g_free(s->user);
>
> + 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);
Maybe:
ssh_free(s->session); /* This frees s->sock */
> }
> + /* s->sock is owned by the ssh_session, which frees it. */
And drop this comment.
> }
>
> static void GCC_FMT_ATTR(3, 4)
> @@ -125,13 +118,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;
You can drop these,
>
> - /* This is not an errno. See <libssh2.h>. */
> - ssh_err_code = libssh2_session_last_error(s->session,
> - &ssh_err, NULL, 0);
> - error_setg(errp, "%s: %s (libssh2 error code: %d)",
> + /* This is not an errno. See <libssh/libssh.h>. */
> + ssh_err = ssh_get_error(s->session);
> + ssh_err_code = ssh_get_error_code(s->session);
> + error_setg(errp, "%s: %s (libssh error code: %d)",
using directly as arguments here. However this might be easier for the
libssh2/libssh switch to do as you did, and clean this later.
> msg, ssh_err, ssh_err_code);
> } else {
> error_setg(errp, "%s", msg);
> @@ -150,18 +143,18 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
> va_end(args);
>
> if (s->sftp) {
> - char *ssh_err;
> + const char *ssh_err;
> int ssh_err_code;
> - unsigned long sftp_err_code;
> + int sftp_err_code;
>
> - /* This is not an errno. See <libssh2.h>. */
> - ssh_err_code = libssh2_session_last_error(s->session,
> - &ssh_err, NULL, 0);
> - /* See <libssh2_sftp.h>. */
> - sftp_err_code = libssh2_sftp_last_error((s)->sftp);
> + /* This is not an errno. See <libssh/libssh.h>. */
> + ssh_err = ssh_get_error(s->session);
> + ssh_err_code = ssh_get_error_code(s->session);
> + /* See <libssh/sftp.h>. */
> + sftp_err_code = sftp_get_error(s->sftp);
>
> error_setg(errp,
> - "%s: %s (libssh2 error code: %d, sftp error code: %lu)",
> + "%s: %s (libssh error code: %d, sftp error code: %d)",
> msg, ssh_err, ssh_err_code, sftp_err_code);
Ditto, etc.
> } else {
> error_setg(errp, "%s", msg);
> @@ -171,15 +164,15 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
>
> static void sftp_error_trace(BDRVSSHState *s, const char *op)
> {
> - char *ssh_err;
> + const char *ssh_err;
> int ssh_err_code;
> - unsigned long sftp_err_code;
> + int sftp_err_code;
>
> - /* This is not an errno. See <libssh2.h>. */
> - ssh_err_code = libssh2_session_last_error(s->session,
> - &ssh_err, NULL, 0);
> - /* See <libssh2_sftp.h>. */
> - sftp_err_code = libssh2_sftp_last_error((s)->sftp);
> + /* This is not an errno. See <libssh/libssh.h>. */
> + ssh_err = ssh_get_error(s->session);
> + ssh_err_code = ssh_get_error_code(s->session);
> + /* See <libssh/sftp.h>. */
> + sftp_err_code = sftp_get_error(s->sftp);
>
> trace_sftp_error(op, ssh_err, ssh_err_code, sftp_err_code);
> }
> @@ -280,82 +273,89 @@ static void ssh_parse_filename(const char *filename, QDict *options,
> parse_uri(filename, options, errp);
> }
>
> -static int check_host_key_knownhosts(BDRVSSHState *s,
> - const char *host, int port, Error **errp)
> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
> {
> - const char *home;
> - char *knh_file = NULL;
> - LIBSSH2_KNOWNHOSTS *knh = NULL;
> - struct libssh2_knownhost *found;
> - int ret, r;
> - const char *hostkey;
> - size_t len;
> - int type;
> + int ret;
> +#if HAVE_LIBSSH_0_8
[1] See previous comment about HAVE_LIBSSH_0_8.
> + enum ssh_known_hosts_e state;
>
> - hostkey = libssh2_session_hostkey(s->session, &len, &type);
> - if (!hostkey) {
> + state = ssh_session_is_known_server(s->session);
> + trace_ssh_server_status(state);
> +
> + switch (state) {
> + case SSH_KNOWN_HOSTS_OK:
> + /* OK */
> + trace_ssh_check_host_key_knownhosts();
> + break;
> + case SSH_KNOWN_HOSTS_CHANGED:
> ret = -EINVAL;
> - session_error_setg(errp, s, "failed to read remote host key");
> + error_setg(errp, "host key does not match the one in known_hosts");
> goto out;
> - }
> -
> - knh = libssh2_knownhost_init(s->session);
> - if (!knh) {
> + case SSH_KNOWN_HOSTS_OTHER:
> ret = -EINVAL;
> - session_error_setg(errp, s,
> - "failed to initialize known hosts support");
> + error_setg(errp,
> + "host key for this server not found, another type exists");
> + goto out;
> + case SSH_KNOWN_HOSTS_UNKNOWN:
> + ret = -ENOENT;
> + error_setg(errp, "no host key was found in known_hosts");
> + goto out;
> + case SSH_KNOWN_HOSTS_NOT_FOUND:
> + ret = -EINVAL;
> + error_setg(errp, "known_hosts file not found");
> + goto out;
> + case SSH_KNOWN_HOSTS_ERROR:
> + ret = -EINVAL;
> + error_setg(errp, "error while checking the host");
> + goto out;
> + default:
I'd rather not use a default case, so if libssh get more
ssh_known_hosts_e, we get a build failure and add the new cases.
> + ret = -EINVAL;
> + error_setg(errp, "error while checking for known server");
> goto out;
> }
> +#else /* !HAVE_LIBSSH_0_8 */
> + int state;
>
> - home = getenv("HOME");
> - if (home) {
> - knh_file = g_strdup_printf("%s/.ssh/known_hosts", home);
> - } else {
> - knh_file = g_strdup_printf("/root/.ssh/known_hosts");
> - }
> -
> - /* Read all known hosts from OpenSSH-style known_hosts file. */
> - libssh2_knownhost_readfile(knh, knh_file, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
> + state = ssh_is_server_known(s->session);
> + trace_ssh_server_status(state);
>
> - r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
> - LIBSSH2_KNOWNHOST_TYPE_PLAIN|
> - LIBSSH2_KNOWNHOST_KEYENC_RAW,
> - &found);
> - switch (r) {
> - case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> + switch (state) {
> + case SSH_SERVER_KNOWN_OK:
> /* OK */
> - trace_ssh_check_host_key_knownhosts(found->key);
> + trace_ssh_check_host_key_knownhosts();
> break;
> - case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> + case SSH_SERVER_KNOWN_CHANGED:
> + ret = -EINVAL;
> + error_setg(errp, "host key does not match the one in known_hosts");
> + goto out;
> + case SSH_SERVER_FOUND_OTHER:
> ret = -EINVAL;
> - session_error_setg(errp, s,
> - "host key does not match the one in known_hosts"
> - " (found key %s)", found->key);
> + error_setg(errp,
> + "host key for this server not found, another type exists");
> + goto out;
> + case SSH_SERVER_FILE_NOT_FOUND:
> + ret = -ENOENT;
> + error_setg(errp, "known_hosts file not found");
> goto out;
> - case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
> + case SSH_SERVER_NOT_KNOWN:
> ret = -EINVAL;
> - session_error_setg(errp, s, "no host key was found in known_hosts");
> + error_setg(errp, "no host key was found in known_hosts");
> goto out;
> - case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
> + case SSH_SERVER_ERROR:
> ret = -EINVAL;
> - session_error_setg(errp, s,
> - "failure matching the host key with known_hosts");
> + error_setg(errp, "server error");
> goto out;
> default:
> ret = -EINVAL;
> - session_error_setg(errp, s, "unknown error matching the host key"
> - " with known_hosts (%d)", r);
> + error_setg(errp, "error while checking for known server");
> goto out;
Here the default is dead code, I'd remove it too.
> }
> +#endif /* !HAVE_LIBSSH_0_8 */
>
> /* known_hosts checking successful. */
> ret = 0;
>
> out:
> - if (knh != NULL) {
> - libssh2_knownhost_free(knh);
> - }
> - g_free(knh_file);
> return ret;
> }
>
> @@ -399,18 +399,34 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
>
> static int
> check_host_key_hash(BDRVSSHState *s, const char *hash,
> - int hash_type, size_t fingerprint_len, Error **errp)
> + enum ssh_publickey_hash_type type, Error **errp)
> {
> - const char *fingerprint;
> + int r;
> + ssh_key pubkey;
> + unsigned char *server_hash;
> + size_t server_hash_len;
>
> - fingerprint = libssh2_hostkey_hash(s->session, hash_type);
> - if (!fingerprint) {
> +#if HAVE_LIBSSH_0_8
> + r = ssh_get_server_publickey(s->session, &pubkey);
> +#else
> + r = ssh_get_publickey(s->session, &pubkey);
> +#endif
> + if (r != SSH_OK) {
> session_error_setg(errp, s, "failed to read remote host key");
> return -EINVAL;
> }
>
> - if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
> - hash) != 0) {
> + r = ssh_get_publickey_hash(pubkey, type, &server_hash, &server_hash_len);
> + ssh_key_free(pubkey);
> + if (r != 0) {
> + session_error_setg(errp, s,
> + "failed reading the hash of the server SSH key");
> + return -EINVAL;
> + }
> +
> + r = compare_fingerprint(server_hash, server_hash_len, hash);
> + ssh_clean_pubkey_hash(&server_hash);
> + if (r != 0) {
> error_setg(errp, "remote host key does not match host_key_check '%s'",
> hash);
> return -EPERM;
> @@ -419,8 +435,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
> return 0;
> }
>
> -static int check_host_key(BDRVSSHState *s, const char *host, int port,
> - SshHostKeyCheck *hkc, Error **errp)
> +static int check_host_key(BDRVSSHState *s, SshHostKeyCheck *hkc, Error **errp)
> {
> SshHostKeyCheckMode mode;
>
> @@ -436,15 +451,15 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
> case SSH_HOST_KEY_CHECK_MODE_HASH:
> if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_MD5) {
> return check_host_key_hash(s, hkc->u.hash.hash,
> - LIBSSH2_HOSTKEY_HASH_MD5, 16, errp);
> + SSH_PUBLICKEY_HASH_MD5, errp);
> } else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA1) {
> return check_host_key_hash(s, hkc->u.hash.hash,
> - LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp);
> + SSH_PUBLICKEY_HASH_SHA1, errp);
> }
> g_assert_not_reached();
At least Fedora 29 uses SSH_PUBLICKEY_HASH_SHA256.
> break;
> case SSH_HOST_KEY_CHECK_MODE_KNOWN_HOSTS:
> - return check_host_key_knownhosts(s, host, port, errp);
> + return check_host_key_knownhosts(s, errp);
> default:
> g_assert_not_reached();
> }
> @@ -452,60 +467,42 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
> return -EINVAL;
> }
>
> -static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
> +static int authenticate(BDRVSSHState *s, Error **errp)
> {
> int r, ret;
> - const char *userauthlist;
> - LIBSSH2_AGENT *agent = NULL;
> - struct libssh2_agent_publickey *identity;
> - struct libssh2_agent_publickey *prev_identity = NULL;
> + int method;
>
> - userauthlist = libssh2_userauth_list(s->session, user, strlen(user));
> - if (strstr(userauthlist, "publickey") == NULL) {
> + /* Try to authenticate with the "none" method. */
> + r = ssh_userauth_none(s->session, NULL);
> + if (r == SSH_AUTH_ERROR) {
> ret = -EPERM;
> - error_setg(errp,
> - "remote server does not support \"publickey\" authentication");
> + session_error_setg(errp, s, "failed to authenticate using none "
> + "authentication");
> goto out;
> - }
> -
> - /* Connect to ssh-agent and try each identity in turn. */
> - agent = libssh2_agent_init(s->session);
> - if (!agent) {
> - ret = -EINVAL;
> - session_error_setg(errp, s, "failed to initialize ssh-agent support");
> - goto out;
> - }
> - if (libssh2_agent_connect(agent)) {
> - ret = -ECONNREFUSED;
> - session_error_setg(errp, s, "failed to connect to ssh-agent");
> - goto out;
> - }
> - if (libssh2_agent_list_identities(agent)) {
> - ret = -EINVAL;
> - session_error_setg(errp, s,
> - "failed requesting identities from ssh-agent");
> + } else if (r == SSH_AUTH_SUCCESS) {
> + /* Authenticated! */
> + ret = 0;
> goto out;
> }
>
> - for(;;) {
> - r = libssh2_agent_get_identity(agent, &identity, prev_identity);
> - if (r == 1) { /* end of list */
> - break;
> - }
> - if (r < 0) {
> + method = ssh_userauth_list(s->session, NULL);
> + trace_ssh_auth_methods(method);
> +
> + /* Try to authenticate with publickey, using the ssh-agent
> + * if available.
> + */
> + if (method & SSH_AUTH_METHOD_PUBLICKEY) {
> + r = ssh_userauth_publickey_auto(s->session, NULL, NULL);
> + if (r == SSH_AUTH_ERROR) {
> ret = -EINVAL;
> - session_error_setg(errp, s,
> - "failed to obtain identity from ssh-agent");
> + session_error_setg(errp, s, "failed to authenticate using "
> + "publickey authentication");
> goto out;
> - }
> - r = libssh2_agent_userauth(agent, user, identity);
> - if (r == 0) {
> + } else if (r == SSH_AUTH_SUCCESS) {
> /* Authenticated! */
> ret = 0;
> goto out;
> }
> - /* Failed to authenticate with this identity, try the next one. */
> - prev_identity = identity;
> }
>
> ret = -EPERM;
> @@ -513,13 +510,6 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
> "and the identities held by your ssh-agent");
>
> out:
> - if (agent != NULL) {
> - /* Note: libssh2 implementation implicitly calls
> - * libssh2_agent_disconnect if necessary.
> - */
> - libssh2_agent_free(agent);
> - }
> -
> return ret;
> }
>
> @@ -638,7 +628,8 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
> int ssh_flags, int creat_mode, Error **errp)
> {
> int r, ret;
> - long port = 0;
> + unsigned int port = 0;
> + int new_sock = -1;
>
> if (opts->has_user) {
> s->user = g_strdup(opts->user);
> @@ -655,71 +646,145 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
> s->inet = opts->server;
> opts->server = NULL;
>
> - if (qemu_strtol(s->inet->port, NULL, 10, &port) < 0) {
> + if (qemu_strtoui(s->inet->port, NULL, 10, &port) < 0) {
> error_setg(errp, "Use only numeric port value");
> ret = -EINVAL;
> goto err;
> }
>
> /* Open the socket and connect. */
> - s->sock = inet_connect_saddr(s->inet, errp);
> - if (s->sock < 0) {
> + new_sock = inet_connect_saddr(s->inet, errp);
> + if (new_sock < 0) {
> ret = -EIO;
> goto err;
> }
>
> + /* Try to disable the Nagle algorithm on TCP sockets to reduce latency,
> + * but do not fail if it cannot be disabled.
> + */
> + r = socket_set_nodelay(new_sock);
> + if (r < 0) {
> + warn_report("setting NODELAY for the ssh server %s failed: %m",
> + s->inet->host);
> + }
> +
> /* Create SSH session. */
> - s->session = libssh2_session_init();
> + s->session = ssh_new();
> if (!s->session) {
> ret = -EINVAL;
> - session_error_setg(errp, s, "failed to initialize libssh2 session");
> + session_error_setg(errp, s, "failed to initialize libssh session");
> goto err;
> }
>
> -#if TRACE_LIBSSH2 != 0
> - libssh2_trace(s->session, TRACE_LIBSSH2);
> -#endif
> + /* Make sure we are in blocking mode during the connection and
> + * authentication phases.
> + */
> + ssh_set_blocking(s->session, 1);
>
> - r = libssh2_session_handshake(s->session, s->sock);
> - if (r != 0) {
> + r = ssh_options_set(s->session, SSH_OPTIONS_USER, s->user);
> + if (r < 0) {
> + ret = -EINVAL;
> + session_error_setg(errp, s,
> + "failed to set the user in the libssh session");
> + goto err;
> + }
> +
> + r = ssh_options_set(s->session, SSH_OPTIONS_HOST, s->inet->host);
> + if (r < 0) {
> + ret = -EINVAL;
> + session_error_setg(errp, s,
> + "failed to set the host in the libssh session");
> + goto err;
> + }
> +
> + if (port > 0) {
> + r = ssh_options_set(s->session, SSH_OPTIONS_PORT, &port);
> + if (r < 0) {
> + ret = -EINVAL;
> + session_error_setg(errp, s,
> + "failed to set the port in the libssh session");
> + goto err;
> + }
> + }
> +
> + r = ssh_options_set(s->session, SSH_OPTIONS_COMPRESSION, "none");
> + if (r < 0) {
> + ret = -EINVAL;
> + session_error_setg(errp, s,
> + "failed to disable the compression in the libssh "
> + "session");
> + goto err;
> + }
> +
> + /* Read ~/.ssh/config. */
> + r = ssh_options_parse_config(s->session, NULL);
> + if (r < 0) {
> + ret = -EINVAL;
> + session_error_setg(errp, s, "failed to parse ~/.ssh/config");
> + goto err;
> + }
> +
> + r = ssh_options_set(s->session, SSH_OPTIONS_FD, &new_sock);
> + if (r < 0) {
> + ret = -EINVAL;
> + session_error_setg(errp, s,
> + "failed to set the socket in the libssh session");
> + goto err;
> + }
> + /* libssh took ownership of the socket. */
> + s->sock = new_sock;
> + new_sock = -1;
> +
> + /* Connect. */
> + r = ssh_connect(s->session);
> + if (r != SSH_OK) {
> ret = -EINVAL;
> session_error_setg(errp, s, "failed to establish SSH session");
> goto err;
> }
>
> /* Check the remote host's key against known_hosts. */
> - ret = check_host_key(s, s->inet->host, port, opts->host_key_check, errp);
> + ret = check_host_key(s, opts->host_key_check, errp);
> if (ret < 0) {
> goto err;
> }
>
> /* Authenticate. */
> - ret = authenticate(s, s->user, errp);
> + ret = authenticate(s, errp);
> if (ret < 0) {
> goto err;
> }
>
> /* Start SFTP. */
> - s->sftp = libssh2_sftp_init(s->session);
> + s->sftp = sftp_new(s->session);
> if (!s->sftp) {
> - session_error_setg(errp, s, "failed to initialize sftp handle");
> + session_error_setg(errp, s, "failed to create sftp handle");
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + r = sftp_init(s->sftp);
> + if (r < 0) {
> + sftp_error_setg(errp, s, "failed to initialize sftp handle");
> ret = -EINVAL;
> goto err;
> }
>
> /* Open the remote file. */
> trace_ssh_connect_to_ssh(opts->path, ssh_flags, creat_mode);
> - s->sftp_handle = libssh2_sftp_open(s->sftp, opts->path, ssh_flags,
> - creat_mode);
> + s->sftp_handle = sftp_open(s->sftp, opts->path, ssh_flags, creat_mode);
> if (!s->sftp_handle) {
> - session_error_setg(errp, s, "failed to open remote file '%s'",
> - opts->path);
> + sftp_error_setg(errp, s, "failed to open remote file '%s'",
> + opts->path);
> ret = -EINVAL;
> goto err;
> }
>
> - r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
> - if (r < 0) {
> + /* Make sure the SFTP file is handled in blocking mode. */
> + sftp_file_set_blocking(s->sftp_handle);
> +
> + s->attrs = sftp_fstat(s->sftp_handle);
> + if (!s->attrs) {
> sftp_error_setg(errp, s, "failed to read file attributes");
> return -EINVAL;
> }
> @@ -727,21 +792,26 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
> return 0;
>
> err:
> + if (s->attrs) {
> + sftp_attributes_free(s->attrs);
> + }
> + s->attrs = NULL;
> if (s->sftp_handle) {
> - libssh2_sftp_close(s->sftp_handle);
> + sftp_close(s->sftp_handle);
> }
> s->sftp_handle = NULL;
> if (s->sftp) {
> - libssh2_sftp_shutdown(s->sftp);
> + sftp_free(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);
> + ssh_disconnect(s->session);
> + ssh_free(s->session);
> }
> s->session = NULL;
> + if (new_sock >= 0) {
> + close(new_sock);
> + }
>
> return ret;
> }
> @@ -756,9 +826,11 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>
> ssh_state_init(s);
>
> - ssh_flags = LIBSSH2_FXF_READ;
> + ssh_flags = 0;
> if (bdrv_flags & BDRV_O_RDWR) {
> - ssh_flags |= LIBSSH2_FXF_WRITE;
> + ssh_flags |= O_RDWR;
> + } else {
> + ssh_flags |= O_RDONLY;
> }
>
> opts = ssh_parse_options(options, errp);
> @@ -773,16 +845,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
> }
>
> /* Go non-blocking. */
> - libssh2_session_set_blocking(s->session, 0);
> + ssh_set_blocking(s->session, 0);
>
> qapi_free_BlockdevOptionsSsh(opts);
>
> return 0;
>
> err:
> - if (s->sock >= 0) {
> - close(s->sock);
> - }
> s->sock = -1;
>
> qapi_free_BlockdevOptionsSsh(opts);
> @@ -795,25 +864,25 @@ static int ssh_grow_file(BDRVSSHState *s, int64_t offset, Error **errp)
> {
> ssize_t ret;
> char c[1] = { '\0' };
> - int was_blocking = libssh2_session_get_blocking(s->session);
> + int was_blocking = ssh_is_blocking(s->session);
>
> /* offset must be strictly greater than the current size so we do
> * not overwrite anything */
> - assert(offset > 0 && offset > s->attrs.filesize);
> + assert(offset > 0 && offset > s->attrs->size);
>
> - libssh2_session_set_blocking(s->session, 1);
> + ssh_set_blocking(s->session, 1);
>
> - libssh2_sftp_seek64(s->sftp_handle, offset - 1);
> - ret = libssh2_sftp_write(s->sftp_handle, c, 1);
> + sftp_seek64(s->sftp_handle, offset - 1);
> + ret = sftp_write(s->sftp_handle, c, 1);
>
> - libssh2_session_set_blocking(s->session, was_blocking);
> + ssh_set_blocking(s->session, was_blocking);
>
> if (ret < 0) {
> sftp_error_setg(errp, s, "Failed to grow file");
> return -EIO;
> }
>
> - s->attrs.filesize = offset;
> + s->attrs->size = offset;
> return 0;
> }
>
> @@ -841,8 +910,7 @@ static int ssh_co_create(BlockdevCreateOptions *options, Error **errp)
> ssh_state_init(&s);
>
> ret = connect_to_ssh(&s, opts->location,
> - LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
> - LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
> + O_RDWR | O_CREAT | O_TRUNC,
> 0644, errp);
> if (ret < 0) {
> goto fail;
> @@ -911,10 +979,8 @@ static int ssh_has_zero_init(BlockDriverState *bs)
> /* Assume false, unless we can positively prove it's true. */
> int has_zero_init = 0;
>
> - if (s->attrs.flags & LIBSSH2_SFTP_ATTR_PERMISSIONS) {
> - if (s->attrs.permissions & LIBSSH2_SFTP_S_IFREG) {
> - has_zero_init = 1;
> - }
> + if (s->attrs->type == SSH_FILEXFER_TYPE_REGULAR) {
> + has_zero_init = 1;
> }
>
> return has_zero_init;
> @@ -951,12 +1017,12 @@ static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
> .co = qemu_coroutine_self()
> };
>
> - r = libssh2_session_block_directions(s->session);
> + r = ssh_get_poll_flags(s->session);
>
> - if (r & LIBSSH2_SESSION_BLOCK_INBOUND) {
> + if (r & SSH_READ_PENDING) {
> rd_handler = restart_coroutine;
> }
> - if (r & LIBSSH2_SESSION_BLOCK_OUTBOUND) {
> + if (r & SSH_WRITE_PENDING) {
> wr_handler = restart_coroutine;
> }
>
> @@ -968,33 +1034,6 @@ static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
> trace_ssh_co_yield_back(s->sock);
> }
>
> -/* SFTP has a function `libssh2_sftp_seek64' which seeks to a position
> - * in the remote file. Notice that it just updates a field in the
> - * sftp_handle structure, so there is no network traffic and it cannot
> - * fail.
> - *
> - * However, `libssh2_sftp_seek64' does have a catastrophic effect on
> - * performance since it causes the handle to throw away all in-flight
> - * reads and buffered readahead data. Therefore this function tries
> - * to be intelligent about when to call the underlying libssh2 function.
> - */
> -#define SSH_SEEK_WRITE 0
> -#define SSH_SEEK_READ 1
> -#define SSH_SEEK_FORCE 2
> -
> -static void ssh_seek(BDRVSSHState *s, int64_t offset, int flags)
> -{
> - bool op_read = (flags & SSH_SEEK_READ) != 0;
> - bool force = (flags & SSH_SEEK_FORCE) != 0;
> -
> - if (force || op_read != s->offset_op_read || offset != s->offset) {
> - trace_ssh_seek(offset);
> - libssh2_sftp_seek64(s->sftp_handle, offset);
> - s->offset = offset;
> - s->offset_op_read = op_read;
> - }
> -}
> -
> static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
> int64_t offset, size_t size,
> QEMUIOVector *qiov)
> @@ -1006,7 +1045,8 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
>
> trace_ssh_read(offset, size);
>
> - ssh_seek(s, offset, SSH_SEEK_READ);
> + trace_ssh_seek(offset);
> + sftp_seek64(s->sftp_handle, offset);
>
> /* This keeps track of the current iovec element ('i'), where we
> * will write to next ('buf'), and the end of the current iovec
> @@ -1016,35 +1056,33 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
> buf = i->iov_base;
> end_of_vec = i->iov_base + i->iov_len;
>
> - /* 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:
I'd use:
const ptrdiff_t request_read_size = MIN(end_of_vec - buf, 16 * KiB);
> - trace_ssh_read_buf(buf, end_of_vec - buf);
> - r = libssh2_sftp_read(s->sftp_handle, buf, end_of_vec - buf);
> - trace_ssh_read_return(r);
> + trace_ssh_read_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 16384));
and use 'request_read_size' here,
> + /* The size of SFTP packets is limited to 32K bytes, so limit
> + * the amount of data requested to 16K, as libssh currently
> + * does not handle multiple requests on its own:
> + * https://red.libssh.org/issues/58
> + */
> + r = sftp_read(s->sftp_handle, buf, MIN(end_of_vec - buf, 16384));
and here.
> + trace_ssh_read_return(r, sftp_get_error(s->sftp));
>
> - if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
> + if (r == SSH_AGAIN) {
> co_yield(s, bs);
> goto again;
> }
> - if (r < 0) {
> - sftp_error_trace(s, "read");
> - s->offset = -1;
> - return -EIO;
> - }
> - if (r == 0) {
> + if (r == SSH_EOF || (r == 0 && sftp_get_error(s->sftp) == SSH_FX_EOF)) {
> /* EOF: Short read so pad the buffer with zeroes and return it. */
> qemu_iovec_memset(qiov, got, 0, size - got);
> return 0;
> }
> + if (r <= 0) {
> + sftp_error_trace(s, "read");
> + return -EIO;
> + }
>
> got += r;
> buf += r;
> - s->offset += r;
> if (buf >= end_of_vec && got < size) {
> i++;
> buf = i->iov_base;
> @@ -1081,7 +1119,8 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
>
> trace_ssh_write(offset, size);
>
> - ssh_seek(s, offset, SSH_SEEK_WRITE);
> + trace_ssh_seek(offset);
> + sftp_seek64(s->sftp_handle, offset);
>
> /* This keeps track of the current iovec element ('i'), where we
> * will read from next ('buf'), and the end of the current iovec
> @@ -1093,45 +1132,34 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
>
> for (written = 0; written < size; ) {
> again:
Ditto 'request_write_size = MIN(..., 128 * KiB)'.
> - trace_ssh_write_buf(buf, end_of_vec - buf);
> - r = libssh2_sftp_write(s->sftp_handle, buf, end_of_vec - buf);
> - trace_ssh_write_return(r);
> + trace_ssh_write_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 131072));
> + /* Avoid too large data packets, as libssh currently does not
> + * handle multiple requests on its own:
> + * https://red.libssh.org/issues/58
> + */
> + r = sftp_write(s->sftp_handle, buf, MIN(end_of_vec - buf, 131072));
> + trace_ssh_write_return(r, sftp_get_error(s->sftp));
>
> - if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
> + if (r == SSH_AGAIN) {
> co_yield(s, bs);
> goto again;
> }
> if (r < 0) {
> sftp_error_trace(s, "write");
> - s->offset = -1;
> return -EIO;
> }
> - /* The libssh2 API is very unclear about this. A comment in
> - * the code says "nothing was acked, and no EAGAIN was
> - * received!" which apparently means that no data got sent
> - * out, and the underlying channel didn't return any EAGAIN
> - * indication. I think this is a bug in either libssh2 or
> - * OpenSSH (server-side). In any case, forcing a seek (to
> - * discard libssh2 internal buffers), and then trying again
> - * works for me.
> - */
> - if (r == 0) {
> - ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
> - co_yield(s, bs);
> - goto again;
> - }
>
> written += r;
> buf += r;
> - s->offset += r;
> if (buf >= end_of_vec && written < size) {
> i++;
> buf = i->iov_base;
> end_of_vec = i->iov_base + i->iov_len;
> }
>
> - if (offset + written > s->attrs.filesize)
> - s->attrs.filesize = offset + written;
> + if (offset + written > s->attrs->size) {
> + s->attrs->size = offset + written;
> + }
> }
>
> return 0;
> @@ -1166,24 +1194,24 @@ static void unsafe_flush_warning(BDRVSSHState *s, const char *what)
> }
> }
>
> -#ifdef HAS_LIBSSH2_SFTP_FSYNC
> +#if HAVE_LIBSSH_0_8
>
> static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
> {
> int r;
>
> trace_ssh_flush();
> +
> + if (!sftp_extension_supported(s->sftp, "fsync@openssh.com", "1")) {
> + unsafe_flush_warning(s, "OpenSSH >= 6.3");
> + return 0;
> + }
> again:
> - r = libssh2_sftp_fsync(s->sftp_handle);
> - if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
> + r = sftp_fsync(s->sftp_handle);
> + if (r == SSH_AGAIN) {
> co_yield(s, bs);
> goto again;
> }
> - if (r == LIBSSH2_ERROR_SFTP_PROTOCOL &&
> - libssh2_sftp_last_error(s->sftp) == LIBSSH2_FX_OP_UNSUPPORTED) {
> - unsafe_flush_warning(s, "OpenSSH >= 6.3");
> - return 0;
> - }
> if (r < 0) {
> sftp_error_trace(s, "fsync");
> return -EIO;
> @@ -1204,25 +1232,25 @@ static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
> return ret;
> }
>
> -#else /* !HAS_LIBSSH2_SFTP_FSYNC */
> +#else /* !HAVE_LIBSSH_0_8 */
>
> static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
> {
> BDRVSSHState *s = bs->opaque;
>
> - unsafe_flush_warning(s, "libssh2 >= 1.4.4");
> + unsafe_flush_warning(s, "libssh >= 0.8.0");
> return 0;
> }
>
> -#endif /* !HAS_LIBSSH2_SFTP_FSYNC */
> +#endif /* !HAVE_LIBSSH_0_8 */
>
> static int64_t ssh_getlength(BlockDriverState *bs)
> {
> BDRVSSHState *s = bs->opaque;
> int64_t length;
>
> - /* Note we cannot make a libssh2 call here. */
> - length = (int64_t) s->attrs.filesize;
> + /* Note we cannot make a libssh call here. */
> + length = (int64_t) s->attrs->size;
> trace_ssh_getlength(length);
>
> return length;
> @@ -1239,12 +1267,12 @@ static int coroutine_fn ssh_co_truncate(BlockDriverState *bs, int64_t offset,
> return -ENOTSUP;
> }
>
> - if (offset < s->attrs.filesize) {
> + if (offset < s->attrs->size) {
> error_setg(errp, "ssh driver does not support shrinking files");
> return -ENOTSUP;
> }
>
> - if (offset == s->attrs.filesize) {
> + if (offset == s->attrs->size) {
> return 0;
> }
>
> @@ -1339,12 +1367,16 @@ static void bdrv_ssh_init(void)
> {
> int r;
>
> - r = libssh2_init(0);
> + r = ssh_init();
> if (r != 0) {
> - fprintf(stderr, "libssh2 initialization failed, %d\n", r);
> + fprintf(stderr, "libssh initialization failed, %d\n", r);
> exit(EXIT_FAILURE);
> }
>
> +#if TRACE_LIBSSH != 0
> + ssh_set_log_level(TRACE_LIBSSH);
> +#endif
> +
> bdrv_register(&bdrv_ssh);
> }
>
> diff --git a/block/trace-events b/block/trace-events
> index eab51497fc..0d2126e840 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -169,19 +169,21 @@ nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags
> # ssh.c
> ssh_restart_coroutine(void *co) "co=%p"
> ssh_flush(void) "fsync"
> -ssh_check_host_key_knownhosts(const char *key) "host key OK: %s"
> +ssh_check_host_key_knownhosts(void) "host key OK"
> ssh_connect_to_ssh(char *path, int flags, int mode) "opening file %s flags=0x%x creat_mode=0%o"
> ssh_co_yield(int sock, void *rd_handler, void *wr_handler) "s->sock=%d rd_handler=%p wr_handler=%p"
> ssh_co_yield_back(int sock) "s->sock=%d - back"
> ssh_getlength(int64_t length) "length=%" PRIi64
> ssh_co_create_opts(uint64_t size) "total_size=%" PRIu64
> ssh_read(int64_t offset, size_t size) "offset=%" PRIi64 " size=%zu"
> -ssh_read_buf(void *buf, size_t size) "sftp_read buf=%p size=%zu"
> -ssh_read_return(ssize_t ret) "sftp_read returned %zd"
> +ssh_read_buf(void *buf, size_t size, size_t actual_size) "sftp_read buf=%p size=%zu (actual size=%zu)"
> +ssh_read_return(ssize_t ret, int sftp_err) "sftp_read returned %zd (sftp error=%d)"
> ssh_write(int64_t offset, size_t size) "offset=%" PRIi64 " size=%zu"
> -ssh_write_buf(void *buf, size_t size) "sftp_write buf=%p size=%zu"
> -ssh_write_return(ssize_t ret) "sftp_write returned %zd"
> +ssh_write_buf(void *buf, size_t size, size_t actual_size) "sftp_write buf=%p size=%zu (actual size=%zu)"
> +ssh_write_return(ssize_t ret, int sftp_err) "sftp_write returned %zd (sftp error=%d)"
> ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
> +ssh_auth_methods(int methods) "auth methods=%x"
Please use 0x prefix.
> +ssh_server_status(int status) "server status=%d"
>
> # curl.c
> curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
> @@ -214,4 +216,4 @@ sheepdog_snapshot_create(const char *sn_name, const char *id) "%s %s"
> sheepdog_snapshot_create_inode(const char *name, uint32_t snap, uint32_t vdi) "s->inode: name %s snap_id 0x%" PRIx32 " vdi 0x%" PRIx32
>
> # ssh.c
> -sftp_error(const char *op, const char *ssh_err, int ssh_err_code, unsigned long sftp_err_code) "%s failed: %s (libssh2 error code: %d, sftp error code: %lu)"
> +sftp_error(const char *op, const char *ssh_err, int ssh_err_code, int sftp_err_code) "%s failed: %s (libssh error code: %d, sftp error code: %d)"
> diff --git a/configure b/configure
> index b091b82cb3..bfdd70c40a 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"
> + --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,17 @@ 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
> - fi
> -fi
> -
> -##########################################
> -# libssh2_sftp_fsync probe
> -
> -if test "$libssh2" = "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;
> -}
> -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"
> + libssh=no
> fi
> fi
>
> @@ -6451,7 +6425,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 +7118,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
> diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
> index ec9823793a..1239d9d648 100644
> --- a/tests/qemu-iotests/207.out
> +++ b/tests/qemu-iotests/207.out
> @@ -68,7 +68,7 @@ virtual size: 4 MiB (4194304 bytes)
>
> {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"mode": "none"}, "path": "/this/is/not/an/existing/path", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 4194304}}}
> {"return": {}}
> -Job failed: failed to open remote file '/this/is/not/an/existing/path': Failed opening remote file (libssh2 error code: -31)
> +Job failed: failed to open remote file '/this/is/not/an/existing/path': SFTP server: No such file (libssh error code: 1, sftp error code: 2)
> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> {"return": {}}
>
>
Pino, this is the list of changes I did, as I commented on IRC:
1/ Build on Ubuntu:
./block/ssh.c:417:10: note: each undeclared identifier is reported only
once for each function it appears in
./block/ssh.c: In function 'check_host_key_hash':
./block/ssh.c:440:5: error: 'ssh_get_publickey' is deprecated
[-Werror=deprecated-declarations]
r = ssh_get_publickey(s->session, &pubkey);
^
In file included from ./block/ssh.c:27:0:
/usr/include/libssh/libssh.h:489:31: note: declared here
SSH_DEPRECATED LIBSSH_API int ssh_get_publickey(ssh_session session,
ssh_key *key);
^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
-- >8 --diff --git a/block/ssh.c b/block/ssh.c
index e62f9ee3b5..cb6fe0b17d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -408,7 +408,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
unsigned char *server_hash;
size_t server_hash_len;
-#if HAVE_LIBSSH_0_8
+#ifdef HAVE_SSH_GET_SERVER_PUBLICKEY
r = ssh_get_server_publickey(s->session, &pubkey);
#else
r = ssh_get_publickey(s->session, &pubkey);
diff --git a/configure b/configure
index bfdd70c40a..93547b4958 100755
--- a/configure
+++ b/configure
@@ -3928,6 +3928,18 @@ if test "$libssh" != "no" ; then
fi
fi
+# check for ssh_get_server_publickey
+if test "$libssh" = "yes" ; then
+ cat > $TMPC << EOF
+#include <libssh/libssh.h>
+int main(void) { return ssh_get_server_publickey(NULL, NULL);}
+EOF
+ if compile_prog "$libssh_cflags" "$libssh_libs"; then
+ libssh_cflags="-DHAVE_SSH_GET_SERVER_PUBLICKEY $libssh_cflags"
+ fi
+fi
+
+
##########################################
# linux-aio probe---
See https://www.redhat.com/archives/libvir-list/2018-May/msg00597.html
2/ Checkpatch (you already fixed the style issues)
-- >8 --
diff --git a/block/trace-events b/block/trace-events
index 0d2126e840..cb91787bd9 100644
@@ -182,7 +182,7 @@ ssh_write(int64_t offset, size_t size) "offset=%"
PRIi64 " size=%zu"
ssh_write_buf(void *buf, size_t size, size_t actual_size) "sftp_write
buf=%p size=%zu (actual size=%zu)"
ssh_write_return(ssize_t ret, int sftp_err) "sftp_write returned %zd
(sftp error=%d)"
ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
-ssh_auth_methods(int methods) "auth methods=%x"
+ssh_auth_methods(int methods) "auth methods=0x%04x"
ssh_server_status(int status) "server status=%d"
---
3/ Travis
-- >8 --
diff --git a/.travis.yml b/.travis.yml
index b053a836a3..a2dac8b7c9 100644
@@ -31,7 +31,7 @@ addons:
- libseccomp-dev
- libspice-protocol-dev
- libspice-server-dev
- - libssh2-1-dev
+ - libssh-dev
- liburcu-dev
- libusb-1.0-0-dev
- libvte-2.91-dev
@@ -261,7 +261,7 @@ matrix:
- libseccomp-dev
- libspice-protocol-dev
- libspice-server-dev
- - libssh2-1-dev
+ - libssh-dev
- liburcu-dev
- libusb-1.0-0-dev
- libvte-2.91-dev
---
4/ Docker
-- >8 --
diff --git a/tests/docker/dockerfiles/fedora.docker
b/tests/docker/dockerfiles/fedora.docker
index afbba29ada..8893c1b957 100644
@@ -35,7 +35,7 @@ ENV PACKAGES \
libpng-devel \
librbd-devel \
libseccomp-devel \
- libssh2-devel \
+ libssh-devel \
libubsan \
libusbx-devel \
libxml2-devel \
@@ -50,7 +50,6 @@ ENV PACKAGES \
mingw32-gtk3 \
mingw32-libjpeg-turbo \
mingw32-libpng \
- mingw32-libssh2 \
mingw32-libtasn1 \
mingw32-nettle \
mingw32-pixman \
@@ -64,7 +63,6 @@ ENV PACKAGES \
mingw64-gtk3 \
mingw64-libjpeg-turbo \
mingw64-libpng \
- mingw64-libssh2 \
mingw64-libtasn1 \
mingw64-nettle \
mingw64-pixman \
diff --git a/tests/docker/dockerfiles/ubuntu.docker
b/tests/docker/dockerfiles/ubuntu.docker
index 36e2b17de5..1bbb7f9598 100644
@@ -44,7 +44,7 @@ ENV PACKAGES flex bison \
libsnappy-dev \
libspice-protocol-dev \
libspice-server-dev \
- libssh2-1-dev \
+ libssh-dev \
libusb-1.0-0-dev \
libusbredirhost-dev \
libvdeplug-dev \
diff --git a/tests/docker/dockerfiles/ubuntu1804.docker
b/tests/docker/dockerfiles/ubuntu1804.docker
index 2e2900150b..9d80b11500 100644
@@ -40,7 +40,7 @@ ENV PACKAGES flex bison \
libsnappy-dev \
libspice-protocol-dev \
libspice-server-dev \
- libssh2-1-dev \
+ libssh-dev \
libusb-1.0-0-dev \
libusbredirhost-dev \
---
Note, libssh is not available on MinGW.
5/ Documentation
-- >8 --
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
index da06a9bc83..91ab0eceae 100644
@@ -782,7 +782,7 @@ print a warning when @code{fsync} is not supported:
warning: ssh server @code{ssh.example.com:22} does not support fsync
-With sufficiently new versions of libssh2 and OpenSSH, @code{fsync} is
+With sufficiently new versions of libssh and OpenSSH, @code{fsync} is
supported.
---
Overall looks OK.
Since iotest #207 is quick, it would be nice to have it in our CI.
I guess a Docker Compose setup should work, I'll see what I can do.
Regards,
Phil.
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-12 13:27 ` Philippe Mathieu-Daudé
@ 2019-06-13 13:02 ` Alex Bennée
2019-06-13 19:41 ` Stefan Weil
1 sibling, 0 replies; 15+ messages in thread
From: Alex Bennée @ 2019-06-13 13:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: kwolf, Daniel P. Berrange, qemu-block, qemu-devel, Stefan Weil,
rjones, mreitz, Pino Toscano
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
>
> On 6/5/19 11:36 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).
>>
>> Signed-off-by: Pino Toscano <ptoscano@redhat.com>
>> ---
>>
>> 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/block/Makefile.objs b/block/Makefile.objs
>> index ae11605c9f..bf01429dd5 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -31,7 +31,7 @@ block-obj-$(CONFIG_CURL) += curl.o
>> block-obj-$(CONFIG_RBD) += rbd.o
>> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>> block-obj-$(CONFIG_VXHS) += vxhs.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
>> @@ -52,8 +52,8 @@ rbd.o-libs := $(RBD_LIBS)
>> gluster.o-cflags := $(GLUSTERFS_CFLAGS)
>> gluster.o-libs := $(GLUSTERFS_LIBS)
>> vxhs.o-libs := $(VXHS_LIBS)
>> -ssh.o-cflags := $(LIBSSH2_CFLAGS)
>> -ssh.o-libs := $(LIBSSH2_LIBS)
>> +ssh.o-cflags := $(LIBSSH_CFLAGS)
>> +ssh.o-libs := $(LIBSSH_LIBS)
>> block-obj-dmg-bz2-$(CONFIG_BZIP2) += dmg-bz2.o
>> block-obj-$(if $(CONFIG_DMG),m,n) += $(block-obj-dmg-bz2-y)
>> dmg-bz2.o-libs := $(BZIP2_LIBS)
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 12fd4f39e8..ce2363a471 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -24,8 +24,8 @@
>>
>> #include "qemu/osdep.h"
>>
>> -#include <libssh2.h>
>> -#include <libssh2_sftp.h>
>> +#include <libssh/libssh.h>
>> +#include <libssh/sftp.h>
>>
>> #include "block/block_int.h"
>> #include "block/qdict.h"
>> @@ -43,14 +43,13 @@
>> #include "qapi/qobject-output-visitor.h"
>> #include "trace.h"
>>
>> -/*
>> - * TRACE_LIBSSH2=<bitmask> 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 <bitmask> is described
>> - * here: http://www.libssh2.org/libssh2_trace.html
>> +/* TRACE_LIBSSH=<level> enables tracing in libssh itself.
>> + * The meaning of <level> is described here:
>> + * http://api.libssh.org/master/group__libssh__log.html
>> */
>> -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
>> +#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
>> +
>> +#define HAVE_LIBSSH_0_8 (LIBSSH_VERSION_INT >= SSH_VERSION_INT(0, 8, 0))
>
> As I noticed with ssh_get_publickey() and reading
> https://www.redhat.com/archives/libvir-list/2018-May/msg00597.html, I'm
> not convinced this definition is accurate. Used in [1].
>
>>
>> typedef struct BDRVSSHState {
>> /* Coroutine. */
>> @@ -58,18 +57,14 @@ typedef struct BDRVSSHState {
>>
>> /* 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;
>>
>> InetSocketAddress *inet;
>>
>> @@ -89,7 +84,6 @@ static void ssh_state_init(BDRVSSHState *s)
>> {
>> memset(s, 0, sizeof *s);
>> s->sock = -1;
>> - s->offset = -1;
>> qemu_co_mutex_init(&s->lock);
>> }
>>
>> @@ -97,21 +91,20 @@ static void ssh_state_free(BDRVSSHState *s)
>> {
>> g_free(s->user);
>>
>> + 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);
>
> Maybe:
>
> ssh_free(s->session); /* This frees s->sock */
>
>> }
>> + /* s->sock is owned by the ssh_session, which frees it. */
>
> And drop this comment.
>
>> }
>>
>> static void GCC_FMT_ATTR(3, 4)
>> @@ -125,13 +118,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;
>
> You can drop these,
>
>>
>> - /* This is not an errno. See <libssh2.h>. */
>> - ssh_err_code = libssh2_session_last_error(s->session,
>> - &ssh_err, NULL, 0);
>> - error_setg(errp, "%s: %s (libssh2 error code: %d)",
>> + /* This is not an errno. See <libssh/libssh.h>. */
>> + ssh_err = ssh_get_error(s->session);
>> + ssh_err_code = ssh_get_error_code(s->session);
>> + error_setg(errp, "%s: %s (libssh error code: %d)",
>
> using directly as arguments here. However this might be easier for the
> libssh2/libssh switch to do as you did, and clean this later.
>
>> msg, ssh_err, ssh_err_code);
>> } else {
>> error_setg(errp, "%s", msg);
>> @@ -150,18 +143,18 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
>> va_end(args);
>>
>> if (s->sftp) {
>> - char *ssh_err;
>> + const char *ssh_err;
>> int ssh_err_code;
>> - unsigned long sftp_err_code;
>> + int sftp_err_code;
>>
>> - /* This is not an errno. See <libssh2.h>. */
>> - ssh_err_code = libssh2_session_last_error(s->session,
>> - &ssh_err, NULL, 0);
>> - /* See <libssh2_sftp.h>. */
>> - sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>> + /* This is not an errno. See <libssh/libssh.h>. */
>> + ssh_err = ssh_get_error(s->session);
>> + ssh_err_code = ssh_get_error_code(s->session);
>> + /* See <libssh/sftp.h>. */
>> + sftp_err_code = sftp_get_error(s->sftp);
>>
>> error_setg(errp,
>> - "%s: %s (libssh2 error code: %d, sftp error code: %lu)",
>> + "%s: %s (libssh error code: %d, sftp error code: %d)",
>> msg, ssh_err, ssh_err_code, sftp_err_code);
>
> Ditto, etc.
>
>> } else {
>> error_setg(errp, "%s", msg);
>> @@ -171,15 +164,15 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
>>
>> static void sftp_error_trace(BDRVSSHState *s, const char *op)
>> {
>> - char *ssh_err;
>> + const char *ssh_err;
>> int ssh_err_code;
>> - unsigned long sftp_err_code;
>> + int sftp_err_code;
>>
>> - /* This is not an errno. See <libssh2.h>. */
>> - ssh_err_code = libssh2_session_last_error(s->session,
>> - &ssh_err, NULL, 0);
>> - /* See <libssh2_sftp.h>. */
>> - sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>> + /* This is not an errno. See <libssh/libssh.h>. */
>> + ssh_err = ssh_get_error(s->session);
>> + ssh_err_code = ssh_get_error_code(s->session);
>> + /* See <libssh/sftp.h>. */
>> + sftp_err_code = sftp_get_error(s->sftp);
>>
>> trace_sftp_error(op, ssh_err, ssh_err_code, sftp_err_code);
>> }
>> @@ -280,82 +273,89 @@ static void ssh_parse_filename(const char *filename, QDict *options,
>> parse_uri(filename, options, errp);
>> }
>>
>> -static int check_host_key_knownhosts(BDRVSSHState *s,
>> - const char *host, int port, Error **errp)
>> +static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
>> {
>> - const char *home;
>> - char *knh_file = NULL;
>> - LIBSSH2_KNOWNHOSTS *knh = NULL;
>> - struct libssh2_knownhost *found;
>> - int ret, r;
>> - const char *hostkey;
>> - size_t len;
>> - int type;
>> + int ret;
>> +#if HAVE_LIBSSH_0_8
>
> [1] See previous comment about HAVE_LIBSSH_0_8.
>
>> + enum ssh_known_hosts_e state;
>>
>> - hostkey = libssh2_session_hostkey(s->session, &len, &type);
>> - if (!hostkey) {
>> + state = ssh_session_is_known_server(s->session);
>> + trace_ssh_server_status(state);
>> +
>> + switch (state) {
>> + case SSH_KNOWN_HOSTS_OK:
>> + /* OK */
>> + trace_ssh_check_host_key_knownhosts();
>> + break;
>> + case SSH_KNOWN_HOSTS_CHANGED:
>> ret = -EINVAL;
>> - session_error_setg(errp, s, "failed to read remote host key");
>> + error_setg(errp, "host key does not match the one in known_hosts");
>> goto out;
>> - }
>> -
>> - knh = libssh2_knownhost_init(s->session);
>> - if (!knh) {
>> + case SSH_KNOWN_HOSTS_OTHER:
>> ret = -EINVAL;
>> - session_error_setg(errp, s,
>> - "failed to initialize known hosts support");
>> + error_setg(errp,
>> + "host key for this server not found, another type exists");
>> + goto out;
>> + case SSH_KNOWN_HOSTS_UNKNOWN:
>> + ret = -ENOENT;
>> + error_setg(errp, "no host key was found in known_hosts");
>> + goto out;
>> + case SSH_KNOWN_HOSTS_NOT_FOUND:
>> + ret = -EINVAL;
>> + error_setg(errp, "known_hosts file not found");
>> + goto out;
>> + case SSH_KNOWN_HOSTS_ERROR:
>> + ret = -EINVAL;
>> + error_setg(errp, "error while checking the host");
>> + goto out;
>> + default:
>
> I'd rather not use a default case, so if libssh get more
> ssh_known_hosts_e, we get a build failure and add the new cases.
>
>> + ret = -EINVAL;
>> + error_setg(errp, "error while checking for known server");
>> goto out;
>> }
>> +#else /* !HAVE_LIBSSH_0_8 */
>> + int state;
>>
>> - home = getenv("HOME");
>> - if (home) {
>> - knh_file = g_strdup_printf("%s/.ssh/known_hosts", home);
>> - } else {
>> - knh_file = g_strdup_printf("/root/.ssh/known_hosts");
>> - }
>> -
>> - /* Read all known hosts from OpenSSH-style known_hosts file. */
>> - libssh2_knownhost_readfile(knh, knh_file, LIBSSH2_KNOWNHOST_FILE_OPENSSH);
>> + state = ssh_is_server_known(s->session);
>> + trace_ssh_server_status(state);
>>
>> - r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
>> - LIBSSH2_KNOWNHOST_TYPE_PLAIN|
>> - LIBSSH2_KNOWNHOST_KEYENC_RAW,
>> - &found);
>> - switch (r) {
>> - case LIBSSH2_KNOWNHOST_CHECK_MATCH:
>> + switch (state) {
>> + case SSH_SERVER_KNOWN_OK:
>> /* OK */
>> - trace_ssh_check_host_key_knownhosts(found->key);
>> + trace_ssh_check_host_key_knownhosts();
>> break;
>> - case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
>> + case SSH_SERVER_KNOWN_CHANGED:
>> + ret = -EINVAL;
>> + error_setg(errp, "host key does not match the one in known_hosts");
>> + goto out;
>> + case SSH_SERVER_FOUND_OTHER:
>> ret = -EINVAL;
>> - session_error_setg(errp, s,
>> - "host key does not match the one in known_hosts"
>> - " (found key %s)", found->key);
>> + error_setg(errp,
>> + "host key for this server not found, another type exists");
>> + goto out;
>> + case SSH_SERVER_FILE_NOT_FOUND:
>> + ret = -ENOENT;
>> + error_setg(errp, "known_hosts file not found");
>> goto out;
>> - case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
>> + case SSH_SERVER_NOT_KNOWN:
>> ret = -EINVAL;
>> - session_error_setg(errp, s, "no host key was found in known_hosts");
>> + error_setg(errp, "no host key was found in known_hosts");
>> goto out;
>> - case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
>> + case SSH_SERVER_ERROR:
>> ret = -EINVAL;
>> - session_error_setg(errp, s,
>> - "failure matching the host key with known_hosts");
>> + error_setg(errp, "server error");
>> goto out;
>> default:
>> ret = -EINVAL;
>> - session_error_setg(errp, s, "unknown error matching the host key"
>> - " with known_hosts (%d)", r);
>> + error_setg(errp, "error while checking for known server");
>> goto out;
>
> Here the default is dead code, I'd remove it too.
>
>> }
>> +#endif /* !HAVE_LIBSSH_0_8 */
>>
>> /* known_hosts checking successful. */
>> ret = 0;
>>
>> out:
>> - if (knh != NULL) {
>> - libssh2_knownhost_free(knh);
>> - }
>> - g_free(knh_file);
>> return ret;
>> }
>>
>> @@ -399,18 +399,34 @@ static int compare_fingerprint(const unsigned char *fingerprint, size_t len,
>>
>> static int
>> check_host_key_hash(BDRVSSHState *s, const char *hash,
>> - int hash_type, size_t fingerprint_len, Error **errp)
>> + enum ssh_publickey_hash_type type, Error **errp)
>> {
>> - const char *fingerprint;
>> + int r;
>> + ssh_key pubkey;
>> + unsigned char *server_hash;
>> + size_t server_hash_len;
>>
>> - fingerprint = libssh2_hostkey_hash(s->session, hash_type);
>> - if (!fingerprint) {
>> +#if HAVE_LIBSSH_0_8
>> + r = ssh_get_server_publickey(s->session, &pubkey);
>> +#else
>> + r = ssh_get_publickey(s->session, &pubkey);
>> +#endif
>> + if (r != SSH_OK) {
>> session_error_setg(errp, s, "failed to read remote host key");
>> return -EINVAL;
>> }
>>
>> - if(compare_fingerprint((unsigned char *) fingerprint, fingerprint_len,
>> - hash) != 0) {
>> + r = ssh_get_publickey_hash(pubkey, type, &server_hash, &server_hash_len);
>> + ssh_key_free(pubkey);
>> + if (r != 0) {
>> + session_error_setg(errp, s,
>> + "failed reading the hash of the server SSH key");
>> + return -EINVAL;
>> + }
>> +
>> + r = compare_fingerprint(server_hash, server_hash_len, hash);
>> + ssh_clean_pubkey_hash(&server_hash);
>> + if (r != 0) {
>> error_setg(errp, "remote host key does not match host_key_check '%s'",
>> hash);
>> return -EPERM;
>> @@ -419,8 +435,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
>> return 0;
>> }
>>
>> -static int check_host_key(BDRVSSHState *s, const char *host, int port,
>> - SshHostKeyCheck *hkc, Error **errp)
>> +static int check_host_key(BDRVSSHState *s, SshHostKeyCheck *hkc, Error **errp)
>> {
>> SshHostKeyCheckMode mode;
>>
>> @@ -436,15 +451,15 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
>> case SSH_HOST_KEY_CHECK_MODE_HASH:
>> if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_MD5) {
>> return check_host_key_hash(s, hkc->u.hash.hash,
>> - LIBSSH2_HOSTKEY_HASH_MD5, 16, errp);
>> + SSH_PUBLICKEY_HASH_MD5, errp);
>> } else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA1) {
>> return check_host_key_hash(s, hkc->u.hash.hash,
>> - LIBSSH2_HOSTKEY_HASH_SHA1, 20, errp);
>> + SSH_PUBLICKEY_HASH_SHA1, errp);
>> }
>> g_assert_not_reached();
>
> At least Fedora 29 uses SSH_PUBLICKEY_HASH_SHA256.
>
>> break;
>> case SSH_HOST_KEY_CHECK_MODE_KNOWN_HOSTS:
>> - return check_host_key_knownhosts(s, host, port, errp);
>> + return check_host_key_knownhosts(s, errp);
>> default:
>> g_assert_not_reached();
>> }
>> @@ -452,60 +467,42 @@ static int check_host_key(BDRVSSHState *s, const char *host, int port,
>> return -EINVAL;
>> }
>>
>> -static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
>> +static int authenticate(BDRVSSHState *s, Error **errp)
>> {
>> int r, ret;
>> - const char *userauthlist;
>> - LIBSSH2_AGENT *agent = NULL;
>> - struct libssh2_agent_publickey *identity;
>> - struct libssh2_agent_publickey *prev_identity = NULL;
>> + int method;
>>
>> - userauthlist = libssh2_userauth_list(s->session, user, strlen(user));
>> - if (strstr(userauthlist, "publickey") == NULL) {
>> + /* Try to authenticate with the "none" method. */
>> + r = ssh_userauth_none(s->session, NULL);
>> + if (r == SSH_AUTH_ERROR) {
>> ret = -EPERM;
>> - error_setg(errp,
>> - "remote server does not support \"publickey\" authentication");
>> + session_error_setg(errp, s, "failed to authenticate using none "
>> + "authentication");
>> goto out;
>> - }
>> -
>> - /* Connect to ssh-agent and try each identity in turn. */
>> - agent = libssh2_agent_init(s->session);
>> - if (!agent) {
>> - ret = -EINVAL;
>> - session_error_setg(errp, s, "failed to initialize ssh-agent support");
>> - goto out;
>> - }
>> - if (libssh2_agent_connect(agent)) {
>> - ret = -ECONNREFUSED;
>> - session_error_setg(errp, s, "failed to connect to ssh-agent");
>> - goto out;
>> - }
>> - if (libssh2_agent_list_identities(agent)) {
>> - ret = -EINVAL;
>> - session_error_setg(errp, s,
>> - "failed requesting identities from ssh-agent");
>> + } else if (r == SSH_AUTH_SUCCESS) {
>> + /* Authenticated! */
>> + ret = 0;
>> goto out;
>> }
>>
>> - for(;;) {
>> - r = libssh2_agent_get_identity(agent, &identity, prev_identity);
>> - if (r == 1) { /* end of list */
>> - break;
>> - }
>> - if (r < 0) {
>> + method = ssh_userauth_list(s->session, NULL);
>> + trace_ssh_auth_methods(method);
>> +
>> + /* Try to authenticate with publickey, using the ssh-agent
>> + * if available.
>> + */
>> + if (method & SSH_AUTH_METHOD_PUBLICKEY) {
>> + r = ssh_userauth_publickey_auto(s->session, NULL, NULL);
>> + if (r == SSH_AUTH_ERROR) {
>> ret = -EINVAL;
>> - session_error_setg(errp, s,
>> - "failed to obtain identity from ssh-agent");
>> + session_error_setg(errp, s, "failed to authenticate using "
>> + "publickey authentication");
>> goto out;
>> - }
>> - r = libssh2_agent_userauth(agent, user, identity);
>> - if (r == 0) {
>> + } else if (r == SSH_AUTH_SUCCESS) {
>> /* Authenticated! */
>> ret = 0;
>> goto out;
>> }
>> - /* Failed to authenticate with this identity, try the next one. */
>> - prev_identity = identity;
>> }
>>
>> ret = -EPERM;
>> @@ -513,13 +510,6 @@ static int authenticate(BDRVSSHState *s, const char *user, Error **errp)
>> "and the identities held by your ssh-agent");
>>
>> out:
>> - if (agent != NULL) {
>> - /* Note: libssh2 implementation implicitly calls
>> - * libssh2_agent_disconnect if necessary.
>> - */
>> - libssh2_agent_free(agent);
>> - }
>> -
>> return ret;
>> }
>>
>> @@ -638,7 +628,8 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
>> int ssh_flags, int creat_mode, Error **errp)
>> {
>> int r, ret;
>> - long port = 0;
>> + unsigned int port = 0;
>> + int new_sock = -1;
>>
>> if (opts->has_user) {
>> s->user = g_strdup(opts->user);
>> @@ -655,71 +646,145 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
>> s->inet = opts->server;
>> opts->server = NULL;
>>
>> - if (qemu_strtol(s->inet->port, NULL, 10, &port) < 0) {
>> + if (qemu_strtoui(s->inet->port, NULL, 10, &port) < 0) {
>> error_setg(errp, "Use only numeric port value");
>> ret = -EINVAL;
>> goto err;
>> }
>>
>> /* Open the socket and connect. */
>> - s->sock = inet_connect_saddr(s->inet, errp);
>> - if (s->sock < 0) {
>> + new_sock = inet_connect_saddr(s->inet, errp);
>> + if (new_sock < 0) {
>> ret = -EIO;
>> goto err;
>> }
>>
>> + /* Try to disable the Nagle algorithm on TCP sockets to reduce latency,
>> + * but do not fail if it cannot be disabled.
>> + */
>> + r = socket_set_nodelay(new_sock);
>> + if (r < 0) {
>> + warn_report("setting NODELAY for the ssh server %s failed: %m",
>> + s->inet->host);
>> + }
>> +
>> /* Create SSH session. */
>> - s->session = libssh2_session_init();
>> + s->session = ssh_new();
>> if (!s->session) {
>> ret = -EINVAL;
>> - session_error_setg(errp, s, "failed to initialize libssh2 session");
>> + session_error_setg(errp, s, "failed to initialize libssh session");
>> goto err;
>> }
>>
>> -#if TRACE_LIBSSH2 != 0
>> - libssh2_trace(s->session, TRACE_LIBSSH2);
>> -#endif
>> + /* Make sure we are in blocking mode during the connection and
>> + * authentication phases.
>> + */
>> + ssh_set_blocking(s->session, 1);
>>
>> - r = libssh2_session_handshake(s->session, s->sock);
>> - if (r != 0) {
>> + r = ssh_options_set(s->session, SSH_OPTIONS_USER, s->user);
>> + if (r < 0) {
>> + ret = -EINVAL;
>> + session_error_setg(errp, s,
>> + "failed to set the user in the libssh session");
>> + goto err;
>> + }
>> +
>> + r = ssh_options_set(s->session, SSH_OPTIONS_HOST, s->inet->host);
>> + if (r < 0) {
>> + ret = -EINVAL;
>> + session_error_setg(errp, s,
>> + "failed to set the host in the libssh session");
>> + goto err;
>> + }
>> +
>> + if (port > 0) {
>> + r = ssh_options_set(s->session, SSH_OPTIONS_PORT, &port);
>> + if (r < 0) {
>> + ret = -EINVAL;
>> + session_error_setg(errp, s,
>> + "failed to set the port in the libssh session");
>> + goto err;
>> + }
>> + }
>> +
>> + r = ssh_options_set(s->session, SSH_OPTIONS_COMPRESSION, "none");
>> + if (r < 0) {
>> + ret = -EINVAL;
>> + session_error_setg(errp, s,
>> + "failed to disable the compression in the libssh "
>> + "session");
>> + goto err;
>> + }
>> +
>> + /* Read ~/.ssh/config. */
>> + r = ssh_options_parse_config(s->session, NULL);
>> + if (r < 0) {
>> + ret = -EINVAL;
>> + session_error_setg(errp, s, "failed to parse ~/.ssh/config");
>> + goto err;
>> + }
>> +
>> + r = ssh_options_set(s->session, SSH_OPTIONS_FD, &new_sock);
>> + if (r < 0) {
>> + ret = -EINVAL;
>> + session_error_setg(errp, s,
>> + "failed to set the socket in the libssh session");
>> + goto err;
>> + }
>> + /* libssh took ownership of the socket. */
>> + s->sock = new_sock;
>> + new_sock = -1;
>> +
>> + /* Connect. */
>> + r = ssh_connect(s->session);
>> + if (r != SSH_OK) {
>> ret = -EINVAL;
>> session_error_setg(errp, s, "failed to establish SSH session");
>> goto err;
>> }
>>
>> /* Check the remote host's key against known_hosts. */
>> - ret = check_host_key(s, s->inet->host, port, opts->host_key_check, errp);
>> + ret = check_host_key(s, opts->host_key_check, errp);
>> if (ret < 0) {
>> goto err;
>> }
>>
>> /* Authenticate. */
>> - ret = authenticate(s, s->user, errp);
>> + ret = authenticate(s, errp);
>> if (ret < 0) {
>> goto err;
>> }
>>
>> /* Start SFTP. */
>> - s->sftp = libssh2_sftp_init(s->session);
>> + s->sftp = sftp_new(s->session);
>> if (!s->sftp) {
>> - session_error_setg(errp, s, "failed to initialize sftp handle");
>> + session_error_setg(errp, s, "failed to create sftp handle");
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> +
>> + r = sftp_init(s->sftp);
>> + if (r < 0) {
>> + sftp_error_setg(errp, s, "failed to initialize sftp handle");
>> ret = -EINVAL;
>> goto err;
>> }
>>
>> /* Open the remote file. */
>> trace_ssh_connect_to_ssh(opts->path, ssh_flags, creat_mode);
>> - s->sftp_handle = libssh2_sftp_open(s->sftp, opts->path, ssh_flags,
>> - creat_mode);
>> + s->sftp_handle = sftp_open(s->sftp, opts->path, ssh_flags, creat_mode);
>> if (!s->sftp_handle) {
>> - session_error_setg(errp, s, "failed to open remote file '%s'",
>> - opts->path);
>> + sftp_error_setg(errp, s, "failed to open remote file '%s'",
>> + opts->path);
>> ret = -EINVAL;
>> goto err;
>> }
>>
>> - r = libssh2_sftp_fstat(s->sftp_handle, &s->attrs);
>> - if (r < 0) {
>> + /* Make sure the SFTP file is handled in blocking mode. */
>> + sftp_file_set_blocking(s->sftp_handle);
>> +
>> + s->attrs = sftp_fstat(s->sftp_handle);
>> + if (!s->attrs) {
>> sftp_error_setg(errp, s, "failed to read file attributes");
>> return -EINVAL;
>> }
>> @@ -727,21 +792,26 @@ static int connect_to_ssh(BDRVSSHState *s, BlockdevOptionsSsh *opts,
>> return 0;
>>
>> err:
>> + if (s->attrs) {
>> + sftp_attributes_free(s->attrs);
>> + }
>> + s->attrs = NULL;
>> if (s->sftp_handle) {
>> - libssh2_sftp_close(s->sftp_handle);
>> + sftp_close(s->sftp_handle);
>> }
>> s->sftp_handle = NULL;
>> if (s->sftp) {
>> - libssh2_sftp_shutdown(s->sftp);
>> + sftp_free(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);
>> + ssh_disconnect(s->session);
>> + ssh_free(s->session);
>> }
>> s->session = NULL;
>> + if (new_sock >= 0) {
>> + close(new_sock);
>> + }
>>
>> return ret;
>> }
>> @@ -756,9 +826,11 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>>
>> ssh_state_init(s);
>>
>> - ssh_flags = LIBSSH2_FXF_READ;
>> + ssh_flags = 0;
>> if (bdrv_flags & BDRV_O_RDWR) {
>> - ssh_flags |= LIBSSH2_FXF_WRITE;
>> + ssh_flags |= O_RDWR;
>> + } else {
>> + ssh_flags |= O_RDONLY;
>> }
>>
>> opts = ssh_parse_options(options, errp);
>> @@ -773,16 +845,13 @@ static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
>> }
>>
>> /* Go non-blocking. */
>> - libssh2_session_set_blocking(s->session, 0);
>> + ssh_set_blocking(s->session, 0);
>>
>> qapi_free_BlockdevOptionsSsh(opts);
>>
>> return 0;
>>
>> err:
>> - if (s->sock >= 0) {
>> - close(s->sock);
>> - }
>> s->sock = -1;
>>
>> qapi_free_BlockdevOptionsSsh(opts);
>> @@ -795,25 +864,25 @@ static int ssh_grow_file(BDRVSSHState *s, int64_t offset, Error **errp)
>> {
>> ssize_t ret;
>> char c[1] = { '\0' };
>> - int was_blocking = libssh2_session_get_blocking(s->session);
>> + int was_blocking = ssh_is_blocking(s->session);
>>
>> /* offset must be strictly greater than the current size so we do
>> * not overwrite anything */
>> - assert(offset > 0 && offset > s->attrs.filesize);
>> + assert(offset > 0 && offset > s->attrs->size);
>>
>> - libssh2_session_set_blocking(s->session, 1);
>> + ssh_set_blocking(s->session, 1);
>>
>> - libssh2_sftp_seek64(s->sftp_handle, offset - 1);
>> - ret = libssh2_sftp_write(s->sftp_handle, c, 1);
>> + sftp_seek64(s->sftp_handle, offset - 1);
>> + ret = sftp_write(s->sftp_handle, c, 1);
>>
>> - libssh2_session_set_blocking(s->session, was_blocking);
>> + ssh_set_blocking(s->session, was_blocking);
>>
>> if (ret < 0) {
>> sftp_error_setg(errp, s, "Failed to grow file");
>> return -EIO;
>> }
>>
>> - s->attrs.filesize = offset;
>> + s->attrs->size = offset;
>> return 0;
>> }
>>
>> @@ -841,8 +910,7 @@ static int ssh_co_create(BlockdevCreateOptions *options, Error **errp)
>> ssh_state_init(&s);
>>
>> ret = connect_to_ssh(&s, opts->location,
>> - LIBSSH2_FXF_READ|LIBSSH2_FXF_WRITE|
>> - LIBSSH2_FXF_CREAT|LIBSSH2_FXF_TRUNC,
>> + O_RDWR | O_CREAT | O_TRUNC,
>> 0644, errp);
>> if (ret < 0) {
>> goto fail;
>> @@ -911,10 +979,8 @@ static int ssh_has_zero_init(BlockDriverState *bs)
>> /* Assume false, unless we can positively prove it's true. */
>> int has_zero_init = 0;
>>
>> - if (s->attrs.flags & LIBSSH2_SFTP_ATTR_PERMISSIONS) {
>> - if (s->attrs.permissions & LIBSSH2_SFTP_S_IFREG) {
>> - has_zero_init = 1;
>> - }
>> + if (s->attrs->type == SSH_FILEXFER_TYPE_REGULAR) {
>> + has_zero_init = 1;
>> }
>>
>> return has_zero_init;
>> @@ -951,12 +1017,12 @@ static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
>> .co = qemu_coroutine_self()
>> };
>>
>> - r = libssh2_session_block_directions(s->session);
>> + r = ssh_get_poll_flags(s->session);
>>
>> - if (r & LIBSSH2_SESSION_BLOCK_INBOUND) {
>> + if (r & SSH_READ_PENDING) {
>> rd_handler = restart_coroutine;
>> }
>> - if (r & LIBSSH2_SESSION_BLOCK_OUTBOUND) {
>> + if (r & SSH_WRITE_PENDING) {
>> wr_handler = restart_coroutine;
>> }
>>
>> @@ -968,33 +1034,6 @@ static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
>> trace_ssh_co_yield_back(s->sock);
>> }
>>
>> -/* SFTP has a function `libssh2_sftp_seek64' which seeks to a position
>> - * in the remote file. Notice that it just updates a field in the
>> - * sftp_handle structure, so there is no network traffic and it cannot
>> - * fail.
>> - *
>> - * However, `libssh2_sftp_seek64' does have a catastrophic effect on
>> - * performance since it causes the handle to throw away all in-flight
>> - * reads and buffered readahead data. Therefore this function tries
>> - * to be intelligent about when to call the underlying libssh2 function.
>> - */
>> -#define SSH_SEEK_WRITE 0
>> -#define SSH_SEEK_READ 1
>> -#define SSH_SEEK_FORCE 2
>> -
>> -static void ssh_seek(BDRVSSHState *s, int64_t offset, int flags)
>> -{
>> - bool op_read = (flags & SSH_SEEK_READ) != 0;
>> - bool force = (flags & SSH_SEEK_FORCE) != 0;
>> -
>> - if (force || op_read != s->offset_op_read || offset != s->offset) {
>> - trace_ssh_seek(offset);
>> - libssh2_sftp_seek64(s->sftp_handle, offset);
>> - s->offset = offset;
>> - s->offset_op_read = op_read;
>> - }
>> -}
>> -
>> static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
>> int64_t offset, size_t size,
>> QEMUIOVector *qiov)
>> @@ -1006,7 +1045,8 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
>>
>> trace_ssh_read(offset, size);
>>
>> - ssh_seek(s, offset, SSH_SEEK_READ);
>> + trace_ssh_seek(offset);
>> + sftp_seek64(s->sftp_handle, offset);
>>
>> /* This keeps track of the current iovec element ('i'), where we
>> * will write to next ('buf'), and the end of the current iovec
>> @@ -1016,35 +1056,33 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
>> buf = i->iov_base;
>> end_of_vec = i->iov_base + i->iov_len;
>>
>> - /* 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:
>
> I'd use:
>
> const ptrdiff_t request_read_size = MIN(end_of_vec - buf, 16 * KiB);
>
>> - trace_ssh_read_buf(buf, end_of_vec - buf);
>> - r = libssh2_sftp_read(s->sftp_handle, buf, end_of_vec - buf);
>> - trace_ssh_read_return(r);
>> + trace_ssh_read_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 16384));
>
> and use 'request_read_size' here,
>
>> + /* The size of SFTP packets is limited to 32K bytes, so limit
>> + * the amount of data requested to 16K, as libssh currently
>> + * does not handle multiple requests on its own:
>> + * https://red.libssh.org/issues/58
>> + */
>> + r = sftp_read(s->sftp_handle, buf, MIN(end_of_vec - buf, 16384));
>
> and here.
>
>> + trace_ssh_read_return(r, sftp_get_error(s->sftp));
>>
>> - if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
>> + if (r == SSH_AGAIN) {
>> co_yield(s, bs);
>> goto again;
>> }
>> - if (r < 0) {
>> - sftp_error_trace(s, "read");
>> - s->offset = -1;
>> - return -EIO;
>> - }
>> - if (r == 0) {
>> + if (r == SSH_EOF || (r == 0 && sftp_get_error(s->sftp) == SSH_FX_EOF)) {
>> /* EOF: Short read so pad the buffer with zeroes and return it. */
>> qemu_iovec_memset(qiov, got, 0, size - got);
>> return 0;
>> }
>> + if (r <= 0) {
>> + sftp_error_trace(s, "read");
>> + return -EIO;
>> + }
>>
>> got += r;
>> buf += r;
>> - s->offset += r;
>> if (buf >= end_of_vec && got < size) {
>> i++;
>> buf = i->iov_base;
>> @@ -1081,7 +1119,8 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
>>
>> trace_ssh_write(offset, size);
>>
>> - ssh_seek(s, offset, SSH_SEEK_WRITE);
>> + trace_ssh_seek(offset);
>> + sftp_seek64(s->sftp_handle, offset);
>>
>> /* This keeps track of the current iovec element ('i'), where we
>> * will read from next ('buf'), and the end of the current iovec
>> @@ -1093,45 +1132,34 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
>>
>> for (written = 0; written < size; ) {
>> again:
>
> Ditto 'request_write_size = MIN(..., 128 * KiB)'.
>
>> - trace_ssh_write_buf(buf, end_of_vec - buf);
>> - r = libssh2_sftp_write(s->sftp_handle, buf, end_of_vec - buf);
>> - trace_ssh_write_return(r);
>> + trace_ssh_write_buf(buf, end_of_vec - buf, MIN(end_of_vec - buf, 131072));
>> + /* Avoid too large data packets, as libssh currently does not
>> + * handle multiple requests on its own:
>> + * https://red.libssh.org/issues/58
>> + */
>> + r = sftp_write(s->sftp_handle, buf, MIN(end_of_vec - buf, 131072));
>> + trace_ssh_write_return(r, sftp_get_error(s->sftp));
>>
>> - if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
>> + if (r == SSH_AGAIN) {
>> co_yield(s, bs);
>> goto again;
>> }
>> if (r < 0) {
>> sftp_error_trace(s, "write");
>> - s->offset = -1;
>> return -EIO;
>> }
>> - /* The libssh2 API is very unclear about this. A comment in
>> - * the code says "nothing was acked, and no EAGAIN was
>> - * received!" which apparently means that no data got sent
>> - * out, and the underlying channel didn't return any EAGAIN
>> - * indication. I think this is a bug in either libssh2 or
>> - * OpenSSH (server-side). In any case, forcing a seek (to
>> - * discard libssh2 internal buffers), and then trying again
>> - * works for me.
>> - */
>> - if (r == 0) {
>> - ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
>> - co_yield(s, bs);
>> - goto again;
>> - }
>>
>> written += r;
>> buf += r;
>> - s->offset += r;
>> if (buf >= end_of_vec && written < size) {
>> i++;
>> buf = i->iov_base;
>> end_of_vec = i->iov_base + i->iov_len;
>> }
>>
>> - if (offset + written > s->attrs.filesize)
>> - s->attrs.filesize = offset + written;
>> + if (offset + written > s->attrs->size) {
>> + s->attrs->size = offset + written;
>> + }
>> }
>>
>> return 0;
>> @@ -1166,24 +1194,24 @@ static void unsafe_flush_warning(BDRVSSHState *s, const char *what)
>> }
>> }
>>
>> -#ifdef HAS_LIBSSH2_SFTP_FSYNC
>> +#if HAVE_LIBSSH_0_8
>>
>> static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
>> {
>> int r;
>>
>> trace_ssh_flush();
>> +
>> + if (!sftp_extension_supported(s->sftp, "fsync@openssh.com", "1")) {
>> + unsafe_flush_warning(s, "OpenSSH >= 6.3");
>> + return 0;
>> + }
>> again:
>> - r = libssh2_sftp_fsync(s->sftp_handle);
>> - if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
>> + r = sftp_fsync(s->sftp_handle);
>> + if (r == SSH_AGAIN) {
>> co_yield(s, bs);
>> goto again;
>> }
>> - if (r == LIBSSH2_ERROR_SFTP_PROTOCOL &&
>> - libssh2_sftp_last_error(s->sftp) == LIBSSH2_FX_OP_UNSUPPORTED) {
>> - unsafe_flush_warning(s, "OpenSSH >= 6.3");
>> - return 0;
>> - }
>> if (r < 0) {
>> sftp_error_trace(s, "fsync");
>> return -EIO;
>> @@ -1204,25 +1232,25 @@ static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
>> return ret;
>> }
>>
>> -#else /* !HAS_LIBSSH2_SFTP_FSYNC */
>> +#else /* !HAVE_LIBSSH_0_8 */
>>
>> static coroutine_fn int ssh_co_flush(BlockDriverState *bs)
>> {
>> BDRVSSHState *s = bs->opaque;
>>
>> - unsafe_flush_warning(s, "libssh2 >= 1.4.4");
>> + unsafe_flush_warning(s, "libssh >= 0.8.0");
>> return 0;
>> }
>>
>> -#endif /* !HAS_LIBSSH2_SFTP_FSYNC */
>> +#endif /* !HAVE_LIBSSH_0_8 */
>>
>> static int64_t ssh_getlength(BlockDriverState *bs)
>> {
>> BDRVSSHState *s = bs->opaque;
>> int64_t length;
>>
>> - /* Note we cannot make a libssh2 call here. */
>> - length = (int64_t) s->attrs.filesize;
>> + /* Note we cannot make a libssh call here. */
>> + length = (int64_t) s->attrs->size;
>> trace_ssh_getlength(length);
>>
>> return length;
>> @@ -1239,12 +1267,12 @@ static int coroutine_fn ssh_co_truncate(BlockDriverState *bs, int64_t offset,
>> return -ENOTSUP;
>> }
>>
>> - if (offset < s->attrs.filesize) {
>> + if (offset < s->attrs->size) {
>> error_setg(errp, "ssh driver does not support shrinking files");
>> return -ENOTSUP;
>> }
>>
>> - if (offset == s->attrs.filesize) {
>> + if (offset == s->attrs->size) {
>> return 0;
>> }
>>
>> @@ -1339,12 +1367,16 @@ static void bdrv_ssh_init(void)
>> {
>> int r;
>>
>> - r = libssh2_init(0);
>> + r = ssh_init();
>> if (r != 0) {
>> - fprintf(stderr, "libssh2 initialization failed, %d\n", r);
>> + fprintf(stderr, "libssh initialization failed, %d\n", r);
>> exit(EXIT_FAILURE);
>> }
>>
>> +#if TRACE_LIBSSH != 0
>> + ssh_set_log_level(TRACE_LIBSSH);
>> +#endif
>> +
>> bdrv_register(&bdrv_ssh);
>> }
>>
>> diff --git a/block/trace-events b/block/trace-events
>> index eab51497fc..0d2126e840 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -169,19 +169,21 @@ nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags
>> # ssh.c
>> ssh_restart_coroutine(void *co) "co=%p"
>> ssh_flush(void) "fsync"
>> -ssh_check_host_key_knownhosts(const char *key) "host key OK: %s"
>> +ssh_check_host_key_knownhosts(void) "host key OK"
>> ssh_connect_to_ssh(char *path, int flags, int mode) "opening file %s flags=0x%x creat_mode=0%o"
>> ssh_co_yield(int sock, void *rd_handler, void *wr_handler) "s->sock=%d rd_handler=%p wr_handler=%p"
>> ssh_co_yield_back(int sock) "s->sock=%d - back"
>> ssh_getlength(int64_t length) "length=%" PRIi64
>> ssh_co_create_opts(uint64_t size) "total_size=%" PRIu64
>> ssh_read(int64_t offset, size_t size) "offset=%" PRIi64 " size=%zu"
>> -ssh_read_buf(void *buf, size_t size) "sftp_read buf=%p size=%zu"
>> -ssh_read_return(ssize_t ret) "sftp_read returned %zd"
>> +ssh_read_buf(void *buf, size_t size, size_t actual_size) "sftp_read buf=%p size=%zu (actual size=%zu)"
>> +ssh_read_return(ssize_t ret, int sftp_err) "sftp_read returned %zd (sftp error=%d)"
>> ssh_write(int64_t offset, size_t size) "offset=%" PRIi64 " size=%zu"
>> -ssh_write_buf(void *buf, size_t size) "sftp_write buf=%p size=%zu"
>> -ssh_write_return(ssize_t ret) "sftp_write returned %zd"
>> +ssh_write_buf(void *buf, size_t size, size_t actual_size) "sftp_write buf=%p size=%zu (actual size=%zu)"
>> +ssh_write_return(ssize_t ret, int sftp_err) "sftp_write returned %zd (sftp error=%d)"
>> ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
>> +ssh_auth_methods(int methods) "auth methods=%x"
>
> Please use 0x prefix.
>
>> +ssh_server_status(int status) "server status=%d"
>>
>> # curl.c
>> curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
>> @@ -214,4 +216,4 @@ sheepdog_snapshot_create(const char *sn_name, const char *id) "%s %s"
>> sheepdog_snapshot_create_inode(const char *name, uint32_t snap, uint32_t vdi) "s->inode: name %s snap_id 0x%" PRIx32 " vdi 0x%" PRIx32
>>
>> # ssh.c
>> -sftp_error(const char *op, const char *ssh_err, int ssh_err_code, unsigned long sftp_err_code) "%s failed: %s (libssh2 error code: %d, sftp error code: %lu)"
>> +sftp_error(const char *op, const char *ssh_err, int ssh_err_code, int sftp_err_code) "%s failed: %s (libssh error code: %d, sftp error code: %d)"
>> diff --git a/configure b/configure
>> index b091b82cb3..bfdd70c40a 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"
>> + --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,17 @@ 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
>> - fi
>> -fi
>> -
>> -##########################################
>> -# libssh2_sftp_fsync probe
>> -
>> -if test "$libssh2" = "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;
>> -}
>> -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"
>> + libssh=no
>> fi
>> fi
>>
>> @@ -6451,7 +6425,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 +7118,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
>> diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out
>> index ec9823793a..1239d9d648 100644
>> --- a/tests/qemu-iotests/207.out
>> +++ b/tests/qemu-iotests/207.out
>> @@ -68,7 +68,7 @@ virtual size: 4 MiB (4194304 bytes)
>>
>> {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"mode": "none"}, "path": "/this/is/not/an/existing/path", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 4194304}}}
>> {"return": {}}
>> -Job failed: failed to open remote file '/this/is/not/an/existing/path': Failed opening remote file (libssh2 error code: -31)
>> +Job failed: failed to open remote file '/this/is/not/an/existing/path': SFTP server: No such file (libssh error code: 1, sftp error code: 2)
>> {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>> {"return": {}}
>>
>>
>
> Pino, this is the list of changes I did, as I commented on IRC:
>
> 1/ Build on Ubuntu:
>
> ./block/ssh.c:417:10: note: each undeclared identifier is reported only
> once for each function it appears in
> ./block/ssh.c: In function 'check_host_key_hash':
> ./block/ssh.c:440:5: error: 'ssh_get_publickey' is deprecated
> [-Werror=deprecated-declarations]
> r = ssh_get_publickey(s->session, &pubkey);
> ^
> In file included from ./block/ssh.c:27:0:
> /usr/include/libssh/libssh.h:489:31: note: declared here
> SSH_DEPRECATED LIBSSH_API int ssh_get_publickey(ssh_session session,
> ssh_key *key);
> ^~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> -- >8 --diff --git a/block/ssh.c b/block/ssh.c
> index e62f9ee3b5..cb6fe0b17d 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -408,7 +408,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
> unsigned char *server_hash;
> size_t server_hash_len;
>
> -#if HAVE_LIBSSH_0_8
> +#ifdef HAVE_SSH_GET_SERVER_PUBLICKEY
> r = ssh_get_server_publickey(s->session, &pubkey);
> #else
> r = ssh_get_publickey(s->session, &pubkey);
> diff --git a/configure b/configure
> index bfdd70c40a..93547b4958 100755
> --- a/configure
> +++ b/configure
> @@ -3928,6 +3928,18 @@ if test "$libssh" != "no" ; then
> fi
> fi
>
> +# check for ssh_get_server_publickey
> +if test "$libssh" = "yes" ; then
> + cat > $TMPC << EOF
> +#include <libssh/libssh.h>
> +int main(void) { return ssh_get_server_publickey(NULL, NULL);}
> +EOF
> + if compile_prog "$libssh_cflags" "$libssh_libs"; then
> + libssh_cflags="-DHAVE_SSH_GET_SERVER_PUBLICKEY $libssh_cflags"
> + fi
> +fi
> +
> +
> ##########################################
> # linux-aio probe---
>
> See https://www.redhat.com/archives/libvir-list/2018-May/msg00597.html
>
> 2/ Checkpatch (you already fixed the style issues)
>
> -- >8 --
> diff --git a/block/trace-events b/block/trace-events
> index 0d2126e840..cb91787bd9 100644
> @@ -182,7 +182,7 @@ ssh_write(int64_t offset, size_t size) "offset=%"
> PRIi64 " size=%zu"
> ssh_write_buf(void *buf, size_t size, size_t actual_size) "sftp_write
> buf=%p size=%zu (actual size=%zu)"
> ssh_write_return(ssize_t ret, int sftp_err) "sftp_write returned %zd
> (sftp error=%d)"
> ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
> -ssh_auth_methods(int methods) "auth methods=%x"
> +ssh_auth_methods(int methods) "auth methods=0x%04x"
> ssh_server_status(int status) "server status=%d"
> ---
>
> 3/ Travis
>
> -- >8 --
> diff --git a/.travis.yml b/.travis.yml
> index b053a836a3..a2dac8b7c9 100644
> @@ -31,7 +31,7 @@ addons:
> - libseccomp-dev
> - libspice-protocol-dev
> - libspice-server-dev
> - - libssh2-1-dev
> + - libssh-dev
> - liburcu-dev
> - libusb-1.0-0-dev
> - libvte-2.91-dev
> @@ -261,7 +261,7 @@ matrix:
> - libseccomp-dev
> - libspice-protocol-dev
> - libspice-server-dev
> - - libssh2-1-dev
> + - libssh-dev
> - liburcu-dev
> - libusb-1.0-0-dev
> - libvte-2.91-dev
> ---
>
> 4/ Docker
>
> -- >8 --
> diff --git a/tests/docker/dockerfiles/fedora.docker
> b/tests/docker/dockerfiles/fedora.docker
> index afbba29ada..8893c1b957 100644
> @@ -35,7 +35,7 @@ ENV PACKAGES \
> libpng-devel \
> librbd-devel \
> libseccomp-devel \
> - libssh2-devel \
> + libssh-devel \
> libubsan \
> libusbx-devel \
> libxml2-devel \
> @@ -50,7 +50,6 @@ ENV PACKAGES \
> mingw32-gtk3 \
> mingw32-libjpeg-turbo \
> mingw32-libpng \
> - mingw32-libssh2 \
> mingw32-libtasn1 \
> mingw32-nettle \
> mingw32-pixman \
> @@ -64,7 +63,6 @@ ENV PACKAGES \
> mingw64-gtk3 \
> mingw64-libjpeg-turbo \
> mingw64-libpng \
> - mingw64-libssh2 \
> mingw64-libtasn1 \
> mingw64-nettle \
> mingw64-pixman \
> diff --git a/tests/docker/dockerfiles/ubuntu.docker
> b/tests/docker/dockerfiles/ubuntu.docker
> index 36e2b17de5..1bbb7f9598 100644
> @@ -44,7 +44,7 @@ ENV PACKAGES flex bison \
> libsnappy-dev \
> libspice-protocol-dev \
> libspice-server-dev \
> - libssh2-1-dev \
> + libssh-dev \
> libusb-1.0-0-dev \
> libusbredirhost-dev \
> libvdeplug-dev \
> diff --git a/tests/docker/dockerfiles/ubuntu1804.docker
> b/tests/docker/dockerfiles/ubuntu1804.docker
> index 2e2900150b..9d80b11500 100644
> @@ -40,7 +40,7 @@ ENV PACKAGES flex bison \
> libsnappy-dev \
> libspice-protocol-dev \
> libspice-server-dev \
> - libssh2-1-dev \
> + libssh-dev \
> libusb-1.0-0-dev \
> libusbredirhost-dev \
> ---
>
> Note, libssh is not available on MinGW.
>
> 5/ Documentation
>
> -- >8 --
> diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
> index da06a9bc83..91ab0eceae 100644
> @@ -782,7 +782,7 @@ print a warning when @code{fsync} is not supported:
>
> warning: ssh server @code{ssh.example.com:22} does not support fsync
>
> -With sufficiently new versions of libssh2 and OpenSSH, @code{fsync} is
> +With sufficiently new versions of libssh and OpenSSH, @code{fsync} is
> supported.
>
> ---
>
> Overall looks OK.
>
> Since iotest #207 is quick, it would be nice to have it in our CI.
>
> I guess a Docker Compose setup should work, I'll see what I can do.
>
> Regards,
>
> Phil.
docker and travis parts:
Acked-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-12 13:27 ` Philippe Mathieu-Daudé
2019-06-13 13:02 ` Alex Bennée
@ 2019-06-13 19:41 ` Stefan Weil
2019-06-14 9:42 ` Kevin Wolf
2019-06-14 13:43 ` Pino Toscano
1 sibling, 2 replies; 15+ messages in thread
From: Stefan Weil @ 2019-06-13 19:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Pino Toscano, qemu-devel, qemu-block
Cc: kwolf, Daniel P. Berrange, Alex Bennée, rjones, mreitz
On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
> Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
[...]
> Note, libssh is not available on MinGW.
Nor is it available for Mingw64:
https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh&arch=x86_64
That makes building for Windows more difficult because there is an
additional dependency which must be built from source.
Regards
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-13 19:41 ` Stefan Weil
@ 2019-06-14 9:42 ` Kevin Wolf
2019-06-14 10:13 ` Philippe Mathieu-Daudé
2019-06-14 13:43 ` Pino Toscano
1 sibling, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2019-06-14 9:42 UTC (permalink / raw)
To: Stefan Weil
Cc: Daniel P. Berrange, qemu-block, qemu-devel, Alex Bennée,
rjones, mreitz, Pino Toscano, Philippe Mathieu-Daudé
Am 13.06.2019 um 21:41 hat Stefan Weil geschrieben:
> On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
> > Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
> [...]
> > Note, libssh is not available on MinGW.
>
> Nor is it available for Mingw64:
>
> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh&arch=x86_64
>
> That makes building for Windows more difficult because there is an
> additional dependency which must be built from source.
How many people do actually use the ssh block driver on Windows, though?
Isn't just building QEMU without it a quite reasonable option, too?
I wouldn't consider this a strong argument why we should keep using an
obsolete library.
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-14 9:42 ` Kevin Wolf
@ 2019-06-14 10:13 ` Philippe Mathieu-Daudé
2019-06-14 12:29 ` Stefan Weil
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-14 10:13 UTC (permalink / raw)
To: Kevin Wolf, Stefan Weil
Cc: Daniel P. Berrange, qemu-block, qemu-devel, rjones, mreitz,
Pino Toscano, Alex Bennée
On 6/14/19 11:42 AM, Kevin Wolf wrote:
> Am 13.06.2019 um 21:41 hat Stefan Weil geschrieben:
>> On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
>>> Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
>> [...]
>>> Note, libssh is not available on MinGW.
>>
>> Nor is it available for Mingw64:
Yes, by "MinGW" I mean both 32/64 implementations.
>>
>> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh&arch=x86_64
>>
>> That makes building for Windows more difficult because there is an
>> additional dependency which must be built from source.
>
> How many people do actually use the ssh block driver on Windows, though?
> Isn't just building QEMU without it a quite reasonable option, too?
>
> I wouldn't consider this a strong argument why we should keep using an
> obsolete library.
I agree with Kevin. The only user of the 'ssh' block driver that I am
aware of is the virt-v2v tool:
http://libguestfs.org/virt-v2v.1.html#convert-from-esxi-hypervisor-over-ssh-to-local-libvirt
Stefan, do you think someone would use it on a Windows host?
Thanks,
Phil.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-14 10:13 ` Philippe Mathieu-Daudé
@ 2019-06-14 12:29 ` Stefan Weil
2019-06-14 12:40 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: Stefan Weil @ 2019-06-14 12:29 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Kevin Wolf
Cc: Daniel P. Berrange, qemu-block, qemu-devel, rjones, mreitz,
Pino Toscano, Alex Bennée
On 14.06.19 12:13, Philippe Mathieu-Daudé wrote:
> I agree with Kevin. The only user of the 'ssh' block driver that I am
> aware of is the virt-v2v tool:
>
> http://libguestfs.org/virt-v2v.1.html#convert-from-esxi-hypervisor-over-ssh-to-local-libvirt
>
> Stefan, do you think someone would use it on a Windows host?
I simply don't know. Typically people contact me if something does not
work. Rarely they send an e-mail to say what they do and that everything
is fine.
If QEMU for Windows builds without libssl, I'll build binaries without
it, add that information to the release notes and wait what happens.
So no objection from my side.
Cheers
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-14 12:29 ` Stefan Weil
@ 2019-06-14 12:40 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-14 12:40 UTC (permalink / raw)
To: Stefan Weil, Kevin Wolf
Cc: Daniel P. Berrange, qemu-block, qemu-devel, rjones, mreitz,
Pino Toscano, Alex Bennée
On 6/14/19 2:29 PM, Stefan Weil wrote:
> On 14.06.19 12:13, Philippe Mathieu-Daudé wrote:
>> I agree with Kevin. The only user of the 'ssh' block driver that I am
>> aware of is the virt-v2v tool:
>>
>> http://libguestfs.org/virt-v2v.1.html#convert-from-esxi-hypervisor-over-ssh-to-local-libvirt
>>
>> Stefan, do you think someone would use it on a Windows host?
>
>
> I simply don't know. Typically people contact me if something does not
> work. Rarely they send an e-mail to say what they do and that everything
> is fine.
>
> If QEMU for Windows builds without libssl, I'll build binaries without
"libssh" ;)
> it, add that information to the release notes and wait what happens.
>
> So no objection from my side.
Glad to hear, thanks!
Pino: Can you improve the commit message to explain that QEMU only uses
libssh for the 'ssh block driver'? Maybe you (or Kevin/Max) can also add
a 1 line description of what is a 'block driver', so reviewer are not
worried that a feature might be removed.
Thanks!
Phil.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
2019-06-13 19:41 ` Stefan Weil
2019-06-14 9:42 ` Kevin Wolf
@ 2019-06-14 13:43 ` Pino Toscano
1 sibling, 0 replies; 15+ messages in thread
From: Pino Toscano @ 2019-06-14 13:43 UTC (permalink / raw)
To: Stefan Weil
Cc: kwolf, Daniel P. Berrange, qemu-block, rjones, Alex Bennée,
qemu-devel, mreitz, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 631 bytes --]
On Thursday, 13 June 2019 21:41:58 CEST Stefan Weil wrote:
> On 12.06.19 15:27, Philippe Mathieu-Daudé wrote:
> > Cc'ing Alex (Docker, Travis) and Stefan (MinGW)
> [...]
> > Note, libssh is not available on MinGW.
>
> Nor is it available for Mingw64:
>
> https://cygwin.com/cgi-bin2/package-grep.cgi?grep=mingw64-x86_64-libssh&arch=x86_64
>
> That makes building for Windows more difficult because there is an
> additional dependency which must be built from source.
Yes, sorry for that. However this can be fixed easily by creating
Mingw packages of libssh (not volunteering myself, sorry).
--
Pino Toscano
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-06-14 14:17 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-05 21:36 [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh Pino Toscano
2019-06-05 22:57 ` no-reply
2019-06-06 11:12 ` Daniel P. Berrangé
2019-06-06 17:51 ` Pino Toscano
2019-06-07 10:08 ` Daniel P. Berrangé
2019-06-07 10:14 ` Philippe Mathieu-Daudé
2019-06-07 10:24 ` Daniel P. Berrangé
2019-06-12 13:27 ` Philippe Mathieu-Daudé
2019-06-13 13:02 ` Alex Bennée
2019-06-13 19:41 ` Stefan Weil
2019-06-14 9:42 ` Kevin Wolf
2019-06-14 10:13 ` Philippe Mathieu-Daudé
2019-06-14 12:29 ` Stefan Weil
2019-06-14 12:40 ` Philippe Mathieu-Daudé
2019-06-14 13:43 ` Pino Toscano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.