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

[Prerna Saxena]

On 27/07/16 6:58 pm, "Michael S. Tsirkin" <address@hidden> wrote:

>On Wed, Jul 27, 2016 at 12:56:18PM +0000, Prerna Saxena wrote:
>> Hi Marc,
>> Thanks, please find my reply inline.
>> 
>> 
>> 
>> 
>> 
>> On 27/07/16 4:35 pm, "Marc-André Lureau" <address@hidden> wrote:
>> 
>> >Hi
>> >
>> >On Wed, Jul 27, 2016 at 1:52 PM, Prerna Saxena <address@hidden> wrote:
>> >> From: Prerna Saxena <address@hidden>
>> >>
>> >> This introduces the VHOST_USER_PROTOCOL_F_REPLY_ACK.
>> >>
>> >> If negotiated, client applications should send a u64 payload in
>> >> response to any message that contains the "need_response" bit set
>> >> on the message flags. Setting the payload to "zero" indicates the
>> >> command finished successfully. Likewise, setting it to "non-zero"
>> >> indicates an error.
>> >>
>> >> Currently implemented only for SET_MEM_TABLE.
>> >>
>> >> Signed-off-by: Prerna Saxena <address@hidden>
>> >> ---
>> >>  docs/specs/vhost-user.txt | 41 +++++++++++++++++++++++++++++++++++++++++
>> >>  hw/virtio/vhost-user.c    | 32 ++++++++++++++++++++++++++++++++
>> >>  2 files changed, 73 insertions(+)
>> >>
>> >> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> >> index 777c49c..57df586 100644
>> >> --- a/docs/specs/vhost-user.txt
>> >> +++ b/docs/specs/vhost-user.txt
>> >> @@ -37,6 +37,8 @@ consists of 3 header fields and a payload:
>> >>   * Flags: 32-bit bit field:
>> >>     - Lower 2 bits are the version (currently 0x01)
>> >>     - Bit 2 is the reply flag - needs to be sent on each reply from the 
>> >> slave
>> >> +   - Bit 3 is the need_response flag - see 
>> >> VHOST_USER_PROTOCOL_F_REPLY_ACK for
>> >> +     details.
>> >
>> >Why need_response and not "need reply"?
>> 
>> (I’d already pointed this out earlier, but looks like I was possibly not 
>> very clear.)
>> Before deciding on the right name for Bit 3, let us see the nomenclature for 
>> Bit 2 above : "Bit 2 is the reply flag - needs to be sent on each reply from 
>> the slave”.
>> So we already have a _reply_ flag in use. If the name Bit 3 as the 
>> _need_reply_ flag, don’t you think it would be ultra-confusing ? I found it 
>> confusing  when I reviewed the documentation with this different term.
>> So I chose the name need_response with much deliberation — it conveys the 
>> essence of what this flag means to achieve, but without adding to confusion.
>
>I don't see confusion, I think I agree with Marc André.


Allright. Posted a new series with the reworded terminology and updated (more 
concise) documentation.

Regards,
Prerna