[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 02/15] block/ssh: Do not report rea
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 02/15] block/ssh: Do not report read/write/flush errors to the user |
Date: |
Tue, 09 Apr 2019 08:09:43 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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!