Re: [Qemu-devel] [Qemu-block] [PATCH 8/8] nbd: Minimal structured read for client

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

06.10.2017 10:09, Vladimir Sementsov-Ogievskiy wrote:
> 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.  READ has a loop that calls
>>>>>>>> read_chunk_header followed by direct reading into the 
>>>>>>>> QEMUIOVector,
>>>>>>>> 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? 
>>>> Then
>>>> we will have separate loop for BLOCK_STATUS and for all future 
>>>> commands
>>>> 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 
>>> common
>>> function to to receive each structured reply chunk into malloc-ed 
>>> memory.
>> 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, then
>> 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 failure
>> 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 
>> received
>> - 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 command with payload
> now is READ (except errors), but you (and me in my patch) suggest to 
> fill this qiov in nbd_co_receive_reply
> (nbd_co_receive_1_reply_or_chunk in my patch), so payload will be 
> unused for now, we can add it
> later if it will be needed for BLOCK_STATUS.
>

Ahm, sorry, I've already forget my original patch, which reads most of 
payload in nbd/client.c code called from reply_entry.. So, ok for me, I 
think I'll post a new version today.

-- 
Best regards,
Vladimir