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 <vsement...@openvz.org>
(a bit weak, as it really hard to imagine all these paths and possible
consequences :/
