Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/1072857/?format=api
{ "id": 1072857, "url": "http://patchwork.ozlabs.org/api/patches/1072857/?format=api", "web_url": "http://patchwork.ozlabs.org/project/qemu-devel/patch/20190401140903.19186-3-eblake@redhat.com/", "project": { "id": 14, "url": "http://patchwork.ozlabs.org/api/projects/14/?format=api", "name": "QEMU Development", "link_name": "qemu-devel", "list_id": "qemu-devel.nongnu.org", "list_email": "qemu-devel@nongnu.org", "web_url": "", "scm_url": "", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20190401140903.19186-3-eblake@redhat.com>", "list_archive_url": null, "date": "2019-04-01T14:08:51", "name": "[PULL,02/14] nbd: Tolerate some server non-compliance in NBD_CMD_BLOCK_STATUS", "commit_ref": null, "pull_url": null, "state": "new", "archived": false, "hash": "316abda49bd25b8701a986f6e5185764b4cb1ba3", "submitter": { "id": 6591, "url": "http://patchwork.ozlabs.org/api/people/6591/?format=api", "name": "Eric Blake", "email": "eblake@redhat.com" }, "delegate": null, "mbox": "http://patchwork.ozlabs.org/project/qemu-devel/patch/20190401140903.19186-3-eblake@redhat.com/mbox/", "series": [ { "id": 100345, "url": "http://patchwork.ozlabs.org/api/series/100345/?format=api", "web_url": "http://patchwork.ozlabs.org/project/qemu-devel/list/?series=100345", "date": "2019-04-01T14:08:49", "name": "[PULL,01/14] qemu-img: Report bdrv_block_status failures", "version": 1, "mbox": "http://patchwork.ozlabs.org/series/100345/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/1072857/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/1072857/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>", "X-Original-To": "incoming@patchwork.ozlabs.org", "Delivered-To": "patchwork-incoming@bilbo.ozlabs.org", "Authentication-Results": [ "ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=209.51.188.17; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=<UNKNOWN>)", "ozlabs.org;\n\tdmarc=fail (p=none dis=none) header.from=redhat.com" ], "Received": [ "from lists.gnu.org (lists.gnu.org [209.51.188.17])\n\t(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 44XvhX3vLjz9sPv\n\tfor <incoming@patchwork.ozlabs.org>;\n\tTue, 2 Apr 2019 01:23:24 +1100 (AEDT)", "from localhost ([127.0.0.1]:36745 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>)\n\tid 1hAxqL-0008Dz-Ru\n\tfor incoming@patchwork.ozlabs.org; Mon, 01 Apr 2019 10:23:21 -0400", "from eggs.gnu.org ([209.51.188.92]:45766)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from <eblake@redhat.com>) id 1hAxce-0001tZ-Jn\n\tfor qemu-devel@nongnu.org; Mon, 01 Apr 2019 10:09:13 -0400", "from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from <eblake@redhat.com>) id 1hAxcd-0004ih-6b\n\tfor qemu-devel@nongnu.org; Mon, 01 Apr 2019 10:09:12 -0400", "from mx1.redhat.com ([209.132.183.28]:48902)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from <eblake@redhat.com>)\n\tid 1hAxca-0004gQ-6W; Mon, 01 Apr 2019 10:09:08 -0400", "from smtp.corp.redhat.com\n\t(int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 859B03D37;\n\tMon, 1 Apr 2019 14:09:07 +0000 (UTC)", "from blue.redhat.com (ovpn-116-75.phx2.redhat.com [10.3.116.75])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id D496419C70;\n\tMon, 1 Apr 2019 14:09:06 +0000 (UTC)" ], "From": "Eric Blake <eblake@redhat.com>", "To": "qemu-devel@nongnu.org", "Date": "Mon, 1 Apr 2019 09:08:51 -0500", "Message-Id": "<20190401140903.19186-3-eblake@redhat.com>", "In-Reply-To": "<20190401140903.19186-1-eblake@redhat.com>", "References": "<20190401140903.19186-1-eblake@redhat.com>", "MIME-Version": "1.0", "X-Scanned-By": "MIMEDefang 2.84 on 10.5.11.23", "X-Greylist": "Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tMon, 01 Apr 2019 14:09:07 +0000 (UTC)", "Content-Transfer-Encoding": "quoted-printable", "X-detected-operating-system": "by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]", "X-Received-From": "209.132.183.28", "Subject": "[Qemu-devel] [PULL 02/14] nbd: Tolerate some server non-compliance\n\tin NBD_CMD_BLOCK_STATUS", "X-BeenThere": "qemu-devel@nongnu.org", "X-Mailman-Version": "2.1.21", "Precedence": "list", "List-Id": "<qemu-devel.nongnu.org>", "List-Unsubscribe": "<https://lists.nongnu.org/mailman/options/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>", "List-Archive": "<http://lists.nongnu.org/archive/html/qemu-devel/>", "List-Post": "<mailto:qemu-devel@nongnu.org>", "List-Help": "<mailto:qemu-devel-request@nongnu.org?subject=help>", "List-Subscribe": "<https://lists.nongnu.org/mailman/listinfo/qemu-devel>,\n\t<mailto:qemu-devel-request@nongnu.org?subject=subscribe>", "Cc": "Kevin Wolf <kwolf@redhat.com>,\n\tVladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,\n\t\"Richard W . M . Jones\" <rjones@redhat.com>,\n\t\"open list:Network Block Dev...\" <qemu-block@nongnu.org>,\n\tMax Reitz <mreitz@redhat.com>", "Errors-To": "qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org", "Sender": "\"Qemu-devel\"\n\t<qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org>" }, "content": "The NBD spec states that NBD_CMD_FLAG_REQ_ONE (which we currently\nalways use) should not reply with an extent larger than our request,\nand that the server's response should be exactly one extent. Right\nnow, that means that if a server sends more than one extent, we treat\nthe server as broken, fail the block status request, and disconnect,\nwhich prevents all further use of the block device. But while good\nsoftware should be strict in what it sends, it should be tolerant in\nwhat it receives.\n\nWhile trying to implement NBD_CMD_BLOCK_STATUS in nbdkit, we\ntemporarily had a non-compliant server sending too many extents in\nspite of REQ_ONE. Oddly enough, 'qemu-img convert' with qemu 3.1\nfailed with a somewhat useful message:\n qemu-img: Protocol error: invalid payload for NBD_REPLY_TYPE_BLOCK_STATUS\n\nwhich then disappeared with commit d8b4bad8, on the grounds that an\nerror message flagged only at the time of coroutine teardown is\npointless, and instead we should rely on the actual failed API to\nreport an error - in other words, the 3.1 behavior was masking the\nfact that qemu-img was not reporting an error. That has since been\nfixed in the previous patch, where qemu-img convert now fails with:\n qemu-img: error while reading block status of sector 0: Invalid argument\n\nBut even that is harsh. Since we already partially relaxed things in\ncommit acfd8f7a to tolerate a server that exceeds the cap (although\nthat change was made prior to the NBD spec actually putting a cap on\nthe extent length during REQ_ONE - in fact, the NBD spec change was\nBECAUSE of the qemu behavior prior to that commit), it's not that much\nharder to argue that we should also tolerate a server that sends too\nmany extents. But at the same time, it's nice to trace when we are\nbeing tolerant of server non-compliance, in order to help server\nwriters fix their implementations to be more portable (if they refer\nto our traces, rather than just stderr).\n\nReported-by: Richard W.M. Jones <rjones@redhat.com>\nSigned-off-by: Eric Blake <eblake@redhat.com>\nMessage-Id: <20190323212639.579-3-eblake@redhat.com>\nReviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>\n---\n block/nbd-client.c | 21 ++++++++++++++++-----\n block/trace-events | 1 +\n 2 files changed, 17 insertions(+), 5 deletions(-)", "diff": "diff --git a/block/nbd-client.c b/block/nbd-client.c\nindex bfbaf7ebe94..8fe660b6093 100644\n--- a/block/nbd-client.c\n+++ b/block/nbd-client.c\n@@ -240,8 +240,8 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,\n }\n\n /* nbd_parse_blockstatus_payload\n- * support only one extent in reply and only for\n- * base:allocation context\n+ * Based on our request, we expect only one extent in reply, for the\n+ * base:allocation context.\n */\n static int nbd_parse_blockstatus_payload(NBDClientSession *client,\n NBDStructuredReplyChunk *chunk,\n@@ -250,7 +250,8 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,\n {\n uint32_t context_id;\n\n- if (chunk->length != sizeof(context_id) + sizeof(*extent)) {\n+ /* The server succeeded, so it must have sent [at least] one extent */\n+ if (chunk->length < sizeof(context_id) + sizeof(*extent)) {\n error_setg(errp, \"Protocol error: invalid payload for \"\n \"NBD_REPLY_TYPE_BLOCK_STATUS\");\n return -EINVAL;\n@@ -276,10 +277,20 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,\n return -EINVAL;\n }\n\n- /* The server is allowed to send us extra information on the final\n- * extent; just clamp it to the length we requested. */\n+ /*\n+ * We used NBD_CMD_FLAG_REQ_ONE, so the server should not have\n+ * sent us any more than one extent, nor should it have included\n+ * status beyond our request in that extent. However, it's easy\n+ * enough to ignore the server's noncompliance without killing the\n+ * connection; just ignore trailing extents, and clamp things to\n+ * the length of our request.\n+ */\n+ if (chunk->length > sizeof(context_id) + sizeof(*extent)) {\n+ trace_nbd_parse_blockstatus_compliance(\"more than one extent\");\n+ }\n if (extent->length > orig_length) {\n extent->length = orig_length;\n+ trace_nbd_parse_blockstatus_compliance(\"extent length too large\");\n }\n\n return 0;\ndiff --git a/block/trace-events b/block/trace-events\nindex e6bb5a8f05c..debb25c0ac8 100644\n--- a/block/trace-events\n+++ b/block/trace-events\n@@ -157,6 +157,7 @@ nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) \"s %p iov[%d] %p pa\n iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, uint64_t bytes, int ret) \"src_lun %p offset %\"PRIu64\" dst_lun %p offset %\"PRIu64\" bytes %\"PRIu64\" ret %d\"\n\n # nbd-client.c\n+nbd_parse_blockstatus_compliance(const char *err) \"ignoring extra data from non-compliant server: %s\"\n nbd_read_reply_entry_fail(int ret, const char *err) \"ret = %d, err: %s\"\n nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) \"Request failed { .from = %\" PRIu64\", .len = %\" PRIu32 \", .handle = %\" PRIu64 \", .flags = 0x%\" PRIx16 \", .type = %\" PRIu16 \" (%s) } ret = %d, err: %s\"\n\n", "prefixes": [ "PULL", "02/14" ] }