Re: [virtio-comment] Feedback for chapter 5

[Thomas Huth <thuth@linux.vnet.ibm.com>]

On Sun, 09 Feb 2014 11:02:24 +1030
Rusty Russell <rusty@au1.ibm.com> wrote:

> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > On Fri, 07 Feb 2014 11:08:37 +1030
> > Rusty Russell <rusty@au1.ibm.com> wrote:
[...]
> >> diff --git a/content.tex b/content.tex
> >> index 48c2d8e..f9a83b6 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> > [...]
> >> @@ -3323,18 +3333,41 @@ struct virtio_console_control {
> >>          le16 event; /* The kind of control event */
> >>          le16 value; /* Extra information for the event */
> >>  };
> >> +\end{lstlisting}
> >>  
> >> -/* Some events for the internal messages (control packets) */
> >> -#define VIRTIO_CONSOLE_DEVICE_READY     0
> >> -#define VIRTIO_CONSOLE_PORT_ADD         1
> >> -#define VIRTIO_CONSOLE_PORT_REMOVE      2
> >> -#define VIRTIO_CONSOLE_PORT_READY       3
> >> -#define VIRTIO_CONSOLE_CONSOLE_PORT     4
> >> -#define VIRTIO_CONSOLE_RESIZE           5
> >> -#define VIRTIO_CONSOLE_PORT_OPEN        6
> >> -#define VIRTIO_CONSOLE_PORT_NAME        7
> >> +\begin{description}
> >> +\item [VIRTIO_CONSOLE_DEVICE_READY (0)] Sent by the driver at initialization
> >> +  to indicate that it is ready to receive control messages.  A value of
> >> +  1 indicates success, and 0 indicates failure.  The port number is unused.
> >> +\item [VIRTIO_CONSOLE_DEVICE_ADD (1)] Sent by the device, to create a new
> >> +  port.  The device MUST NOT specify a port which exists.  event is unused.
> >> +\item [VIRTIO_CONSOLE_DEVICE_READY (2)] Sent by the driver in response
> >> +  to the device's VIRTIO_CONSOLE_PORT_ADD message, to indicate that
> >> +  the port is ready to be used. A value of 1 indicates success, and 0
> >> +  indicates failure.
> >
> > There is something wrong in the above definitions ... you specified
> > VIRTIO_CONSOLE_DEVICE_READY twice, as 0 and 2. And what about the REMOVE event
> > that was originally specified here, too?
> 
> Yes, re-worked again to match numbering, fix DEVICE_READY (should be
> PORT_READY) and add PORT_REMOVE:
> 
> Here's the resulting de-TeXified text for easier reading:
> 
> The values for event are:
> 
> VIRTIO_CONSOLE_DEVICE_READY (0): Sent by the driver at initialization
>   to indicate that it is ready to receive control messages.  A value of
>   1 indicates success, and 0 indicates failure.  The port number is unused.
> 
> VIRTIO_CONSOLE_DEVICE_ADD (1): Sent by the device, to create a new
>   port.  The device MUST NOT specify a port which exists.  value is unused.
> 
> VIRTIO_CONSOLE_DEVICE_REMOVE (2): Sent by the device, to remove an
>   existing port.  The device MUST NOT specify a port which has not been
>   created with VIRTIO_CONSOLE_DEVICE_ADD.  value is unused.
> 
> VIRTIO_CONSOLE_PORT_READY (3): Sent by the driver in response
>   to the device's VIRTIO_CONSOLE_PORT_ADD message, to indicate that
>   the port is ready to be used. A value of 1 indicates success, and 0
>   indicates failure.
> 
> VIRTIO_CONSOLE_CONSOLE_PORT (4): Sent by the device to nominate
>   a port as a console port.  There may be more than one console port.
>   The driver SHOULD treat the port in a manner suitable for text
>   console access; the driver MUST respond with a VIRTIO_CONSOLE_PORT_OPEN
>   message.  The driver MUST set value to 1.

I assume the "set value to 1" refers to the VIRTIO_CONSOLE_PORT_OPEN
event, not the VIRTIO_CONSOLE_CONSOLE_PORT event, right? So I'd maybe
merge the two sentences to make this more explicit:

"the driver MUST respond with a VIRTIO_CONSOLE_PORT_OPEN message, with
 value set to 1."

Apart from that, the changes look fine to me now, thanks!

 Regards,
  Thomas