Adding some cc's; my reply truncates, so if you want to catch up to the full original mail, see https://www.redhat.com/archives/libguestfs/2019-March/msg00124.html On 3/23/19 12:00 PM, Richard W.M. Jones wrote: ... Thanks, that helps. So you hack the server into sending adjacent replies that were not coalesced, and sending a reply that covers 128k of the image regardless of how much length the client requested. There are several ways in which that can violate protocol (but hey - testing qemu resiliency is WORTH temporary protocol violations): - If the client requested REQ_ONE, you sent too many extents (sadly, qemu still uses REQ_ONE always, even though I'd like to lift that restriction in the future; the rest of my bullets are applicable when REQ_ONE is not in use) - If the client requested an offset that is less than 128k from end-of-image, you sent a reply that exceeds the advertised image size - If the client requested a length < 127k, you sent too many extents But there are other situations where your reply is valid: - If the client requested a length in [127k, 128k), your reply gave more information than the client asked for, but the protocol allows that because your extra information lies in the final extent when REQ_ONE is not in use - If the client requested a length > 128k, your reply ended early, but the protocol allows that So the immediate NBD_CMD_DISC is reasonable, implying that the client flagged one of the ways you could have violated protocol (but whether qemu should just hang up, or should just ignore that particular reply but still keep the connection alive, is a QoI question for qemu). Which says that qemu used to be noisy at diagnosing the protocol error, and now it is not. Ideally, qemu as a client shouldn't encounter a server that is violating protocol, but when it does, it makes sense for qemu to call attention to the buggy server. That sounds like a regression in qemu worth fixing, so I'll whip up a patch. That's also helpful, as it pinpoints where we regressed at flagging the protocol error. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

[adding qemu] On 3/23/19 6:57 AM, Eric Blake wrote: Ouch - upstream qemu still has a latent bug in this area. When qemu as server sends an error to NBD_CMD_BLOCK_STATUS, it uses a structured error reply, so we haven't tickled it before. But the NBD spec permits a simple error reply to any command except NBD_CMD_READ when structured replies are negotiated, and that's what nbdkit currently implements. If I add this hack patch to qemu's server (to intentionally fail NBD_CMD_BLOCK_STATUS with a simple error), then the qemu client _should_ be able to just report that block status failed and keep the connection alive, rather than its current behavior of hanging up. So we still need a patch for qemu 4.0 for the client to accept simple errors. :( With the hack applied, I got: $ ./qemu-io -f raw nbd://localhost:10809 --trace=nbd_\* -c map ... 17218@1553343857.048440:nbd_send_request Sending request to server: { .from = 0, .len = 1049088, .handle = 94796979915216, .flags = 0x8, .type = 7 (block status) } 17218@1553343857.048643:nbd_receive_simple_reply Got simple reply: { .error = 22 (EINVAL), handle = 94796979915216 } 17218@1553343857.048655:nbd_co_request_fail Request failed { .from = 0, .len = 1049088, .handle = 94796979915216, .flags = 0x8, .type = 7 (block status) } ret = -22, err: Protocol error: simple reply when structured reply chunk was expected Failed to get allocation status: Invalid argument diff --git i/nbd/server.c w/nbd/server.c index fd013a2817a..35f549abbf9 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -2148,16 +2148,16 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, * Returns 0 if connection is still live, -errno on failure to talk to client */ static coroutine_fn int nbd_send_generic_reply(NBDClient *client, - uint64_t handle, + NBDRequest *request, int ret, const char *error_msg, Error **errp) { - if (client->structured_reply && ret < 0) { - return nbd_co_send_structured_error(client, handle, -ret, error_msg, + if (client->structured_reply && ret < 0 && request->type == NBD_CMD_READ) { + return nbd_co_send_structured_error(client, request->handle, -ret, error_msg, errp); } else { - return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0, + return nbd_co_send_simple_reply(client, request->handle, ret < 0 ? -ret : 0, NULL, 0, errp); } } @@ -2177,7 +2177,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, if (request->flags & NBD_CMD_FLAG_FUA) { ret = blk_co_flush(exp->blk); if (ret < 0) { - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "flush failed", errp); } } @@ -2192,7 +2192,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request, ret = blk_pread(exp->blk, request->from + exp->dev_offset, data, request->len); if (ret < 0 || request->type == NBD_CMD_CACHE) { - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "reading from file failed", errp); } @@ -2234,7 +2234,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } ret = blk_pwrite(exp->blk, request->from + exp->dev_offset, data, request->len, flags); - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "writing to file failed", errp); case NBD_CMD_WRITE_ZEROES: @@ -2247,7 +2247,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, } ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset, request->len, flags); - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "writing to file failed", errp); case NBD_CMD_DISC: @@ -2256,7 +2256,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, case NBD_CMD_FLUSH: ret = blk_co_flush(exp->blk); - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "flush failed", errp); case NBD_CMD_TRIM: @@ -2265,12 +2265,14 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) { ret = blk_co_flush(exp->blk); } - return nbd_send_generic_reply(client, request->handle, ret, + return nbd_send_generic_reply(client, request, ret, "discard failed", errp); case NBD_CMD_BLOCK_STATUS: + return nbd_send_generic_reply(client, request, -EINVAL, + "no status for you today", errp); if (!request->len) { - return nbd_send_generic_reply(client, request->handle, -EINVAL, + return nbd_send_generic_reply(client, request, -EINVAL, "need non-zero length", errp); } if (client->export_meta.valid && @@ -2304,7 +2306,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, return ret; } else { - return nbd_send_generic_reply(client, request->handle, -EINVAL, + return nbd_send_generic_reply(client, request, -EINVAL, "CMD_BLOCK_STATUS not negotiated", errp); } @@ -2312,7 +2314,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, default: msg = g_strdup_printf("invalid request type (%" PRIu32 ") received", request->type); - ret = nbd_send_generic_reply(client, request->handle, -EINVAL, msg, + ret = nbd_send_generic_reply(client, request, -EINVAL, msg, errp); g_free(msg); return ret; @@ -2357,7 +2359,7 @@ static coroutine_fn void nbd_trip(void *opaque) Error *export_err = local_err; local_err = NULL; - ret = nbd_send_generic_reply(client, request.handle, -EINVAL, + ret = nbd_send_generic_reply(client, &request, -EINVAL, error_get_pretty(export_err), &local_err); error_free(export_err); } else { -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org