From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45211) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e0MlH-0000N8-Ew for qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:09:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e0MlC-00031A-Fi for qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:09:31 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:32027 helo=relay.sw.ru) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1e0MlB-0002zZ-Vm for qemu-devel@nongnu.org; Fri, 06 Oct 2017 03:09:26 -0400 References: <20170925135801.144261-1-vsementsov@virtuozzo.com> <20170925135801.144261-9-vsementsov@virtuozzo.com> <9c3e29b4-3ec9-4fd7-3d80-122f908d059f@virtuozzo.com> <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 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 Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured read for client List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Paolo Bonzini , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, den@openvz.org, mreitz@redhat.com 06.10.2017 01:12, Eric Blake wrote: > On 10/05/2017 05:36 AM, Paolo Bonzini wrote: >> On 05/10/2017 12:02, Vladimir Sementsov-Ogievskiy wrote: >>> 03.10.2017 17:06, Paolo Bonzini wrote: >>>> On 03/10/2017 15:35, Vladimir Sementsov-Ogievskiy wrote: >>>>>>> In the end this probably means that you have a read_chunk_header >>>>>>> function and a read_chunk function.=C2=A0 READ has a loop that ca= lls >>>>>>> read_chunk_header followed by direct reading into the QEMUIOVecto= r, >>>>>>> while everyone else calls read_chunk. >>>>>> accordingly to spec, we can receive several error reply chunks to = any >>>>>> request, >>>>>> so loop, receiving them should be common for all requests I think >>>>> as well as handling error chunks should be common.. >>>> Yes, reading error chunks should be part of read_chunk_header. >>>> >>>> Paolo >>> So, you want a loop in READ, and separate loop for other commands? Th= en >>> we will have separate loop for BLOCK_STATUS and for all future comman= ds >>> with specific replies? >> There should be a separate loop for each command. >> >> The only difference between READ and other commands is that READ >> receives directly in QEMUIOVector, while other commands can use a comm= on >> function to to receive each structured reply chunk into malloc-ed memo= ry. > To make sure we're on the same page, here's how I see it. At a high > level, we have: > > Each command calls nbd_co_send_request once, then calls > nbd_co_receive_reply in a loop until there is an indication of the last > packet. nbd_co_receive_reply waits for data to come from the server, > and parses the header. > > If the packet is unrecognized, report failure and request a quit > (negative return value) > > If it is old-style: > - if the command is read, and we did not negotiate structured read, the= n > we also read the payload into qiov > - if the command is read, but we negotiated structured read, the server > is buggy, so report the bug and request a quit > - for all other commands, there is no payload, return success or failur= e > to the caller based on the header payload > - at any rate, the reply to the caller is that this is the final packet= , > and there is no payload returned (so we return negative or 1, but never= 0) > > Otherwise, it is new-style: > - if we did not negotiate structured reply, the server is buggy, so > report the bug and request a quit (negative return) > - if the chunk is an error, we process the entire packet and report the > error; if we have any commands that care about the error details, we > could return the error in a malloc'd discriminated union, but we can > probably get by without that. If callers don't care about details, but > the error chunk is not final, it may be easier to just store the fact > that an error occurred and return 0 to tell the caller to keep looping, > and return the negative value later when the final chunk is finally rec= eived > - if the chunk is NBD_REPLY_TYPE_NONE, there is no payload, and this > should be the final chunk, so the return to the caller can be the same > as for old-style (return 1 if we had no earlier error packets, or the > saved negative value corresponding to the first error received) > - if the command is read, we can read the payload into qiov (saves > malloc'ing space for the reply only to copy it into the qiov), so we > don't have to return any data > - for any other command, we malloc space for the non-error payload, and > then it is up to the command's loop to process the payload > > so the signature can be something like: > > int nbd_co_receive_reply(NBDClientSession *s, QEMUIOVector *qiov, > void **payload) > > where it returns -errno on failure, 0 if the command is not complete, > and 1 if the command is done. READ passes qiov, which is fully > populated when the function returns 1; all other commands pass NULL. > Commands pass NULL for payload if they don't expect a payload return > (this includes READ); but a command that expects a payload > (BLOCK_STATUS) passes a pointer in payload and gets malloc'd space > stored there if return is 0 or 1. > > Does that sound like we're on the right design track? > Hmm. and what is the difference with my patch? Except payload? The only=20 command with payload now is READ (except errors), but you (and me in my patch) suggest to=20 fill this qiov in nbd_co_receive_reply (nbd_co_receive_1_reply_or_chunk in my patch), so payload will be unused=20 for now, we can add it later if it will be needed for BLOCK_STATUS. --=20 Best regards, Vladimir