It is unnecessary to check nbd_client_connected() because every time
s->state is moved out of NBD_CLIENT_CONNECTED the socket is shut down
and all coroutines are resumed.
The only case where it was actually needed is when the NBD
server disconnects and there is no reconnect-delay. In that
case, nbd_receive_replies() does not set s->reply.handle and
nbd_co_do_receive_one_chunk() cannot continue. For that one case,
check the return value of nbd_receive_replies().
As to the others:
* nbd_receive_replies() can put the current coroutine to sleep if another
reply is ongoing; then it will be woken by nbd_channel_error(), called
by the ongoing reply. Or it can try itself to read a reply header and
fail, thus calling nbd_channel_error() itself.
* nbd_co_send_request() will write the body of the request and fail
* nbd_reply_chunk_iter_receive() will call nbd_co_receive_one_chunk()
and then nbd_co_do_receive_one_chunk(), which will handle the failure as
above; or it will just detect a previous call to nbd_iter_channel_error()
via iter->ret < 0.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
block/nbd.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 3cc4ea4430..a30603ce87 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -409,10 +409,6 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle)
return 0;
}
- if (!nbd_client_connected(s)) {
- return -EIO;
- }
-
if (s->reply.handle != 0) {
/*
* Some other request is being handled now. It should already be
@@ -512,7 +508,7 @@ static int coroutine_fn nbd_co_send_request(BlockDriverState *bs,
if (qiov) {
qio_channel_set_cork(s->ioc, true);
rc = nbd_send_request(s->ioc, request);
- if (nbd_client_connected(s) && rc >= 0) {
+ if (rc >= 0) {
if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
NULL) < 0) {
rc = -EIO;
@@ -829,8 +825,8 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
}
*request_ret = 0;
- nbd_receive_replies(s, handle);
- if (!nbd_client_connected(s)) {
+ ret = nbd_receive_replies(s, handle);
+ if (ret < 0) {
error_setg(errp, "Connection closed");
return -EIO;
}
@@ -982,11 +978,6 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
NBDReply local_reply;
NBDStructuredReplyChunk *chunk;
Error *local_err = NULL;
- if (!nbd_client_connected(s)) {
- error_setg(&local_err, "Connection closed");
- nbd_iter_channel_error(iter, -EIO, &local_err);
- goto break_loop;
- }
if (iter->done) {
/* Previous iteration was last. */
@@ -1007,7 +998,7 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
}
/* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
- if (nbd_reply_is_simple(reply) || !nbd_client_connected(s)) {
+ if (nbd_reply_is_simple(reply) || iter->ret < 0) {
goto break_loop;
}
--
2.35.114.04.2022 20:57, Paolo Bonzini wrote:
> It is unnecessary to check nbd_client_connected() because every time
> s->state is moved out of NBD_CLIENT_CONNECTED the socket is shut down
> and all coroutines are resumed.
>
> The only case where it was actually needed is when the NBD
> server disconnects and there is no reconnect-delay. In that
> case, nbd_receive_replies() does not set s->reply.handle and
> nbd_co_do_receive_one_chunk() cannot continue. For that one case,
> check the return value of nbd_receive_replies().
>
> As to the others:
>
> * nbd_receive_replies() can put the current coroutine to sleep if another
> reply is ongoing; then it will be woken by nbd_channel_error(), called
> by the ongoing reply. Or it can try itself to read a reply header and
> fail, thus calling nbd_channel_error() itself.
>
> * nbd_co_send_request() will write the body of the request and fail
>
> * nbd_reply_chunk_iter_receive() will call nbd_co_receive_one_chunk()
> and then nbd_co_do_receive_one_chunk(), which will handle the failure as
> above; or it will just detect a previous call to nbd_iter_channel_error()
> via iter->ret < 0.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/nbd.c | 17 ++++-------------
> 1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 3cc4ea4430..a30603ce87 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -409,10 +409,6 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle)
> return 0;
> }
>
> - if (!nbd_client_connected(s)) {
> - return -EIO;
> - }
> -
> if (s->reply.handle != 0) {
> /*
> * Some other request is being handled now. It should already be
> @@ -512,7 +508,7 @@ static int coroutine_fn nbd_co_send_request(BlockDriverState *bs,
> if (qiov) {
> qio_channel_set_cork(s->ioc, true);
> rc = nbd_send_request(s->ioc, request);
> - if (nbd_client_connected(s) && rc >= 0) {
> + if (rc >= 0) {
> if (qio_channel_writev_all(s->ioc, qiov->iov, qiov->niov,
> NULL) < 0) {
> rc = -EIO;
> @@ -829,8 +825,8 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
> }
> *request_ret = 0;
>
> - nbd_receive_replies(s, handle);
> - if (!nbd_client_connected(s)) {
> + ret = nbd_receive_replies(s, handle);
> + if (ret < 0) {
> error_setg(errp, "Connection closed");
> return -EIO;
> }
> @@ -982,11 +978,6 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
> NBDReply local_reply;
> NBDStructuredReplyChunk *chunk;
> Error *local_err = NULL;
> - if (!nbd_client_connected(s)) {
> - error_setg(&local_err, "Connection closed");
> - nbd_iter_channel_error(iter, -EIO, &local_err);
> - goto break_loop;
> - }
Probably we should still check iter->ret here. It's strange to start new iteration, when user set iter->ret in previous iteration of NBD_FOREACH_REPLY_CHUNK()
Or, maybe we should set iter->done in nbd_iter_channel_error ?
>
> if (iter->done) {
> /* Previous iteration was last. */
> @@ -1007,7 +998,7 @@ static bool nbd_reply_chunk_iter_receive(BDRVNBDState *s,
> }
>
> /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple reply. */
> - if (nbd_reply_is_simple(reply) || !nbd_client_connected(s)) {
> + if (nbd_reply_is_simple(reply) || iter->ret < 0) {
And then here, may be we can just goto break_loop from previous "if (ret < 0)". Then we'll not have to check iter->ret.
> goto break_loop;
> }
>
anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
(a bit weak, as it really hard to imagine all these paths and possible consequences :/
--
Best regards,
Vladimir
Hi,
thanks for the careful review and sorry I'm only replying now.
On 4/15/22 19:01, Vladimir Sementsov-Ogievskiy wrote:
>> @@ -982,11 +978,6 @@ static bool
>> nbd_reply_chunk_iter_receive(BDRVNBDState *s,
>> NBDReply local_reply;
>> NBDStructuredReplyChunk *chunk;
>> Error *local_err = NULL;
>> - if (!nbd_client_connected(s)) {
>> - error_setg(&local_err, "Connection closed");
>> - nbd_iter_channel_error(iter, -EIO, &local_err);
>> - goto break_loop;
>> - }
>
> Probably we should still check iter->ret here. It's strange to start new
> iteration, when user set iter->ret in previous iteration of
> NBD_FOREACH_REPLY_CHUNK()
>
> Or, maybe we should set iter->done in nbd_iter_channel_error ?
Yes, this second one is a possibility. I chose to check iter->ret below
because it was a bit more self-contained ("before reading again check
that the error code is not overwritten"), but it is also less obvious.
Paolo
>> if (iter->done) {
>> /* Previous iteration was last. */
>> @@ -1007,7 +998,7 @@ static bool
>> nbd_reply_chunk_iter_receive(BDRVNBDState *s,
>> }
>> /* Do not execute the body of NBD_FOREACH_REPLY_CHUNK for simple
>> reply. */
>> - if (nbd_reply_is_simple(reply) || !nbd_client_connected(s)) {
>> + if (nbd_reply_is_simple(reply) || iter->ret < 0) {
>
> And then here, may be we can just goto break_loop from previous "if (ret
> < 0)". Then we'll not have to check iter->ret.
>
>> goto break_loop;
>> }
>
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>
> (a bit weak, as it really hard to imagine all these paths and possible
> consequences :/
>
© 2016 - 2026 Red Hat, Inc.