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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org