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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Nov 01, 2018 at 05:21:39PM +0000, Ian Jackson wrote:
> 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.

Note that this commit _does not_ invent any new protocol at all. It merely
add support for the protocol that is used by additional consoles. The
backend side was implemented by qemu only before, now I add support for
it to xenconsoled.

This is about (additional) consoles living in
/local/domain/$DOMID/device/console/$DEVID, not the special-kind-of-thing
living in /local/domain/$DOMID/console. Is there any protocol specification
for it already anywhere? I can't see it xen/include/public/io/ as it's
for other device types - console.h have only struct xencons_interface
declaration without any comment. I can't find it in other places either.
If there is one, it should be added to docs/misc/console.txt and/or
xen/include/public/io/console.h. Otherwise I can add some basic spec to
docs/misc/console.txt.

> 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' ?

Actually, I'm doing similar thing here ;) previously xenconsoled didn't
know anything about 'state' field and it was one of the reason it
couldn't handle multiple consoles per domain.

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

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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