[{"id":1775001,"web_url":"http://patchwork.ozlabs.org/comment/1775001/","msgid":"","list_archive_url":null,"date":"2017-09-25T22:19:07","subject":"Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/25/2017 08:58 AM, Vladimir Sementsov-Ogievskiy wrote:\n> Minimal implementation: drop most of additional error information.\n> \n> Signed-off-by: Vladimir Sementsov-Ogievskiy \n> ---\n> block/nbd-client.h | 2 +\n> include/block/nbd.h | 15 ++++-\n> block/nbd-client.c | 97 +++++++++++++++++++++++++-----\n> nbd/client.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++-----\n> 4 files changed, 249 insertions(+), 34 deletions(-)\n> \n\n> +++ b/include/block/nbd.h\n> @@ -57,11 +57,17 @@ struct NBDRequest {\n> };\n> typedef struct NBDRequest NBDRequest;\n> \n> -struct NBDReply {\n> +typedef struct NBDReply {\n> + bool simple;\n> uint64_t handle;\n> uint32_t error;\n> -};\n> -typedef struct NBDReply NBDReply;\n> +\n> + uint16_t flags;\n> + uint16_t type;\n> + uint32_t tail_length;\n> + uint64_t offset;\n> + uint32_t hole_size;\n> +} NBDReply;\n\nThis feels like it should be a discriminated union, rather than a struct\ncontaining fields that are only sometimes valid...\n\n> \n> #define NBD_SREP_TYPE_NONE 0\n> #define NBD_SREP_TYPE_OFFSET_DATA 1\n> +#define NBD_SREP_TYPE_OFFSET_HOLE 2\n> #define NBD_SREP_TYPE_ERROR NBD_SREP_ERR(1)\n> +#define NBD_SREP_TYPE_ERROR_OFFSET NBD_SREP_ERR(2)\n\n...especially since there is more than one type of SREP packet layout.\n\nI also wonder why you are defining constants in a piecemeal fashion,\nrather than all at once (even if your minimal server implementation\ndoesn't send a particular constant, there's no harm in defining them all\nup front).\n\n> +++ b/block/nbd-client.c\n> @@ -179,9 +179,10 @@ err:\n> return rc;\n> }\n> \n> -static int nbd_co_receive_reply(NBDClientSession *s,\n> - uint64_t handle,\n> - QEMUIOVector *qiov)\n> +static int nbd_co_receive_1_reply_or_chunk(NBDClientSession *s,\n\nLong name, and unusual to mix in \"1\" instead of \"one\". Would it be\nbetter to name it nbd_co_receive_chunk, where we declare that a simple\nreply is (roughly) the same amount of effort as a chunk in a structured\nreply?\n\n> + uint64_t handle,\n> + bool *cont,\n> + QEMUIOVector *qiov)\n> {\n\nNo documentation of the function?\n\n> int ret;\n> int i = HANDLE_TO_INDEX(s, handle);\n> @@ -191,29 +192,95 @@ static int nbd_co_receive_reply(NBDClientSession *s,\n> qemu_coroutine_yield();\n> s->requests[i].receiving = false;\n> if (!s->ioc || s->quit) {\n> - ret = -EIO;\n> - } else {\n> - assert(s->reply.handle == handle);\n> - ret = -s->reply.error;\n> - if (qiov && s->reply.error == 0) {\n> + *cont = false;\n> + return -EIO;\n> + }\n> +\n> + assert(s->reply.handle == handle);\n> + *cont = !(s->reply.simple || (s->reply.flags & NBD_SREP_FLAG_DONE));\n\nWe need to validate that the server is not sending us SREP chunks unless\nwe negotiated them. I'm thinking it might be better to do it here\n(maybe you did it somewhere else, but I haven't seen it yet; I'm\nreviewing the patch in textual order rather than the order in which\nthings are called).\n\n> + ret = -s->reply.error;\n> + if (ret < 0) {\n> + goto out;\n> + }\n> +\n> + if (s->reply.simple) {\n> + if (qiov) {\n> if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n> - NULL) < 0) {\n> - ret = -EIO;\n> - s->quit = true;\n> + NULL) < 0)\n> + {\n> + goto fatal;\n> }\n> }\n> + goto out;\n> + }\n> \n> - /* Tell the read handler to read another header. */\n> - s->reply.handle = 0;\n> + /* here we deal with successful structured reply */\n> + switch (s->reply.type) {\n> + QEMUIOVector sub_qiov;\n> + case NBD_SREP_TYPE_OFFSET_DATA:\n\nThis is putting a LOT of smarts directly into the receive routine.\nHere's where I was previously wondering (and I think Paolo as well)\nwhether it might be better to split the efforts: the generic function\nreads off the chunk information and any payload, but a per-command\ncallback function then parses the chunks. Especially since the\ndefinition of the chunks differs on a per-command basis (yes, the NBD\nspec will try to not reuse an SREP chunk type across multiple commands\nunless the semantics are similar, but that's a bit more fragile). This\nparticularly matters given my statement above that you want a\ndiscriminated union, rather than a struct that contains unused fields,\nfor handling different SREP chunk types.\n\nMy review has to pause here for now...","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y1JSQ4vjLz9s7G\n\tfor ;\n\tTue, 26 Sep 2017 08:20:38 +1000 (AEST)","from localhost ([::1]:44550 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwbjw-0000Ic-NC\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 18:20:36 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:58750)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwbif-00088K-N4\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 18:19:19 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwbie-00075J-Gr\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 18:19:17 -0400","from mx1.redhat.com ([209.132.183.28]:38200)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dwbiY-00073b-J0; Mon, 25 Sep 2017 18:19:10 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\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 8D6FA5D68C;\n\tMon, 25 Sep 2017 22:19:09 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 5AD556047C;\n\tMon, 25 Sep 2017 22:19:08 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 8D6FA5D68C","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"","Date":"Mon, 25 Sep 2017 17:19:07 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170925135801.144261-9-vsementsov@virtuozzo.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"6lEvbFlCtbXBckPlw0CQ9lSjRsU4TEpkN\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tMon, 25 Sep 2017 22:19:09 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1776202,"web_url":"http://patchwork.ozlabs.org/comment/1776202/","msgid":"","list_archive_url":null,"date":"2017-09-27T10:05:38","subject":"Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"26.09.2017 01:19, Eric Blake wrote:\n> On 09/25/2017 08:58 AM, Vladimir Sementsov-Ogievskiy wrote:\n>> Minimal implementation: drop most of additional error information.\n>>\n>> Signed-off-by: Vladimir Sementsov-Ogievskiy \n>> ---\n>> block/nbd-client.h | 2 +\n>> include/block/nbd.h | 15 ++++-\n>> block/nbd-client.c | 97 +++++++++++++++++++++++++-----\n>> nbd/client.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++-----\n>> 4 files changed, 249 insertions(+), 34 deletions(-)\n>>\n>> +++ b/include/block/nbd.h\n>> @@ -57,11 +57,17 @@ struct NBDRequest {\n>> };\n>> typedef struct NBDRequest NBDRequest;\n>> \n>> -struct NBDReply {\n>> +typedef struct NBDReply {\n>> + bool simple;\n>> uint64_t handle;\n>> uint32_t error;\n>> -};\n>> -typedef struct NBDReply NBDReply;\n>> +\n>> + uint16_t flags;\n>> + uint16_t type;\n>> + uint32_t tail_length;\n>> + uint64_t offset;\n>> + uint32_t hole_size;\n>> +} NBDReply;\n> This feels like it should be a discriminated union, rather than a struct\n> containing fields that are only sometimes valid...\n\nNo:\n\nsimple, handle and error are always valid\nflags, type, tail_length are valid for all structured replies\noffset and hole_size are valid for structured hole reply\n\nso, we have one case when all fields are valid.. how do you see it with \nunion, what is the real difference? I think it would be just a complication.\n\n>\n>> \n>> #define NBD_SREP_TYPE_NONE 0\n>> #define NBD_SREP_TYPE_OFFSET_DATA 1\n>> +#define NBD_SREP_TYPE_OFFSET_HOLE 2\n>> #define NBD_SREP_TYPE_ERROR NBD_SREP_ERR(1)\n>> +#define NBD_SREP_TYPE_ERROR_OFFSET NBD_SREP_ERR(2)\n> ...especially since there is more than one type of SREP packet layout.\n>\n> I also wonder why you are defining constants in a piecemeal fashion,\n> rather than all at once (even if your minimal server implementation\n> doesn't send a particular constant, there's no harm in defining them all\n> up front).\n\nhmm. just to not define unused constants. It doesn't matter, I can \ndefine them all if you prefer.\n\n>\n>> +++ b/block/nbd-client.c\n>> @@ -179,9 +179,10 @@ err:\n>> return rc;\n>> }\n>> \n>> -static int nbd_co_receive_reply(NBDClientSession *s,\n>> - uint64_t handle,\n>> - QEMUIOVector *qiov)\n>> +static int nbd_co_receive_1_reply_or_chunk(NBDClientSession *s,\n> Long name, and unusual to mix in \"1\" instead of \"one\". Would it be\n> better to name it nbd_co_receive_chunk, where we declare that a simple\n> reply is (roughly) the same amount of effort as a chunk in a structured\n> reply?\n>\n>> + uint64_t handle,\n>> + bool *cont,\n>> + QEMUIOVector *qiov)\n>> {\n> No documentation of the function?\n>\n>> int ret;\n>> int i = HANDLE_TO_INDEX(s, handle);\n>> @@ -191,29 +192,95 @@ static int nbd_co_receive_reply(NBDClientSession *s,\n>> qemu_coroutine_yield();\n>> s->requests[i].receiving = false;\n>> if (!s->ioc || s->quit) {\n>> - ret = -EIO;\n>> - } else {\n>> - assert(s->reply.handle == handle);\n>> - ret = -s->reply.error;\n>> - if (qiov && s->reply.error == 0) {\n>> + *cont = false;\n>> + return -EIO;\n>> + }\n>> +\n>> + assert(s->reply.handle == handle);\n>> + *cont = !(s->reply.simple || (s->reply.flags & NBD_SREP_FLAG_DONE));\n> We need to validate that the server is not sending us SREP chunks unless\n> we negotiated them. I'm thinking it might be better to do it here\n> (maybe you did it somewhere else, but I haven't seen it yet; I'm\n> reviewing the patch in textual order rather than the order in which\n> things are called).\n\nNo, I didn't. Will add (may be early, in reply_entry).\n\n>\n>> + ret = -s->reply.error;\n>> + if (ret < 0) {\n>> + goto out;\n>> + }\n>> +\n>> + if (s->reply.simple) {\n>> + if (qiov) {\n>> if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\n>> - NULL) < 0) {\n>> - ret = -EIO;\n>> - s->quit = true;\n>> + NULL) < 0)\n>> + {\n>> + goto fatal;\n>> }\n>> }\n>> + goto out;\n>> + }\n>> \n>> - /* Tell the read handler to read another header. */\n>> - s->reply.handle = 0;\n>> + /* here we deal with successful structured reply */\n>> + switch (s->reply.type) {\n>> + QEMUIOVector sub_qiov;\n>> + case NBD_SREP_TYPE_OFFSET_DATA:\n> This is putting a LOT of smarts directly into the receive routine.\n> Here's where I was previously wondering (and I think Paolo as well)\n> whether it might be better to split the efforts: the generic function\n> reads off the chunk information and any payload, but a per-command\n\nHmm. it was my idea to move all reading into one coroutine (in my \nrefactoring series, but Paolo was against).\n\nOr you mean to read a payload as raw? It would lead to double copying it \nto destination qiov, which I dislike..\n\n> callback function then parses the chunks.\n\nper-command? Then callback for CMD_READ would have all of these \n\"smarts\", so the whole code would not be simpler.. (However it will \nsimplify adding block-status later).\n\n> Especially since the\n> definition of the chunks differs on a per-command basis (yes, the NBD\n> spec will try to not reuse an SREP chunk type across multiple commands\n> unless the semantics are similar, but that's a bit more fragile). This\n> particularly matters given my statement above that you want a\n> discriminated union, rather than a struct that contains unused fields,\n> for handling different SREP chunk types.\n>\n> My review has to pause here for now...\n>\n>","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y2D4B24CBz9sPm\n\tfor ;\n\tWed, 27 Sep 2017 20:06:18 +1000 (AEST)","from localhost ([::1]:53822 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dx9EO-0007Jy-Co\n\tfor incoming@patchwork.ozlabs.org; Wed, 27 Sep 2017 06:06:16 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36488)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dx9E2-0007Jn-Um\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 06:05:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dx9Dw-00058I-VV\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 06:05:54 -0400","from mailhub.sw.ru ([195.214.232.25]:33828 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dx9Dw-00056f-Bu\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 06:05:48 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v8RA5cBU022419;\n\tWed, 27 Sep 2017 13:05:38 +0300 (MSK)"],"To":"Eric Blake , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"","Date":"Wed, 27 Sep 2017 13:05:38 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1776306,"web_url":"http://patchwork.ozlabs.org/comment/1776306/","msgid":"<407cbf8e-3854-e657-deb7-03d5c808bd82@virtuozzo.com>","list_archive_url":null,"date":"2017-09-27T12:32:18","subject":"Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"27.09.2017 13:05, Vladimir Sementsov-Ogievskiy wrote:\r\n> 26.09.2017 01:19, Eric Blake wrote:\r\n>> On 09/25/2017 08:58 AM, Vladimir Sementsov-Ogievskiy wrote:\r\n>>> Minimal implementation: drop most of additional error information.\r\n>>>\r\n>>> Signed-off-by: Vladimir Sementsov-Ogievskiy \r\n>>> ---\r\n>>>   block/nbd-client.h  |   2 +\r\n>>>   include/block/nbd.h |  15 ++++-\r\n>>>   block/nbd-client.c  |  97 +++++++++++++++++++++++++-----\r\n>>>   nbd/client.c        | 169 \r\n>>> +++++++++++++++++++++++++++++++++++++++++++++++-----\r\n>>>   4 files changed, 249 insertions(+), 34 deletions(-)\r\n>>>\r\n>>> +++ b/include/block/nbd.h\r\n>>> @@ -57,11 +57,17 @@ struct NBDRequest {\r\n>>>   };\r\n>>>   typedef struct NBDRequest NBDRequest;\r\n>>>   -struct NBDReply {\r\n>>> +typedef struct NBDReply {\r\n>>> +    bool simple;\r\n>>>       uint64_t handle;\r\n>>>       uint32_t error;\r\n>>> -};\r\n>>> -typedef struct NBDReply NBDReply;\r\n>>> +\r\n>>> +    uint16_t flags;\r\n>>> +    uint16_t type;\r\n>>> +    uint32_t tail_length;\r\n>>> +    uint64_t offset;\r\n>>> +    uint32_t hole_size;\r\n>>> +} NBDReply;\r\n>> This feels like it should be a discriminated union, rather than a struct\r\n>> containing fields that are only sometimes valid...\r\n>\r\n> No:\r\n>\r\n> simple, handle and error are always valid\r\n> flags, type, tail_length are valid for all structured replies\r\n> offset and hole_size are valid for structured hole reply\r\n>\r\n> so, we have one case when all fields are valid.. how do you see it \r\n> with union, what is the real difference? I think it would be just a \r\n> complication.\r\n>\r\n>>\r\n>>>     #define NBD_SREP_TYPE_NONE 0\r\n>>>   #define NBD_SREP_TYPE_OFFSET_DATA   1\r\n>>> +#define NBD_SREP_TYPE_OFFSET_HOLE   2\r\n>>>   #define NBD_SREP_TYPE_ERROR         NBD_SREP_ERR(1)\r\n>>> +#define NBD_SREP_TYPE_ERROR_OFFSET  NBD_SREP_ERR(2)\r\n>> ...especially since there is more than one type of SREP packet layout.\r\n>>\r\n>> I also wonder why you are defining constants in a piecemeal fashion,\r\n>> rather than all at once (even if your minimal server implementation\r\n>> doesn't send a particular constant, there's no harm in defining them all\r\n>> up front).\r\n>\r\n> hmm. just to not define unused constants. It doesn't matter, I can \r\n> define them all if you prefer.\r\n>\r\n>>\r\n>>> +++ b/block/nbd-client.c\r\n>>> @@ -179,9 +179,10 @@ err:\r\n>>>       return rc;\r\n>>>   }\r\n>>>   -static int nbd_co_receive_reply(NBDClientSession *s,\r\n>>> -                                uint64_t handle,\r\n>>> -                                QEMUIOVector *qiov)\r\n>>> +static int nbd_co_receive_1_reply_or_chunk(NBDClientSession *s,\r\n>> Long name, and unusual to mix in \"1\" instead of \"one\".  Would it be\r\n>> better to name it nbd_co_receive_chunk, where we declare that a simple\r\n>> reply is (roughly) the same amount of effort as a chunk in a structured\r\n>> reply?\r\n>>\r\n>>> + uint64_t handle,\r\n>>> +                                           bool *cont,\r\n>>> +                                           QEMUIOVector *qiov)\r\n>>>   {\r\n>> No documentation of the function?\r\n>>\r\n>>>       int ret;\r\n>>>       int i = HANDLE_TO_INDEX(s, handle);\r\n>>> @@ -191,29 +192,95 @@ static int \r\n>>> nbd_co_receive_reply(NBDClientSession *s,\r\n>>>       qemu_coroutine_yield();\r\n>>>       s->requests[i].receiving = false;\r\n>>>       if (!s->ioc || s->quit) {\r\n>>> -        ret = -EIO;\r\n>>> -    } else {\r\n>>> -        assert(s->reply.handle == handle);\r\n>>> -        ret = -s->reply.error;\r\n>>> -        if (qiov && s->reply.error == 0) {\r\n>>> +        *cont = false;\r\n>>> +        return -EIO;\r\n>>> +    }\r\n>>> +\r\n>>> +    assert(s->reply.handle == handle);\r\n>>> +    *cont = !(s->reply.simple || (s->reply.flags & \r\n>>> NBD_SREP_FLAG_DONE));\r\n>> We need to validate that the server is not sending us SREP chunks unless\r\n>> we negotiated them.  I'm thinking it might be better to do it here\r\n>> (maybe you did it somewhere else, but I haven't seen it yet; I'm\r\n>> reviewing the patch in textual order rather than the order in which\r\n>> things are called).\r\n>\r\n> No, I didn't. Will add (may be early, in reply_entry).\r\n>\r\n>>\r\n>>> +    ret = -s->reply.error;\r\n>>> +    if (ret < 0) {\r\n>>> +        goto out;\r\n>>> +    }\r\n>>> +\r\n>>> +    if (s->reply.simple) {\r\n>>> +        if (qiov) {\r\n>>>               if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,\r\n>>> -                                      NULL) < 0) {\r\n>>> -                ret = -EIO;\r\n>>> -                s->quit = true;\r\n>>> +                                      NULL) < 0)\r\n>>> +            {\r\n>>> +                goto fatal;\r\n>>>               }\r\n>>>           }\r\n>>> +        goto out;\r\n>>> +    }\r\n>>>   -        /* Tell the read handler to read another header. */\r\n>>> -        s->reply.handle = 0;\r\n>>> +    /* here we deal with successful structured reply */\r\n>>> +    switch (s->reply.type) {\r\n>>> +        QEMUIOVector sub_qiov;\r\n>>> +    case NBD_SREP_TYPE_OFFSET_DATA:\r\n>> This is putting a LOT of smarts directly into the receive routine.\r\n>> Here's where I was previously wondering (and I think Paolo as well)\r\n>> whether it might be better to split the efforts: the generic function\r\n>> reads off the chunk information and any payload, but a per-command\r\n>\r\n> Hmm. it was my idea to move all reading into one coroutine (in my \r\n> refactoring series, but Paolo was against).\r\n>\r\n> Or you mean to read a payload as raw? It would lead to double copying \r\n> it to destination qiov, which I dislike..\r\n>\r\n>> callback function then parses the chunks.\r\n>\r\n> per-command? Then callback for CMD_READ would have all of these \r\n> \"smarts\", so the whole code would not be simpler.. (However it will \r\n> simplify adding block-status later).\r\n>\r\n>>    Especially since the\r\n>> definition of the chunks differs on a per-command basis (yes, the NBD\r\n>> spec will try to not reuse an SREP chunk type across multiple commands\r\n>> unless the semantics are similar, but that's a bit more fragile).  This\r\n>> particularly matters given my statement above that you want a\r\n>> discriminated union, rather than a struct that contains unused fields,\r\n>> for handling different SREP chunk types.\r\n>>\r\n>> My review has to pause here for now...\r\n>>\r\n>>\r\n>\r\n>\r\n\r\nI'll post a proposal of extendable architecture for this today to have a \r\nmore concrete discussion.\r\n\r\n\r\n-- \r\nBest regards,\r\nVladimir","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y2HKW4pFbz9tXT\n\tfor ;\n\tWed, 27 Sep 2017 22:33:02 +1000 (AEST)","from localhost ([::1]:54632 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dxBWN-0005rK-4O\n\tfor incoming@patchwork.ozlabs.org; Wed, 27 Sep 2017 08:32:59 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:43194)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dxBVw-0005oW-L3\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 08:32:34 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dxBVt-00086r-Dm\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 08:32:32 -0400","from mailhub.sw.ru ([195.214.232.25]:16932 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dxBVs-00086B-TA\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 08:32:29 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v8RCWIlA023394;\n\tWed, 27 Sep 2017 15:32:18 +0300 (MSK)"],"From":"Vladimir Sementsov-Ogievskiy ","To":"Eric Blake , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t","Message-ID":"<407cbf8e-3854-e657-deb7-03d5c808bd82@virtuozzo.com>","Date":"Wed, 27 Sep 2017 15:32:18 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"base64","X-MIME-Autoconverted":"from 8bit to base64 by relay.sw.ru id v8RCWIlA023394","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, pbonzini@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1778817,"web_url":"http://patchwork.ozlabs.org/comment/1778817/","msgid":"","list_archive_url":null,"date":"2017-10-03T10:07:59","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 26/09/2017 00:19, Eric Blake wrote:\n>> + /* here we deal with successful structured reply */\n>> + switch (s->reply.type) {\n>> + QEMUIOVector sub_qiov;\n>> + case NBD_SREP_TYPE_OFFSET_DATA:\n> This is putting a LOT of smarts directly into the receive routine.\n> Here's where I was previously wondering (and I think Paolo as well)\n> whether it might be better to split the efforts: the generic function\n> reads off the chunk information and any payload, but a per-command\n> callback function then parses the chunks. Especially since the\n> definition of the chunks differs on a per-command basis (yes, the NBD\n> spec will try to not reuse an SREP chunk type across multiple commands\n> unless the semantics are similar, but that's a bit more fragile). This\n> particularly matters given my statement above that you want a\n> discriminated union, rather than a struct that contains unused fields,\n> for handling different SREP chunk types.\n\nI think there should be two kinds of replies: 1) read directly into a\nQEMUIOVector, using structured replies only as an encapsulation of the\npayload; 2) read a chunk at a time into malloc-ed memory, yielding back\nto the calling coroutine after receiving one complete chunk.\n\nIn the end this probably means that you have a read_chunk_header\nfunction and a read_chunk function. READ has a loop that calls\nread_chunk_header followed by direct reading into the QEMUIOVector,\nwhile everyone else calls read_chunk.\n\nMaybe qio_channel_readv/writev_full could have \"offset\" and \"bytes\"\narguments. Most code in iov_send_recv could be cut-and-pasted. (When\nsheepdog is converted to QIOChannel, iov_send_recv can go away).\n\nPaolo","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y5vr140jYz9t5c\n\tfor ;\n\tTue, 3 Oct 2017 21:08:32 +1100 (AEDT)","from localhost ([::1]:57549 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dzK7p-0005tt-MC\n\tfor incoming@patchwork.ozlabs.org; Tue, 03 Oct 2017 06:08:29 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35443)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dzK7U-0005tW-Mo\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 06:08:09 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dzK7P-0006Z8-0K\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 06:08:08 -0400","from mail-wm0-f52.google.com ([74.125.82.52]:50766)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16)\n\t(Exim 4.71) (envelope-from ) id 1dzK7O-0006Yn-PC\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 06:08:02 -0400","by mail-wm0-f52.google.com with SMTP id u138so15502041wmu.5\n\tfor ; Tue, 03 Oct 2017 03:08:02 -0700 (PDT)","from [192.168.10.150]\n\t(dynamic-adsl-78-12-246-117.clienti.tiscali.it.\n\t[78.12.246.117]) by smtp.gmail.com with ESMTPSA id\n\tg25sm8800220wmc.16.2017.10.03.03.08.00\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tTue, 03 Oct 2017 03:08:00 -0700 (PDT)"],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:cc:references:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-language\n\t:content-transfer-encoding;\n\tbh=+MaCi2wo1LUVFMBqrUQgj/kFExpuispkOUP6JCDKxo8=;\n\tb=ftnPkUyHqFtiVl9the/4sFsHjFRtY3P0xPWFk0hdhxfNlMO81zoEM3hyYkHF8zn6hY\n\tqlS1QI+y6D67kTjWV4SUvwJ+3a24Fi6P0c5hs6vynk0bwohkObav0gP0250Ph5OQquEm\n\t55imG6Y5i3/7ypdWsmFRdZQi/pFXFgvWVqtLl/DrpZ0p4K/zf0fc7qqN6wQVyY3dSTGh\n\t9urZqvye9jaaqA6GSin+exIwFCh6sa5wxXNVV19c+uSxMb3iaNb6WLM8mpOsuP5siRuA\n\tZyQ2ypmmiOWTKeTgnRF58c1lRGN4Z1fYnCvsvnfafeQ7lZ3C3UypNKMesVTykFL8DjWW\n\tOi7A==","X-Gm-Message-State":"AHPjjUjASQ65futG6wvO5g3PjxxLGf3e9te9X/g7vfKH0km2yM9tHq7K\n\tSo7KByjD1ijhGnJyvsYphzyg9g==","X-Google-Smtp-Source":"AOwi7QDHxCvkkcYWAWlmImTmQVuV9pCkO+rtah3inmETE05NCESWjO5z+li3/E+IzdXTRJLyYn4+8Q==","X-Received":"by 10.28.175.197 with SMTP id y188mr11382286wme.20.1507025281681;\n\tTue, 03 Oct 2017 03:08:01 -0700 (PDT)","To":"Eric Blake ,\n\tVladimir Sementsov-Ogievskiy ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t","From":"Paolo Bonzini ","Message-ID":"","Date":"Tue, 3 Oct 2017 12:07:59 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"74.125.82.52","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1778893,"web_url":"http://patchwork.ozlabs.org/comment/1778893/","msgid":"<3b2e222e-87ea-e134-64a8-f6394e502a14@virtuozzo.com>","list_archive_url":null,"date":"2017-10-03T12:26:10","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"03.10.2017 13:07, Paolo Bonzini wrote:\n> On 26/09/2017 00:19, Eric Blake wrote:\n>>> + /* here we deal with successful structured reply */\n>>> + switch (s->reply.type) {\n>>> + QEMUIOVector sub_qiov;\n>>> + case NBD_SREP_TYPE_OFFSET_DATA:\n>> This is putting a LOT of smarts directly into the receive routine.\n>> Here's where I was previously wondering (and I think Paolo as well)\n>> whether it might be better to split the efforts: the generic function\n>> reads off the chunk information and any payload, but a per-command\n>> callback function then parses the chunks. Especially since the\n>> definition of the chunks differs on a per-command basis (yes, the NBD\n>> spec will try to not reuse an SREP chunk type across multiple commands\n>> unless the semantics are similar, but that's a bit more fragile). This\n>> particularly matters given my statement above that you want a\n>> discriminated union, rather than a struct that contains unused fields,\n>> for handling different SREP chunk types.\n> I think there should be two kinds of replies: 1) read directly into a\n> QEMUIOVector, using structured replies only as an encapsulation of the\n\nwho should read to qiov? reply_entry, or calling coroutine?.. \nreply_entry should anyway\nparse reply, to understand should it read it all or read it to qiov (or \nyield back, and then\ncalling coroutine will read to qiov)..\n\n> payload; 2) read a chunk at a time into malloc-ed memory, yielding back\n> to the calling coroutine after receiving one complete chunk.\n>\n> In the end this probably means that you have a read_chunk_header\n> function and a read_chunk function. READ has a loop that calls\n> read_chunk_header followed by direct reading into the QEMUIOVector,\n> while everyone else calls read_chunk.\n>\n> Maybe qio_channel_readv/writev_full could have \"offset\" and \"bytes\"\n> arguments. Most code in iov_send_recv could be cut-and-pasted. (When\n> sheepdog is converted to QIOChannel, iov_send_recv can go away).\n>\n> Paolo","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y5yvd3xWhz9t2h\n\tfor ;\n\tTue, 3 Oct 2017 23:26:52 +1100 (AEDT)","from localhost ([::1]:58359 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dzMHh-0007Xq-Qp\n\tfor incoming@patchwork.ozlabs.org; Tue, 03 Oct 2017 08:26:49 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45761)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dzMHJ-0007VQ-TU\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 08:26:31 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dzMHD-0008Ln-UK\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 08:26:25 -0400","from mailhub.sw.ru ([195.214.232.25]:7522 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dzMHD-0008KL-IY\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 08:26:19 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v93CQAYS013681;\n\tTue, 3 Oct 2017 15:26:10 +0300 (MSK)"],"To":"Paolo Bonzini , Eric Blake ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<3b2e222e-87ea-e134-64a8-f6394e502a14@virtuozzo.com>","Date":"Tue, 3 Oct 2017 15:26:10 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1778929,"web_url":"http://patchwork.ozlabs.org/comment/1778929/","msgid":"","list_archive_url":null,"date":"2017-10-03T12:58:42","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"03.10.2017 13:07, Paolo Bonzini wrote:\n> On 26/09/2017 00:19, Eric Blake wrote:\n>>> + /* here we deal with successful structured reply */\n>>> + switch (s->reply.type) {\n>>> + QEMUIOVector sub_qiov;\n>>> + case NBD_SREP_TYPE_OFFSET_DATA:\n>> This is putting a LOT of smarts directly into the receive routine.\n>> Here's where I was previously wondering (and I think Paolo as well)\n>> whether it might be better to split the efforts: the generic function\n>> reads off the chunk information and any payload, but a per-command\n>> callback function then parses the chunks. Especially since the\n>> definition of the chunks differs on a per-command basis (yes, the NBD\n>> spec will try to not reuse an SREP chunk type across multiple commands\n>> unless the semantics are similar, but that's a bit more fragile). This\n>> particularly matters given my statement above that you want a\n>> discriminated union, rather than a struct that contains unused fields,\n>> for handling different SREP chunk types.\n> I think there should be two kinds of replies: 1) read directly into a\n> QEMUIOVector, using structured replies only as an encapsulation of the\n> payload; 2) read a chunk at a time into malloc-ed memory, yielding back\n> to the calling coroutine after receiving one complete chunk.\n>\n> In the end this probably means that you have a read_chunk_header\n> function and a read_chunk function. READ has a loop that calls\n> read_chunk_header followed by direct reading into the QEMUIOVector,\n> while everyone else calls read_chunk.\n\naccordingly to spec, we can receive several error reply chunks to any \nrequest,\nso loop, receiving them should be common for all requests I think\n\n>\n> Maybe qio_channel_readv/writev_full could have \"offset\" and \"bytes\"\n> arguments. Most code in iov_send_recv could be cut-and-pasted. (When\n> sheepdog is converted to QIOChannel, iov_send_recv can go away).\n>\n> Paolo","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y5zd964qpz9s03\n\tfor ;\n\tTue, 3 Oct 2017 23:59:25 +1100 (AEDT)","from localhost ([::1]:58466 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dzMnE-000142-00\n\tfor incoming@patchwork.ozlabs.org; Tue, 03 Oct 2017 08:59:24 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:54693)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dzMml-00011y-Lq\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 08:58:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dzMmg-0001ut-Sh\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 08:58:55 -0400","from mailhub.sw.ru ([195.214.232.25]:29760 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dzMmg-0001ry-DQ\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 08:58:50 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v93Cwg7D004220;\n\tTue, 3 Oct 2017 15:58:42 +0300 (MSK)"],"To":"Paolo Bonzini , Eric Blake ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"","Date":"Tue, 3 Oct 2017 15:58:42 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1778978,"web_url":"http://patchwork.ozlabs.org/comment/1778978/","msgid":"","list_archive_url":null,"date":"2017-10-03T13:35:03","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"03.10.2017 15:58, Vladimir Sementsov-Ogievskiy wrote:\n> 03.10.2017 13:07, Paolo Bonzini wrote:\n>> On 26/09/2017 00:19, Eric Blake wrote:\n>>>> +    /* here we deal with successful structured reply */\n>>>> +    switch (s->reply.type) {\n>>>> +        QEMUIOVector sub_qiov;\n>>>> +    case NBD_SREP_TYPE_OFFSET_DATA:\n>>> This is putting a LOT of smarts directly into the receive routine.\n>>> Here's where I was previously wondering (and I think Paolo as well)\n>>> whether it might be better to split the efforts: the generic function\n>>> reads off the chunk information and any payload, but a per-command\n>>> callback function then parses the chunks.  Especially since the\n>>> definition of the chunks differs on a per-command basis (yes, the NBD\n>>> spec will try to not reuse an SREP chunk type across multiple commands\n>>> unless the semantics are similar, but that's a bit more fragile).  This\n>>> particularly matters given my statement above that you want a\n>>> discriminated union, rather than a struct that contains unused fields,\n>>> for handling different SREP chunk types.\n>> I think there should be two kinds of replies: 1) read directly into a\n>> QEMUIOVector, using structured replies only as an encapsulation of the\n>> payload; 2) read a chunk at a time into malloc-ed memory, yielding back\n>> to the calling coroutine after receiving one complete chunk.\n>>\n>> In the end this probably means that you have a read_chunk_header\n>> function and a read_chunk function.  READ has a loop that calls\n>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>> while everyone else calls read_chunk.\n>\n> accordingly to spec, we can receive several error reply chunks to any \n> request,\n> so loop, receiving them should be common for all requests I think\n\nas well as handling error chunks should be common.. What do you think \nabout my DRAFT proposal?\n\n\n>\n>>\n>> Maybe qio_channel_readv/writev_full could have \"offset\" and \"bytes\"\n>> arguments.  Most code in iov_send_recv could be cut-and-pasted. (When\n>> sheepdog is converted to QIOChannel, iov_send_recv can go away).\n>>\n>> Paolo\n>\n>","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y60SP0vzcz9t2M\n\tfor ;\n\tWed, 4 Oct 2017 00:36:53 +1100 (AEDT)","from localhost ([::1]:58612 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dzNNT-0005zN-9Q\n\tfor incoming@patchwork.ozlabs.org; Tue, 03 Oct 2017 09:36:51 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36662)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dzNLx-0005C5-Pp\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 09:35:22 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dzNLs-0000dU-49\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 09:35:17 -0400","from mailhub.sw.ru ([195.214.232.25]:11433 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dzNLr-0000Pm-OC\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 09:35:12 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v93DZ3tG027903;\n\tTue, 3 Oct 2017 16:35:03 +0300 (MSK)"],"From":"Vladimir Sementsov-Ogievskiy ","To":"Paolo Bonzini , Eric Blake ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t","Message-ID":"","Date":"Tue, 3 Oct 2017 16:35:03 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv93DZ3tG027903","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1779001,"web_url":"http://patchwork.ozlabs.org/comment/1779001/","msgid":"","list_archive_url":null,"date":"2017-10-03T14:05:55","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 03/10/2017 14:26, Vladimir Sementsov-Ogievskiy wrote:\n> 03.10.2017 13:07, Paolo Bonzini wrote:\n>> On 26/09/2017 00:19, Eric Blake wrote:\n>>>> +    /* here we deal with successful structured reply */\n>>>> +    switch (s->reply.type) {\n>>>> +        QEMUIOVector sub_qiov;\n>>>> +    case NBD_SREP_TYPE_OFFSET_DATA:\n>>> This is putting a LOT of smarts directly into the receive routine.\n>>> Here's where I was previously wondering (and I think Paolo as well)\n>>> whether it might be better to split the efforts: the generic function\n>>> reads off the chunk information and any payload, but a per-command\n>>> callback function then parses the chunks.  Especially since the\n>>> definition of the chunks differs on a per-command basis (yes, the NBD\n>>> spec will try to not reuse an SREP chunk type across multiple commands\n>>> unless the semantics are similar, but that's a bit more fragile).  This\n>>> particularly matters given my statement above that you want a\n>>> discriminated union, rather than a struct that contains unused fields,\n>>> for handling different SREP chunk types.\n>> I think there should be two kinds of replies: 1) read directly into a\n>> QEMUIOVector, using structured replies only as an encapsulation of the\n> \n> who should read to qiov? reply_entry, or calling coroutine?..\n> reply_entry should anyway\n> parse reply, to understand should it read it all or read it to qiov (or\n> yield back, and then\n> calling coroutine will read to qiov)..\n\nThe CMD_READ coroutine should---either directly or, in case you have a\nstructured reply, after reading each chunk header.\n\nPaolo\n\n>> payload; 2) read a chunk at a time into malloc-ed memory, yielding back\n>> to the calling coroutine after receiving one complete chunk.\n>>\n>> In the end this probably means that you have a read_chunk_header\n>> function and a read_chunk function.  READ has a loop that calls\n>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>> while everyone else calls read_chunk.\n>>\n>> Maybe qio_channel_readv/writev_full could have \"offset\" and \"bytes\"\n>> arguments.  Most code in iov_send_recv could be cut-and-pasted.  (When\n>> sheepdog is converted to QIOChannel, iov_send_recv can go away).\n>>\n>> Paolo\n> \n>","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y617D219Wz9t2Q\n\tfor ;\n\tWed, 4 Oct 2017 01:07:03 +1100 (AEDT)","from localhost ([::1]:58779 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dzNqe-0006R8-Eo\n\tfor incoming@patchwork.ozlabs.org; Tue, 03 Oct 2017 10:07:00 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47208)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dzNpy-0006K7-7t\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 10:06:23 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dzNpt-0003RA-Fq\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 10:06:18 -0400","from mx1.redhat.com ([209.132.183.28]:24992)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dzNph-0003CZ-Q2; Tue, 03 Oct 2017 10:06:01 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\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 90AE8C04B30E;\n\tTue, 3 Oct 2017 14:06:00 +0000 (UTC)","from [10.36.116.209] (ovpn-116-209.ams2.redhat.com [10.36.116.209])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id AC1B461F4D;\n\tTue, 3 Oct 2017 14:05:57 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 90AE8C04B30E","To":"Vladimir Sementsov-Ogievskiy ,\n\tEric Blake , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t<3b2e222e-87ea-e134-64a8-f6394e502a14@virtuozzo.com>","From":"Paolo Bonzini ","Message-ID":"","Date":"Tue, 3 Oct 2017 16:05:55 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<3b2e222e-87ea-e134-64a8-f6394e502a14@virtuozzo.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]);\n\tTue, 03 Oct 2017 14:06:00 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1779002,"web_url":"http://patchwork.ozlabs.org/comment/1779002/","msgid":"","list_archive_url":null,"date":"2017-10-03T14:06:37","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>\n>>> In the end this probably means that you have a read_chunk_header\n>>> function and a read_chunk function.  READ has a loop that calls\n>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>> while everyone else calls read_chunk.\n>>\n>> accordingly to spec, we can receive several error reply chunks to any\n>> request,\n>> so loop, receiving them should be common for all requests I think\n> \n> as well as handling error chunks should be common..\n\nYes, reading error chunks should be part of read_chunk_header.\n\nPaolo","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y61873HYcz9t2Q\n\tfor ;\n\tWed, 4 Oct 2017 01:07:51 +1100 (AEDT)","from localhost ([::1]:58798 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dzNrR-00078z-CD\n\tfor incoming@patchwork.ozlabs.org; Tue, 03 Oct 2017 10:07:49 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47715)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dzNqh-0006zN-JJ\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 10:07:09 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dzNqY-0004KU-56\n\tfor qemu-devel@nongnu.org; Tue, 03 Oct 2017 10:07:03 -0400","from mx1.redhat.com ([209.132.183.28]:60400)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dzNqL-00043G-NW; Tue, 03 Oct 2017 10:06:41 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\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 A82AF806C3;\n\tTue, 3 Oct 2017 14:06:40 +0000 (UTC)","from [10.36.116.209] (ovpn-116-209.ams2.redhat.com [10.36.116.209])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 9F29060E3B;\n\tTue, 3 Oct 2017 14:06:38 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A82AF806C3","To":"Vladimir Sementsov-Ogievskiy ,\n\tEric Blake , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t","From":"Paolo Bonzini ","Message-ID":"","Date":"Tue, 3 Oct 2017 16:06:37 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tTue, 03 Oct 2017 14:06:40 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1780427,"web_url":"http://patchwork.ozlabs.org/comment/1780427/","msgid":"<0dbd80a0-e6ff-43d6-eab3-7e81675e2a2b@virtuozzo.com>","list_archive_url":null,"date":"2017-10-05T09:59:15","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"03.10.2017 17:06, Paolo Bonzini wrote:\n> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>> In the end this probably means that you have a read_chunk_header\n>>>> function and a read_chunk function.  READ has a loop that calls\n>>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>>> while everyone else calls read_chunk.\n>>> accordingly to spec, we can receive several error reply chunks to any\n>>> request,\n>>> so loop, receiving them should be common for all requests I think\n>> as well as handling error chunks should be common..\n> Yes, reading error chunks should be part of read_chunk_header.\n>\n> Paolo","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y77ZF5Mdlz9t3R\n\tfor ;\n\tThu, 5 Oct 2017 21:00:52 +1100 (AEDT)","from localhost ([::1]:38744 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e02xV-0004bA-Oh\n\tfor incoming@patchwork.ozlabs.org; Thu, 05 Oct 2017 06:00:49 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:38364)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e02wE-0003zB-6F\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 05:59:31 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e02w9-00042R-CP\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 05:59:30 -0400","from mailhub.sw.ru ([195.214.232.25]:6045 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e02w9-00041Q-08\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 05:59:25 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v959xFV6002963;\n\tThu, 5 Oct 2017 12:59:16 +0300 (MSK)"],"To":"Paolo Bonzini , Eric Blake ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<0dbd80a0-e6ff-43d6-eab3-7e81675e2a2b@virtuozzo.com>","Date":"Thu, 5 Oct 2017 12:59:15 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv959xFV6002963","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1780428,"web_url":"http://patchwork.ozlabs.org/comment/1780428/","msgid":"<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>","list_archive_url":null,"date":"2017-10-05T10:02:12","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"03.10.2017 17:06, Paolo Bonzini wrote:\n> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>> In the end this probably means that you have a read_chunk_header\n>>>> function and a read_chunk function.  READ has a loop that calls\n>>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>>> while everyone else calls read_chunk.\n>>> accordingly to spec, we can receive several error reply chunks to any\n>>> request,\n>>> so loop, receiving them should be common for all requests I think\n>> as well as handling error chunks should be common..\n> Yes, reading error chunks should be part of read_chunk_header.\n>\n> Paolo\n\nSo, you want a loop in READ, and separate loop for other commands? Then \nwe will have separate loop for BLOCK_STATUS and for all future commands \nwith specific replies?","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y77gN0nLDz9t3R\n\tfor ;\n\tThu, 5 Oct 2017 21:05:20 +1100 (AEDT)","from localhost ([::1]:38776 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e031q-0008Ca-80\n\tfor incoming@patchwork.ozlabs.org; Thu, 05 Oct 2017 06:05:18 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:39386)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e02z1-0006Ae-Lg\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 06:02:29 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e02yx-0006sf-43\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 06:02:23 -0400","from mailhub.sw.ru ([195.214.232.25]:32623 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e02yw-0006pm-Og\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 06:02:19 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v95A2C2e003678;\n\tThu, 5 Oct 2017 13:02:13 +0300 (MSK)"],"To":"Paolo Bonzini , Eric Blake ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>","Date":"Thu, 5 Oct 2017 13:02:12 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv95A2C2e003678","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1780450,"web_url":"http://patchwork.ozlabs.org/comment/1780450/","msgid":"","list_archive_url":null,"date":"2017-10-05T10:36:36","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":2701,"url":"http://patchwork.ozlabs.org/api/people/2701/","name":"Paolo Bonzini","email":"pbonzini@redhat.com"},"content":"On 05/10/2017 12:02, Vladimir Sementsov-Ogievskiy wrote:\n> 03.10.2017 17:06, Paolo Bonzini wrote:\n>> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>>> In the end this probably means that you have a read_chunk_header\n>>>>> function and a read_chunk function.  READ has a loop that calls\n>>>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>>>> while everyone else calls read_chunk.\n>>>> accordingly to spec, we can receive several error reply chunks to any\n>>>> request,\n>>>> so loop, receiving them should be common for all requests I think\n>>> as well as handling error chunks should be common..\n>> Yes, reading error chunks should be part of read_chunk_header.\n>>\n>> Paolo\n> \n> So, you want a loop in READ, and separate loop for other commands? Then\n> we will have separate loop for BLOCK_STATUS and for all future commands\n> with specific replies?\n\nThere should be a separate loop for each command.\n\nThe only difference between READ and other commands is that READ\nreceives directly in QEMUIOVector, while other commands can use a common\nfunction to to receive each structured reply chunk into malloc-ed memory.\n\nPaolo","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=pbonzini@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y78NJ6STXz9t43\n\tfor ;\n\tThu, 5 Oct 2017 21:37:20 +1100 (AEDT)","from localhost ([::1]:38982 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e03Wp-0008E3-2E\n\tfor incoming@patchwork.ozlabs.org; Thu, 05 Oct 2017 06:37:19 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:50475)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e03WR-0008C2-RY\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 06:36:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e03WQ-0006gR-VS\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 06:36:55 -0400","from mx1.redhat.com ([209.132.183.28]:43978)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e03WK-0006aI-LO; Thu, 05 Oct 2017 06:36:48 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\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 AAD42820EB;\n\tThu, 5 Oct 2017 10:36:47 +0000 (UTC)","from [10.36.117.253] (ovpn-117-253.ams2.redhat.com [10.36.117.253])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 030BC60F8B;\n\tThu, 5 Oct 2017 10:36:42 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com AAD42820EB","To":"Vladimir Sementsov-Ogievskiy ,\n\tEric Blake , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t\n\t<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>","From":"Paolo Bonzini ","Message-ID":"","Date":"Thu, 5 Oct 2017 12:36:36 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tThu, 05 Oct 2017 10:36:47 +0000 (UTC)","Content-Transfer-Encoding":"quoted-printable","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1781077,"web_url":"http://patchwork.ozlabs.org/comment/1781077/","msgid":"<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>","list_archive_url":null,"date":"2017-10-05T22:12:30","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 10/05/2017 05:36 AM, Paolo Bonzini wrote:\n> On 05/10/2017 12:02, Vladimir Sementsov-Ogievskiy wrote:\n>> 03.10.2017 17:06, Paolo Bonzini wrote:\n>>> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>>>> In the end this probably means that you have a read_chunk_header\n>>>>>> function and a read_chunk function.  READ has a loop that calls\n>>>>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>>>>> while everyone else calls read_chunk.\n>>>>> accordingly to spec, we can receive several error reply chunks to any\n>>>>> request,\n>>>>> so loop, receiving them should be common for all requests I think\n>>>> as well as handling error chunks should be common..\n>>> Yes, reading error chunks should be part of read_chunk_header.\n>>>\n>>> Paolo\n>>\n>> So, you want a loop in READ, and separate loop for other commands? Then\n>> we will have separate loop for BLOCK_STATUS and for all future commands\n>> with specific replies?\n> \n> There should be a separate loop for each command.\n> \n> The only difference between READ and other commands is that READ\n> receives directly in QEMUIOVector, while other commands can use a common\n> function to to receive each structured reply chunk into malloc-ed memory.\n\nTo make sure we're on the same page, here's how I see it. At a high\nlevel, we have:\n\nEach command calls nbd_co_send_request once, then calls\nnbd_co_receive_reply in a loop until there is an indication of the last\npacket. nbd_co_receive_reply waits for data to come from the server,\nand parses the header.\n\nIf the packet is unrecognized, report failure and request a quit\n(negative return value)\n\nIf it is old-style:\n- if the command is read, and we did not negotiate structured read, then\nwe also read the payload into qiov\n- if the command is read, but we negotiated structured read, the server\nis buggy, so report the bug and request a quit\n- for all other commands, there is no payload, return success or failure\nto the caller based on the header payload\n- at any rate, the reply to the caller is that this is the final packet,\nand there is no payload returned (so we return negative or 1, but never 0)\n\nOtherwise, it is new-style:\n- if we did not negotiate structured reply, the server is buggy, so\nreport the bug and request a quit (negative return)\n- if the chunk is an error, we process the entire packet and report the\nerror; if we have any commands that care about the error details, we\ncould return the error in a malloc'd discriminated union, but we can\nprobably get by without that. If callers don't care about details, but\nthe error chunk is not final, it may be easier to just store the fact\nthat an error occurred and return 0 to tell the caller to keep looping,\nand return the negative value later when the final chunk is finally received\n- if the chunk is NBD_REPLY_TYPE_NONE, there is no payload, and this\nshould be the final chunk, so the return to the caller can be the same\nas for old-style (return 1 if we had no earlier error packets, or the\nsaved negative value corresponding to the first error received)\n- if the command is read, we can read the payload into qiov (saves\nmalloc'ing space for the reply only to copy it into the qiov), so we\ndon't have to return any data\n- for any other command, we malloc space for the non-error payload, and\nthen it is up to the command's loop to process the payload\n\nso the signature can be something like:\n\nint nbd_co_receive_reply(NBDClientSession *s, QEMUIOVector *qiov,\n void **payload)\n\nwhere it returns -errno on failure, 0 if the command is not complete,\nand 1 if the command is done. READ passes qiov, which is fully\npopulated when the function returns 1; all other commands pass NULL.\nCommands pass NULL for payload if they don't expect a payload return\n(this includes READ); but a command that expects a payload\n(BLOCK_STATUS) passes a pointer in payload and gets malloc'd space\nstored there if return is 0 or 1.\n\nDoes that sound like we're on the right design track?","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y7RqG18yBz9t5Y\n\tfor ;\n\tFri, 6 Oct 2017 09:13:12 +1100 (AEDT)","from localhost ([::1]:42187 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e0EOE-0000V5-0t\n\tfor incoming@patchwork.ozlabs.org; Thu, 05 Oct 2017 18:13:10 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:42957)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e0ENm-0000T7-9l\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 18:12:43 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e0ENl-0006SV-1J\n\tfor qemu-devel@nongnu.org; Thu, 05 Oct 2017 18:12:42 -0400","from mx1.redhat.com ([209.132.183.28]:52864)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e0ENe-0006Ot-SX; Thu, 05 Oct 2017 18:12:35 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\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 B3C83C057F93;\n\tThu, 5 Oct 2017 22:12:33 +0000 (UTC)","from [10.10.120.2] (ovpn-120-2.rdu2.redhat.com [10.10.120.2])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 598065D9CB;\n\tThu, 5 Oct 2017 22:12:31 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com B3C83C057F93","To":"Paolo Bonzini ,\n\tVladimir Sementsov-Ogievskiy ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t\n\t<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>\n\t","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>","Date":"Thu, 5 Oct 2017 17:12:30 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"EH66b2s8pFixido4mKTfDPGxAfFsopfrO\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tThu, 05 Oct 2017 22:12:33 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1781261,"web_url":"http://patchwork.ozlabs.org/comment/1781261/","msgid":"<11162a3c-49c6-17a8-29d2-227266b2c020@virtuozzo.com>","list_archive_url":null,"date":"2017-10-06T07:09:19","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"06.10.2017 01:12, Eric Blake wrote:\n> On 10/05/2017 05:36 AM, Paolo Bonzini wrote:\n>> On 05/10/2017 12:02, Vladimir Sementsov-Ogievskiy wrote:\n>>> 03.10.2017 17:06, Paolo Bonzini wrote:\n>>>> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>>>>> In the end this probably means that you have a read_chunk_header\n>>>>>>> function and a read_chunk function.  READ has a loop that calls\n>>>>>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>>>>>> while everyone else calls read_chunk.\n>>>>>> accordingly to spec, we can receive several error reply chunks to any\n>>>>>> request,\n>>>>>> so loop, receiving them should be common for all requests I think\n>>>>> as well as handling error chunks should be common..\n>>>> Yes, reading error chunks should be part of read_chunk_header.\n>>>>\n>>>> Paolo\n>>> So, you want a loop in READ, and separate loop for other commands? Then\n>>> we will have separate loop for BLOCK_STATUS and for all future commands\n>>> with specific replies?\n>> There should be a separate loop for each command.\n>>\n>> The only difference between READ and other commands is that READ\n>> receives directly in QEMUIOVector, while other commands can use a common\n>> function to to receive each structured reply chunk into malloc-ed memory.\n> To make sure we're on the same page, here's how I see it. At a high\n> level, we have:\n>\n> Each command calls nbd_co_send_request once, then calls\n> nbd_co_receive_reply in a loop until there is an indication of the last\n> packet. nbd_co_receive_reply waits for data to come from the server,\n> and parses the header.\n>\n> If the packet is unrecognized, report failure and request a quit\n> (negative return value)\n>\n> If it is old-style:\n> - if the command is read, and we did not negotiate structured read, then\n> we also read the payload into qiov\n> - if the command is read, but we negotiated structured read, the server\n> is buggy, so report the bug and request a quit\n> - for all other commands, there is no payload, return success or failure\n> to the caller based on the header payload\n> - at any rate, the reply to the caller is that this is the final packet,\n> and there is no payload returned (so we return negative or 1, but never 0)\n>\n> Otherwise, it is new-style:\n> - if we did not negotiate structured reply, the server is buggy, so\n> report the bug and request a quit (negative return)\n> - if the chunk is an error, we process the entire packet and report the\n> error; if we have any commands that care about the error details, we\n> could return the error in a malloc'd discriminated union, but we can\n> probably get by without that. If callers don't care about details, but\n> the error chunk is not final, it may be easier to just store the fact\n> that an error occurred and return 0 to tell the caller to keep looping,\n> and return the negative value later when the final chunk is finally received\n> - if the chunk is NBD_REPLY_TYPE_NONE, there is no payload, and this\n> should be the final chunk, so the return to the caller can be the same\n> as for old-style (return 1 if we had no earlier error packets, or the\n> saved negative value corresponding to the first error received)\n> - if the command is read, we can read the payload into qiov (saves\n> malloc'ing space for the reply only to copy it into the qiov), so we\n> don't have to return any data\n> - for any other command, we malloc space for the non-error payload, and\n> then it is up to the command's loop to process the payload\n>\n> so the signature can be something like:\n>\n> int nbd_co_receive_reply(NBDClientSession *s, QEMUIOVector *qiov,\n> void **payload)\n>\n> where it returns -errno on failure, 0 if the command is not complete,\n> and 1 if the command is done. READ passes qiov, which is fully\n> populated when the function returns 1; all other commands pass NULL.\n> Commands pass NULL for payload if they don't expect a payload return\n> (this includes READ); but a command that expects a payload\n> (BLOCK_STATUS) passes a pointer in payload and gets malloc'd space\n> stored there if return is 0 or 1.\n>\n> Does that sound like we're on the right design track?\n>\n\n\nHmm. and what is the difference with my patch? Except payload? The only \ncommand with payload\nnow is READ (except errors), but you (and me in my patch) suggest to \nfill this qiov in nbd_co_receive_reply\n(nbd_co_receive_1_reply_or_chunk in my patch), so payload will be unused \nfor now, we can add it\nlater if it will be needed for BLOCK_STATUS.","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y7gkf1gz9z9t5C\n\tfor ;\n\tFri, 6 Oct 2017 18:10:01 +1100 (AEDT)","from localhost ([::1]:43305 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e0Mlj-0000OV-Vw\n\tfor incoming@patchwork.ozlabs.org; Fri, 06 Oct 2017 03:10:00 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:45211)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e0MlH-0000N8-Ew\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:09:32 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e0MlC-00031A-Fi\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:09:31 -0400","from mailhub.sw.ru ([195.214.232.25]:32027 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e0MlB-0002zZ-Vm\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:09:26 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v9679JYu008893;\n\tFri, 6 Oct 2017 10:09:19 +0300 (MSK)"],"To":"Eric Blake , Paolo Bonzini ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t\n\t<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>\n\t\n\t<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<11162a3c-49c6-17a8-29d2-227266b2c020@virtuozzo.com>","Date":"Fri, 6 Oct 2017 10:09:19 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv9679JYu008893","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1781265,"web_url":"http://patchwork.ozlabs.org/comment/1781265/","msgid":"<7bd80c97-23ae-2f35-27a7-37b1d7ce4d2f@virtuozzo.com>","list_archive_url":null,"date":"2017-10-06T07:23:28","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"06.10.2017 10:09, Vladimir Sementsov-Ogievskiy wrote:\n> 06.10.2017 01:12, Eric Blake wrote:\n>> On 10/05/2017 05:36 AM, Paolo Bonzini wrote:\n>>> On 05/10/2017 12:02, Vladimir Sementsov-Ogievskiy wrote:\n>>>> 03.10.2017 17:06, Paolo Bonzini wrote:\n>>>>> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>>>>>> In the end this probably means that you have a read_chunk_header\n>>>>>>>> function and a read_chunk function.  READ has a loop that calls\n>>>>>>>> read_chunk_header followed by direct reading into the \n>>>>>>>> QEMUIOVector,\n>>>>>>>> while everyone else calls read_chunk.\n>>>>>>> accordingly to spec, we can receive several error reply chunks \n>>>>>>> to any\n>>>>>>> request,\n>>>>>>> so loop, receiving them should be common for all requests I think\n>>>>>> as well as handling error chunks should be common..\n>>>>> Yes, reading error chunks should be part of read_chunk_header.\n>>>>>\n>>>>> Paolo\n>>>> So, you want a loop in READ, and separate loop for other commands? \n>>>> Then\n>>>> we will have separate loop for BLOCK_STATUS and for all future \n>>>> commands\n>>>> with specific replies?\n>>> There should be a separate loop for each command.\n>>>\n>>> The only difference between READ and other commands is that READ\n>>> receives directly in QEMUIOVector, while other commands can use a \n>>> common\n>>> function to to receive each structured reply chunk into malloc-ed \n>>> memory.\n>> To make sure we're on the same page, here's how I see it.  At a high\n>> level, we have:\n>>\n>> Each command calls nbd_co_send_request once, then calls\n>> nbd_co_receive_reply in a loop until there is an indication of the last\n>> packet.  nbd_co_receive_reply waits for data to come from the server,\n>> and parses the header.\n>>\n>> If the packet is unrecognized, report failure and request a quit\n>> (negative return value)\n>>\n>> If it is old-style:\n>> - if the command is read, and we did not negotiate structured read, then\n>> we also read the payload into qiov\n>> - if the command is read, but we negotiated structured read, the server\n>> is buggy, so report the bug and request a quit\n>> - for all other commands, there is no payload, return success or failure\n>> to the caller based on the header payload\n>> - at any rate, the reply to the caller is that this is the final packet,\n>> and there is no payload returned (so we return negative or 1, but \n>> never 0)\n>>\n>> Otherwise, it is new-style:\n>> - if we did not negotiate structured reply, the server is buggy, so\n>> report the bug and request a quit (negative return)\n>> - if the chunk is an error, we process the entire packet and report the\n>> error; if we have any commands that care about the error details, we\n>> could return the error in a malloc'd discriminated union, but we can\n>> probably get by without that. If callers don't care about details, but\n>> the error chunk is not final, it may be easier to just store the fact\n>> that an error occurred and return 0 to tell the caller to keep looping,\n>> and return the negative value later when the final chunk is finally \n>> received\n>> - if the chunk is NBD_REPLY_TYPE_NONE, there is no payload, and this\n>> should be the final chunk, so the return to the caller can be the same\n>> as for old-style (return 1 if we had no earlier error packets, or the\n>> saved negative value corresponding to the first error received)\n>> - if the command is read, we can read the payload into qiov (saves\n>> malloc'ing space for the reply only to copy it into the qiov), so we\n>> don't have to return any data\n>> - for any other command, we malloc space for the non-error payload, and\n>> then it is up to the command's loop to process the payload\n>>\n>> so the signature can be something like:\n>>\n>> int nbd_co_receive_reply(NBDClientSession *s, QEMUIOVector *qiov,\n>>                           void **payload)\n>>\n>> where it returns -errno on failure, 0 if the command is not complete,\n>> and 1 if the command is done.  READ passes qiov, which is fully\n>> populated when the function returns 1; all other commands pass NULL.\n>> Commands pass NULL for payload if they don't expect a payload return\n>> (this includes READ); but a command that expects a payload\n>> (BLOCK_STATUS) passes a pointer in payload and gets malloc'd space\n>> stored there if return is 0 or 1.\n>>\n>> Does that sound like we're on the right design track?\n>>\n>\n>\n> Hmm. and what is the difference with my patch? Except payload? The \n> only command with payload\n> now is READ (except errors), but you (and me in my patch) suggest to \n> fill this qiov in nbd_co_receive_reply\n> (nbd_co_receive_1_reply_or_chunk in my patch), so payload will be \n> unused for now, we can add it\n> later if it will be needed for BLOCK_STATUS.\n>\n\nAhm, sorry, I've already forget my original patch, which reads most of \npayload in nbd/client.c code called from reply_entry.. So, ok for me, I \nthink I'll post a new version today.","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y7h2y2HPXz9t5C\n\tfor ;\n\tFri, 6 Oct 2017 18:24:09 +1100 (AEDT)","from localhost ([::1]:43338 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e0MzM-0004G1-CA\n\tfor incoming@patchwork.ozlabs.org; Fri, 06 Oct 2017 03:24:04 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47640)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e0Myx-0004DV-QF\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:23:44 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e0Myu-0005wD-K1\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:23:39 -0400","from mailhub.sw.ru ([195.214.232.25]:15415 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e0Myu-0005vL-37\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:23:36 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v967NSNM013030;\n\tFri, 6 Oct 2017 10:23:28 +0300 (MSK)"],"From":"Vladimir Sementsov-Ogievskiy ","To":"Eric Blake , Paolo Bonzini ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t\n\t<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>\n\t\n\t<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>\n\t<11162a3c-49c6-17a8-29d2-227266b2c020@virtuozzo.com>","Message-ID":"<7bd80c97-23ae-2f35-27a7-37b1d7ce4d2f@virtuozzo.com>","Date":"Fri, 6 Oct 2017 10:23:28 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<11162a3c-49c6-17a8-29d2-227266b2c020@virtuozzo.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv967NSNM013030","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1781279,"web_url":"http://patchwork.ozlabs.org/comment/1781279/","msgid":"","list_archive_url":null,"date":"2017-10-06T07:34:50","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"06.10.2017 01:12, Eric Blake wrote:\n> On 10/05/2017 05:36 AM, Paolo Bonzini wrote:\n>> On 05/10/2017 12:02, Vladimir Sementsov-Ogievskiy wrote:\n>>> 03.10.2017 17:06, Paolo Bonzini wrote:\n>>>> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote:\n>>>>>>> In the end this probably means that you have a read_chunk_header\n>>>>>>> function and a read_chunk function.  READ has a loop that calls\n>>>>>>> read_chunk_header followed by direct reading into the QEMUIOVector,\n>>>>>>> while everyone else calls read_chunk.\n>>>>>> accordingly to spec, we can receive several error reply chunks to any\n>>>>>> request,\n>>>>>> so loop, receiving them should be common for all requests I think\n>>>>> as well as handling error chunks should be common..\n>>>> Yes, reading error chunks should be part of read_chunk_header.\n>>>>\n>>>> Paolo\n>>> So, you want a loop in READ, and separate loop for other commands? Then\n>>> we will have separate loop for BLOCK_STATUS and for all future commands\n>>> with specific replies?\n>> There should be a separate loop for each command.\n>>\n>> The only difference between READ and other commands is that READ\n>> receives directly in QEMUIOVector, while other commands can use a common\n>> function to to receive each structured reply chunk into malloc-ed memory.\n> To make sure we're on the same page, here's how I see it. At a high\n> level, we have:\n>\n> Each command calls nbd_co_send_request once, then calls\n> nbd_co_receive_reply in a loop until there is an indication of the last\n> packet. nbd_co_receive_reply waits for data to come from the server,\n> and parses the header.\n>\n> If the packet is unrecognized, report failure and request a quit\n> (negative return value)\n>\n> If it is old-style:\n> - if the command is read, and we did not negotiate structured read, then\n> we also read the payload into qiov\n> - if the command is read, but we negotiated structured read, the server\n> is buggy, so report the bug and request a quit\n> - for all other commands, there is no payload, return success or failure\n> to the caller based on the header payload\n> - at any rate, the reply to the caller is that this is the final packet,\n> and there is no payload returned (so we return negative or 1, but never 0)\n>\n> Otherwise, it is new-style:\n> - if we did not negotiate structured reply, the server is buggy, so\n> report the bug and request a quit (negative return)\n> - if the chunk is an error, we process the entire packet and report the\n> error; if we have any commands that care about the error details, we\n> could return the error in a malloc'd discriminated union, but we can\n> probably get by without that. If callers don't care about details, but\n> the error chunk is not final, it may be easier to just store the fact\n> that an error occurred and return 0 to tell the caller to keep looping,\n> and return the negative value later when the final chunk is finally received\n> - if the chunk is NBD_REPLY_TYPE_NONE, there is no payload, and this\n> should be the final chunk, so the return to the caller can be the same\n> as for old-style (return 1 if we had no earlier error packets, or the\n> saved negative value corresponding to the first error received)\n> - if the command is read, we can read the payload into qiov (saves\n> malloc'ing space for the reply only to copy it into the qiov), so we\n\nbut here you said \"This is putting a LOT of smarts directly into the \nreceive routine\"\n\n> don't have to return any data\n> - for any other command, we malloc space for the non-error payload, and\n> then it is up to the command's loop to process the payload\n>\n> so the signature can be something like:\n>\n> int nbd_co_receive_reply(NBDClientSession *s, QEMUIOVector *qiov,\n> void **payload)\n>\n> where it returns -errno on failure, 0 if the command is not complete,\n> and 1 if the command is done. READ passes qiov, which is fully\n> populated when the function returns 1; all other commands pass NULL.\n> Commands pass NULL for payload if they don't expect a payload return\n> (this includes READ); but a command that expects a payload\n> (BLOCK_STATUS) passes a pointer in payload and gets malloc'd space\n> stored there if return is 0 or 1.\n>\n> Does that sound like we're on the right design track?\n>","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y7hJ953LCz9t5C\n\tfor ;\n\tFri, 6 Oct 2017 18:35:37 +1100 (AEDT)","from localhost ([::1]:43381 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e0NAV-0001js-OY\n\tfor incoming@patchwork.ozlabs.org; Fri, 06 Oct 2017 03:35:35 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:50904)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e0N9x-0001hm-5r\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:35:03 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e0N9u-0007v0-2Q\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:35:01 -0400","from mailhub.sw.ru ([195.214.232.25]:23737 helo=relay.sw.ru)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e0N9t-0007sC-Kf\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:34:57 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v967YoPh017412;\n\tFri, 6 Oct 2017 10:34:50 +0300 (MSK)"],"To":"Eric Blake , Paolo Bonzini ,\n\tqemu-devel@nongnu.org, qemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t\n\t<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>\n\t\n\t<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"","Date":"Fri, 6 Oct 2017 10:34:50 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Language":"en-US","Content-Transfer-Encoding":"quoted-printable","X-MIME-Autoconverted":"from 8bit to quoted-printable by relay.sw.ru id\n\tv967YoPh017412","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1781660,"web_url":"http://patchwork.ozlabs.org/comment/1781660/","msgid":"<27d79f8f-61df-9a7b-7bd7-2ac296070fb0@redhat.com>","list_archive_url":null,"date":"2017-10-06T13:44:46","subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 10/06/2017 02:34 AM, Vladimir Sementsov-Ogievskiy wrote:\n\n>> - if the chunk is NBD_REPLY_TYPE_NONE, there is no payload, and this\n>> should be the final chunk, so the return to the caller can be the same\n>> as for old-style (return 1 if we had no earlier error packets, or the\n>> saved negative value corresponding to the first error received)\n>> - if the command is read, we can read the payload into qiov (saves\n>> malloc'ing space for the reply only to copy it into the qiov), so we\n> \n> but here you said \"This is putting a LOT of smarts directly into the\n> receive routine\"\n\nAbout the only reason to justify putting smarts into the receive routine\nis if it is the most common case, where the overhead of not\nspecial-casing will penalize us. READ happens to be a frequent command,\nall other commands, like BLOCK_STATUS, will probably be infrequent.\nMaking READ malloc the result, only to then copy it into the qiov, is a\nwaste of time; no other structured command will pass a qiov. So yeah,\nmaybe I'm stating things a bit differently than in my earlier messages,\nbut that's because I've now stated reasonings for why it is okay to\nspecial-case READ with a qiov differently from all other commands (none\nof which will pass a qiov to the receive routine).\n\nThanks for persisting with this, and I'm looking forward to the next\nrevision that you post; hopefully, by taking this discussion, we are\nmaking sure that the design is solid.","headers":{"Return-Path":"","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=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y7rVv0rbkz9t3m\n\tfor ;\n\tSat, 7 Oct 2017 00:45:27 +1100 (AEDT)","from localhost ([::1]:45003 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e0SwP-0006Zo-8I\n\tfor incoming@patchwork.ozlabs.org; Fri, 06 Oct 2017 09:45:25 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:41145)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e0Svw-0006WL-Rw\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 09:44:57 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e0Svv-0008VZ-Q5\n\tfor qemu-devel@nongnu.org; Fri, 06 Oct 2017 09:44:56 -0400","from mx1.redhat.com ([209.132.183.28]:57964)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e0Svq-0008Ls-Se; Fri, 06 Oct 2017 09:44:51 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\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 9C7CCD6F18;\n\tFri, 6 Oct 2017 13:44:49 +0000 (UTC)","from [10.10.120.2] (ovpn-120-2.rdu2.redhat.com [10.10.120.2])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id CE46F649A8;\n\tFri, 6 Oct 2017 13:44:47 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 9C7CCD6F18","To":"Vladimir Sementsov-Ogievskiy ,\n\tPaolo Bonzini , qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org","References":"<20170925135801.144261-1-vsementsov@virtuozzo.com>\n\t<20170925135801.144261-9-vsementsov@virtuozzo.com>\n\t\n\t\n\t\n\t\n\t\n\t<9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com>\n\t\n\t<8f9f485a-b3b0-f14a-20ed-86578a96644e@redhat.com>\n\t","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<27d79f8f-61df-9a7b-7bd7-2ac296070fb0@redhat.com>","Date":"Fri, 6 Oct 2017 08:44:46 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"EBQlUMUd5EJpNmI1k0p0OdbhoKIHbvnac\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tFri, 06 Oct 2017 13:44:49 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured\n\tread for client","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, den@openvz.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]