| Message ID | 20241113115000.2494785-1-rjones@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [ssh] ssh: Do not switch session to non-blocking mode | expand |
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
On Wed, Nov 13, 2024 at 03:02:59PM +0300, Michael Tokarev wrote: > 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'll just note that I'm only forwarding on the patch from Jakub. I didn't write it. I did lightly test it, and it seems to work. However it seems also likely that it is causing qemu to block internally. Probably not noticable for light use, but not something that we'd want for serious use. However if libssh doesn't support non-blocking SFTP there's not much we can do about that in qemu. I would recommend using nbdkit-ssh-plugin instead anyway as it is much more featureful and doesn't have this problem as we use real threads instead of coroutines. > 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. I posted a v2 which removes the blank line but is otherwise the same. Rich.
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;