Re: [PATCH ssh] ssh: Do not switch session to non-blocking mode

[Michael Tokarev]

Heh. I was creating a qemu bug report on gitlab when this email arrived :)

13.11.2024 14:49, Richard W.M. Jones wrote:
> From: Jakub Jelen <jjelen@redhat.com>
>
> The libssh does not handle non-blocking mode in SFTP correctly. The
> driver code already changes the mode to blocking for the SFTP
> initialization, but for some reason changes to non-blocking mode.

"changes to non-blocking mode LATER", I guess, - or else it's a bit
difficult to read.  But this works too.

> This used to work accidentally until libssh in 0.11 branch merged
> the patch to avoid infinite looping in case of network errors:
>
> https://gitlab.com/libssh/libssh-mirror/-/merge_requests/498
>
> Since then, the ssh driver in qemu fails to read files over SFTP
> as the first SFTP messages exchanged after switching the session
> to non-blocking mode return SSH_AGAIN, but that message is lost
> int the SFTP internals and interpretted as SSH_ERROR, which is
> returned to the caller:
>
> https://gitlab.com/libssh/libssh-mirror/-/issues/280
>
> This is indeed an issue in libssh that we should address in the
> long term, but it will require more work on the internals. For
> now, the SFTP is not supported in non-blocking mode.

The comment at init where the code sets socket to blocking mode, says:

    /*
     * Make sure we are in blocking mode during the connection and
     * authentication phases.
     */
    ssh_set_blocking(s->session, 1);


There are a few other places where the code expect "some" blocking
mode, changes it to blocking, and restores the mode later, - eg,
see ssh_grow_file().  It looks all this has to be fixed too.

I wonder if qemu ssh driver needs to mess with blocking mode of this
socket in the first place, ever.  Is there a way qemu can get non-blocking
socket in this context?  I can only think of fd=NNN, but is it
possible for this socket to be non-blocking?

> Fixes: https://gitlab.com/libssh/libssh-mirror/-/issues/280
> Signed-off-by: Jakub Jelen <jjelen@redhat.com>
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>   block/ssh.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/block/ssh.c b/block/ssh.c
> index 9f8140bcb6..e1529cfda9 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -866,8 +866,6 @@ static int ssh_open(BlockDriverState *bs, QDict *options, 
> int bdrv_flags,
>           goto err;
>       }
>   
> -    /* Go non-blocking. */
> -    ssh_set_blocking(s->session, 0);
>   
>       if (s->attrs->type == SSH_FILEXFER_TYPE_REGULAR) {
>           bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;

Please remove the empty line too.

/mjt