Re: [Qemu-devel] [PATCH 4/4] configure: Log the libssh version detected
On Wednesday, 14 August 2019 14:15:27 CEST Philippe Mathieu-Daudé wrote:
> Log wether the version is 0.7 or 0.8 to better understand
> user reports.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> configure | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index 040aa8eb6c..d06cee0ba0 100755
> --- a/configure
> +++ b/configure
> @@ -3930,6 +3930,7 @@ if test "$libssh" != "no" ; then
>if $pkg_config --exists libssh; then
> libssh_cflags=$($pkg_config libssh --cflags)
> libssh_libs=$($pkg_config libssh --libs)
> +libssh_version=$($pkg_config libssh --modversion)
> libssh=yes
>else
> if test "$libssh" = "yes" ; then
> @@ -3960,6 +3961,9 @@ int main(void) { return ssh_get_publickey(NULL, NULL); }
> EOF
>if compile_object "$libssh_cflags -DHAVE_LIBSSH_0_8"; then
> libssh_cflags="-DHAVE_LIBSSH_0_8 $libssh_cflags"
> + else
> +# If this is not libssh 0.8, this is likely 0.7
> +libssh_version="0.7"
>fi
Not sure why this though -- please leave it out, and just log the
version as found.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH 3/4] configure: Improve checking libssh version is 0.8
On Wednesday, 14 August 2019 14:15:26 CEST Philippe Mathieu-Daudé wrote:
> To figure out which libssh version is installed, checking for
> ssh_get_server_publickey() is not sufficient.
>
> ssh_get_server_publickey() has been introduced in libssh
> commit bbd052202 (predating 0.8) but distributions also
> backported other pre-0.8 patches, such libssh commit
> 963c46e4f which introduce the ssh_known_hosts_e enum.
>
> Check the enum is available to assume the version is 0.8.
>
> This fixes build failure on Ubuntu 18.04:
>
> CC block/ssh.o
> block/ssh.c: In function 'check_host_key_knownhosts':
> block/ssh.c:281:28: error: storage size of 'state' isn't known
>enum ssh_known_hosts_e state;
> ^
> rules.mak:69: recipe for target 'block/ssh.o' failed
> make: *** [block/ssh.o] Error 1
>
> Reported-by: 周文青 <1151451...@qq.com>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838763
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> configure | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/configure b/configure
> index fe3fef9309..040aa8eb6c 100755
> --- a/configure
> +++ b/configure
> @@ -3949,18 +3949,24 @@ fi
> if test "$libssh" = "yes"; then
>cat > $TMPC < #include
> +#ifdef HAVE_LIBSSH_0_8
> +static const enum ssh_known_hosts_e val = SSH_KNOWN_HOSTS_OK;
> +#endif
> #ifdef HAVE_SSH_GET_SERVER_PUBLICKEY
> int main(void) { return ssh_get_server_publickey(NULL, NULL); }
> #else
> int main(void) { return ssh_get_publickey(NULL, NULL); }
> #endif
> EOF
> - if compile_object "$libssh_cflags"; then
> + if compile_object "$libssh_cflags -DHAVE_LIBSSH_0_8"; then
> libssh_cflags="-DHAVE_LIBSSH_0_8 $libssh_cflags"
>fi
>if compile_object "$libssh_cflags -DHAVE_SSH_GET_SERVER_PUBLICKEY"; then
> libssh_cflags="-DHAVE_SSH_GET_SERVER_PUBLICKEY $libssh_cflags"
>fi
> + if ! compile_object "$libssh_cflags"; then
> +error_exit "cannot use with libssh (is it broken?)"
> + fi
Ugh no, this is way more twisted and complex than really needed.
Instead, just add another build time check for
ssh_session_is_known_server, and change check_host_key_knownhosts to
use it only when found.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [Qemu-devel] [RFC PATCH 1/4] configure: Improve libssh check
On Wednesday, 14 August 2019 14:15:24 CEST Philippe Mathieu-Daudé wrote: > The libssh pkg-config is not complete, the libraries required to > link with libssh are not returned. For example on Ubuntu 18.04: > > $ dpkg -l|fgrep libssh > ii libssh-4:arm64 0.8.0~20170825.94fa1e38-1ubuntu0.2 arm64 tiny C SSH > library (OpenSSL flavor) > ii libssh-dev 0.8.0~20170825.94fa1e38-1ubuntu0.2 arm64 tiny C SSH library. > Development files (OpenSSL flavor) > > $ pkg-config libssh --libs > -lssh > > Since the ./configure script tries to link an object to figure if > libssh is available, it fails: > > $ cat config.log > [...] > cc -pthread -I/usr/include/glib-2.0 [...] -o config-temp/qemu-conf.exe > config-temp/qemu-conf.c -m64 -static -g -lssh > /usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/libssh.a(dh.c.o): > In function `ssh_crypto_init': > (.text+0x1a9): undefined reference to `BN_new' > (.text+0x1c2): undefined reference to `BN_set_word' > (.text+0x1c7): undefined reference to `BN_new' > (.text+0x1e7): undefined reference to `BN_bin2bn' > (.text+0x1ec): undefined reference to `BN_new' > (.text+0x20c): undefined reference to `BN_bin2bn' > (.text+0x218): undefined reference to `OPENSSL_init_crypto' > [...] > collect2: error: ld returned 1 exit status Sigh :/ > --- > Should we check for libcrypto? > > $ pkg-config --libs libssh openssl > -lssh -lssl -lcrypto Definitely not! The crypto library is an internal implementation of libssh, so please do not assume libssh is built against openssl. -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH 2/4] configure: Avoid using libssh deprecated API
On Wednesday, 14 August 2019 14:15:25 CEST Philippe Mathieu-Daudé wrote:
> The libssh packaged by a distribution can predate version 0.8,
> but still provides the newer API introduced after version 0.7.
>
> Using the deprecated API leads to build failure, as on Ubuntu 18.04:
>
> CC block/ssh.o
> block/ssh.c: In function 'check_host_key_hash':
> block/ssh.c:444:5: error: 'ssh_get_publickey' is deprecated
> [-Werror=deprecated-declarations]
>r = ssh_get_publickey(s->session, );
>^
> 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);
> ^
> rules.mak:69: recipe for target 'block/ssh.o' failed
> make: *** [block/ssh.o] Error 1
>
> Fix by using the newer API if available.
>
> Suggested-by: Andrea Bolognani
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> block/ssh.c | 2 +-
> configure | 7 +++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/ssh.c b/block/ssh.c
> index 501933b855..f5fea921c6 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -438,7 +438,7 @@ check_host_key_hash(BDRVSSHState *s, const char *hash,
> unsigned char *server_hash;
> size_t server_hash_len;
>
> -#ifdef HAVE_LIBSSH_0_8
> +#ifdef HAVE_SSH_GET_SERVER_PUBLICKEY
> r = ssh_get_server_publickey(s->session, );
> #else
> r = ssh_get_publickey(s->session, );
> diff --git a/configure b/configure
> index 1d5c07de1f..fe3fef9309 100755
> --- a/configure
> +++ b/configure
> @@ -3949,11 +3949,18 @@ fi
> if test "$libssh" = "yes"; then
>cat > $TMPC < #include
> +#ifdef HAVE_SSH_GET_SERVER_PUBLICKEY
> int main(void) { return ssh_get_server_publickey(NULL, NULL); }
> +#else
> +int main(void) { return ssh_get_publickey(NULL, NULL); }
> +#endif
> EOF
>if compile_object "$libssh_cflags"; then
> libssh_cflags="-DHAVE_LIBSSH_0_8 $libssh_cflags"
>fi
> + if compile_object "$libssh_cflags -DHAVE_SSH_GET_SERVER_PUBLICKEY"; then
> +libssh_cflags="-DHAVE_SSH_GET_SERVER_PUBLICKEY $libssh_cflags"
> + fi
Why try to compile it twice? If the check for ssh_get_server_publickey
works, then it is available...
Just add an additional HAVE_SSH_GET_SERVER_PUBLICKEY define when this
test succeeds, and change the usage of ssh_get_server_publickey based
on this.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH 2/2] ssh: implement private key authentication
On Monday, 29 July 2019 12:57:40 CEST Markus Armbruster wrote: > Pino Toscano writes: > > > On Friday, 26 July 2019 16:24:34 CEST Eric Blake wrote: > >> On 7/26/19 9:09 AM, Pino Toscano wrote: > >> > Add a 'private-key' option which represents the path of a private key > >> > to use for authentication, and 'private-key-secret' as the name of an > >> > object with its passphrase. > >> > > >> > Signed-off-by: Pino Toscano > >> > >> > +++ b/qapi/block-core.json > >> > @@ -3226,6 +3226,11 @@ > >> > # @password-secret: ID of a QCryptoSecret object providing a > >> > password > >> > # for authentication (since 4.2) > >> > # > >> > +# @private-key: path to the private key (since 4.2) > >> > +# > >> > +# @private-key-secret: ID of a QCryptoSecret object providing the > >> > passphrase > >> > +# for 'private-key' (since 4.2) > >> > >> Is password-secret intended to be mutually-exclusive with > >> private-key/private-key-secret? > > > > My initial thought was to allow users to specify data for all the > > authentication methods possible. Either ways (all of them, or a single > > one) are fine for me. > > How does this work at the libssh level? Can you configure multiple > authentication methods, and let negotiation pick the one to be used? The remote servers sends all the authentication methods supported, and the user of libssh can decide which one(s) to attempt. See for example the small tutorial in the libssh documentation: http://api.libssh.org/stable/libssh_tutor_authentication.html -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH 2/2] ssh: implement private key authentication
On Friday, 26 July 2019 16:24:34 CEST Eric Blake wrote: > On 7/26/19 9:09 AM, Pino Toscano wrote: > > Add a 'private-key' option which represents the path of a private key > > to use for authentication, and 'private-key-secret' as the name of an > > object with its passphrase. > > > > Signed-off-by: Pino Toscano > > > +++ b/qapi/block-core.json > > @@ -3226,6 +3226,11 @@ > > # @password-secret: ID of a QCryptoSecret object providing a password > > # for authentication (since 4.2) > > # > > +# @private-key: path to the private key (since 4.2) > > +# > > +# @private-key-secret: ID of a QCryptoSecret object providing the > > passphrase > > +# for 'private-key' (since 4.2) > > Is password-secret intended to be mutually-exclusive with > private-key/private-key-secret? My initial thought was to allow users to specify data for all the authentication methods possible. Either ways (all of them, or a single one) are fine for me. -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
On Friday, 26 July 2019 16:27:11 CEST Richard W.M. Jones wrote:
> On Fri, Jul 26, 2019 at 04:09:52PM +0200, Pino Toscano wrote:
> > These two patches add the password and private key authentication
> > methods to the ssh block driver, using secure objects for
> > passwords/passphrases.
>
> I was attempting to test this but couldn't work out the full command
> line to use it (with qemu-img). I got as far as:
>
> $ ./qemu-img convert -p 'json:{ "file.driver": "ssh", "file.host": "devr7",
> "file.path": "/var/tmp/root", "file.password-secret": "..." }' /var/tmp/root
>
> I guess the secret should be specified using --object, but at that
> point I gave up.
Almost there :) add e.g.
--object 'secret,id=sec0,file=passwd'
as parameter for the convert command (so after it, not before), and then
set 'sec0' as value for file.password-secret. Of course 'sec0' is
arbitrary, any other QEMU id will do.
A long helpful comment in include/crypto/secret.h explains the basics
of the crypto objects.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
[Qemu-devel] [PATCH 2/2] ssh: implement private key authentication
Add a 'private-key' option which represents the path of a private key
to use for authentication, and 'private-key-secret' as the name of an
object with its passphrase.
Signed-off-by: Pino Toscano
---
block/ssh.c | 98
block/trace-events | 1 +
docs/qemu-block-drivers.texi | 12 -
qapi/block-core.json | 9 +++-
4 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 04ae223282..1b7c1f4108 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -500,6 +500,89 @@ static int check_host_key(BDRVSSHState *s, SshHostKeyCheck
*hkc, Error **errp)
return -EINVAL;
}
+static int authenticate_privkey(BDRVSSHState *s, BlockdevOptionsSsh *opts,
+Error **errp)
+{
+int err;
+int ret;
+char *pubkey_file = NULL;
+ssh_key public_key = NULL;
+ssh_key private_key = NULL;
+char *passphrase;
+
+pubkey_file = g_strdup_printf("%s.pub", opts->private_key);
+
+/* load the private key */
+trace_ssh_auth_key_passphrase(opts->private_key_secret, opts->private_key);
+passphrase = qcrypto_secret_lookup_as_utf8(opts->private_key_secret, errp);
+if (!passphrase) {
+err = SSH_AUTH_ERROR;
+goto error;
+}
+ret = ssh_pki_import_privkey_file(opts->private_key, passphrase,
+ NULL, NULL, _key);
+g_free(passphrase);
+if (ret == SSH_EOF) {
+error_setg(errp, "Cannot read private key '%s'", opts->private_key);
+err = SSH_AUTH_ERROR;
+goto error;
+} else if (ret == SSH_ERROR) {
+error_setg(errp,
+ "Cannot open private key '%s', maybe the passphrase is "
+ "wrong",
+ opts->private_key);
+err = SSH_AUTH_ERROR;
+goto error;
+}
+
+/* try to open the public part of the private key */
+ret = ssh_pki_import_pubkey_file(pubkey_file, _key);
+if (ret == SSH_ERROR) {
+error_setg(errp, "Cannot read public key '%s'", pubkey_file);
+err = SSH_AUTH_ERROR;
+goto error;
+} else if (ret == SSH_EOF) {
+/* create the public key from the private key */
+ret = ssh_pki_export_privkey_to_pubkey(private_key, _key);
+if (ret == SSH_ERROR) {
+error_setg(errp,
+ "Cannot export the public key from the private key "
+ "'%s'",
+ opts->private_key);
+err = SSH_AUTH_ERROR;
+goto error;
+}
+}
+
+ret = ssh_userauth_try_publickey(s->session, NULL, public_key);
+if (ret != SSH_AUTH_SUCCESS) {
+err = SSH_AUTH_DENIED;
+goto error;
+}
+
+ret = ssh_userauth_publickey(s->session, NULL, private_key);
+if (ret != SSH_AUTH_SUCCESS) {
+err = SSH_AUTH_DENIED;
+goto error;
+}
+
+ssh_key_free(private_key);
+ssh_key_free(public_key);
+g_free(pubkey_file);
+
+return SSH_AUTH_SUCCESS;
+
+ error:
+if (private_key) {
+ssh_key_free(private_key);
+}
+if (public_key) {
+ssh_key_free(public_key);
+}
+g_free(pubkey_file);
+return err;
+}
+
static int authenticate(BDRVSSHState *s, BlockdevOptionsSsh *opts,
Error **errp)
{
@@ -538,6 +621,21 @@ static int authenticate(BDRVSSHState *s,
BlockdevOptionsSsh *opts,
ret = 0;
goto out;
}
+
+/*
+ * Try to authenticate with private key, if available.
+ */
+if (opts->has_private_key && opts->has_private_key_secret) {
+r = authenticate_privkey(s, opts, errp);
+if (r == SSH_AUTH_ERROR) {
+ret = -EINVAL;
+goto out;
+} else if (r == SSH_AUTH_SUCCESS) {
+/* Authenticated! */
+ret = 0;
+goto out;
+}
+}
}
/*
diff --git a/block/trace-events b/block/trace-events
index 391aae03e6..ccb51b9992 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -187,6 +187,7 @@ ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
ssh_auth_methods(int methods) "auth methods=0x%x"
ssh_server_status(int status) "server status=%d"
ssh_option_secret_object(const char *path) "using password from object %s"
+ssh_auth_key_passphrase(const char *path, const char *key) "using passphrase
from object %s for private key %s"
# curl.c
curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
index c77ef2dd69..5513bf261c 100644
--- a/docs/qemu-block-drivers.texi
+++ b/docs/qemu-block-drivers.texi
@@ -774,8 +774,16 @@ tools only use MD5 to print fingerpri
[Qemu-devel] [PATCH 1/2] ssh: implement password authentication
Add a 'password-secret' option which represents the name of an object
with the password of the user.
Signed-off-by: Pino Toscano
---
block/ssh.c | 35 ---
block/trace-events | 1 +
docs/qemu-block-drivers.texi | 7 +--
qapi/block-core.json | 6 +-
tests/qemu-iotests/207.out | 2 +-
5 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/block/ssh.c b/block/ssh.c
index 501933b855..04ae223282 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -43,6 +43,7 @@
#include "qapi/qmp/qstring.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
+#include "crypto/secret.h"
#include "trace.h"
/*
@@ -499,7 +500,8 @@ static int check_host_key(BDRVSSHState *s, SshHostKeyCheck
*hkc, Error **errp)
return -EINVAL;
}
-static int authenticate(BDRVSSHState *s, Error **errp)
+static int authenticate(BDRVSSHState *s, BlockdevOptionsSsh *opts,
+Error **errp)
{
int r, ret;
int method;
@@ -538,9 +540,35 @@ static int authenticate(BDRVSSHState *s, Error **errp)
}
}
+/*
+ * Try to authenticate with password, if available.
+ */
+if (method & SSH_AUTH_METHOD_PASSWORD && opts->has_password_secret) {
+char *password;
+
+trace_ssh_option_secret_object(opts->password_secret);
+password = qcrypto_secret_lookup_as_utf8(opts->password_secret, errp);
+if (!password) {
+ret = -EINVAL;
+goto out;
+}
+r = ssh_userauth_password(s->session, NULL, password);
+g_free(password);
+if (r == SSH_AUTH_ERROR) {
+ret = -EINVAL;
+session_error_setg(errp, s, "failed to authenticate using "
+"password authentication");
+goto out;
+} else if (r == SSH_AUTH_SUCCESS) {
+/* Authenticated! */
+ret = 0;
+goto out;
+}
+}
+
ret = -EPERM;
error_setg(errp, "failed to authenticate using publickey authentication "
- "and the identities held by your ssh-agent");
+ "and the identities held by your ssh-agent, or using password");
out:
return ret;
@@ -785,7 +813,7 @@ static int connect_to_ssh(BDRVSSHState *s,
BlockdevOptionsSsh *opts,
}
/* Authenticate. */
-ret = authenticate(s, errp);
+ret = authenticate(s, opts, errp);
if (ret < 0) {
goto err;
}
@@ -1376,6 +1404,7 @@ static const char *const ssh_strong_runtime_opts[] = {
"user",
"host_key_check",
"server.",
+"password-secret",
NULL
};
diff --git a/block/trace-events b/block/trace-events
index d724df0117..391aae03e6 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -186,6 +186,7 @@ ssh_write_return(ssize_t ret, int sftp_err) "sftp_write
returned %zd (sftp error
ssh_seek(int64_t offset) "seeking to offset=%" PRIi64
ssh_auth_methods(int methods) "auth methods=0x%x"
ssh_server_status(int status) "server status=%d"
+ssh_option_secret_object(const char *path) "using password from object %s"
# curl.c
curl_timer_cb(long timeout_ms) "timer callback timeout_ms %ld"
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
index 91ab0eceae..c77ef2dd69 100644
--- a/docs/qemu-block-drivers.texi
+++ b/docs/qemu-block-drivers.texi
@@ -771,8 +771,11 @@ matches a specific fingerprint:
(@code{sha1:} can also be used as a prefix, but note that OpenSSH
tools only use MD5 to print fingerprints).
-Currently authentication must be done using ssh-agent. Other
-authentication methods may be supported in future.
+The optional @var{password-secret} parameter provides the ID of a
+@code{secret} object that contains the password for authenticating.
+
+Currently authentication must be done using ssh-agent, or providing a
+password. Other authentication methods may be supported in future.
Note: Many ssh servers do not support an @code{fsync}-style operation.
The ssh driver cannot guarantee that disk flush requests are
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..1244562c7b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3223,13 +3223,17 @@
# @host-key-check: Defines how and what to check the host key against
# (default: known_hosts)
#
+# @password-secret: ID of a QCryptoSecret object providing a password
+# for authentication (since 4.2)
+#
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsSsh',
'data': { 'server': 'InetSocketAddress',
'path': 'str',
'*user': 'str',
-'*host-key-check': 'SshHostKeyCheck' } }
+'*host-key
[Qemu-devel] [PATCH 0/2] ssh: add password and privkey auth methods
These two patches add the password and private key authentication methods to the ssh block driver, using secure objects for passwords/passphrases. Pino Toscano (2): ssh: implement password authentication ssh: implement private key authentication block/ssh.c | 133 ++- block/trace-events | 2 + docs/qemu-block-drivers.texi | 15 +++- qapi/block-core.json | 13 +++- tests/qemu-iotests/207.out | 2 +- 5 files changed, 158 insertions(+), 7 deletions(-) -- 2.21.0
Re: [Qemu-devel] [PULL 0/8] Block patches
On Monday, 24 June 2019 14:20:11 CEST Max Reitz wrote:
> On 23.06.19 19:18, Peter Maydell wrote:
> > On Fri, 21 Jun 2019 at 14:23, Max Reitz wrote:
> >>
> >> The following changes since commit
> >> 33d609990621dea6c7d056c86f707b8811320ac1:
> >>
> >> Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into
> >> staging (2019-06-18 17:00:52 +0100)
> >>
> >> are available in the Git repository at:
> >>
> >> https://github.com/XanClic/qemu.git tags/pull-block-2019-06-21
> >>
> >> for you to fetch changes up to e2a76186f7948b8b75d1b2b52638de7c2f7f7472:
> >>
> >> iotests: Fix 205 for concurrent runs (2019-06-21 14:40:28 +0200)
> >>
> >>
> >> Block patches:
> >> - The SSH block driver now uses libssh instead of libssh2
> >> - The VMDK block driver gets read-only support for the seSparse
> >> subformat
> >> - Various fixes
> >>
> >
> > Hi; this failed to build on my s390 box:
> >
> > /home/linux1/qemu/block/ssh.c: In function ‘check_host_key_knownhosts’:
> > /home/linux1/qemu/block/ssh.c:367:27: error: implicit declaration of
> > function ‘ssh_get_fingerprint_hash’
> > [-Werror=implicit-function-declaration]
> > fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
> >^
> > /home/linux1/qemu/block/ssh.c:367:13: error: nested extern declaration
> > of ‘ssh_get_fingerprint_hash’ [-Werror=nested-externs]
> > fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
> > ^
> > /home/linux1/qemu/block/ssh.c:367:25: error: assignment makes pointer
> > from integer without a cast [-Werror=int-conversion]
> > fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
> > ^
> >
> > It looks like that function was introduced in libssh 0.8.3, and this box
> > has 0.6.3. (configure has correctly not defined HAVE_LIBSSH_0_8
> > but this usage is inside a bit of code that's compiled even when
> > that is not defined.)
Oops, sorry, I did not test the latest versions with that old libssh.
> Pino, would you be OK with dropping that piece of code for pre-0.8 and
> just replacing it with the else-error_setg()?
Some the variables in check_host_key_knownhosts must be moved within
the HAVE_LIBSSH_0_8 block now; attached fixup patch, please squash with
my patch (I can submit a v12, if needed/wanted).
--
Pino Toscanodiff --git a/block/ssh.c b/block/ssh.c
index 048d0cc924..501933b855 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -277,14 +277,14 @@ static void ssh_parse_filename(const char *filename, QDict *options,
static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
{
int ret;
+#ifdef HAVE_LIBSSH_0_8
+enum ssh_known_hosts_e state;
int r;
ssh_key pubkey;
enum ssh_keytypes_e pubkey_type;
unsigned char *server_hash = NULL;
size_t server_hash_len;
char *fingerprint = NULL;
-#ifdef HAVE_LIBSSH_0_8
-enum ssh_known_hosts_e state;
state = ssh_session_is_known_server(s->session);
trace_ssh_server_status(state);
@@ -356,30 +356,9 @@ static int check_host_key_knownhosts(BDRVSSHState *s, Error **errp)
break;
case SSH_SERVER_KNOWN_CHANGED:
ret = -EINVAL;
-r = ssh_get_publickey(s->session, );
-if (r == 0) {
-r = ssh_get_publickey_hash(pubkey, SSH_PUBLICKEY_HASH_SHA1,
- _hash, _hash_len);
-pubkey_type = ssh_key_type(pubkey);
-ssh_key_free(pubkey);
-}
-if (r == 0) {
-fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
- server_hash,
- server_hash_len);
-ssh_clean_pubkey_hash(_hash);
-}
-if (fingerprint) {
-error_setg(errp,
- "host key (%s key with fingerprint %s) does not match "
- "the one in known_hosts; this may be a possible attack",
- ssh_key_type_to_char(pubkey_type), fingerprint);
-ssh_string_free_char(fingerprint);
-} else {
-error_setg(errp,
- "host key does not match the one in known_hosts; this "
- "may be a possible attack");
-}
+error_setg(errp,
+ "host key does not match the one in known_hosts; this "
+ "may be a possible attack");
goto out;
case SSH_SERVER_FOUND_OTHER:
ret = -EINVAL;
signature.asc
Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH v10] ssh: switch from libssh2 to libssh
On Thursday, 20 June 2019 14:58:40 CEST Max Reitz wrote: > On 20.06.19 11:49, Pino Toscano wrote: > > On Tuesday, 18 June 2019 15:14:30 CEST Max Reitz wrote: > >> On 18.06.19 11:24, Pino Toscano wrote: > >>> Rewrite the implementation of the ssh block driver to use libssh instead > >>> of libssh2. The libssh library has various advantages over libssh2: > >>> - easier API for authentication (for example for using ssh-agent) > >>> - easier API for known_hosts handling > >>> - supports newer types of keys in known_hosts > >>> > >>> Use APIs/features available in libssh 0.8 conditionally, to support > >>> older versions (which are not recommended though). > >>> > >>> Adjust the tests according to the different error message, and to the > >>> newer host keys (ed25519) that are used by default with OpenSSH >= 6.7 > >>> and libssh >= 0.7.0. > >>> > >>> Adjust the various Docker/Travis scripts to use libssh when available > >>> instead of libssh2. The mingw/mxe testing is dropped for now, as there > >>> are no packages for it. > >>> > >>> Signed-off-by: Pino Toscano > >>> Tested-by: Philippe Mathieu-Daudé > >>> Acked-by: Alex Bennée > >>> --- > >>> > >>> Changes from v9: > >>> - restored "default" case in the server status switch for libssh < 0.8.0 > >>> - print the host key type & fingerprint on mismatch with known_hosts > >>> - improve/fix message for failed socket_set_nodelay() > >>> - reset s->sock properly > >>> > >>> Changes from v8: > >>> - use a newer key type in iotest 207 > >>> - improve the commit message > >>> > >>> Changes from v7: > >>> - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8 > >>> - ptrdiff_t -> size_t > >>> > >>> Changes from v6: > >>> - fixed few checkpatch style issues > >>> - detect libssh 0.8 via symbol detection > >>> - adjust travis/docker test material > >>> - remove dead "default" case in a switch > >>> - use variables for storing MIN() results > >>> - adapt a documentation bit > >>> > >>> Changes from v5: > >>> - adapt to newer tracing APIs > >>> - disable ssh compression (mimic what libssh2 does by default) > >>> - use build time checks for libssh 0.8, and use newer APIs directly > >>> > >>> Changes from v4: > >>> - fix wrong usages of error_setg/session_error_setg/sftp_error_setg > >>> - fix few return code checks > >>> - remove now-unused parameters in few internal functions > >>> - allow authentication with "none" method > >>> - switch to unsigned int for the port number > >>> - enable TCP_NODELAY on the socket > >>> - fix one reference error message in iotest 207 > >>> > >>> Changes from v3: > >>> - fix socket cleanup in connect_to_ssh() > >>> - add comments about the socket cleanup > >>> - improve the error reporting (closer to what was with libssh2) > >>> - improve EOF detection on sftp_read() > >>> > >>> Changes from v2: > >>> - used again an own fd > >>> - fixed co_yield() implementation > >>> > >>> Changes from v1: > >>> - fixed jumbo packets writing > >>> - fixed missing 'err' assignment > >>> - fixed commit message > >>> > >>> .travis.yml | 4 +- > >>> block/Makefile.objs | 6 +- > >>> block/ssh.c | 665 ++ > >>> block/trace-events| 14 +- > >>> configure | 65 +- > >>> docs/qemu-block-drivers.texi | 2 +- > >>> .../dockerfiles/debian-win32-cross.docker | 1 - > >>> .../dockerfiles/debian-win64-cross.docker | 1 - > >>> tests/docker/dockerfiles/fedora.docker| 4 +- > >>> tests/docker/dockerfiles/ubuntu.docker| 2 +- > >>> tests/docker/dockerfiles/ubuntu1804.docker| 2 +- > >>> tests/qemu-iotests/207| 4 +- > >>> tests/qemu-iotests/207.out| 2 +- > >>> 13 files changed, 4
[Qemu-devel] [PATCH v11] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead of libssh2. The libssh library has various advantages over libssh2: - easier API for authentication (for example for using ssh-agent) - easier API for known_hosts handling - supports newer types of keys in known_hosts Use APIs/features available in libssh 0.8 conditionally, to support older versions (which are not recommended though). Adjust the iotest 207 according to the different error message, and to find the default key type for localhost (to properly compare the fingerprint with). Contributed-by: Max Reitz Adjust the various Docker/Travis scripts to use libssh when available instead of libssh2. The mingw/mxe testing is dropped for now, as there are no packages for it. Signed-off-by: Pino Toscano Tested-by: Philippe Mathieu-Daudé Acked-by: Alex Bennée --- Changes from v10: - improve error message for key mismatch - integrate Max Reitz' fix to iotest 207 to detect the key type used by localhost Changes from v9: - restored "default" case in the server status switch for libssh < 0.8.0 - print the host key type & fingerprint on mismatch with known_hosts - improve/fix message for failed socket_set_nodelay() - reset s->sock properly Changes from v8: - use a newer key type in iotest 207 - improve the commit message Changes from v7: - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8 - ptrdiff_t -> size_t Changes from v6: - fixed few checkpatch style issues - detect libssh 0.8 via symbol detection - adjust travis/docker test material - remove dead "default" case in a switch - use variables for storing MIN() results - adapt a documentation bit Changes from v5: - adapt to newer tracing APIs - disable ssh compression (mimic what libssh2 does by default) - use build time checks for libssh 0.8, and use newer APIs directly Changes from v4: - fix wrong usages of error_setg/session_error_setg/sftp_error_setg - fix few return code checks - remove now-unused parameters in few internal functions - allow authentication with "none" method - switch to unsigned int for the port number - enable TCP_NODELAY on the socket - fix one reference error message in iotest 207 Changes from v3: - fix socket cleanup in connect_to_ssh() - add comments about the socket cleanup - improve the error reporting (closer to what was with libssh2) - improve EOF detection on sftp_read() Changes from v2: - used again an own fd - fixed co_yield() implementation Changes from v1: - fixed jumbo packets writing - fixed missing 'err' assignment - fixed commit message .travis.yml | 4 +- block/Makefile.objs | 6 +- block/ssh.c | 669 ++ block/trace-events| 14 +- configure | 65 +- docs/qemu-block-drivers.texi | 2 +- .../dockerfiles/debian-win32-cross.docker | 1 - .../dockerfiles/debian-win64-cross.docker | 1 - tests/docker/dockerfiles/fedora.docker| 4 +- tests/docker/dockerfiles/ubuntu.docker| 2 +- tests/docker/dockerfiles/ubuntu1804.docker| 2 +- tests/qemu-iotests/207| 54 +- tests/qemu-iotests/207.out| 2 +- 13 files changed, 468 insertions(+), 358 deletions(-) diff --git a/.travis.yml b/.travis.yml index aeb9b211cd..279658b116 100644 --- a/.travis.yml +++ b/.travis.yml @@ -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 @@ -270,7 +270,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 diff --git a/block/Makefile.objs b/block/Makefile.objs index dbd1522722..35f3bca4d9 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/ss
Re: [Qemu-devel] [PATCH v10] ssh: switch from libssh2 to libssh
On Tuesday, 18 June 2019 15:14:30 CEST Max Reitz wrote:
> On 18.06.19 11:24, Pino Toscano wrote:
> > Rewrite the implementation of the ssh block driver to use libssh instead
> > of libssh2. The libssh library has various advantages over libssh2:
> > - easier API for authentication (for example for using ssh-agent)
> > - easier API for known_hosts handling
> > - supports newer types of keys in known_hosts
> >
> > Use APIs/features available in libssh 0.8 conditionally, to support
> > older versions (which are not recommended though).
> >
> > Adjust the tests according to the different error message, and to the
> > newer host keys (ed25519) that are used by default with OpenSSH >= 6.7
> > and libssh >= 0.7.0.
> >
> > Adjust the various Docker/Travis scripts to use libssh when available
> > instead of libssh2. The mingw/mxe testing is dropped for now, as there
> > are no packages for it.
> >
> > Signed-off-by: Pino Toscano
> > Tested-by: Philippe Mathieu-Daudé
> > Acked-by: Alex Bennée
> > ---
> >
> > Changes from v9:
> > - restored "default" case in the server status switch for libssh < 0.8.0
> > - print the host key type & fingerprint on mismatch with known_hosts
> > - improve/fix message for failed socket_set_nodelay()
> > - reset s->sock properly
> >
> > Changes from v8:
> > - use a newer key type in iotest 207
> > - improve the commit message
> >
> > Changes from v7:
> > - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8
> > - ptrdiff_t -> size_t
> >
> > Changes from v6:
> > - fixed few checkpatch style issues
> > - detect libssh 0.8 via symbol detection
> > - adjust travis/docker test material
> > - remove dead "default" case in a switch
> > - use variables for storing MIN() results
> > - adapt a documentation bit
> >
> > Changes from v5:
> > - adapt to newer tracing APIs
> > - disable ssh compression (mimic what libssh2 does by default)
> > - use build time checks for libssh 0.8, and use newer APIs directly
> >
> > Changes from v4:
> > - fix wrong usages of error_setg/session_error_setg/sftp_error_setg
> > - fix few return code checks
> > - remove now-unused parameters in few internal functions
> > - allow authentication with "none" method
> > - switch to unsigned int for the port number
> > - enable TCP_NODELAY on the socket
> > - fix one reference error message in iotest 207
> >
> > Changes from v3:
> > - fix socket cleanup in connect_to_ssh()
> > - add comments about the socket cleanup
> > - improve the error reporting (closer to what was with libssh2)
> > - improve EOF detection on sftp_read()
> >
> > Changes from v2:
> > - used again an own fd
> > - fixed co_yield() implementation
> >
> > Changes from v1:
> > - fixed jumbo packets writing
> > - fixed missing 'err' assignment
> > - fixed commit message
> >
> > .travis.yml | 4 +-
> > block/Makefile.objs | 6 +-
> > block/ssh.c | 665 ++
> > block/trace-events| 14 +-
> > configure | 65 +-
> > docs/qemu-block-drivers.texi | 2 +-
> > .../dockerfiles/debian-win32-cross.docker | 1 -
> > .../dockerfiles/debian-win64-cross.docker | 1 -
> > tests/docker/dockerfiles/fedora.docker| 4 +-
> > tests/docker/dockerfiles/ubuntu.docker| 2 +-
> > tests/docker/dockerfiles/ubuntu1804.docker| 2 +-
> > tests/qemu-iotests/207| 4 +-
> > tests/qemu-iotests/207.out| 2 +-
> > 13 files changed, 423 insertions(+), 349 deletions(-)
>
> [...]
>
> > diff --git a/block/ssh.c b/block/ssh.c
> > index 6da7b9cbfe..644ae8b82c 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
>
> [...]
>
> > +case SSH_SERVER_KNOWN_CHANGED:
> > +ret = -EINVAL;
> > +r = ssh_get_publickey(s->session, );
> > +if (r == 0) {
> > +r = ssh_get_publickey_hash(pubkey, SSH_PUBLICKEY_HASH_SHA1,
> > + _hash, _hash_len);
> > +pubkey_type = ssh_key_type(pubkey);
> > +ssh_key_free(pubkey);
> > +}
> > +if (r == 0) {
> > +fingerprint = ssh_get_fingerprint_hash(SSH_PUBLICKEY_HASH_SHA1,
> > +
[Qemu-devel] [PATCH v10] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead of libssh2. The libssh library has various advantages over libssh2: - easier API for authentication (for example for using ssh-agent) - easier API for known_hosts handling - supports newer types of keys in known_hosts Use APIs/features available in libssh 0.8 conditionally, to support older versions (which are not recommended though). Adjust the tests according to the different error message, and to the newer host keys (ed25519) that are used by default with OpenSSH >= 6.7 and libssh >= 0.7.0. Adjust the various Docker/Travis scripts to use libssh when available instead of libssh2. The mingw/mxe testing is dropped for now, as there are no packages for it. Signed-off-by: Pino Toscano Tested-by: Philippe Mathieu-Daudé Acked-by: Alex Bennée --- Changes from v9: - restored "default" case in the server status switch for libssh < 0.8.0 - print the host key type & fingerprint on mismatch with known_hosts - improve/fix message for failed socket_set_nodelay() - reset s->sock properly Changes from v8: - use a newer key type in iotest 207 - improve the commit message Changes from v7: - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8 - ptrdiff_t -> size_t Changes from v6: - fixed few checkpatch style issues - detect libssh 0.8 via symbol detection - adjust travis/docker test material - remove dead "default" case in a switch - use variables for storing MIN() results - adapt a documentation bit Changes from v5: - adapt to newer tracing APIs - disable ssh compression (mimic what libssh2 does by default) - use build time checks for libssh 0.8, and use newer APIs directly Changes from v4: - fix wrong usages of error_setg/session_error_setg/sftp_error_setg - fix few return code checks - remove now-unused parameters in few internal functions - allow authentication with "none" method - switch to unsigned int for the port number - enable TCP_NODELAY on the socket - fix one reference error message in iotest 207 Changes from v3: - fix socket cleanup in connect_to_ssh() - add comments about the socket cleanup - improve the error reporting (closer to what was with libssh2) - improve EOF detection on sftp_read() Changes from v2: - used again an own fd - fixed co_yield() implementation Changes from v1: - fixed jumbo packets writing - fixed missing 'err' assignment - fixed commit message .travis.yml | 4 +- block/Makefile.objs | 6 +- block/ssh.c | 665 ++ block/trace-events| 14 +- configure | 65 +- docs/qemu-block-drivers.texi | 2 +- .../dockerfiles/debian-win32-cross.docker | 1 - .../dockerfiles/debian-win64-cross.docker | 1 - tests/docker/dockerfiles/fedora.docker| 4 +- tests/docker/dockerfiles/ubuntu.docker| 2 +- tests/docker/dockerfiles/ubuntu1804.docker| 2 +- tests/qemu-iotests/207| 4 +- tests/qemu-iotests/207.out| 2 +- 13 files changed, 423 insertions(+), 349 deletions(-) diff --git a/.travis.yml b/.travis.yml index 08502c0aa2..75f47df3d2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -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 @@ -268,7 +268,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 diff --git a/block/Makefile.objs b/block/Makefile.objs index dbd1522722..35f3bca4d9 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 6da7b9cbfe..644ae8b82c 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -24,8 +24,8 @@ #include "qemu/osdep.h" -#include -
Re: [Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh
On Thursday, 13 June 2019 21:31:40 CEST Max Reitz wrote:
> On 13.06.19 15:20, Pino Toscano wrote:
> > -switch (r) {
> > -case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> > +switch (state) {
> > +case SSH_KNOWN_HOSTS_OK:
> > /* OK */
> > -trace_ssh_check_host_key_knownhosts(found->key);
> > +trace_ssh_check_host_key_knownhosts();
> > break;
> > -case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> > +case SSH_KNOWN_HOSTS_CHANGED:
> > 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 does not match the one in known_hosts");
>
> So what about the possible attack warning that the specification
> technically requires us to print? O:-)
There is the API since libssh 0.8.0... which unfortunately is not
usable, as they forgot to properly export the symbol :-(
>
> > goto out;
> > -case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
> > +case SSH_KNOWN_HOSTS_OTHER:
> > ret = -EINVAL;
> > -session_error_setg(errp, s, "no host key was found in
> > known_hosts");
> > +error_setg(errp,
> > + "host key for this server not found, another type
> > exists");
> > goto out;
> > -case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
> > +case SSH_KNOWN_HOSTS_UNKNOWN:
> > ret = -EINVAL;
> > -session_error_setg(errp, s,
> > - "failure matching the host key with known_hosts");
> > +error_setg(errp, "no host key was found in known_hosts");
> > +goto out;
> > +case SSH_KNOWN_HOSTS_NOT_FOUND:
> > +ret = -ENOENT;
> > +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;
> > -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;
> > }
> > +#else /* !HAVE_LIBSSH_0_8 */
> > +int state;
> > +
> > +state = ssh_is_server_known(s->session);
> > +trace_ssh_server_status(state);
> > +
> > +switch (state) {
> > +case SSH_SERVER_KNOWN_OK:
> > +/* OK */
> > +trace_ssh_check_host_key_knownhosts();
> > +break;
> > +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;
> > +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 SSH_SERVER_NOT_KNOWN:
> > +ret = -EINVAL;
> > +error_setg(errp, "no host key was found in known_hosts");
> > +goto out;
> > +case SSH_SERVER_ERROR:
> > +ret = -EINVAL;
> > +error_setg(errp, "server error");
> > +goto out;
>
> No default here?
This switch is for libssh < 0.8.0, so enumerating all the possible
values of the enum of the old API is enough.
> > @@ -775,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;
>
> Shouldn’t connect_to_ssh() set this to -1 after closing the session?
It should, although it will not make a difference. connect_to_ssh() is
used in only two places:
- in ssh_file_open, and s-
Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
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=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 signature.asc Description: This is a digitally signed message part.
[Qemu-devel] [PATCH v9] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead of libssh2. The libssh library has various advantages over libssh2: - easier API for authentication (for example for using ssh-agent) - easier API for known_hosts handling - supports newer types of keys in known_hosts Use APIs/features available in libssh 0.8 conditionally, to support older versions (which are not recommended though). Adjust the tests according to the different error message, and to the newer host keys (ed25519) that are used by default with OpenSSH >= 6.7 and libssh >= 0.7.0. Adjust the various Docker/Travis scripts to use libssh when available instead of libssh2. The mingw/mxe testing is dropped for now, as there are no packages for it. Signed-off-by: Pino Toscano Tested-by: Philippe Mathieu-Daudé Acked-by: Alex Bennée --- Changes from v8: - use a newer key type in iotest 207 - improve the commit message Changes from v7: - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8 - ptrdiff_t -> size_t Changes from v6: - fixed few checkpatch style issues - detect libssh 0.8 via symbol detection - adjust travis/docker test material - remove dead "default" case in a switch - use variables for storing MIN() results - adapt a documentation bit Changes from v5: - adapt to newer tracing APIs - disable ssh compression (mimic what libssh2 does by default) - use build time checks for libssh 0.8, and use newer APIs directly Changes from v4: - fix wrong usages of error_setg/session_error_setg/sftp_error_setg - fix few return code checks - remove now-unused parameters in few internal functions - allow authentication with "none" method - switch to unsigned int for the port number - enable TCP_NODELAY on the socket - fix one reference error message in iotest 207 Changes from v3: - fix socket cleanup in connect_to_ssh() - add comments about the socket cleanup - improve the error reporting (closer to what was with libssh2) - improve EOF detection on sftp_read() Changes from v2: - used again an own fd - fixed co_yield() implementation Changes from v1: - fixed jumbo packets writing - fixed missing 'err' assignment - fixed commit message .travis.yml | 4 +- block/Makefile.objs | 6 +- block/ssh.c | 622 +- block/trace-events| 14 +- configure | 65 +- docs/qemu-block-drivers.texi | 2 +- .../dockerfiles/debian-win32-cross.docker | 1 - .../dockerfiles/debian-win64-cross.docker | 1 - tests/docker/dockerfiles/fedora.docker| 4 +- tests/docker/dockerfiles/ubuntu.docker| 2 +- tests/docker/dockerfiles/ubuntu1804.docker| 2 +- tests/qemu-iotests/207| 4 +- tests/qemu-iotests/207.out| 2 +- 13 files changed, 376 insertions(+), 353 deletions(-) diff --git a/.travis.yml b/.travis.yml index 08502c0aa2..75f47df3d2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -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 @@ -268,7 +268,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 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 6da7b9cbfe..fb458d4548 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -24,8 +24,8 @@ #include "qemu/osdep.h" -#include -#include +#include +#include #include "block/block_int.h" #include "block/qdict.h" @@ -46,13 +46,11 @@ #include "trace.h" /* - * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note - * that this requires that libssh2
[Qemu-devel] [PATCH v8] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead of libssh2. The libssh library has various advantages over libssh2: - easier API for authentication (for example for using ssh-agent) - easier API for known_hosts handling - supports newer types of keys in known_hosts Use APIs/features available in libssh 0.8 conditionally, to support older versions (which are not recommended though). Adjust the various Docker/Travis scripts to use libssh when available instead of libssh2. Signed-off-by: Pino Toscano --- Changes from v7: - #if HAVE_LIBSSH_0_8 -> #ifdef HAVE_LIBSSH_0_8 - ptrdiff_t -> size_t Changes from v6: - fixed few checkpatch style issues - detect libssh 0.8 via symbol detection - adjust travis/docker test material - remove dead "default" case in a switch - use variables for storing MIN() results - adapt a documentation bit Changes from v5: - adapt to newer tracing APIs - disable ssh compression (mimic what libssh2 does by default) - use build time checks for libssh 0.8, and use newer APIs directly Changes from v4: - fix wrong usages of error_setg/session_error_setg/sftp_error_setg - fix few return code checks - remove now-unused parameters in few internal functions - allow authentication with "none" method - switch to unsigned int for the port number - enable TCP_NODELAY on the socket - fix one reference error message in iotest 207 Changes from v3: - fix socket cleanup in connect_to_ssh() - add comments about the socket cleanup - improve the error reporting (closer to what was with libssh2) - improve EOF detection on sftp_read() Changes from v2: - used again an own fd - fixed co_yield() implementation Changes from v1: - fixed jumbo packets writing - fixed missing 'err' assignment - fixed commit message .travis.yml | 4 +- block/Makefile.objs | 6 +- block/ssh.c | 622 +- block/trace-events| 14 +- configure | 65 +- docs/qemu-block-drivers.texi | 2 +- .../dockerfiles/debian-win32-cross.docker | 1 - .../dockerfiles/debian-win64-cross.docker | 1 - tests/docker/dockerfiles/fedora.docker| 4 +- tests/docker/dockerfiles/ubuntu.docker| 2 +- tests/docker/dockerfiles/ubuntu1804.docker| 2 +- tests/qemu-iotests/207.out| 2 +- 12 files changed, 374 insertions(+), 351 deletions(-) diff --git a/.travis.yml b/.travis.yml index a08a7b7278..c70dd055ed 100644 --- a/.travis.yml +++ b/.travis.yml @@ -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 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 6da7b9cbfe..fb458d4548 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -24,8 +24,8 @@ #include "qemu/osdep.h" -#include -#include +#include +#include #include "block/block_int.h" #include "block/qdict.h" @@ -46,13 +46,11 @@ #include "trace.h" /* - * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note - * that this requires that libssh2 was specially compiled with the - * `./configure --enable-debug' option, so most likely you will have - * to compile it yourself. The meaning of is described - * here: http://www.libssh2.org/libssh2_trace.html + * TRACE_LIBSSH= enables tracing in libssh itself. + * The meaning of is described here: + * http://api.libssh.org/master/group__libssh__log.html */ -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */ +#define T
[Qemu-devel] [PATCH v7] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead
of libssh2. The libssh library has various advantages over libssh2:
- easier API for authentication (for example for using ssh-agent)
- easier API for known_hosts handling
- supports newer types of keys in known_hosts
Use APIs/features available in libssh 0.8 conditionally, to support
older versions (which are not recommended though).
Adjust the various Docker/Travis scripts to use libssh when available
instead of libssh2.
Signed-off-by: Pino Toscano
---
Changes from v6:
- fixed few checkpatch style issues
- detect libssh 0.8 via symbol detection
- adjust travis/docker test material
- remove dead "default" case in a switch
- use variables for storing MIN() results
- adapt a documentation bit
Changes from v5:
- adapt to newer tracing APIs
- disable ssh compression (mimic what libssh2 does by default)
- use build time checks for libssh 0.8, and use newer APIs directly
Changes from v4:
- fix wrong usages of error_setg/session_error_setg/sftp_error_setg
- fix few return code checks
- remove now-unused parameters in few internal functions
- allow authentication with "none" method
- switch to unsigned int for the port number
- enable TCP_NODELAY on the socket
- fix one reference error message in iotest 207
Changes from v3:
- fix socket cleanup in connect_to_ssh()
- add comments about the socket cleanup
- improve the error reporting (closer to what was with libssh2)
- improve EOF detection on sftp_read()
Changes from v2:
- used again an own fd
- fixed co_yield() implementation
Changes from v1:
- fixed jumbo packets writing
- fixed missing 'err' assignment
- fixed commit message
.travis.yml | 4 +-
block/Makefile.objs | 6 +-
block/ssh.c | 622 +-
block/trace-events| 14 +-
configure | 65 +-
docs/qemu-block-drivers.texi | 2 +-
.../dockerfiles/debian-win32-cross.docker | 1 -
.../dockerfiles/debian-win64-cross.docker | 1 -
tests/docker/dockerfiles/fedora.docker| 4 +-
tests/docker/dockerfiles/ubuntu.docker| 2 +-
tests/docker/dockerfiles/ubuntu1804.docker| 2 +-
tests/qemu-iotests/207.out| 2 +-
12 files changed, 374 insertions(+), 351 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index b053a836a3..a2dac8b7c9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -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
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..13768fad98 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include
-#include
+#include
+#include
#include "block/block_int.h"
#include "block/qdict.h"
@@ -44,13 +44,11 @@
#include "trace.h"
/*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
typedef struct BDRVSSHState {
/* Coroutine. */
@@
Re: [Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
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 > > --- > > > > 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 signature.asc Description: This is a digitally signed message part.
[Qemu-devel] [PATCH v6] ssh: switch from libssh2 to libssh
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
---
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
-#include
+#include
+#include
#include "block/block_int.h"
#include "block/qdict.h"
@@ -43,14 +43,13 @@
#include "qapi/qobject-output-visitor.h"
#include "trace.h"
-/*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+/* TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
-#define 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(>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) {
-libssh
[Qemu-devel] [PATCH v5] ssh: switch from libssh2 to libssh
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
Kerberos authentication can be enabled once the libssh bug for it [1] is
fixed.
The development version of libssh (i.e. the future 0.8.x) supports
fsync, so reuse the build time check for this.
[1] https://red.libssh.org/issues/242
Signed-off-by: Pino Toscano
---
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| 566 ++---
configure | 65 +++--
tests/qemu-iotests/207.out | 2 +-
4 files changed, 307 insertions(+), 332 deletions(-)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 899bfb5e2c..9c3b3bfb99 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -21,7 +21,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
@@ -42,8 +42,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-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
diff --git a/block/ssh.c b/block/ssh.c
index da7bbf73e2..787245230a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include
-#include
+#include
+#include
#include "block/block_int.h"
#include "block/qdict.h"
@@ -45,14 +45,12 @@
/* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
* this block driver code.
*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
#define DEBUG_SSH 0
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
#define DPRINTF(fmt, ...) \
do {\
@@ -68,18 +66,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;
@@ -91,27 +85,25 @@ static void ssh_state_init(BDRVSSHState *s)
{
memset(s, 0, sizeof *s);
s->sock = -1;
-s->offset = -1;
qemu_co_mutex_init(>lock);
}
static void ssh_state_free(BDRVSSHState *s)
{
+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_shutdow
Re: [Qemu-devel] [PATCH v4] ssh: switch from libssh2 to libssh
On Tuesday, 13 February 2018 19:49:12 CET Max Reitz wrote:
> On 2018-01-18 17:44, 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
> >
> > Kerberos authentication can be enabled once the libssh buxg for it [1] is
> > fixed.
> >
> > The development version of libssh (i.e. the future 0.8.x) supports
> > fsync, so reuse the build time check for this.
> >
> > [1] https://red.libssh.org/issues/242
> >
> > Signed-off-by: Pino Toscano
> > ---
Sorry for the (very late) reply.
I fixed basically all the code issues noted with this review; follow
few replies/notes that are not just "fixed".
> > 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
>
> One thing: The performance seems to have dropped hugely, from what I can
> tell.
>
> Before this patch, running the iotests 1-10 over ssh (raw/ssh) took
> 12.6 s. With this patch, they take 59.3 s. Perhaps the starkest
> contrast can be seen in test 1, which took 4 s before and 27 s after --
> this test simply reads and writes 128 MB of continuous data.
>
> I like having elliptic curves, but I think this patch needs optimization
> work before we can replace libssh2.
One thing that I discovered helping a lot is setting the TCP_NODELAY
option on the socket, to disable the Nagle algorithm; this drastically
reduced the overhead, which now does not seem to be more than 200% on
the very intensive tests (at least with my benchmarks).
Also using libssh from master shows more improvements too (and a bit
more of instability though, but that's a different story), and the
resulting overhead seems more acceptable to me now.
>
> > block/Makefile.objs | 6 +-
> > block/ssh.c | 522
> >
> > configure | 65 ---
> > 3 files changed, 278 insertions(+), 315 deletions(-)
>
> [...]
>
> > diff --git a/block/ssh.c b/block/ssh.c
> > index b049a16eb9..2975fc27d8 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
>
> [...]
>
> > @@ -87,27 +81,25 @@ static void ssh_state_init(BDRVSSHState *s)
> > {
> > memset(s, 0, sizeof *s);
> > s->sock = -1;
> > -s->offset = -1;
> > qemu_co_mutex_init(>lock);
> > }
> >
> > static void ssh_state_free(BDRVSSHState *s)
> > {
> > +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 free's it. */
>
> s/free's/frees/
>
> > }
> >
> > static void GCC_FMT_ATTR(3, 4)
> > @@ -121,13 +113,13 @@ session_error_setg(Error **errp, BDRVSSHState *s,
> > const char *fs, ...)
> > va_end(args);
> >
> > if (s->session) {
> > -char *ssh_err;
> > +const char *ssh_err;
> > int ssh_err_code;
> >
> > -/* This is not an errno. See . */
> > -ssh_err_code = libssh2_session_last_error(s->session,
> > - _err, NULL, 0);
> > -error_setg(errp, "%s: %s (libssh2 error code: %d)",
> &
Re: [Qemu-devel] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file
On Wednesday, 21 March 2018 14:45:38 CET Eric Blake wrote: > [adding qemu lists] > > On 03/21/2018 07:51 AM, Richard W.M. Jones wrote: > > On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote: > >> Newer versions of qemu use file locking for the images by default, and > >> apparently that does not work with /dev/null. Since this test just > >> calls qemu-img to get the format of an empty image, create a temporary > >> one instead. > > > > ACK, but feels like this is a bug in qemu-img to me. > > You're right that file locking on a character device like /dev/null is > not going to work as expected, but is it a case where fcntl() actually > fails, or is it worse where the fcntl() claiming the locks "succeeds" > but doesn't do what we want? That is, what were the actual error > messages you ran into? $ qemu-img --version qemu-img version 2.10.1(qemu-2.10.1-2.fc27) Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers $ qemu-img info /dev/null qemu-img: Could not open '/dev/null': Failed to get "consistent read" lock Is another process using the image? -- Pino Toscano signature.asc Description: This is a digitally signed message part.
[Qemu-devel] [PATCH v4] ssh: switch from libssh2 to libssh
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
Kerberos authentication can be enabled once the libssh bug for it [1] is
fixed.
The development version of libssh (i.e. the future 0.8.x) supports
fsync, so reuse the build time check for this.
[1] https://red.libssh.org/issues/242
Signed-off-by: Pino Toscano <ptosc...@redhat.com>
---
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 | 522
configure | 65 ---
3 files changed, 278 insertions(+), 315 deletions(-)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 6eaf78a046..c0df21d0b4 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -20,7 +20,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
@@ -41,8 +41,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-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
diff --git a/block/ssh.c b/block/ssh.c
index b049a16eb9..2975fc27d8 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include
-#include
+#include
+#include
#include "block/block_int.h"
#include "qapi/error.h"
@@ -41,14 +41,12 @@
/* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
* this block driver code.
*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
#define DEBUG_SSH 0
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
#define DPRINTF(fmt, ...) \
do {\
@@ -64,18 +62,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;
@@ -87,27 +81,25 @@ static void ssh_state_init(BDRVSSHState *s)
{
memset(s, 0, sizeof *s);
s->sock = -1;
-s->offset = -1;
qemu_co_mutex_init(>lock);
}
static void ssh_state_free(BDRVSSHState *s)
{
+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->
Re: [Qemu-devel] [PATCH v3] ssh: switch from libssh2 to libssh
On Monday, 18 December 2017 20:55:19 CET Jeff Cody wrote:
> On Wed, Nov 15, 2017 at 05:26:48PM +0100, 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
> >
> > Kerberos authentication can be enabled once the libssh bug for it [1] is
> > fixed.
> >
> > The development version of libssh (i.e. the future 0.8.x) supports
> > fsync, so reuse the build time check for this.
> >
> > [1] https://red.libssh.org/issues/242
> >
> > Signed-off-by: Pino Toscano <ptosc...@redhat.com>
> > ---
> >
> > 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 | 494
> >
> > configure | 65 ---
> > 3 files changed, 259 insertions(+), 306 deletions(-)
> >
> > diff --git a/block/Makefile.objs b/block/Makefile.objs
> > index 6eaf78a046..c0df21d0b4 100644
> > --- a/block/Makefile.objs
> > +++ b/block/Makefile.objs
> > @@ -20,7 +20,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
> > @@ -41,8 +41,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-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
> > dmg-bz2.o-libs := $(BZIP2_LIBS)
> > qcow.o-libs:= -lz
> > diff --git a/block/ssh.c b/block/ssh.c
> > index b049a16eb9..9e96c9d684 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
> > @@ -24,8 +24,8 @@
> >
> > #include "qemu/osdep.h"
> >
> > -#include
> > -#include
> > +#include
> > +#include
> >
> > #include "block/block_int.h"
> > #include "qapi/error.h"
> > @@ -41,14 +41,12 @@
> > /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
> > * this block driver code.
> > *
> > - * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
> > - * that this requires that libssh2 was specially compiled with the
> > - * `./configure --enable-debug' option, so most likely you will have
> > - * to compile it yourself. The meaning of is described
> > - * here: http://www.libssh2.org/libssh2_trace.html
> > + * TRACE_LIBSSH= enables tracing in libssh itself.
> > + * The meaning of is described here:
> > + * http://api.libssh.org/master/group__libssh__log.html
> > */
> > #define DEBUG_SSH 0
> > -#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
> > +#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
> >
> > #define DPRINTF(fmt, ...) \
> > do {\
> > @@ -64,18 +62,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 t
[Qemu-devel] [PATCH v3] ssh: switch from libssh2 to libssh
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
Kerberos authentication can be enabled once the libssh bug for it [1] is
fixed.
The development version of libssh (i.e. the future 0.8.x) supports
fsync, so reuse the build time check for this.
[1] https://red.libssh.org/issues/242
Signed-off-by: Pino Toscano <ptosc...@redhat.com>
---
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 | 494
configure | 65 ---
3 files changed, 259 insertions(+), 306 deletions(-)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 6eaf78a046..c0df21d0b4 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -20,7 +20,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
@@ -41,8 +41,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-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
qcow.o-libs:= -lz
diff --git a/block/ssh.c b/block/ssh.c
index b049a16eb9..9e96c9d684 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include
-#include
+#include
+#include
#include "block/block_int.h"
#include "qapi/error.h"
@@ -41,14 +41,12 @@
/* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
* this block driver code.
*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
#define DEBUG_SSH 0
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
#define DPRINTF(fmt, ...) \
do {\
@@ -64,18 +62,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;
@@ -87,26 +81,23 @@ static void ssh_state_init(BDRVSSHState *s)
{
memset(s, 0, sizeof *s);
s->sock = -1;
-s->offset = -1;
qemu_co_mutex_init(>lock);
}
static void ssh_state_free(BDRVSSHState *s)
{
+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);
}
}
@@ -121,13 +112,13 @@ session_error_setg(Error **errp, BDRVSSHState *s, const
char *fs, ...)
va_en
Re: [Qemu-devel] [PATCH for-2.8] po: add missing translations in de, fr, it, zh
On Wednesday, 14 December 2016 14:47:13 CET Stefan Hajnoczi wrote: > There are missing translations for the new "Copy" menu item. > > The following people provided them to me on IRC just in time for the > QEMU 2.8 release: > > * de_DE - Stefan Hajnoczi <stefa...@redhat.com> > * fr_FR - Laurent Vivier <laur...@vivier.eu> > * it- Pino Toscano <ptosc...@redhat.com> > * zh_CN - Fam Zheng <f...@redhat.com> > > Reported-by: Kevin Wolf <kw...@redhat.com> > Cc: Fam Zheng <f...@redhat.com> > Cc: Pino Toscano <ptosc...@redhat.com> > Cc: Laurent Vivier <laur...@vivier.eu> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > [...] > diff --git a/po/it.po b/po/it.po > index 8eadf2f..bfae84e 100644 > --- a/po/it.po > +++ b/po/it.po > @@ -42,11 +42,11 @@ msgstr "_Esci" > > #: ui/gtk.c:2029 > msgid "_Fullscreen" > -msgstr "" > +msgstr "A t_utto schermo" > > #: ui/gtk.c:2032 > msgid "_Copy" > -msgstr "" > +msgstr "_Copia" > > #: ui/gtk.c:2048 > msgid "Zoom _In" ACK. Should PO-Revision-Date be updated as well? Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH] qapi: fix memory leak in QmpOutputVisitor
On Friday, 21 October 2016 16:20:30 CEST Eric Blake wrote: > On 10/18/2016 06:22 AM, Pino Toscano wrote: > > On Tuesday, 18 October 2016 06:13:30 CEST Eric Blake wrote: > >> On 10/18/2016 04:17 AM, Pino Toscano wrote: > >>> qmp_output_start_struct() and qmp_output_start_list() create a new > >>> QObject (QDict, QList) and push it to the stack of the QmpOutputVisitor, > >>> where it is saved as 'value'. When freeing the iterator in > >>> qmp_output_free(), these values are never freed properly. > >> > >> Do any of the tests (perhaps run under valgrind) show this leak? If not, > >> maybe we should enhance their coverage. > > > > Running a simple `qemu-img info file.qcow2` under valgrind was enough > > for me to show the leak. > > I'm still not reproducing it. :( Funny, now I cannot either, not even by resetting master back to the day when I did the patches. And I'm pretty sure that it was an issue, since doing only this patch did not fully fix the leak: valgrind runs were fine, so a full run of the libguestfs test suite with the rebuild qemu as hypervisor. > > In this case, another simple fix is needed to fully fix the leak: > > http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html > > In fact, isn't that fix alone enough to fix the leak? The more I think > about this patch (and the thread on v2), the more I think it is too > prone to double-freeing things. I agree on the "this is not needed part", so let's just drop this patch. Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH v2] ssh: switch from libssh2 to libssh
On Friday, 21 October 2016 12:25:40 CEST Daniel P. Berrange wrote: > On Fri, Oct 21, 2016 at 01:16:11PM +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 > > > > Kerberos authentication can be enabled once the libssh bug for it [1] is > > fixed. > > IIUC from the code this relies on QEMU being able to talk to an ssh > agent to do public key auth. Is there a way to directly provide the > passphase for the private key (avoiding need for an agent), or to > provide a plani password to libssh ? Yes, both are supported by libssh. > If so, you could use the QEMU 'secret' object type to provide these > passphrases & passwords to QEMU, which can in turn pass them to > libssh. > > Avoiding the need for ssh agent in this way would make it possible > to use this driver with libvirt in more circumstances. > > eg for plain passwords you could do > > $QEMU -object secret,id=sec0,data=mypassword > -drive driver=ssh,,password-secret=sec0 > > while for private key passphrases > > $QEMU -object secret,id=sec0,data=mypassphrase > -drive driver=ssh,,key-passphrase-secret=sec0 > > > No need to do this all as part of this patch though - it'd be cleaner to > do this as a separate patch Right, good idea. Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH v2] ssh: switch from libssh2 to libssh
On Friday, 21 October 2016 13:02:21 CEST Richard W.M. Jones wrote: > On Fri, Oct 21, 2016 at 01:16:11PM +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 > > > > Kerberos authentication can be enabled once the libssh bug for it [1] is > > fixed. > > > > The development version of libssh (i.e. the future 0.8.x) supports > > fsync, so reuse the build time check for this. > > > > [1] https://red.libssh.org/issues/242 > > > > Signed-off-by: Pino Toscano <ptosc...@redhat.com> > > --- > > > > Changes from v1: > > - fixed jumbo packets writing > > - fixed missing 'err' assignment > > - fixed commit message > > This version works, but I also switched from using a remote server to > using this over localhost. Could you please give it a try with the remote server case as well? > It seems as if the timeout might be a bit short. Could that be made > controllable? Or increased to match whatever libssh2 was using? Which timeout are you referring to? Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
[Qemu-devel] [PATCH v2] ssh: switch from libssh2 to libssh
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
Kerberos authentication can be enabled once the libssh bug for it [1] is
fixed.
The development version of libssh (i.e. the future 0.8.x) supports
fsync, so reuse the build time check for this.
[1] https://red.libssh.org/issues/242
Signed-off-by: Pino Toscano <ptosc...@redhat.com>
---
Changes from v1:
- fixed jumbo packets writing
- fixed missing 'err' assignment
- fixed commit message
block/Makefile.objs | 6 +-
block/ssh.c | 551 +---
configure | 65 +++
3 files changed, 256 insertions(+), 366 deletions(-)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 67a036a..602a182 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -19,7 +19,7 @@ block-obj-$(CONFIG_CURL) += curl.o
block-obj-$(CONFIG_RBD) += rbd.o
block-obj-$(CONFIG_GLUSTERFS) += gluster.o
block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
-block-obj-$(CONFIG_LIBSSH2) += ssh.o
+block-obj-$(CONFIG_LIBSSH) += ssh.o
block-obj-y += accounting.o dirty-bitmap.o
block-obj-y += write-threshold.o
block-obj-y += backup.o
@@ -38,8 +38,8 @@ rbd.o-cflags := $(RBD_CFLAGS)
rbd.o-libs := $(RBD_LIBS)
gluster.o-cflags := $(GLUSTERFS_CFLAGS)
gluster.o-libs := $(GLUSTERFS_LIBS)
-ssh.o-cflags := $(LIBSSH2_CFLAGS)
-ssh.o-libs := $(LIBSSH2_LIBS)
+ssh.o-cflags := $(LIBSSH_CFLAGS)
+ssh.o-libs := $(LIBSSH_LIBS)
archipelago.o-libs := $(ARCHIPELAGO_LIBS)
block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
diff --git a/block/ssh.c b/block/ssh.c
index 5ce12b6..9f6c1db 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include
-#include
+#include
+#include
#include "block/block_int.h"
#include "qapi/error.h"
@@ -38,14 +38,12 @@
/* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
* this block driver code.
*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
#define DEBUG_SSH 0
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
#define DPRINTF(fmt, ...) \
do {\
@@ -60,50 +58,39 @@ typedef struct BDRVSSHState {
CoMutex lock;
/* SSH connection. */
-int sock; /* socket */
-LIBSSH2_SESSION *session; /* ssh session */
-LIBSSH2_SFTP *sftp; /* sftp session */
-LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */
+ssh_session session; /* ssh session */
+sftp_session sftp;/* sftp session */
+sftp_file sftp_handle;/* sftp remote file handle */
-/* See ssh_seek() function below. */
-int64_t offset;
-bool offset_op_read;
-
-/* File attributes at open. We try to keep the .filesize field
+/* File attributes at open. We try to keep the .size field
* updated if it changes (eg by writing at the end of the file).
*/
-LIBSSH2_SFTP_ATTRIBUTES attrs;
+sftp_attributes attrs;
/* Used to warn if 'flush' is not supported. */
-char *hostport;
bool unsafe_flush_warning;
} BDRVSSHState;
static void ssh_state_init(BDRVSSHState *s)
{
memset(s, 0, sizeof *s);
-s->sock = -1;
-s->offset = -1;
qemu_co_mutex_init(>lock);
}
static void ssh_state_free(BDRVSSHState *s)
{
-g_free(s->hostport);
+if (s->attrs) {
+sftp_attributes_free(s->attrs);
+}
if (s->sftp_handle) {
-libssh2_sftp_close(s->sftp_handle);
+sftp_close(s->sftp_handle);
}
if (s->sftp) {
-libssh2_sftp_shutdown(s->sftp);
+sftp_free(s->sftp);
}
if (s->session) {
-libssh2_session_disconnect(s->session,
- "from qemu ssh client: "
- "user closed the connection");
-libssh2_session_free(s->session);
-}
-if (s->sock >= 0) {
-close(s->sock);
+ssh_disconnect(s->session);
+ssh_free(s->session);
}
}
@@ -118,13 +105,13 @@ session_error_setg(Err
Re: [Qemu-devel] [PATCH] ssh: switch from libssh2 to libssh
On Thursday, 20 October 2016 18:04:34 CEST Stefan Weil wrote: > Am 20.10.2016 um 17:15 schrieb Pino Toscano: > > Rewrite the implementation of the ssh block driver to use libssh instead > > of libssh. The libssh library has various advantages over libssh: > > libssh instead of libssh? In both sentences a "2" is missing. Right, they should be "... instead of libssh2" and "advantages over libssh2" -- fixed locally. > Cygwin does not provide libssh for Mingw-w64, see > https://cygwin.com/cgi-bin2/package-grep.cgi?grep=libssh=x86_64 I guess I could ask them for these versions once this patch is approved (so there's an existing use case). -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH] ssh: switch from libssh2 to libssh
On Thursday, 20 October 2016 16:32:50 CEST Richard W.M. Jones wrote:
> On Thu, Oct 20, 2016 at 05:15:24PM +0200, Pino Toscano wrote:
> > Rewrite the implementation of the ssh block driver to use libssh instead
> > of libssh. The libssh library has various advantages over libssh:
> > - easier API for authentication (for example for using ssh-agent)
> > - easier API for known_hosts handling
> > - supports newer types of keys in known_hosts
> >
> > Kerberos authentication can be enabled once the libssh bug for it [1] is
> > fixed.
> >
> > The development version of libssh (i.e. the future 0.8.x) supports
> > fsync, so reuse the build time check for this.
> >
> > [1] https://red.libssh.org/issues/242
> >
> > Signed-off-by: Pino Toscano <ptosc...@redhat.com>
>
>
> When I applied this patch and compiled it with warnings enabled:
>
> block/ssh.c: In function ‘connect_to_ssh’:
> block/ssh.c:643:12: warning: ‘ret’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> return ret;
> ^~~
Interesting, there was no warning for me. Anyway, I think this:
diff --git a/block/ssh.c b/block/ssh.c
index 7c316db..7ff376e 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -519,6 +519,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
/* Create SSH session. */
s->session = ssh_new();
if (!s->session) {
+ret = -EINVAL;
goto err;
}
should fix it (added already locally).
> To test the patch, I used virt-builder to create a virtual machine
> disk image on another machine (accessible over ssh). Then from my
> laptop I ran:
>
> ./x86_64-softmmu/qemu-system-x86_64 -nodefconfig \
> -M accel=kvm -cpu host -m 2048 \
> -drive
> file.driver=ssh,file.user=[user],file.host=[host],file.path=/var/tmp/fedora-24.img,format=raw,if=virtio
> \
> -serial stdio
>
> Unfortunately this failed with a large number of sftp errors:
>
> read failed: (sftp error code: 0)
>
> and subsequently hung. So I'm afraid I couldn't test the patch at all :-(
Can you please enable the logging of the ssh driver, and libssh own
logging too? Basically (see lines 45-46) set:
#define DEBUG_SSH 1
#define TRACE_LIBSSH 4
> Also fsync was not supported for me, but I'm using 0.7.3 and the code
> says I need 0.8.0.
Yes, this is correct.
Thanks,
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
[Qemu-devel] [PATCH] ssh: switch from libssh2 to libssh
Rewrite the implementation of the ssh block driver to use libssh instead
of libssh. The libssh library has various advantages over libssh:
- easier API for authentication (for example for using ssh-agent)
- easier API for known_hosts handling
- supports newer types of keys in known_hosts
Kerberos authentication can be enabled once the libssh bug for it [1] is
fixed.
The development version of libssh (i.e. the future 0.8.x) supports
fsync, so reuse the build time check for this.
[1] https://red.libssh.org/issues/242
Signed-off-by: Pino Toscano <ptosc...@redhat.com>
---
block/Makefile.objs | 6 +-
block/ssh.c | 543 +---
configure | 65 ---
3 files changed, 249 insertions(+), 365 deletions(-)
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 67a036a..602a182 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -19,7 +19,7 @@ block-obj-$(CONFIG_CURL) += curl.o
block-obj-$(CONFIG_RBD) += rbd.o
block-obj-$(CONFIG_GLUSTERFS) += gluster.o
block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
-block-obj-$(CONFIG_LIBSSH2) += ssh.o
+block-obj-$(CONFIG_LIBSSH) += ssh.o
block-obj-y += accounting.o dirty-bitmap.o
block-obj-y += write-threshold.o
block-obj-y += backup.o
@@ -38,8 +38,8 @@ rbd.o-cflags := $(RBD_CFLAGS)
rbd.o-libs := $(RBD_LIBS)
gluster.o-cflags := $(GLUSTERFS_CFLAGS)
gluster.o-libs := $(GLUSTERFS_LIBS)
-ssh.o-cflags := $(LIBSSH2_CFLAGS)
-ssh.o-libs := $(LIBSSH2_LIBS)
+ssh.o-cflags := $(LIBSSH_CFLAGS)
+ssh.o-libs := $(LIBSSH_LIBS)
archipelago.o-libs := $(ARCHIPELAGO_LIBS)
block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o
dmg-bz2.o-libs := $(BZIP2_LIBS)
diff --git a/block/ssh.c b/block/ssh.c
index 5ce12b6..7c316db 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -24,8 +24,8 @@
#include "qemu/osdep.h"
-#include
-#include
+#include
+#include
#include "block/block_int.h"
#include "qapi/error.h"
@@ -38,14 +38,12 @@
/* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
* this block driver code.
*
- * TRACE_LIBSSH2= enables tracing in libssh2 itself. Note
- * that this requires that libssh2 was specially compiled with the
- * `./configure --enable-debug' option, so most likely you will have
- * to compile it yourself. The meaning of is described
- * here: http://www.libssh2.org/libssh2_trace.html
+ * TRACE_LIBSSH= enables tracing in libssh itself.
+ * The meaning of is described here:
+ * http://api.libssh.org/master/group__libssh__log.html
*/
#define DEBUG_SSH 0
-#define TRACE_LIBSSH2 0 /* or try: LIBSSH2_TRACE_SFTP */
+#define TRACE_LIBSSH 0 /* see: SSH_LOG_* */
#define DPRINTF(fmt, ...) \
do {\
@@ -60,50 +58,39 @@ typedef struct BDRVSSHState {
CoMutex lock;
/* SSH connection. */
-int sock; /* socket */
-LIBSSH2_SESSION *session; /* ssh session */
-LIBSSH2_SFTP *sftp; /* sftp session */
-LIBSSH2_SFTP_HANDLE *sftp_handle; /* sftp remote file handle */
+ssh_session session; /* ssh session */
+sftp_session sftp;/* sftp session */
+sftp_file sftp_handle;/* sftp remote file handle */
-/* See ssh_seek() function below. */
-int64_t offset;
-bool offset_op_read;
-
-/* File attributes at open. We try to keep the .filesize field
+/* File attributes at open. We try to keep the .size field
* updated if it changes (eg by writing at the end of the file).
*/
-LIBSSH2_SFTP_ATTRIBUTES attrs;
+sftp_attributes attrs;
/* Used to warn if 'flush' is not supported. */
-char *hostport;
bool unsafe_flush_warning;
} BDRVSSHState;
static void ssh_state_init(BDRVSSHState *s)
{
memset(s, 0, sizeof *s);
-s->sock = -1;
-s->offset = -1;
qemu_co_mutex_init(>lock);
}
static void ssh_state_free(BDRVSSHState *s)
{
-g_free(s->hostport);
+if (s->attrs) {
+sftp_attributes_free(s->attrs);
+}
if (s->sftp_handle) {
-libssh2_sftp_close(s->sftp_handle);
+sftp_close(s->sftp_handle);
}
if (s->sftp) {
-libssh2_sftp_shutdown(s->sftp);
+sftp_free(s->sftp);
}
if (s->session) {
-libssh2_session_disconnect(s->session,
- "from qemu ssh client: "
- "user closed the connection");
-libssh2_session_free(s->session);
-}
-if (s->sock >= 0) {
-close(s->sock);
+ssh_disconnect(s->session);
+ssh_free(s->session);
}
}
@@ -118,13 +105,13 @@ session_error_setg(Error **errp, BDRVSSHState *s, const
char *fs, ...)
va_end(args);
if (s->session) {
-
Re: [Qemu-devel] [PATCH] qapi: fix memory leak in QmpOutputVisitor
On Tuesday, 18 October 2016 06:13:30 CEST Eric Blake wrote:
> On 10/18/2016 04:17 AM, Pino Toscano wrote:
> > qmp_output_start_struct() and qmp_output_start_list() create a new
> > QObject (QDict, QList) and push it to the stack of the QmpOutputVisitor,
> > where it is saved as 'value'. When freeing the iterator in
> > qmp_output_free(), these values are never freed properly.
>
> Do any of the tests (perhaps run under valgrind) show this leak? If not,
> maybe we should enhance their coverage.
Running a simple `qemu-img info file.qcow2` under valgrind was enough
for me to show the leak.
In this case, another simple fix is needed to fully fix the leak:
http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg04023.html
(Yes, I just saw your ACK on this, Eric, just leaving it here for
reference.)
> >
> > The simple solution is to qobject_decref() them.
> > ---
> > qapi/qmp-output-visitor.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
> >
> > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> > index 9e3b67c..eedf256 100644
> > --- a/qapi/qmp-output-visitor.c
> > +++ b/qapi/qmp-output-visitor.c
> > @@ -220,6 +220,7 @@ static void qmp_output_free(Visitor *v)
> > while (!QSLIST_EMPTY(>stack)) {
> > e = QSLIST_FIRST(>stack);
> > QSLIST_REMOVE_HEAD(>stack, node);
> > +qobject_decref(e->value);
> > g_free(e);
> > }
> >
> >
>
>
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
[Qemu-devel] [PATCH v2] qapi: fix memory leak in bdrv_image_info_specific_dump
The 'obj' result of the visitor was not properly freed, like done in other places doing a similar job. Signed-off-by: Pino Toscano <ptosc...@redhat.com> --- Changes in v2: - added Signed-off-by block/qapi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qapi.c b/block/qapi.c index 6f947e3..50d3090 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f, assert(qobject_type(obj) == QTYPE_QDICT); data = qdict_get(qobject_to_qdict(obj), "data"); dump_qobject(func_fprintf, f, 1, data); +qobject_decref(obj); visit_free(v); } -- 2.7.4
[Qemu-devel] [PATCH v2] qapi: fix memory leak in QmpOutputVisitor
qmp_output_start_struct() and qmp_output_start_list() create a new
QObject (QDict, QList) and push it to the stack of the QmpOutputVisitor,
where it is saved as 'value'. When freeing the iterator in
qmp_output_free(), these values are never freed properly.
The simple solution is to qobject_decref() them.
Signed-off-by: Pino Toscano <ptosc...@redhat.com>
---
Changes in v2:
- added Signed-off-by
qapi/qmp-output-visitor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 9e3b67c..eedf256 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -220,6 +220,7 @@ static void qmp_output_free(Visitor *v)
while (!QSLIST_EMPTY(>stack)) {
e = QSLIST_FIRST(>stack);
QSLIST_REMOVE_HEAD(>stack, node);
+qobject_decref(e->value);
g_free(e);
}
--
2.7.4
Re: [Qemu-devel] [PATCH] qapi: fix memory leak in bdrv_image_info_specific_dump
On Tuesday, 18 October 2016 11:44:26 CEST Kevin Wolf wrote: > Am 18.10.2016 um 11:18 hat Pino Toscano geschrieben: > > The 'obj' result of the visitor was not properly freed, like done in > > other places doing a similar job. > > --- > > block/qapi.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/block/qapi.c b/block/qapi.c > > index 6f947e3..50d3090 100644 > > --- a/block/qapi.c > > +++ b/block/qapi.c > > @@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function > > func_fprintf, void *f, > > assert(qobject_type(obj) == QTYPE_QDICT); > > data = qdict_get(qobject_to_qdict(obj), "data"); > > dump_qobject(func_fprintf, f, 1, data); > > +qobject_decref(obj); > > visit_free(v); > > } > > The change looks good, but without a Signed-off-by line I can't accept > the patch: http://wiki.qemu.org/Contribute/SubmitAPatch D'oh, sorry -- apparently I didn't send the right version... v2 coming. Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
[Qemu-devel] [PATCH] qapi: fix memory leak in QmpOutputVisitor
qmp_output_start_struct() and qmp_output_start_list() create a new
QObject (QDict, QList) and push it to the stack of the QmpOutputVisitor,
where it is saved as 'value'. When freeing the iterator in
qmp_output_free(), these values are never freed properly.
The simple solution is to qobject_decref() them.
---
qapi/qmp-output-visitor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 9e3b67c..eedf256 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -220,6 +220,7 @@ static void qmp_output_free(Visitor *v)
while (!QSLIST_EMPTY(>stack)) {
e = QSLIST_FIRST(>stack);
QSLIST_REMOVE_HEAD(>stack, node);
+qobject_decref(e->value);
g_free(e);
}
--
2.7.4
[Qemu-devel] [PATCH] qapi: fix memory leak in bdrv_image_info_specific_dump
The 'obj' result of the visitor was not properly freed, like done in other places doing a similar job. --- block/qapi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/qapi.c b/block/qapi.c index 6f947e3..50d3090 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f, assert(qobject_type(obj) == QTYPE_QDICT); data = qdict_get(qobject_to_qdict(obj), "data"); dump_qobject(func_fprintf, f, 1, data); +qobject_decref(obj); visit_free(v); } -- 2.7.4
Re: [Qemu-devel] [PATCH] iSCSI: start moving options also for -drive
On Tuesday, 12 April 2016 16:57:42 CEST Pino Toscano wrote: > to overcome the limitations of the options handling [1], I'm planning > to move more options for iSCSI also as block options, so it is possible > to specify them with -drive. > > The only patch in this series is for initiator-target, as I want to be > sure the approach is correct, and wanted. > > [1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html > > Thanks, > > Pino Toscano (1): > iscsi: allow "initiator-name" as block option > > block/iscsi.c | 22 -- > 1 file changed, 16 insertions(+), 6 deletions(-) Ping. Thanks, -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] [PATCH for 2.6] block: convert iscsi target to a valid ID for -iscsi arg lookup
On Wednesday 13 April 2016 15:18:20 Daniel P. Berrange wrote:
> The iSCSI block driver has a very strange approach whereby it
> does not accept options directly as part of the -drive arg,
> but instead takes them indirectly from a -iscsi arg. To make
> up -driver and -iscsi args, it takes the iSCSI target name
> and uses that as an ID value for the -iscsi arg lookup.
>
> For example, given a -drive arg that looks like
>
> -drive
> file=iscsi://192.168.122.1:3260/iqn.2013-12.com.example%3Aiscsi-chap-netpool/1,format=raw,if=none,id=drive-virtio-disk0
>
> The iSCSI driver will try to find the -iscsi arg with an
> id of "iqn.2013-12.com.example:iscsi-chap-netpool" eg it
> expects
>
> -iscsi
> id=iqn.2013-12.com.example:iscsi-chap-netpool,user=fred,password-secret=secret0
>
> Unfortunately this is impossible to actually do in practice
> because almost all iSCSI target names contain a ':' which
> is not a valid ID character for QEMU:
>
> $ qemu-system-x86_64: -iscsi
> id=iqn.2013-12.com.example:iscsi-chap-netpool,user=redhat,password=123456:
> Parameter 'id' expects an identifier
> Identifiers consist of letters, digits, '-', '.', '_', starting with a
> letter.
>
> So for this to work we need to remove the invalid characters
> in some manner. This patch takes the simplest possible approach
> of just converting all invalid characters into underscores. eg
> you can now use
>
> $ qemu-system-x86_64: -iscsi
> id=iqn.2013-12.com.example_iscsi-chap-netpool,user=redhat,password=123456:
> Parameter 'id' expects an identifier
>
> There is theoretically the possibility for collison with this
> approach if there were 2 iSCSI luns attached to the same VM
> with target names that clash on the escaped char: eg
>
> iqn.2013-12.com.example:iscsi-chap-netpool
> iqn.2013-12.com.example_iscsi-chap-netpool
>
> but in reality this will never happen. In addition in QEMU 2.7
> the need to use the -iscsi arg will be removed by allowing all
> the desired options to be listed directly with -drive. IOW this
> quick stripping of invalid characters is just a short term fix
> that will ultimately go away. For this reason it is not thought
> worthwhile to invent a full collision-free escaping syntax for
> iSCSI target IDs.
Maybe it would be worth as workaround for 2.6, although ...
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> ---
>
> Note this problem was previously raised:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html
>
> and discussed the following month:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg00112.html
>
> block/iscsi.c | 21 -
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 302baf8..7475cc9 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1070,6 +1070,22 @@ retry:
> return 0;
> }
>
> +
> +static char *convert_target_to_id(const char *target)
> +{
> +char *id = g_strdup(target);
> +size_t i;
> +
> +for (i = 1; id[i]; i++) {
> +if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
> +id[i] = '_';
> +}
> +}
> +
> +return id;
> +}
> +
> +
> static void parse_chap(struct iscsi_context *iscsi, const char *target,
> Error **errp)
> {
> @@ -1079,13 +1095,16 @@ static void parse_chap(struct iscsi_context *iscsi,
> const char *target,
> const char *password = NULL;
> const char *secretid;
> char *secret = NULL;
> +char *targetid = NULL;
>
> list = qemu_find_opts("iscsi");
> if (!list) {
> return;
> }
>
> -opts = qemu_opts_find(list, target);
> +targetid = convert_target_to_id(target);
> +opts = qemu_opts_find(list, targetid);
> +g_free(targetid);
> if (opts == NULL) {
> opts = QTAILQ_FIRST(>head);
> if (!opts) {
... the commit message seems to suggest that it applies to all the
iSCSI parameter, but it actually works on the authentication-related
ones.
IMHO a better way would be using convert_target_to_id directly in
iscsi_open on iscsi_url->target (right after the url parsing) passing
the converted id to parse_initiator_name, iscsi_set_targetname,
parse_chap, and parse_header_digest: this way it can apply to all the
parameters.
Thanks,
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
[Qemu-devel] [PATCH] iSCSI: start moving options also for -drive
Hi, to overcome the limitations of the options handling [1], I'm planning to move more options for iSCSI also as block options, so it is possible to specify them with -drive. The only patch in this series is for initiator-target, as I want to be sure the approach is correct, and wanted. [1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html Thanks, Pino Toscano (1): iscsi: allow "initiator-name" as block option block/iscsi.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-) -- 2.5.5
[Qemu-devel] [PATCH] iscsi: allow "initiator-name" as block option
Allow the "initiator-name" for both the -iscsi and the block options:
this way it is possible to set it directly as option in the -drive
specification.
The current way to specify the initiator name for a certain iSCSI
target is:
-iscsi id=TARGET,initiator-name=IQN
which cannot be actually done when TARGET has the optional part, as
colon is not accepted as id for QemuOpts [1].
Hence, allow the "initiator-name" also in block options: this way
it is possible to set it directly as option in -drive, e.g.:
-drive file=URI,driver=iscsi,initiator-name=IQN
[1] http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg06501.html
Signed-off-by: Pino Toscano <ptosc...@redhat.com>
---
block/iscsi.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 302baf8..4a1c300 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1161,7 +1161,7 @@ static void parse_header_digest(struct iscsi_context
*iscsi, const char *target,
}
}
-static char *parse_initiator_name(const char *target)
+static char *parse_initiator_name(QDict *options, const char *target)
{
QemuOptsList *list;
QemuOpts *opts;
@@ -1169,6 +1169,11 @@ static char *parse_initiator_name(const char *target)
char *iscsi_name;
UuidInfo *uuid_info;
+name = qdict_get_try_str(options, "initiator-name");
+if (name != NULL) {
+return g_strdup(name);
+}
+
list = qemu_find_opts("iscsi");
if (list) {
opts = qemu_opts_find(list, target);
@@ -1304,11 +1309,19 @@ static void iscsi_readcapacity_sync(IscsiLun *iscsilun,
Error **errp)
}
}
+#define COMMON_ISCSI_OPTS \
+{ \
+.name = "initiator-name", \
+.type = QEMU_OPT_STRING, \
+.help = "Initiator iqn name to use when connecting", \
+}
+
/* TODO Convert to fine grained options */
static QemuOptsList runtime_opts = {
.name = "iscsi",
.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
.desc = {
+COMMON_ISCSI_OPTS,
{
.name = "filename",
.type = QEMU_OPT_STRING,
@@ -1473,7 +1486,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
memset(iscsilun, 0, sizeof(IscsiLun));
-initiator_name = parse_initiator_name(iscsi_url->target);
+initiator_name = parse_initiator_name(bs->options, iscsi_url->target);
iscsi = iscsi_create_context(initiator_name);
if (iscsi == NULL) {
@@ -1864,6 +1877,7 @@ static QemuOptsList qemu_iscsi_opts = {
.name = "iscsi",
.head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
.desc = {
+COMMON_ISCSI_OPTS,
{
.name = "user",
.type = QEMU_OPT_STRING,
@@ -1883,10 +1897,6 @@ static QemuOptsList qemu_iscsi_opts = {
.help = "HeaderDigest setting. "
"{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
},{
-.name = "initiator-name",
-.type = QEMU_OPT_STRING,
-.help = "Initiator iqn name to use when connecting",
-},{
.name = "timeout",
.type = QEMU_OPT_NUMBER,
.help = "Request timeout in seconds (default 0 = no timeout)",
--
2.5.5
Re: [Qemu-devel] iSCSI options for IQN with colons
On Tuesday 01 December 2015 10:27:28 Markus Armbruster wrote:
> Beware, I know next to nothing about iSCSI.
>
> Pino Toscano <ptosc...@redhat.com> writes:
>
> > Hi,
> >
> > while testing the integration of QEMU with iSCSI, I was setting up an
> > environment with both target and initiator IQNs with colons. Then I
> > tried to connect to two different targets using two different initiator
> > IQN, like the following:
> >
> > $ qemu ... \
> > -iscsi
> > id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator
> > \
> > -iscsi
> > id=iqn.2015-11.com.bla:suffix2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator
> > \
> > -drive
> > file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none
> > \
> > -drive
> > file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none
> > \
> > ...
> >
> > which didn't work at first:
> >
> > qemu-system-x86_64: -iscsi
> > id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator:
> > Parameter 'id' expects an identifier
> >
> > which, according to id_wellformed in id.c, is true. Allowing colons in
> > id=... like in the following patch
> >
> > diff --git a/util/id.c b/util/id.c
> > index bcc64d8..25fca9d 100644
> > --- a/util/id.c
> > +++ b/util/id.c
> > @@ -20,7 +20,7 @@ bool id_wellformed(const char *id)
> > return false;
> > }
> > for (i = 1; id[i]; i++) {
> > -if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
> > +if (!qemu_isalnum(id[i]) && !strchr("-._:", id[i])) {
> > return false;
> > }
> > }
> >
> > allowed me to work run QEMU with the attached disks.
> >
> > The question basically boils down to whether it is right to reject
> > colons in id:
> > - if so, then there should be a way to allow them only in id of -iscsi
> > (since colons can be part of IQNs)
> > - if not, whether allowing them could cause regressions in option
> > parsing
>
> Have you tried
>
> -iscsi id=iscsi.1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator \
> -iscsi id=iscsi.2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator \
> -drive
> file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none
> \
> -drive
> file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none
> \
This won't work, as the various parse_* in iscsi.c (e.g.
parse_initiator_name for the above cases) use the target IQN as
identifier for the parameters.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
[Qemu-devel] iSCSI options for IQN with colons
Hi,
while testing the integration of QEMU with iSCSI, I was setting up an
environment with both target and initiator IQNs with colons. Then I
tried to connect to two different targets using two different initiator
IQN, like the following:
$ qemu ... \
-iscsi
id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator
\
-iscsi
id=iqn.2015-11.com.bla:suffix2,initiator-name=iqn.2015-11.com.bla:suffix2-initiator
\
-drive
file=iscsi://server/iqn.2015-11.com.bla%3Asuffix1/0,format=raw,id=hd1,if=none \
-drive
file=iscsi://server/iqn.2015-11.com.bla%3Asuffix2/0,format=raw,id=hd2,if=none \
...
which didn't work at first:
qemu-system-x86_64: -iscsi
id=iqn.2015-11.com.bla:suffix1,initiator-name=iqn.2015-11.com.bla:suffix1-initiator:
Parameter 'id' expects an identifier
which, according to id_wellformed in id.c, is true. Allowing colons in
id=... like in the following patch
diff --git a/util/id.c b/util/id.c
index bcc64d8..25fca9d 100644
--- a/util/id.c
+++ b/util/id.c
@@ -20,7 +20,7 @@ bool id_wellformed(const char *id)
return false;
}
for (i = 1; id[i]; i++) {
-if (!qemu_isalnum(id[i]) && !strchr("-._", id[i])) {
+if (!qemu_isalnum(id[i]) && !strchr("-._:", id[i])) {
return false;
}
}
allowed me to work run QEMU with the attached disks.
The question basically boils down to whether it is right to reject
colons in id:
- if so, then there should be a way to allow them only in id of -iscsi
(since colons can be part of IQNs)
- if not, whether allowing them could cause regressions in option
parsing
Thanks,
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
