Re: [RFC PATCH v1] virtio-vsock: use C style defines for constants

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Mar 22, 2021 at 09:37:59AM +0300, Arseny Krasnov wrote:
> This:
> 1) Replaces enums with C style defines.

Why?

> 2) Adds defines for some constants.

Definitions need to be referenced somewhere to explain their use. It's
not enough to add a constant, some text in the spec should mention that
constant. (The exception to this might be a group of constants where
individual constants don't need to be mentioned explicitly.)

> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> ---
>  virtio-vsock.tex | 42 ++++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index da7e641..62ab6b3 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -86,23 +86,18 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
>  operation constants:
>  
>  \begin{lstlisting}
> -enum {
> -	VIRTIO_VSOCK_OP_INVALID = 0,
> -
> -	/* Connect operations */
> -	VIRTIO_VSOCK_OP_REQUEST = 1,
> -	VIRTIO_VSOCK_OP_RESPONSE = 2,
> -	VIRTIO_VSOCK_OP_RST = 3,
> -	VIRTIO_VSOCK_OP_SHUTDOWN = 4,
> -
> -	/* To send payload */
> -	VIRTIO_VSOCK_OP_RW = 5,
> -
> -	/* Tell the peer our credit info */
> -	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
> -	/* Request the peer to send the credit info to us */
> -	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
> -};
> +#define VIRTIO_VSOCK_OP_INVALID        0
> +/* Connect operations */
> +#define VIRTIO_VSOCK_OP_REQUEST        1
> +#define VIRTIO_VSOCK_OP_RESPONSE       2
> +#define VIRTIO_VSOCK_OP_RST            3
> +#define VIRTIO_VSOCK_OP_SHUTDOWN       4
> +/* To send payload */
> +#define VIRTIO_VSOCK_OP_RW             5
> +/* Tell the peer our credit info */
> +#define VIRTIO_VSOCK_OP_CREDIT_UPDATE  6
> +/* Request the peer to send the credit info to us */
> +#define VIRTIO_VSOCK_OP_CREDIT_REQUEST 7
>  \end{lstlisting}
>  
>  \subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket Device / Device Operation / Virtqueue Flow Control}
> @@ -143,6 +138,10 @@ \subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
>  Currently only stream sockets are supported. \field{type} is 1 for stream
>  socket types.
>  
> +\begin{lstlisting}
> +#define VIRTIO_VSOCK_TYPE_STREAM 1
> +\end{lstlisting}
> +
>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
>  without message boundaries.
>  
> @@ -227,6 +226,11 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
>  hints are permanent once sent and successive packets with bits clear do not
>  reset them.
>  
> +\begin{lstlisting}
> +#define VIRTIO_VSOCK_SHUTDOWN_RECEIVE_BIT 0
> +#define VIRTIO_VSOCK_SHUTDOWN_SEND_BIT    1
> +\end{lstlisting}

The spec has no other _BIT constants. It uses bitmasks instead. I
suggest following that for consistency:

  #define VIRTIO_VSOCK_SHUTDOWN_RECEIVE (1 << 0)
  #define VIRTIO_VSOCK_SHUTDOWN_SEND    (1 << 1)

Attachment: signature.asc
Description: PGP signature