On Thu, Nov 01, 2018 at 05:21:39PM +, 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?
signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
