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

[Michael S. Tsirkin]

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.

> >
> >> +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.

-- 
MST