| Message ID | 20190325190104.30213-2-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: simple errors to BLOCK_STATUS | expand |
25.03.2019 22:01, Eric Blake wrote: > When the server replies with a (structured [*]) error to > NBD_CMD_BLOCK_STATUS, without any extent information sent first, the > client code was blindly throwing away the server's error code and > instead telling the caller that EIO occurred. This has been broken > since its introduction in 78a33ab5 (v2.12, where we should have called: > error_setg(&local_err, "Server did not reply with any status extents"); > nbd_iter_error(&iter, false, -EIO, &local_err); > to declare the situation as a non-fatal error if no earlier error had > already been flagged, rather than just blindly slamming iter.err and > iter.ret), although it is more noticeable since commit 7f86068d, which > actually tries hard to preserve the server's code thanks to a separate > iter.request_ret. > > [*] The spec is clear that the server is also permitted to reply with > a simple error, but that's a separate fix. > > I was able to provoke this scenario with a hack to the server, then > seeing whether ENOMEM makes it back to the caller: > > | diff --git a/nbd/server.c b/nbd/server.c > | index fd013a2817a..29c7995de02 100644 > | --- a/nbd/server.c > | +++ b/nbd/server.c > | @@ -2269,6 +2269,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, > | "discard failed", errp); > | > | case NBD_CMD_BLOCK_STATUS: > | + return nbd_send_generic_reply(client, request->handle, -ENOMEM, > | + "no status for you today", errp); > | if (!request->len) { > | return nbd_send_generic_reply(client, request->handle, -EINVAL, > | "need non-zero length", errp); > | -- > > Signed-off-by: Eric Blake <eblake@redhat.com> > Message-Id: <20190323142455.5301-1-eblake@redhat.com> > --- > block/nbd-client.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/block/nbd-client.c b/block/nbd-client.c > index 8fe660b6093..b37a5963013 100644 > --- a/block/nbd-client.c > +++ b/block/nbd-client.c > @@ -769,12 +769,9 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, > payload = NULL; > } > > - if (!extent->length && !iter.err) { > - error_setg(&iter.err, > - "Server did not reply with any status extents"); > - if (!iter.ret) { > - iter.ret = -EIO; > - } > + if (!extent->length && !iter.request_ret) { > + error_setg(&local_err, "Server did not reply with any status extents"); > + nbd_iter_channel_error(&iter, -EIO, &local_err); > } So, replying with error and not sending any extents is not violation of the protocol and not a reason for channel_error, agreed. Thank you for describing and splitting this. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/block/nbd-client.c b/block/nbd-client.c index 8fe660b6093..b37a5963013 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -769,12 +769,9 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s, payload = NULL; } - if (!extent->length && !iter.err) { - error_setg(&iter.err, - "Server did not reply with any status extents"); - if (!iter.ret) { - iter.ret = -EIO; - } + if (!extent->length && !iter.request_ret) { + error_setg(&local_err, "Server did not reply with any status extents"); + nbd_iter_channel_error(&iter, -EIO, &local_err); } error_propagate(errp, iter.err);
When the server replies with a (structured [*]) error to NBD_CMD_BLOCK_STATUS, without any extent information sent first, the client code was blindly throwing away the server's error code and instead telling the caller that EIO occurred. This has been broken since its introduction in 78a33ab5 (v2.12, where we should have called: error_setg(&local_err, "Server did not reply with any status extents"); nbd_iter_error(&iter, false, -EIO, &local_err); to declare the situation as a non-fatal error if no earlier error had already been flagged, rather than just blindly slamming iter.err and iter.ret), although it is more noticeable since commit 7f86068d, which actually tries hard to preserve the server's code thanks to a separate iter.request_ret. [*] The spec is clear that the server is also permitted to reply with a simple error, but that's a separate fix. I was able to provoke this scenario with a hack to the server, then seeing whether ENOMEM makes it back to the caller: | diff --git a/nbd/server.c b/nbd/server.c | index fd013a2817a..29c7995de02 100644 | --- a/nbd/server.c | +++ b/nbd/server.c | @@ -2269,6 +2269,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, | "discard failed", errp); | | case NBD_CMD_BLOCK_STATUS: | + return nbd_send_generic_reply(client, request->handle, -ENOMEM, | + "no status for you today", errp); | if (!request->len) { | return nbd_send_generic_reply(client, request->handle, -EINVAL, | "need non-zero length", errp); | -- Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20190323142455.5301-1-eblake@redhat.com> --- block/nbd-client.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)