Re: [Xen-devel] [RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry
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
Re: [Xen-devel] [RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [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.
Signed-off-by: Marek Marczykowski-Górecki
---
tools/console/daemon/io.c | 86 ++--
1 file changed, 83 insertions(+), 3 deletions(-)
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 4d4e223..82806b2 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
#include
@@ -110,6 +111,7 @@ struct console {
struct domain *d;
bool optional;
bool use_gnttab;
+ bool have_state;
};
struct console_type {
@@ -118,6 +120,7 @@ struct console_type {
char *log_suffix;
bool optional;
bool use_gnttab;
+ bool have_state; // uses 'state' xenstore entry
};
static struct console_type console_type[] = {
@@ -157,6 +160,8 @@ typedef void (*VOID_ITER_FUNC_ARG2)(struct console *, void
*);
typedef int (*INT_ITER_FUNC_ARG3)(struct console *,
struct domain *dom, void **);
+static void handle_tty_write(struct console *con);
+
static inline bool console_enabled(struct console *con)
{
return con->local_port != -1;
@@ -672,6 +677,20 @@ static int xs_gather(struct xs_handle *xs, const char
*dir, ...)
return ret;
}
+static void set_backend_state(struct console *con, int state)
+{
+ char path[PATH_MAX], state_str[2], *be_path;
+
+ snprintf(state_str, sizeof(state_str), "%d", state);
+ snprintf(path, sizeof(path), "%s/backend", con->xspath);
+ be_path = xs_read(xs, XBT_NULL, path, NULL);
+ if (be_path) {
+ snprintf(path, sizeof(path), "%s/state", be_path);
+ xs_write(xs, XBT_NULL, path, state_str, 1);
+ free(be_path);
+ }
+}
+
static void console_unmap_interface(struct console *con)
{
if (con->interface == NULL)
@@ -683,7 +702,7 @@ static void console_unmap_interface(struct console *con)
con->interface = NULL;
con->ring_ref = -1;
}
-
+
static int console_create_ring(struct console *con)
{
int err, remote_port, ring_ref, rc;
@@ -787,10 +806,70 @@ static int console_create_ring(struct console *con)
if (log_guest && (con->log_fd == -1))
con->log_fd = create_console_log(con);
+ /* if everything ok, signal backend readiness, in backend tree */
+ set_backend_state(con, XenbusStateConnected);
+
out:
return err;
}
+/* gracefully close console */
+static int console_close(struct console *con) {
+
+ if (con->interface && con->master_fd != -1 && con->buffer.data) {
+ /* handle remaining data in buffers */
+ buffer_append(con);
+
+ /* write it out, if possible */
+ if (con->master_pollfd_idx != -1) {
+ if (fds[con->master_pollfd_idx].revents &
+ POLLOUT)
+ handle_tty_write(con);
+ }
+ }
+
+ console_close_tty(con);
+ console_unmap_interface(con);
+ set_backend_state(con, XenbusStateClosed);
+
+ return 0;
+}
+
+
+static int handle_console_state(struct console *con) {
+ int err, state;
+
+ if (!con->have_state)
+ return console_create_ring(con);
+
+ err = xs_gather(xs, con->xspath,
+ "state", "%u", ,
+ NULL);
+ if (err)
+ /* no 'state' entry, assume removal */
+ state = XenbusStateClosed;
+
+ switch (state) {
+ case XenbusStateInitialising:
+ case XenbusStateInitWait:
+ /* wait for frontent init */
+ return 0;
+ case XenbusStateInitialised:
+ case XenbusStateConnected:
+ /* ok, init backend (also on restart) */
+ return console_create_ring(con);
+ case XenbusStateClosing:
+ case XenbusStateClosed:
+ /* close requested */
+ return console_close(con);
+ default:
+ dolog(LOG_ERR,
+ "Invalid state %d set by console %s of domain
%d\n",
+ state, con->xspath, con->d->domid);
+
