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

From: Prerna Saxena <prerna.saxena@nutanix.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Eric Blake <eblake@redhat.com>,
	Prerna Saxena <saxenap.ltc@gmail.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Anil Kumar Boggarapu <anilkumar.boggarapu@nutanix.com>,
	Felipe Franciosi <felipe@nutanix.com>,
	"marcandre.lureau@gmail.com" <marcandre.lureau@gmail.com>
Subject: Re: [Qemu-devel] [PATCH for-2.7 v5.1 1/2] vhost-user: Introduce a new protocol feature REPLY_ACK.
Date: Fri, 5 Aug 2016 14:53:29 +0000	[thread overview]
Message-ID: <B2989014-66A0-4BF5-9FE4-6C0C4DBD6091@nutanix.com> (raw)
In-Reply-To: <20160804070906-mutt-send-email-mst@kernel.org>

On 04/08/16 9:41 am, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>On Sat, Jul 30, 2016 at 06:38:23AM +0000, Prerna Saxena wrote:
>> 
>> 
>> 
>> 
>> 
>> On 30/07/16 2:19 am, "Eric Blake" <eblake@redhat.com> wrote:
>> 
>> >On 07/28/2016 01:07 AM, Prerna Saxena wrote:
>> >> From: Prerna Saxena <prerna.saxena@nutanix.com>
>> >> 
>> >> 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,