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

[Markus Armbruster]

Eric Blake <address@hidden> 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" <address@hidden>
>> Cc: Kevin Wolf <address@hidden>
>> Cc: Max Reitz <address@hidden>
>> Cc: address@hidden
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!