Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.

[Prerna Saxena]

On 30/07/16 2:19 am, "Eric Blake" <address@hidden> wrote:

>On 07/28/2016 01:07 AM, Prerna Saxena wrote:
>> From: Prerna Saxena <address@hidden>
>> 
>> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>> 
>
>> +
>> +With this protocol extension negotiated, the sender (QEMU) can set the
>> +"need_reply" [Bit 3] flag to any command. This indicates that
>> +the client MUST respond with a Payload VhostUserMsg indicating success or
>> +failure. The payload should be set to zero on success or non-zero on 
>> failure.
>> +(Unless the message already has an explicit reply body)
>
>Rather than make this parenthetical, I would go with:
>
>The payload should be set to zero on success or non-zero on failure,
>unless the message already has an explicit reply body.

Hi Eric,
Thank you for taking a look, but I think you possibly missed the latest 
patchset posted last night.
This had already been incorporated in v6 that I’d posted last night before your 
message.
See https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06772.html


>
>> +
>> +This indicates to QEMU that the requested operation has deterministically
>> +been met or not. Today, QEMU is expected to terminate the main vhost-user
>
>Reads awkwardly; maybe:
>
>The response payload gives QEMU a deterministic indication of the result
>of the command.

Hmm, it is more of personal taste, so I’ll refrain from commenting either way.

>
>> +loop upon receiving such errors. In future, qemu could be taught to be more
>> +resilient for selective requests.
>> +
>> +For the message types that already solicit a reply from the client, the
>> +presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set 
>> brings
>> +no behaviourial change. (See the 'Communication' section for details.)
>
>s/behaviourial/behavioural/ (or if the document widely favors US
>spelling, behavioral)


The last 3 iterations of this patchset have only seen review comments focussed 
on documentation suggestions and indentation of code, but nothing on the 
idea/code itself. This gives me hope that the patch is possibly close to 
merging within 2.7 timeframe :-)
May I request the maintainers to please correct this tiny spelling typo as this 
is checked in?

Regards,
Prerna