Add timeout to xenconsole to fix race condition in xm create -c

PATCH In review Anthony Liguori 2 participants 3 messages Aug 30, 2005 – Aug 31, 2005

lore

Share & tools

lore

Anthony Liguori

author

2005-08-30 21:55 UTC #0 lore raw

PATCH 1/1 Add timeout to xenconsole to fix race condition in xm create -c

This should address the problems people are having now.

Wait a little bit for tty to appear. There is a race condition that occurs after xend creates a domain. Since no event triggers consoled to re-examine existing domains, we'll often not see the new pty by the time we're here. Since consoled sleeps for 2 second periods, a 5 second timeout should keep us covered.

A xenstore watch isn't much better since we don't want to block forever if given an invalid domain or worse yes, a domain that someone else has connected to.

Signed-off-by Anthony Liguori

Regards,

Anthony Liguori

# HG changeset patch
# User Anthony Liguori <[email protected]> # Node ID fe6c5ecea53aabedc6b53988da25910e108eafe9 # Parent 551870a55f240791695d30fd7fa92a1bf4e48387 Wait for domain tty to become available.

tools/console/client/main.c +26 -1

diff -r 551870a55f24 -r fe6c5ecea53a tools/console/client/main.c
@@ -176,6 +176,7 @@
 	unsigned int len = 0;
 	struct xs_handle *xs;
 	char *end;
+	time_t now;
 
 	while((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
 		switch(ch) {

@@ -215,13 +216,37 @@
 
 	snprintf(path, sizeof(path), "/console/%d/tty", domid);
 	str_pty = xs_read(xs, path, &len);
+
 	/* FIXME consoled currently does not assume domain-0 doesn't have a
 	   console which is good when we break domain-0 up.  To keep us
 	   user friendly, we'll bail out here since no data will ever show
 	   up on domain-0. */
-	if (domid == 0 || str_pty == NULL) {
+	if (domid == 0) {
 		err(errno, "Could not read tty from store");
 	}
+
+	/* FIXME wait a little bit for tty to appear.  There is a race
+	   condition that occurs after xend creates a domain.  Since no event
+           triggers consoled to re-examine existing domains, we'll often not
+           see the new pty by the time we're here.  Since consoled sleeps for
+	   2 second periods, a 5 second timeout should keep us covered.
+
+	   A xenstore watch isn't much better since we don't want to block
+           forever if given an invalid domain or worse yes, a domain that
+           someone else has connected to. */
+
+	now = time(0);
+	while (str_pty == NULL && (now + 5) > time(0)) {
+		struct timeval tv = { 0, 500 };
+		select(0, NULL, NULL, NULL, &tv); /* pause briefly */
+
+		str_pty = xs_read(xs, path, &len);
+	}
+
+	if (str_pty == NULL) {
+		err(errno, "Could not read tty from store");
+	}
+
 	spty = open(str_pty, O_RDWR | O_NOCTTY);
 	if (spty == -1) {
 		err(errno, "Could not open tty `%s'", str_pty);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Christian Limpach

2005-08-31 10:31 UTC #1 lore raw

Quoted 7 lines · Anthony Liguori

This should address the problems people are having now.
Wait a little bit for tty to appear. There is a race condition that occurs after xend creates a domain. Since no event triggers consoled to re-examine existing domains, we'll often not see the new pty by the time we're here. Since consoled sleeps for 2 second periods, a 5 second timeout should keep us covered.

I've applied this with slighly changed comments since xenconsoled now uses watches and should setup the tty almost immediately. The race still exists though.

Quoted 3 lines

A xenstore watch isn't much better since we don't want to block forever if given an invalid domain or worse yes, a domain that someone else has connected to.

It would improve the responsiveness but a timeout is still needed for the reasons you pointed out.

Finally, there's still an issue with storing the console tty information in /console/<domid> -- there might be stale data from a previous domain having the same domid and the client might run before the console daemon has rewritten the tty entry. I think this is best fixed by moving the console tty information into the domain's /domain tree.

christian

Anthony Liguori

author

2005-08-31 20:42 UTC #2 lore raw

Quoted 13 lines · Christian Limpach

Quoted 9 lines · Anthony Liguori
This should address the problems people are having now.
Wait a little bit for tty to appear. There is a race condition that occurs after xend creates a domain. Since no event triggers consoled to re-examine existing domains, we'll often not see the new pty by the time we're here. Since consoled sleeps for 2 second periods, a 5 second timeout should keep us covered.

Quoted 5 lines

I've applied this with slighly changed comments since xenconsoled now uses watches and should setup the tty almost immediately. The race still exists though.

Yes, what we really want is a event-dispatch mechanism. It's something we've talked about a bit here but without flushing out any sort of actual design.

Quoted 9 lines

Quoted 5 lines
A xenstore watch isn't much better since we don't want to block forever if given an invalid domain or worse yes, a domain that someone else has connected to.
It would improve the responsiveness but a timeout is still needed for the reasons you pointed out.

Quoted 8 lines

Finally, there's still an issue with storing the console tty information in /console/<domid> -- there might be stale data from a previous domain having the same domid and the client might run before the console daemon has rewritten the tty entry. I think this is best fixed by moving the console tty information into the domain's /domain tree.

Rusty had some objections to this originally--this is why its not there now. Rusty?

Regards,

Anthony Liguori

Quoted 4 lines

christian