On Mon, Mar 22, 2021 at 11:52:22AM +0000, Stefan Hajnoczi wrote:
> On Mon, Mar 22, 2021 at 09:37:59AM +0300, Arseny Krasnov wrote:
> > This:
> > 1) Replaces enums with C style defines.
>
> Why?
I am guessing this is referencing
[PATCH] introduction: document #define syntax
> > 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@xxxxxxxxxxxxx>
> > ---
> > 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
So e.g.
Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM) 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.
True. Sometimes there's an _F_ somewhere there instead.
Also, it is possible to put the defines near the flags
field. Compare, for example:
\begin{lstlisting}
struct virtq_desc {
/* Address (guest-physical). */
le64 addr;
/* Length. */
le32 len;
/* This marks a buffer as continuing via the next field. */
#define VIRTQ_DESC_F_NEXT 1
/* This marks a buffer as device write-only (otherwise device read-only). */
#define VIRTQ_DESC_F_WRITE 2
/* This means the buffer contains a list of buffer descriptors. */
#define VIRTQ_DESC_F_INDIRECT 4
/* The flags as indicated above. */
le16 flags;
/* Next field if flags & NEXT */
le16 next;
};
\end{lstlisting}
> It uses bitmasks instead.
Not universally.
> I
> suggest following that for consistency:
>
> #define VIRTIO_VSOCK_SHUTDOWN_RECEIVE (1 << 0)
> #define VIRTIO_VSOCK_SHUTDOWN_SEND (1 << 1)
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
