Re: [Xen-devel] [RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 11/17] xenconsoled: add 
support for consoles using 'state' xenstore entry"):
> Add support for standard xenbus initialization protocol using 'state'
> xenstore entry. It will be necessary for secondary consoles.
> For consoles supporting it, read 'state' entry on the frontend and
> proceed accordingly - either init console or close it. When closing,
> make sure all the in-transit data is flushed (both from shared ring and
> from local buffer), if possible. This is especially important for
> MiniOS-based qemu stubdomain, which closes console just after writing
> device model state to it.
> For consoles without 'state' field behavior is unchanged - on any watch
> event try to connect console, as long as domain is alive.

I'm not opposed to the introduction of this state field.  The code
looks plausible.

But:

Firstly, you have put the protocol description in your commit
message (and it seems rather informal).  Can you please provide
a comprehensive protocol specification in-tree ?  You need to patch
   docs/misc/console.txt
I think.

I say `comprehensive' because your text is not particularly clearly
about who is supposed to `flush' which data exactly when.  Nor what
`flushing' means (does it ever mean discarding?)

Secondly: what about backwards compatibility ?  I think we need to at
least think about the possibility that there are some guest frontends
out there which may look for a `state' node and do something
undesirable with it.  I think this possibility is remote but it should
be mentioned in the commit message.

What about the possibility that one or the other end of the connection
may be replaced by a different implementation, so that the peer
appears to gain or lose support for `state' ?

I'll be able to review the code more effectively when there is a
formal protocol spec to compare it to...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel