On 14.03.2017 12:11, Paolo Bonzini wrote: > After the switch to reading replies in a coroutine, nothing is > reentering pending receive coroutines if the connection hangs. > Move nbd_recv_coroutines_enter_all to the reply read coroutine, > which is the place where hangups are detected. nbd_teardown_connection > can simply wait for the reply read coroutine to detect the hangup > and clean up after itself. > > This wouldn't be enough though because nbd_receive_reply returns 0 > (rather than -EPIPE or similar) when reading from a hung connection. > Fix the return value check in nbd_read_reply_entry. > > This fixes qemu-iotests 083. > > Reported-by: Max Reitz <mre...@redhat.com> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > --- > block/nbd-client.c | 12 ++++++------ > nbd/client.c | 2 +- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/block/nbd-client.c b/block/nbd-client.c > index 0dc12c2..1e2952f 100644 > --- a/block/nbd-client.c > +++ b/block/nbd-client.c > @@ -33,17 +33,15 @@ > #define HANDLE_TO_INDEX(bs, handle) ((handle) ^ ((uint64_t)(intptr_t)bs)) > #define INDEX_TO_HANDLE(bs, index) ((index) ^ ((uint64_t)(intptr_t)bs)) > > -static void nbd_recv_coroutines_enter_all(BlockDriverState *bs) > +static void nbd_recv_coroutines_enter_all(NBDClientSession *s) > { > - NBDClientSession *s = nbd_get_client_session(bs); > int i; > > for (i = 0; i < MAX_NBD_REQUESTS; i++) { > if (s->recv_coroutine[i]) { > - qemu_coroutine_enter(s->recv_coroutine[i]); > + aio_co_wake(s->recv_coroutine[i]); > } > } > - BDRV_POLL_WHILE(bs, s->read_reply_co); > } > > static void nbd_teardown_connection(BlockDriverState *bs) > @@ -58,7 +56,7 @@ static void nbd_teardown_connection(BlockDriverState *bs) > qio_channel_shutdown(client->ioc, > QIO_CHANNEL_SHUTDOWN_BOTH, > NULL); > - nbd_recv_coroutines_enter_all(bs); > + BDRV_POLL_WHILE(bs, client->read_reply_co); > > nbd_client_detach_aio_context(bs); > object_unref(OBJECT(client->sioc)); > @@ -76,7 +74,7 @@ static coroutine_fn void nbd_read_reply_entry(void *opaque) > for (;;) { > assert(s->reply.handle == 0); > ret = nbd_receive_reply(s->ioc, &s->reply); > - if (ret < 0) { > + if (ret <= 0) { > break; > } > > @@ -103,6 +101,8 @@ static coroutine_fn void nbd_read_reply_entry(void > *opaque) > aio_co_wake(s->recv_coroutine[i]); > qemu_coroutine_yield(); > } > + > + nbd_recv_coroutines_enter_all(s); > s->read_reply_co = NULL; > } > > diff --git a/nbd/client.c b/nbd/client.c > index 5c9dee3..746e9a7 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -812,6 +812,6 @@ ssize_t nbd_receive_reply(QIOChannel *ioc, NBDReply > *reply) > LOG("invalid magic (got 0x%" PRIx32 ")", magic); > return -EINVAL; > } > - return 0; > + return sizeof(buf);
I'd (weakly) prefer for this function to instead return an error value (e.g. -EPIPE as the commit message says) if read_sync() returned 0. This is because a return value of 0 is often used to signify success and it's always a bit weird if functions to not adhere to this convention. Just a rather weak preference, though. So: Reviewed-by: Max Reitz <mre...@redhat.com> And generously leaving it to you (whether) to apply the patch. O:-) Max > } > >
signature.asc
Description: OpenPGP digital signature