Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Michael S. Tsirkin]

On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote:
> Make the language about feature negotiation explicitly clear about the
> handling of the VHOST_USER_F_PROTOCOL_FEATURES feature bit. Try and
> avoid the sort of bug introduced in vhost.rs REPLY_ACK processing:
> 
>   https://github.com/rust-vmm/vhost/pull/24
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Jiang Liu <gerry@linux.alibaba.com>
> Message-Id: <20210226111619.21178-1-alex.bennee@linaro.org>
> 
> ---
> v2
>   - use Stefan's suggested wording
>   - Be super explicit in the message descriptions
> ---
>  docs/interop/vhost-user.rst | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 2918d7c757..7c1fb8c209 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -307,6 +307,18 @@ bit was dedicated for this purpose::
>  
>    #define VHOST_USER_F_PROTOCOL_FEATURES 30
>  
> +Note that VHOST_USER_F_PROTOCOL_FEATURES is the UNUSED (30) feature
> +bit defined in `VIRTIO 1.1 6.3 Legacy Interface: Reserved Feature Bits
> +<https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4130003>`_.
> +VIRTIO devices do not advertise this feature bit and therefore VIRTIO
> +drivers cannot negotiate it.
> +
> +This reserved feature bit was reused by the vhost-user protocol to add
> +vhost-user protocol feature negotiation in a backwards compatible
> +fashion. Old vhost-user master and slave implementations continue to
> +work even though they are not aware of vhost-user protocol feature
> +negotiation.
> +
>  Ring states
>  -----------
>  
> @@ -865,7 +877,8 @@ Front-end message types
>    Get the protocol feature bitmask from the underlying vhost
>    implementation.  Only legal if feature bit
>    ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in
> -  ``VHOST_USER_GET_FEATURES``.
> +  ``VHOST_USER_GET_FEATURES``.  It does not need to be acknowledged by
> +  ``VHOST_USER_SET_FEATURES``.
>  
>  .. Note::
>     Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must
> @@ -881,7 +894,8 @@ Front-end message types
>    Enable protocol features in the underlying vhost implementation.
>  
>    Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in
> -  ``VHOST_USER_GET_FEATURES``.
> +  ``VHOST_USER_GET_FEATURES``.  It does not need to be acknowledged by
> +  ``VHOST_USER_SET_FEATURES``.
>  
>  .. Note::
>     Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must support


Not really clear what does "It" refer to here.
Also, are we sure it's ok to send the messages and then send
VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear?
Looks more like a violation to me ...


How about: It -> this bit
does not need to be -> before ... has been

so:

    Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in
 -  ``VHOST_USER_GET_FEATURES``, and even before this bit has been
        acknowledged by VHOST_USER_SET_FEATURES.




> -- 
> 2.20.1