Callbacks ssh_co_readv(), ssh_co_writev(), ssh_co_flush() report
errors to the user with error_printf().  They shouldn't, it's their
caller's job.  Replace by a suitable trace point.

Perhaps we should convert this part of the block driver interface to
Error, so block drivers can pass more detail to their callers.  Not
today.

Cc: "Richard W.M. Jones" <rjones@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/ssh.c        | 36 ++++++++++++------------------------
 block/trace-events |  3 +++
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/block/ssh.c b/block/ssh.c
index 190ef95300..382fc04fbf 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -159,31 +159,19 @@ sftp_error_setg(Error **errp, BDRVSSHState *s, const char *fs, ...)
     g_free(msg);
 }
 
-static void GCC_FMT_ATTR(2, 3)
-sftp_error_report(BDRVSSHState *s, const char *fs, ...)
+static void sftp_error_trace(BDRVSSHState *s, const char *op)
 {
-    va_list args;
+    char *ssh_err;
+    int ssh_err_code;
+    unsigned long sftp_err_code;
 
-    va_start(args, fs);
-    error_vprintf(fs, args);
-
-    if ((s)->sftp) {
-        char *ssh_err;
-        int ssh_err_code;
-        unsigned long sftp_err_code;
-
-        /* This is not an errno.  See <libssh2.h>. */
-        ssh_err_code = libssh2_session_last_error(s->session,
+    /* This is not an errno.  See <libssh2.h>. */
+    ssh_err_code = libssh2_session_last_error(s->session,
                                                   &ssh_err, NULL, 0);
-        /* See <libssh2_sftp.h>. */
-        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
+    /* See <libssh2_sftp.h>. */
+    sftp_err_code = libssh2_sftp_last_error((s)->sftp);
 
-        error_printf(": %s (libssh2 error code: %d, sftp error code: %lu)",
-                     ssh_err, ssh_err_code, sftp_err_code);
-    }
-
-    va_end(args);
-    error_printf("\n");
+    trace_sftp_error(op, ssh_err, ssh_err_code, sftp_err_code);
 }
 
 static int parse_uri(const char *filename, QDict *options, Error **errp)
@@ -1035,7 +1023,7 @@ static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
             goto again;
         }
         if (r < 0) {
-            sftp_error_report(s, "read failed");
+            sftp_error_trace(s, "read");
             s->offset = -1;
             return -EIO;
         }
@@ -1105,7 +1093,7 @@ static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
             goto again;
         }
         if (r < 0) {
-            sftp_error_report(s, "write failed");
+            sftp_error_trace(s, "write");
             s->offset = -1;
             return -EIO;
         }
@@ -1188,7 +1176,7 @@ static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
         return 0;
     }
     if (r < 0) {
-        sftp_error_report(s, "fsync failed");
+        sftp_error_trace(s, "fsync");
         return -EIO;
     }
 
diff --git a/block/trace-events b/block/trace-events
index 7335a42540..79ccd8d824 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -208,3 +208,6 @@ sheepdog_co_rw_vector_new(uint64_t oid) "new oid 0x%" PRIx64
 sheepdog_snapshot_create_info(const char *sn_name, const char *id, const char *name, int64_t size, int is_snapshot) "sn_info: name %s id_str %s s: name %s vm_state_size %" PRId64 " " "is_snapshot %d"
 sheepdog_snapshot_create(const char *sn_name, const char *id) "%s %s"
 sheepdog_snapshot_create_inode(const char *name, uint32_t snap, uint32_t vdi) "s->inode: name %s snap_id 0x%" PRIx32 " vdi 0x%" PRIx32
+
+# ssh.c
+sftp_error(const char *op, const char *ssh_err, int ssh_err_code, unsigned long sftp_err_code) "%s failed: %s (libssh2 error code: %d, sftp error code: %lu)"
-- 
2.17.2

On 4/8/19 3:36 AM, Markus Armbruster wrote:
> Callbacks ssh_co_readv(), ssh_co_writev(), ssh_co_flush() report
> errors to the user with error_printf().  They shouldn't, it's their
> caller's job.  Replace by a suitable trace point.
> 
> Perhaps we should convert this part of the block driver interface to
> Error, so block drivers can pass more detail to their callers.  Not
> today.
> 
> Cc: "Richard W.M. Jones" <rjones@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/ssh.c        | 36 ++++++++++++------------------------
>  block/trace-events |  3 +++
>  2 files changed, 15 insertions(+), 24 deletions(-)
> 

> -static void GCC_FMT_ATTR(2, 3)
> -sftp_error_report(BDRVSSHState *s, const char *fs, ...)
> +static void sftp_error_trace(BDRVSSHState *s, const char *op)
>  {
> -    va_list args;
> +    char *ssh_err;
> +    int ssh_err_code;
> +    unsigned long sftp_err_code;
>  
> -    va_start(args, fs);
> -    error_vprintf(fs, args);
> -
> -    if ((s)->sftp) {

The old code was conditional,

> -        char *ssh_err;
> -        int ssh_err_code;
> -        unsigned long sftp_err_code;
> -
> -        /* This is not an errno.  See <libssh2.h>. */
> -        ssh_err_code = libssh2_session_last_error(s->session,
> +    /* This is not an errno.  See <libssh2.h>. */
> +    ssh_err_code = libssh2_session_last_error(s->session,
>                                                    &ssh_err, NULL, 0);

Indentation looks off now.

> -        /* See <libssh2_sftp.h>. */
> -        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
> +    /* See <libssh2_sftp.h>. */
> +    sftp_err_code = libssh2_sftp_last_error((s)->sftp);

but it appears the new code can call libssh2_sftp_last_error(NULL). Am I
missing something, or could that be a problem?

/me rescans the full file...

Okay, connect_to_ssh() won't succeed unless s->sftp is set, and it
appears that all callers to the trace function can only be reached if
connect succeeded.

Indentation fixup can be done by maintainer,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 4/8/19 3:36 AM, Markus Armbruster wrote:
>> Callbacks ssh_co_readv(), ssh_co_writev(), ssh_co_flush() report
>> errors to the user with error_printf().  They shouldn't, it's their
>> caller's job.  Replace by a suitable trace point.
>> 
>> Perhaps we should convert this part of the block driver interface to
>> Error, so block drivers can pass more detail to their callers.  Not
>> today.
>> 
>> Cc: "Richard W.M. Jones" <rjones@redhat.com>
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: qemu-block@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/ssh.c        | 36 ++++++++++++------------------------
>>  block/trace-events |  3 +++
>>  2 files changed, 15 insertions(+), 24 deletions(-)
>> 
>
>> -static void GCC_FMT_ATTR(2, 3)
>> -sftp_error_report(BDRVSSHState *s, const char *fs, ...)
>> +static void sftp_error_trace(BDRVSSHState *s, const char *op)
>>  {
>> -    va_list args;
>> +    char *ssh_err;
>> +    int ssh_err_code;
>> +    unsigned long sftp_err_code;
>>  
>> -    va_start(args, fs);
>> -    error_vprintf(fs, args);
>> -
>> -    if ((s)->sftp) {
>
> The old code was conditional,
>
>> -        char *ssh_err;
>> -        int ssh_err_code;
>> -        unsigned long sftp_err_code;
>> -
>> -        /* This is not an errno.  See <libssh2.h>. */
>> -        ssh_err_code = libssh2_session_last_error(s->session,
>> +    /* This is not an errno.  See <libssh2.h>. */
>> +    ssh_err_code = libssh2_session_last_error(s->session,
>>                                                    &ssh_err, NULL, 0);
>
> Indentation looks off now.

Will fix.

>> -        /* See <libssh2_sftp.h>. */
>> -        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>> +    /* See <libssh2_sftp.h>. */
>> +    sftp_err_code = libssh2_sftp_last_error((s)->sftp);
>
> but it appears the new code can call libssh2_sftp_last_error(NULL). Am I
> missing something, or could that be a problem?
>
> /me rescans the full file...
>
> Okay, connect_to_ssh() won't succeed unless s->sftp is set, and it
> appears that all callers to the trace function can only be reached if
> connect succeeded.

I can add to the commit message

    While there, drop the unreachable !s->sftp case.

> Indentation fixup can be done by maintainer,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

I don't know much about this patch which looks like internal qemu
rearrangements so I guess fine.  However I do have a few things to say
about the ssh driver ...

As you know I wrote this a few years ago, and it uses libssh2.
libssh2 has not evolved as quickly as we'd like and it may be better
to use libssh instead -- despite the names, these are two separate and
unrelated libraries.  libssh supports a wider range of SSH encryption
and has more features.  It's generally more likely to work against a
random SSH server.  It has also been through the FIPS process.  Indeed
Red Hat made the decision to switch exclusively to libssh in RHEL 8,
if that carries any weight.

Pino posted a libssh2 -> libssh conversion patch a while back, but it
has been somewhat stuck in review.  If I recall the latest concern was
whether it performs as well as the libssh2 version.

  https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07267.html

In the meantime I added libssh support to nbdkit.  nbdkit can be used
as a complete replacement for qemu's ssh driver.

  nbdkit ssh host=foo.example.com disk.img -U tmpdirXXXXXX/sock
  qemu -hda nbd:unix:tmpdirXXXXXX/sock

In fact it's somewhat superior (IMHO) because all of the tricky code
handling libssh runs outside qemu in a separate process, improving
isolation and potentially allowing separate, restrictive security
policies to be applied.  For example it would no longer be necessary
to give qemu permission to connect to remote SSH servers.

Could we make this really smooth somehow?  nbdkit has a concept
[https://www.mankier.com/1/nbdkit-captive] where we make it easy to
manage external commands owned by nbdkit.  Is there an equivalent
feature of qemu where:

  qemu -object exec,id=nbd1,cmd='nbdkit -f -U $sock ssh ...' \
       -drive file.driver=nbd,file.socket=nbd1

would run the command but also allocate a socket and kill the
subcommand on exit (of qemu)?

Basically I'm trying to think about how to make this a reality:

  https://rwmj.files.wordpress.com/2018/10/drawing2-svg.png

Rich.

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

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

> I don't know much about this patch which looks like internal qemu
> rearrangements so I guess fine.  However I do have a few things to say
> about the ssh driver ...
>
> As you know I wrote this a few years ago, and it uses libssh2.
> libssh2 has not evolved as quickly as we'd like and it may be better
> to use libssh instead -- despite the names, these are two separate and
> unrelated libraries.  libssh supports a wider range of SSH encryption
> and has more features.  It's generally more likely to work against a
> random SSH server.  It has also been through the FIPS process.  Indeed
> Red Hat made the decision to switch exclusively to libssh in RHEL 8,
> if that carries any weight.
>
> Pino posted a libssh2 -> libssh conversion patch a while back, but it
> has been somewhat stuck in review.  If I recall the latest concern was
> whether it performs as well as the libssh2 version.
>
>   https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07267.html

I doubt we'd need "as well as" for this driver.  But Max observed a
factor of five with v4.  Pino reported improvements with v5 ("no more
than 200%"), and some more with libssh master instead of 0.7, but didn't
quantify those.  To make progress, we need a rebased patch with actual
performance numbers, I think.

> In the meantime I added libssh support to nbdkit.  nbdkit can be used
> as a complete replacement for qemu's ssh driver.
>
>   nbdkit ssh host=foo.example.com disk.img -U tmpdirXXXXXX/sock
>   qemu -hda nbd:unix:tmpdirXXXXXX/sock
>
> In fact it's somewhat superior (IMHO) because all of the tricky code
> handling libssh runs outside qemu in a separate process, improving
> isolation and potentially allowing separate, restrictive security
> policies to be applied.  For example it would no longer be necessary
> to give qemu permission to connect to remote SSH servers.

Good point.

> Could we make this really smooth somehow?  nbdkit has a concept
> [https://www.mankier.com/1/nbdkit-captive] where we make it easy to
> manage external commands owned by nbdkit.  Is there an equivalent
> feature of qemu where:
>
>   qemu -object exec,id=nbd1,cmd='nbdkit -f -U $sock ssh ...' \
>        -drive file.driver=nbd,file.socket=nbd1
>
> would run the command but also allocate a socket and kill the
> subcommand on exit (of qemu)?

I'm not aware of general infrastructure to run helper processes.  But
I'm sure we could come up with something.

> Basically I'm trying to think about how to make this a reality:
>
>   https://rwmj.files.wordpress.com/2018/10/drawing2-svg.png

Looks like you're also targeting curl.c's drivers.  Any others?

Got a backward compatibility story other than "let's deprecate these
drivers"?

On Mon, Apr 08, 2019 at 08:07:00PM +0200, Markus Armbruster wrote:
> "Richard W.M. Jones" <rjones@redhat.com> writes:
> 
> > I don't know much about this patch which looks like internal qemu
> > rearrangements so I guess fine.  However I do have a few things to say
> > about the ssh driver ...
> >
> > As you know I wrote this a few years ago, and it uses libssh2.
> > libssh2 has not evolved as quickly as we'd like and it may be better
> > to use libssh instead -- despite the names, these are two separate and
> > unrelated libraries.  libssh supports a wider range of SSH encryption
> > and has more features.  It's generally more likely to work against a
> > random SSH server.  It has also been through the FIPS process.  Indeed
> > Red Hat made the decision to switch exclusively to libssh in RHEL 8,
> > if that carries any weight.
> >
> > Pino posted a libssh2 -> libssh conversion patch a while back, but it
> > has been somewhat stuck in review.  If I recall the latest concern was
> > whether it performs as well as the libssh2 version.
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07267.html
> 
> I doubt we'd need "as well as" for this driver.  But Max observed a
> factor of five with v4.  Pino reported improvements with v5 ("no more
> than 200%"), and some more with libssh master instead of 0.7, but didn't
> quantify those.  To make progress, we need a rebased patch with actual
> performance numbers, I think.
> 
> > In the meantime I added libssh support to nbdkit.  nbdkit can be used
> > as a complete replacement for qemu's ssh driver.
> >
> >   nbdkit ssh host=foo.example.com disk.img -U tmpdirXXXXXX/sock
> >   qemu -hda nbd:unix:tmpdirXXXXXX/sock
> >
> > In fact it's somewhat superior (IMHO) because all of the tricky code
> > handling libssh runs outside qemu in a separate process, improving
> > isolation and potentially allowing separate, restrictive security
> > policies to be applied.  For example it would no longer be necessary
> > to give qemu permission to connect to remote SSH servers.
> 
> Good point.
> 
> > Could we make this really smooth somehow?  nbdkit has a concept
> > [https://www.mankier.com/1/nbdkit-captive] where we make it easy to
> > manage external commands owned by nbdkit.  Is there an equivalent
> > feature of qemu where:
> >
> >   qemu -object exec,id=nbd1,cmd='nbdkit -f -U $sock ssh ...' \
> >        -drive file.driver=nbd,file.socket=nbd1
> >
> > would run the command but also allocate a socket and kill the
> > subcommand on exit (of qemu)?
> 
> I'm not aware of general infrastructure to run helper processes.  But
> I'm sure we could come up with something.
> 
> > Basically I'm trying to think about how to make this a reality:
> >
> >   https://rwmj.files.wordpress.com/2018/10/drawing2-svg.png
> 
> Looks like you're also targeting curl.c's drivers.  Any others?

As you know I wrote a read-only version of vvfat so it could be
deprecated in qemu.  It doesn't do the vvfat write madness however.

While there are other interesting plugins for nbdkit, we are steering
clear of needlessly duplicating qemu block functionality.  There would
have to be a reason to do it, and I think the reasons are clear enough
for ssh, curl and vvfat.

> Got a backward compatibility story other than "let's deprecate these
> drivers"?

That's if we do deprecate them in qemu at all.  We can have both if
people would prefer that.  If not I suppose we could have a
replacement stub driver which would run nbdkit.  It adds a dependency,
but nbdkit is intended to have very minimal deps.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On 08.04.19 19:22, Richard W.M. Jones wrote:
> I don't know much about this patch which looks like internal qemu
> rearrangements so I guess fine.  However I do have a few things to say
> about the ssh driver ...
> 
> As you know I wrote this a few years ago, and it uses libssh2.
> libssh2 has not evolved as quickly as we'd like and it may be better
> to use libssh instead -- despite the names, these are two separate and
> unrelated libraries.  libssh supports a wider range of SSH encryption
> and has more features.  It's generally more likely to work against a
> random SSH server.  It has also been through the FIPS process.  Indeed
> Red Hat made the decision to switch exclusively to libssh in RHEL 8,
> if that carries any weight.
> 
> Pino posted a libssh2 -> libssh conversion patch a while back, but it
> has been somewhat stuck in review.  If I recall the latest concern was
> whether it performs as well as the libssh2 version.
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07267.html
> 
> In the meantime I added libssh support to nbdkit.  nbdkit can be used
> as a complete replacement for qemu's ssh driver.
> 
>   nbdkit ssh host=foo.example.com disk.img -U tmpdirXXXXXX/sock
>   qemu -hda nbd:unix:tmpdirXXXXXX/sock
> 
> In fact it's somewhat superior (IMHO) because all of the tricky code
> handling libssh runs outside qemu in a separate process, improving
> isolation and potentially allowing separate, restrictive security
> policies to be applied.  For example it would no longer be necessary
> to give qemu permission to connect to remote SSH servers.
> 
> Could we make this really smooth somehow?  nbdkit has a concept
> [https://www.mankier.com/1/nbdkit-captive] where we make it easy to
> manage external commands owned by nbdkit.  Is there an equivalent
> feature of qemu where:
> 
>   qemu -object exec,id=nbd1,cmd='nbdkit -f -U $sock ssh ...' \
>        -drive file.driver=nbd,file.socket=nbd1
> 
> would run the command but also allocate a socket and kill the
> subcommand on exit (of qemu)?
> 
> Basically I'm trying to think about how to make this a reality:
> 
>   https://rwmj.files.wordpress.com/2018/10/drawing2-svg.png
> 
> Rich.

I don’t disagree with anything you say.  I would prefer to move the less
well maintained drivers (for which there is no strict performance
requirement) into a separate process.  nbdkit is perfectly suited for
that, and the drivers are there, as you say (ssh, curl, vvfat).

Having a nicer interface in qemu would make the transition simple,
because we could tell users exactly how to change their command line so
their use case continues to work.  I’m not sure whether it really works,
though, because I don’t think there is such a simple replacement for
being able to simply pass "ssh://host/path" to qemu and have it work.

But I think it’s still worth it.

Max

Max Reitz <mreitz@redhat.com> writes:

> On 08.04.19 19:22, Richard W.M. Jones wrote:
>> I don't know much about this patch which looks like internal qemu
>> rearrangements so I guess fine.  However I do have a few things to say
>> about the ssh driver ...
>> 
>> As you know I wrote this a few years ago, and it uses libssh2.
>> libssh2 has not evolved as quickly as we'd like and it may be better
>> to use libssh instead -- despite the names, these are two separate and
>> unrelated libraries.  libssh supports a wider range of SSH encryption
>> and has more features.  It's generally more likely to work against a
>> random SSH server.  It has also been through the FIPS process.  Indeed
>> Red Hat made the decision to switch exclusively to libssh in RHEL 8,
>> if that carries any weight.
>> 
>> Pino posted a libssh2 -> libssh conversion patch a while back, but it
>> has been somewhat stuck in review.  If I recall the latest concern was
>> whether it performs as well as the libssh2 version.
>> 
>>   https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg07267.html
>> 
>> In the meantime I added libssh support to nbdkit.  nbdkit can be used
>> as a complete replacement for qemu's ssh driver.
>> 
>>   nbdkit ssh host=foo.example.com disk.img -U tmpdirXXXXXX/sock
>>   qemu -hda nbd:unix:tmpdirXXXXXX/sock
>> 
>> In fact it's somewhat superior (IMHO) because all of the tricky code
>> handling libssh runs outside qemu in a separate process, improving
>> isolation and potentially allowing separate, restrictive security
>> policies to be applied.  For example it would no longer be necessary
>> to give qemu permission to connect to remote SSH servers.
>> 
>> Could we make this really smooth somehow?  nbdkit has a concept
>> [https://www.mankier.com/1/nbdkit-captive] where we make it easy to
>> manage external commands owned by nbdkit.  Is there an equivalent
>> feature of qemu where:
>> 
>>   qemu -object exec,id=nbd1,cmd='nbdkit -f -U $sock ssh ...' \
>>        -drive file.driver=nbd,file.socket=nbd1
>> 
>> would run the command but also allocate a socket and kill the
>> subcommand on exit (of qemu)?
>> 
>> Basically I'm trying to think about how to make this a reality:
>> 
>>   https://rwmj.files.wordpress.com/2018/10/drawing2-svg.png
>> 
>> Rich.
>
> I don’t disagree with anything you say.  I would prefer to move the less
> well maintained drivers (for which there is no strict performance
> requirement) into a separate process.  nbdkit is perfectly suited for
> that, and the drivers are there, as you say (ssh, curl, vvfat).
>
> Having a nicer interface in qemu would make the transition simple,
> because we could tell users exactly how to change their command line so
> their use case continues to work.  I’m not sure whether it really works,
> though, because I don’t think there is such a simple replacement for
> being able to simply pass "ssh://host/path" to qemu and have it work.
>
> But I think it’s still worth it.

I guess that boils down to "patches welcome".

For v1, I wouldn't worry about making the transition simple.  Just show
us some working code.