Re: [Qemu-devel] [PATCH 02/15] block/ssh: Do not report read/write/flush errors to the user

[Eric Blake <eblake@redhat.com>]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]