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

[Prerna Saxena]

On 04/08/16 9:41 am, "Michael S. Tsirkin" <address@hidden> wrote:



>On Sat, Jul 30, 2016 at 06:38:23AM +0000, Prerna Saxena wrote:
>> 
>> 
>> 
>> 
>> 
>> 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.
>
>I prefer Eric's form too. "that ... or not" isn't very clear.

Done.

>
>> >
>> >> +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
>
>Probably easier to post v7 with above minor things.

Posted a v7 which incorporates all suggestions made by Eric.
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01027.html

Regards,