[PATCH] xenconsole: Allow non-interactive use

[PATCH] xenconsole: Allow non-interactive use

[Martin Lucina]

If xenconsole is run with stdin closed or redirected to /dev/null,
console_loop() will return immediately due to failure to read from
STDIN_FILENO. This patch tests if stdin and stdout are both connected to
a TTY and, if not, xenconsole will not attempt to read from stdin or
modify stdout terminal attributes.

Existing behaviour when xenconsole is run from a terminal does not
change.

This allows for non-interactive use, eg. running "xl create -c" under
systemd or piping the output of "xl console" to another command.

Signed-off-by: Martin Lucina <martin@lucina.net>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/console/client/main.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index f4c783b..8a42101 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -168,7 +168,8 @@ static void restore_term(int fd, struct termios *old)
 	tcsetattr(fd, TCSANOW, old);
 }
 
-static int console_loop(int fd, struct xs_handle *xs, char *pty_path)
+static int console_loop(int fd, struct xs_handle *xs, char *pty_path,
+		        bool interactive)
 {
 	int ret, xs_fd = xs_fileno(xs), max_fd;
 
@@ -176,8 +177,13 @@ static int console_loop(int fd, struct xs_handle *xs, char *pty_path)
 		fd_set fds;
 
 		FD_ZERO(&fds);
-		FD_SET(STDIN_FILENO, &fds);
-		max_fd = STDIN_FILENO;
+		if (interactive) {
+			FD_SET(STDIN_FILENO, &fds);
+			max_fd = STDIN_FILENO;
+		}
+		else {
+			max_fd = -1;
+		}
 		FD_SET(xs_fd, &fds);
 		if (xs_fd > max_fd) max_fd = xs_fd;
 		if (fd != -1) FD_SET(fd, &fds);
@@ -284,6 +290,10 @@ int main(int argc, char **argv)
 	struct xs_handle *xs;
 	char *end;
 	console_type type = CONSOLE_INVAL;
+	bool interactive = 0;
+
+	if (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO))
+		interactive = 1;
 
 	while((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
 		switch(ch) {
@@ -390,9 +400,11 @@ int main(int argc, char **argv)
 	}
 
 	init_term(spty, &attr);
-	init_term(STDIN_FILENO, &stdin_old_attr);
-	atexit(restore_term_stdin); /* if this fails, oh dear */
-	console_loop(spty, xs, path);
+	if (interactive) {
+		init_term(STDIN_FILENO, &stdin_old_attr);
+		atexit(restore_term_stdin); /* if this fails, oh dear */
+	}
+	console_loop(spty, xs, path, interactive);
 
 	free(path);
 	free(dom_path);
-- 
2.1.4

Re: [PATCH] xenconsole: Allow non-interactive use

[Ian Campbell]

On Wed, 2015-07-22 at 19:08 +0200, Martin Lucina wrote:
> If xenconsole is run with stdin closed or redirected to /dev/null,
> console_loop() will return immediately due to failure to read from
> STDIN_FILENO. This patch tests if stdin and stdout are both connected 
> to
> a TTY and, if not, xenconsole will not attempt to read from stdin or
> modify stdout terminal attributes.
> 
> Existing behaviour when xenconsole is run from a terminal does not
> change.
> 
> This allows for non-interactive use, eg. running "xl create -c" under
> systemd or piping the output of "xl console" to another command.
> 
> Signed-off-by: Martin Lucina <martin@lucina.net>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

WRT the 4.6 freeze I'm torn between calling this a feature or a bugfix.

A pair of nits, which probably aren't worth acting on:

> @@ -176,8 +177,13 @@ static int console_loop(int fd, struct xs_handle
> *xs, char *pty_path)
>  		fd_set fds;
>  
>  		FD_ZERO(&fds);
> -		FD_SET(STDIN_FILENO, &fds);
> -		max_fd = STDIN_FILENO;
> +		if (interactive) {
> +			FD_SET(STDIN_FILENO, &fds);
> +			max_fd = STDIN_FILENO;
> +		}
> +		else {
> +			max_fd = -1;
> +		}

Looking at the rest of the file and tools/console subtree it seems the
prevailing coding style is:

		} else
                	                max_fd = -1;

(i.e. } brace on the same line as the else and no {} for single
statements after an else).

But maybe it would be better to set max_fd = -1 on declaration and do
the max dance here as with the following cases?

Ian.

Re: [PATCH] xenconsole: Allow non-interactive use

[Wei Liu]

On Thu, Jul 23, 2015 at 09:48:50AM +0100, Ian Campbell wrote:
> On Wed, 2015-07-22 at 19:08 +0200, Martin Lucina wrote:
> > If xenconsole is run with stdin closed or redirected to /dev/null,
> > console_loop() will return immediately due to failure to read from
> > STDIN_FILENO. This patch tests if stdin and stdout are both connected 
> > to
> > a TTY and, if not, xenconsole will not attempt to read from stdin or
> > modify stdout terminal attributes.
> > 
> > Existing behaviour when xenconsole is run from a terminal does not
> > change.
> > 
> > This allows for non-interactive use, eg. running "xl create -c" under
> > systemd or piping the output of "xl console" to another command.
> > 
> > Signed-off-by: Martin Lucina <martin@lucina.net>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> WRT the 4.6 freeze I'm torn between calling this a feature or a bugfix.
> 

I'm inclined to say it's a bugfix. It is reasonable that users want to
pipe console output to logging daemon.

Wei.

> A pair of nits, which probably aren't worth acting on:
> 
> > @@ -176,8 +177,13 @@ static int console_loop(int fd, struct xs_handle
> > *xs, char *pty_path)
> >  		fd_set fds;
> >  
> >  		FD_ZERO(&fds);
> > -		FD_SET(STDIN_FILENO, &fds);
> > -		max_fd = STDIN_FILENO;
> > +		if (interactive) {
> > +			FD_SET(STDIN_FILENO, &fds);
> > +			max_fd = STDIN_FILENO;
> > +		}
> > +		else {
> > +			max_fd = -1;
> > +		}
> 
> Looking at the rest of the file and tools/console subtree it seems the
> prevailing coding style is:
> 
> 		} else
>                 	                max_fd = -1;
> 
> (i.e. } brace on the same line as the else and no {} for single
> statements after an else).
> 
> But maybe it would be better to set max_fd = -1 on declaration and do
> the max dance here as with the following cases?
> 
> Ian.

Re: [PATCH] xenconsole: Allow non-interactive use

[Martin Lucina]

On Thursday, 23.07.2015 at 09:48, Ian Campbell wrote:
> On Wed, 2015-07-22 at 19:08 +0200, Martin Lucina wrote:
> > If xenconsole is run with stdin closed or redirected to /dev/null,
> > console_loop() will return immediately due to failure to read from
> > STDIN_FILENO. This patch tests if stdin and stdout are both connected 
> > to
> > a TTY and, if not, xenconsole will not attempt to read from stdin or
> > modify stdout terminal attributes.
> > 
> > Existing behaviour when xenconsole is run from a terminal does not
> > change.
> > 
> > This allows for non-interactive use, eg. running "xl create -c" under
> > systemd or piping the output of "xl console" to another command.
> > 
> > Signed-off-by: Martin Lucina <martin@lucina.net>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> WRT the 4.6 freeze I'm torn between calling this a feature or a bugfix.
> 
> A pair of nits, which probably aren't worth acting on:
> 
> > @@ -176,8 +177,13 @@ static int console_loop(int fd, struct xs_handle
> > *xs, char *pty_path)
> >  		fd_set fds;
> >  
> >  		FD_ZERO(&fds);
> > -		FD_SET(STDIN_FILENO, &fds);
> > -		max_fd = STDIN_FILENO;
> > +		if (interactive) {
> > +			FD_SET(STDIN_FILENO, &fds);
> > +			max_fd = STDIN_FILENO;
> > +		}
> > +		else {
> > +			max_fd = -1;
> > +		}
> 
> Looking at the rest of the file and tools/console subtree it seems the
> prevailing coding style is:
> 
> 		} else
>                 	                max_fd = -1;
> 
> (i.e. } brace on the same line as the else and no {} for single
> statements after an else).
> 
> But maybe it would be better to set max_fd = -1 on declaration and do
> the max dance here as with the following cases?

Declaring max_fd = -1 is indeed clearer, I can do a v2 with that change if
you like.

One other bug that my change makes potentially easier to trigger is that
you can run "xl console DOMID" multiple times with the same DOMID and the
result is broken; each instance gets part of the data written to the
console.

Any ideas on how to address this in a simple fashion?

Martin

Re: [PATCH] xenconsole: Allow non-interactive use

[Ian Campbell]

On Thu, 2015-07-23 at 17:09 +0200, Martin Lucina wrote:

> > But maybe it would be better to set max_fd = -1 on declaration and 
> > do
> > the max dance here as with the following cases?
> 
> Declaring max_fd = -1 is indeed clearer, I can do a v2 with that 
> change if you like.

If you are happy to then yes, please.

> One other bug that my change makes potentially easier to trigger is that
> you can run "xl console DOMID" multiple times with the same DOMID and the
> result is broken; each instance gets part of the data written to the
> console.
> 
> Any ideas on how to address this in a simple fashion?

Perhaps the client should take some exclusive lock (fcntl based?) on an
fd of an open file with domid in the name. Failure to get the lock
should result in the client exiting with some message indicating the
console is in use.

Ian.

[PATCH v2] xenconsole: Allow non-interactive use

[Martin Lucina]

If xenconsole is run with stdin closed or redirected to /dev/null,
console_loop() will return immediately due to failure to read from
STDIN_FILENO. This patch tests if stdin and stdout are both connected to
a TTY and, if not, xenconsole will not attempt to read from stdin or
modify stdout terminal attributes.

Existing behaviour when xenconsole is run from a terminal does not
change.

This allows for non-interactive use, eg. running "xl create -c" under
systemd or piping the output of "xl console" to another command.

Signed-off-by: Martin Lucina <martin@lucina.net>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/console/client/main.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index f4c783b..753b3aa 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -168,16 +168,19 @@ static void restore_term(int fd, struct termios *old)
 	tcsetattr(fd, TCSANOW, old);
 }
 
-static int console_loop(int fd, struct xs_handle *xs, char *pty_path)
+static int console_loop(int fd, struct xs_handle *xs, char *pty_path,
+		        bool interactive)
 {
-	int ret, xs_fd = xs_fileno(xs), max_fd;
+	int ret, xs_fd = xs_fileno(xs), max_fd = -1;
 
 	do {
 		fd_set fds;
 
 		FD_ZERO(&fds);
-		FD_SET(STDIN_FILENO, &fds);
-		max_fd = STDIN_FILENO;
+		if (interactive) {
+			FD_SET(STDIN_FILENO, &fds);
+			max_fd = STDIN_FILENO;
+		}
 		FD_SET(xs_fd, &fds);
 		if (xs_fd > max_fd) max_fd = xs_fd;
 		if (fd != -1) FD_SET(fd, &fds);
@@ -284,6 +287,10 @@ int main(int argc, char **argv)
 	struct xs_handle *xs;
 	char *end;
 	console_type type = CONSOLE_INVAL;
+	bool interactive = 0;
+
+	if (isatty(STDIN_FILENO) && isatty(STDOUT_FILENO))
+		interactive = 1;
 
 	while((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
 		switch(ch) {
@@ -390,9 +397,11 @@ int main(int argc, char **argv)
 	}
 
 	init_term(spty, &attr);
-	init_term(STDIN_FILENO, &stdin_old_attr);
-	atexit(restore_term_stdin); /* if this fails, oh dear */
-	console_loop(spty, xs, path);
+	if (interactive) {
+		init_term(STDIN_FILENO, &stdin_old_attr);
+		atexit(restore_term_stdin); /* if this fails, oh dear */
+	}
+	console_loop(spty, xs, path, interactive);
 
 	free(path);
 	free(dom_path);
-- 
2.1.4

Re: [PATCH] xenconsole: Allow non-interactive use

[Martin Lucina]

On Thursday, 23.07.2015 at 16:23, Ian Campbell wrote:
> On Thu, 2015-07-23 at 17:09 +0200, Martin Lucina wrote:
> 
> > > But maybe it would be better to set max_fd = -1 on declaration and 
> > > do
> > > the max dance here as with the following cases?
> > 
> > Declaring max_fd = -1 is indeed clearer, I can do a v2 with that 
> > change if you like.
> 
> If you are happy to then yes, please.
> 
> > One other bug that my change makes potentially easier to trigger is that
> > you can run "xl console DOMID" multiple times with the same DOMID and the
> > result is broken; each instance gets part of the data written to the
> > console.
> > 
> > Any ideas on how to address this in a simple fashion?
> 
> Perhaps the client should take some exclusive lock (fcntl based?) on an
> fd of an open file with domid in the name. Failure to get the lock
> should result in the client exiting with some message indicating the
> console is in use.

That begs the question of where to put the lock file. eg. NetBSD does not
seem to have /var/lock.

I tried using flock(pty_fd, LOCK_EX | LOCK_NB) locks directly on the pty
device and contrary to the documentation claiming support for regular files
only it worked fine. I tested using the xenconsole code on my Debian dom0,
and flock(1) on NetBSD. Would you consider this approach?

Martin

Re: [PATCH] xenconsole: Allow non-interactive use

[Ian Campbell]

On Fri, 2015-07-24 at 13:36 +0200, Martin Lucina wrote:
> On Thursday, 23.07.2015 at 16:23, Ian Campbell wrote:
> > On Thu, 2015-07-23 at 17:09 +0200, Martin Lucina wrote:
> > 
> > > > But maybe it would be better to set max_fd = -1 on declaration 
> > > > and 
> > > > do
> > > > the max dance here as with the following cases?
> > > 
> > > Declaring max_fd = -1 is indeed clearer, I can do a v2 with that 
> > > change if you like.
> > 
> > If you are happy to then yes, please.
> > 
> > > One other bug that my change makes potentially easier to trigger 
> > > is that
> > > you can run "xl console DOMID" multiple times with the same DOMID 
> > > and the
> > > result is broken; each instance gets part of the data written to 
> > > the
> > > console.
> > > 
> > > Any ideas on how to address this in a simple fashion?
> > 
> > Perhaps the client should take some exclusive lock (fcntl based?) 
> > on an
> > fd of an open file with domid in the name. Failure to get the lock
> > should result in the client exiting with some message indicating 
> > the
> > console is in use.
> 
> That begs the question of where to put the lock file. eg. NetBSD does 
> not
> seem to have /var/lock.

Configure ends up defining XEN_LOCK_DIR, so it should go in there
alongside the other locks.

> I tried using flock(pty_fd, LOCK_EX | LOCK_NB) locks directly on the pty
> device and contrary to the documentation claiming support for regular files
> only it worked fine. I tested using the xenconsole code on my Debian dom0,
> and flock(1) on NetBSD. Would you consider this approach?

Only if that functionality was required by POSIX IMHO.

Ian.

[PATCH] xenconsole: Ensure exclusive access to console using locks

[Martin Lucina]

If more than one instance of xenconsole is run against the same DOMID
then each instance will only get some data. This change ensures
exclusive access to the console by creating and obtaining an exclusive
lock on <XEN_LOCK_DIR>/xenconsole.<DOMID>.

Signed-off-by: Martin Lucina <martin@lucina.net>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/console/Makefile      |  2 +-
 tools/console/client/main.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/tools/console/Makefile b/tools/console/Makefile
index 71f8088..b6a51eb 100644
--- a/tools/console/Makefile
+++ b/tools/console/Makefile
@@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 CFLAGS  += -Werror
 
-CFLAGS  += $(CFLAGS_libxenctrl)
+CFLAGS  += $(CFLAGS_libxenctrl) -I$(XEN_ROOT)/tools/libxc
 CFLAGS  += $(CFLAGS_libxenstore)
 LDLIBS += $(LDLIBS_libxenctrl)
 LDLIBS += $(LDLIBS_libxenstore)
diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 753b3aa..709abc1 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -18,6 +18,7 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 \*/
 
+#include <sys/file.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/un.h>
@@ -40,10 +41,13 @@
 
 #include <xenstore.h>
 #include "xenctrl.h"
+#include "_paths.h"
 
 #define ESCAPE_CHARACTER 0x1d
 
 static volatile sig_atomic_t received_signal = 0;
+static char *lockfile = NULL;
+static int lockfd = -1;
 
 static void sighandler(int signum)
 {
@@ -267,6 +271,30 @@ static void restore_term_stdin(void)
 	restore_term(STDIN_FILENO, &stdin_old_attr);
 }
 
+static void console_lock(int domid)
+{
+	lockfile = malloc(PATH_MAX);
+	if (lockfile == NULL)
+		err(ENOMEM, "malloc");
+	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", XEN_LOCK_DIR, domid);
+
+	lockfd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
+	if (lockfd == -1)
+		err(errno, "Could not open %s", lockfile);
+	if (flock(lockfd, LOCK_EX|LOCK_NB) != 0)
+		err(errno, "Could not lock %s", lockfile);
+}
+
+static void console_unlock(void)
+{
+	if (lockfd != -1) {
+		flock(lockfd, LOCK_UN);
+		close(lockfd);
+	}
+	if (lockfile != NULL)
+		unlink(lockfile);
+}
+
 int main(int argc, char **argv)
 {
 	struct termios attr;
@@ -382,6 +410,9 @@ int main(int argc, char **argv)
 		exit(EINVAL);
 	}
 
+	console_lock(domid);
+	atexit(console_unlock);
+
 	/* Set a watch on this domain's console pty */
 	if (!xs_watch(xs, path, ""))
 		err(errno, "Can't set watch for console pty");
-- 
2.1.4

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Ian Campbell]

On Fri, 2015-07-24 at 14:51 +0200, Martin Lucina wrote:
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by creating and obtaining an 
> exclusive
> lock on <XEN_LOCK_DIR>/xenconsole.<DOMID>.
> 
> Signed-off-by: Martin Lucina <martin@lucina.net>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/console/Makefile      |  2 +-
>  tools/console/client/main.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/console/Makefile b/tools/console/Makefile
> index 71f8088..b6a51eb 100644
> --- a/tools/console/Makefile
> +++ b/tools/console/Makefile
> @@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
>  
>  CFLAGS  += -Werror
>  
> -CFLAGS  += $(CFLAGS_libxenctrl)
> +CFLAGS  += $(CFLAGS_libxenctrl) -I$(XEN_ROOT)/tools/libxc


I'm afraid this (delving into another components private headers) isn't
allowed. Instead you should add the two lines to tools/console/Makefile
to generate a local _paths.h:

genpath-target = $(call buildmakevars2header,_paths.h)
$(eval $(genpath-target))

Plus a dependency on it in the xenconsole rule and a .gitignore entry.

You might want to generate client/_paths.h instead to avoid needing to
muck around with CFLAGS.

>  CFLAGS  += $(CFLAGS_libxenstore)
>  LDLIBS += $(LDLIBS_libxenctrl)
>  LDLIBS += $(LDLIBS_libxenstore)
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 753b3aa..709abc1 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -18,6 +18,7 @@
>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  \*/
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -40,10 +41,13 @@
>  
>  #include 
>  #include "xenctrl.h"
> +#include "_paths.h"
>  
>  #define ESCAPE_CHARACTER 0x1d
>  
>  static volatile sig_atomic_t received_signal = 0;
> +static char *lockfile = NULL;
> +static int lockfd = -1;
>  
>  static void sighandler(int signum)
>  {
> @@ -267,6 +271,30 @@ static void restore_term_stdin(void)
>  > 	> restore_term(STDIN_FILENO, &stdin_old_attr);
>  }
>  
> +static void console_lock(int domid)
> +{
> +> 	> lockfile = malloc(PATH_MAX);
> +> 	> if (lockfile == NULL)
> +> 	> 	> err(ENOMEM, "malloc");

malloc sets errno, so I think you can pass that here as well.

Also, a static buffer would be fine in this context I think.

> +	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", XEN_LOCK_DIR, domid);

I think you can use string concatenation here, e.g.
    XEN_LOCK_DIR "/xenconsole.%d"


Given the known limits on the size of domid it would probably be
possible to figure out a tighter limit than PATH_MAX.

> +
> +> 	> lockfd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
> +> 	> if (lockfd == -1)
> +> 	> 	> err(errno, "Could not open %s", lockfile);
> +> 	> if (flock(lockfd, LOCK_EX|LOCK_NB) != 0)
> +> 	> 	> err(errno, "Could not lock %s", lockfile);
> +}
> +
> +static void console_unlock(void)
> +{
> +> 	> if (lockfd != -1) {
> +> 	> 	> flock(lockfd, LOCK_UN);
> +> 	> 	> close(lockfd);
> +> 	> }
> +> 	> if (lockfile != NULL)
> +> 	> 	> unlink(lockfile);


I think this introduces a race, if another client arrives between the
unlock/close and the unlink then you will delete the file which that
second client now has a lock on, resulting in a third client being able
to take the lock (by creating a new file) when it should already be
locked.

I think the answer is to either not remove the lockfile or to do so
_before_ dropping the lock, which makes the unlink itself the effective
unlock point.

Ian.

Re: [PATCH v2] xenconsole: Allow non-interactive use

[Ian Campbell]

On Fri, 2015-07-24 at 13:30 +0200, Martin Lucina wrote:
> If xenconsole is run with stdin closed or redirected to /dev/null,
> console_loop() will return immediately due to failure to read from
> STDIN_FILENO. This patch tests if stdin and stdout are both connected 
> to
> a TTY and, if not, xenconsole will not attempt to read from stdin or
> modify stdout terminal attributes.
> 
> Existing behaviour when xenconsole is run from a terminal does not
> change.
> 
> This allows for non-interactive use, eg. running "xl create -c" under
> systemd or piping the output of "xl console" to another command.
> 
> Signed-off-by: Martin Lucina <martin@lucina.net>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked + applied, thanks.

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Ian Jackson]

Martin Lucina writes ("[PATCH] xenconsole: Ensure exclusive access to console using locks"):
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by creating and obtaining an exclusive
> lock on <XEN_LOCK_DIR>/xenconsole.<DOMID>.

It is a shame that we are still using ptys for the xenconsole
connection.  If it were a socket we could allow as many clients as we
like.  But I haven't got round to fixing this for the last n years so
I think the general plan you have makes sense.


> +static void console_lock(int domid)
> +{
> +	lockfile = malloc(PATH_MAX);
> +	if (lockfile == NULL)
> +		err(ENOMEM, "malloc");
> +	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", XEN_LOCK_DIR, domid);

Why not use asprintf ?


> +	lockfd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
> +	if (lockfd == -1)
> +		err(errno, "Could not open %s", lockfile);
> +	if (flock(lockfd, LOCK_EX|LOCK_NB) != 0)
> +		err(errno, "Could not lock %s", lockfile);
> +}

This locking strategy is not safe if the lockfile is ever unlinked,
because it allows:

    A   open         flock              .o{ I have the lock }
 
    B        unlink

    C                      open flock   .o{ I have the lock }

> +static void console_unlock(void)
> +{
> +	if (lockfd != -1) {
> +		flock(lockfd, LOCK_UN);
> +		close(lockfd);
> +	}
> +	if (lockfile != NULL)
> +		unlink(lockfile);

And this unlinking strategy is not safe even if we are more careful
with our locking.  You must only unlink with the lock held.


You should use the same recipe as with-lock-ex (from chiark-utils)

  http://www.chiark.greenend.org.uk/ucgi/~ian/git?p=chiark-utils.git;a=blob;f=cprogs/with-lock-ex.c;h=1850d1f0283bd61fc1da12458cba7ec0a227c572;hb=HEAD
  
(which we also use in tools/hotplug/Linux/locking.sh and
tools/libxl/libxl_internal.c:libxl__lock_domain_userdata)


Ian.

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] xenconsole: Ensure exclusive access to console using locks"):
> I think the answer is to either not remove the lockfile

That is a bit messy (although not entirely wrong).

> or to do so _before_ dropping the lock, which makes the unlink
> itself the effective unlock point.

This is necessary but not sufficient.  With only that change:

  A  open flock    unlink    close

  B            open               flock  .o{ I have the lock }

  C                      open     flock  .o{ I have the lock }

B is mistaken because it olds an fd onto an unlinked file.

Ian.

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Martin Lucina]

On Friday, 24.07.2015 at 14:11, Ian Campbell wrote:
> I'm afraid this (delving into another components private headers) isn't
> allowed. Instead you should add the two lines to tools/console/Makefile
> to generate a local _paths.h:
> 
> genpath-target = $(call buildmakevars2header,_paths.h)
> $(eval $(genpath-target))
> 
> Plus a dependency on it in the xenconsole rule and a .gitignore entry.
> 
> You might want to generate client/_paths.h instead to avoid needing to
> muck around with CFLAGS.

The above works, but causes genpath-target to be re-run each time "make" is
called in tools/console, i.e. no longer says "Nothing to be done for all".
I presume that's ok?

> I think you can use string concatenation here, e.g.
>     XEN_LOCK_DIR "/xenconsole.%d"
> 
> 
> Given the known limits on the size of domid it would probably be
> possible to figure out a tighter limit than PATH_MAX.

Ok, I can just make it the usual 255 ...

Martin

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Martin Lucina]

On Friday, 24.07.2015 at 14:32, Ian Jackson wrote:
> > +static void console_lock(int domid)
> > +{
> > +	lockfile = malloc(PATH_MAX);
> > +	if (lockfile == NULL)
> > +		err(ENOMEM, "malloc");
> > +	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", XEN_LOCK_DIR, domid);
> 
> Why not use asprintf ?

asprintf is a GNU/BSD extension, not available on e.g. Solaris. Do we care
about that?

> You should use the same recipe as with-lock-ex (from chiark-utils)
> 
>   http://www.chiark.greenend.org.uk/ucgi/~ian/git?p=chiark-utils.git;a=blob;f=cprogs/with-lock-ex.c;h=1850d1f0283bd61fc1da12458cba7ec0a227c572;hb=HEAD
>   
> (which we also use in tools/hotplug/Linux/locking.sh and
> tools/libxl/libxl_internal.c:libxl__lock_domain_userdata)

Thanks for the pointers, I'll use that strategy.

Martin

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Ian Campbell]

On Fri, 2015-07-24 at 14:32 +0100, Ian Jackson wrote:
> 
> 
> > +static void console_lock(int domid)
> > +{
> > +	lockfile = malloc(PATH_MAX);
> > +	if (lockfile == NULL)
> > +		err(ENOMEM, "malloc");
> > +	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", 
> > XEN_LOCK_DIR, domid);
> 
> Why not use asprintf ?

I thought that, but it isn't generally available (not on one of the BSDs, I
think) e.g. libxl provides its own (libxl_osdep.c) if configure says it
isn't available.

It's a shame that isn't more generally useful throughout the tree.

But IMHO a static buffer should be fine for this use case.

Ian.

Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Ian Campbell]

On Fri, 2015-07-24 at 15:40 +0200, Martin Lucina wrote:
> On Friday, 24.07.2015 at 14:11, Ian Campbell wrote:
> > I'm afraid this (delving into another components private headers) isn't
> > allowed. Instead you should add the two lines to tools/console/Makefile
> > to generate a local _paths.h:
> > 
> > genpath-target = $(call buildmakevars2header,_paths.h)
> > $(eval $(genpath-target))
> > 
> > Plus a dependency on it in the xenconsole rule and a .gitignore entry.
> > 
> > You might want to generate client/_paths.h instead to avoid needing to
> > muck around with CFLAGS.
> 
> The above works, but causes genpath-target to be re-run each time "make" 
> is
> called in tools/console, i.e. no longer says "Nothing to be done for 
> all".
> I presume that's ok?

I think so, yes.

> > I think you can use string concatenation here, e.g.
> >     XEN_LOCK_DIR "/xenconsole.%d"
> > 
> > 
> > Given the known limits on the size of domid it would probably be
> > possible to figure out a tighter limit than PATH_MAX.
> 
> Ok, I can just make it the usual 255 ...

Or sizeof(XEN_LOCK_DIR"/xenconsole.") + 8 ? (8 being more digits that any
valid domid can have), which avoids problems with people putting the lock
dir somewhere ludicrous.

Ian.

[PATCH v2] xenconsole: Ensure exclusive access to console using locks

[Martin Lucina]

If more than one instance of xenconsole is run against the same DOMID
then each instance will only get some data. This change ensures
exclusive access to the console by obtaining an exclusive lock on
<XEN_LOCK_DIR>/xenconsole.<DOMID>.

The locking strategy used is based on
tools/libxl/libxl_internal.c:libxl__lock_domain_userdata().

Signed-off-by: Martin Lucina <martin@lucina.net>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 .gitignore                  |  1 +
 tools/console/Makefile      |  5 ++++-
 tools/console/client/main.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index f6ddb00..bccbd12 100644
--- a/.gitignore
+++ b/.gitignore
@@ -100,6 +100,7 @@ tools/blktap2/vhd/vhd-update
 tools/blktap2/vhd/vhd-util
 tools/console/xenconsole
 tools/console/xenconsoled
+tools/console/client/_paths.h
 tools/debugger/gdb/gdb-6.2.1-linux-i386-xen/*
 tools/debugger/gdb/gdb-6.2.1/*
 tools/debugger/gdb/gdb-6.2.1.tar.bz2
diff --git a/tools/console/Makefile b/tools/console/Makefile
index 71f8088..1ce5590 100644
--- a/tools/console/Makefile
+++ b/tools/console/Makefile
@@ -28,9 +28,12 @@ distclean: clean
 xenconsoled: $(patsubst %.c,%.o,$(wildcard daemon/*.c))
 	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
 
-xenconsole: $(patsubst %.c,%.o,$(wildcard client/*.c))
+xenconsole: $(patsubst %.c,%.o,$(wildcard client/*.c)) client/_paths.h
 	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_xenconsole) $(APPEND_LDFLAGS)
 
+genpath-target = $(call buildmakevars2header,client/_paths.h)
+$(eval $(genpath-target))
+
 .PHONY: install
 install: $(BIN)
 	$(INSTALL_DIR) $(DESTDIR)/$(sbindir)
diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 753b3aa..eeb6e70 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -18,7 +18,9 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 \*/
 
+#include <sys/file.h>
 #include <sys/types.h>
+#include <sys/stat.h>
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <stdio.h>
@@ -40,10 +42,13 @@
 
 #include <xenstore.h>
 #include "xenctrl.h"
+#include "_paths.h"
 
 #define ESCAPE_CHARACTER 0x1d
 
 static volatile sig_atomic_t received_signal = 0;
+static char lockfile[sizeof (XEN_LOCK_DIR "/xenconsole.") + 8] = { 0 };
+static int lockfd = -1;
 
 static void sighandler(int signum)
 {
@@ -267,6 +272,51 @@ static void restore_term_stdin(void)
 	restore_term(STDIN_FILENO, &stdin_old_attr);
 }
 
+/* The following locking strategy is based on that from
+ * libxl__domain_userdata_lock(), with the difference that we want to fail if we
+ * cannot acquire the lock rather than wait indefinitely.
+ */
+static void console_lock(int domid)
+{
+	struct stat stab, fstab;
+
+	snprintf(lockfile, sizeof lockfile, "%s%d", XEN_LOCK_DIR "/xenconsole.", domid);
+
+	while (true) {
+		lockfd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
+		if (lockfd < 0)
+			err(errno, "Could not open %s", lockfile);
+
+		while (flock(lockfd, LOCK_EX | LOCK_NB)) {
+			if (errno == EINTR)
+				continue;
+			else
+				err(errno, "Could not lock %s", lockfile);
+		}
+		if (fstat(lockfd, &fstab))
+			err(errno, "Could not fstat %s", lockfile);
+		if (stat(lockfile, &stab)) {
+			if (errno != ENOENT)
+				err(errno, "Could not stat %s", lockfile);
+		} else {
+			if (stab.st_dev == fstab.st_dev && stab.st_ino == fstab.st_ino)
+				break;
+		}
+
+		close(lockfd);
+	}
+	
+	return;
+}
+
+static void console_unlock(void)
+{
+	if (lockfile[0])
+		unlink(lockfile);
+	if (lockfd != -1)
+		close(lockfd);
+}
+
 int main(int argc, char **argv)
 {
 	struct termios attr;
@@ -382,6 +432,9 @@ int main(int argc, char **argv)
 		exit(EINVAL);
 	}
 
+	console_lock(domid);
+	atexit(console_unlock);
+
 	/* Set a watch on this domain's console pty */
 	if (!xs_watch(xs, path, ""))
 		err(errno, "Can't set watch for console pty");
-- 
2.1.4

Re: [PATCH v2] xenconsole: Ensure exclusive access to console using locks

[Ian Jackson]

Martin Lucina writes ("[PATCH v2] xenconsole: Ensure exclusive access to console using locks"):
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by obtaining an exclusive lock on
> <XEN_LOCK_DIR>/xenconsole.<DOMID>.
...
> +static void console_unlock(void)
> +{
> +	if (lockfile[0])
> +		unlink(lockfile);
> +	if (lockfd != -1)
> +		close(lockfd);
> +}

If called when unlocked this could unlink the lockfile anyway.

Ian.

Re: [PATCH v2] xenconsole: Ensure exclusive access to console using locks

[Ian Campbell]

On Fri, 2015-07-24 at 16:47 +0200, Martin Lucina wrote:
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by obtaining an exclusive lock on
> <XEN_LOCK_DIR>/xenconsole.<DOMID>.
> 
> The locking strategy used is based on
> tools/libxl/libxl_internal.c:libxl__lock_domain_userdata().

I'll let Ian deal with reviewing the locking but, only one small comment
from me:
> 
> diff --git a/tools/console/Makefile b/tools/console/Makefile
> index 71f8088..1ce5590 100644
> --- a/tools/console/Makefile
> +++ b/tools/console/Makefile

I forgot to mention adding the paths file to the clean rule too, sorry.

[PATCH v3] xenconsole: Ensure exclusive access to console using locks

[Martin Lucina]

If more than one instance of xenconsole is run against the same DOMID
then each instance will only get some data. This change ensures
exclusive access to the console by obtaining an exclusive lock on
<XEN_LOCK_DIR>/xenconsole.<DOMID>.

The locking strategy used is based on
tools/libxl/libxl_internal.c:libxl__lock_domain_userdata().

Signed-off-by: Martin Lucina <martin@lucina.net>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 .gitignore                  |  1 +
 tools/console/Makefile      |  6 ++++-
 tools/console/client/main.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index f6ddb00..bccbd12 100644
--- a/.gitignore
+++ b/.gitignore
@@ -100,6 +100,7 @@ tools/blktap2/vhd/vhd-update
 tools/blktap2/vhd/vhd-util
 tools/console/xenconsole
 tools/console/xenconsoled
+tools/console/client/_paths.h
 tools/debugger/gdb/gdb-6.2.1-linux-i386-xen/*
 tools/debugger/gdb/gdb-6.2.1/*
 tools/debugger/gdb/gdb-6.2.1.tar.bz2
diff --git a/tools/console/Makefile b/tools/console/Makefile
index 71f8088..77e8f29 100644
--- a/tools/console/Makefile
+++ b/tools/console/Makefile
@@ -21,6 +21,7 @@ all: $(BIN)
 clean:
 	$(RM) *.a *.so *.o *.rpm $(BIN) $(DEPS)
 	$(RM) client/*.o daemon/*.o
+	$(RM) client/_paths.h
 
 .PHONY: distclean
 distclean: clean
@@ -28,9 +29,12 @@ distclean: clean
 xenconsoled: $(patsubst %.c,%.o,$(wildcard daemon/*.c))
 	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
 
-xenconsole: $(patsubst %.c,%.o,$(wildcard client/*.c))
+xenconsole: client/_paths.h $(patsubst %.c,%.o,$(wildcard client/*.c))
 	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_xenconsole) $(APPEND_LDFLAGS)
 
+genpath-target = $(call buildmakevars2header,client/_paths.h)
+$(eval $(genpath-target))
+
 .PHONY: install
 install: $(BIN)
 	$(INSTALL_DIR) $(DESTDIR)/$(sbindir)
diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 753b3aa..ff9ac7d 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -18,7 +18,9 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 \*/
 
+#include <sys/file.h>
 #include <sys/types.h>
+#include <sys/stat.h>
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <stdio.h>
@@ -40,10 +42,13 @@
 
 #include <xenstore.h>
 #include "xenctrl.h"
+#include "_paths.h"
 
 #define ESCAPE_CHARACTER 0x1d
 
 static volatile sig_atomic_t received_signal = 0;
+static char lockfile[sizeof (XEN_LOCK_DIR "/xenconsole.") + 8] = { 0 };
+static int lockfd = -1;
 
 static void sighandler(int signum)
 {
@@ -267,6 +272,53 @@ static void restore_term_stdin(void)
 	restore_term(STDIN_FILENO, &stdin_old_attr);
 }
 
+/* The following locking strategy is based on that from
+ * libxl__domain_userdata_lock(), with the difference that we want to fail if we
+ * cannot acquire the lock rather than wait indefinitely.
+ */
+static void console_lock(int domid)
+{
+	struct stat stab, fstab;
+	int fd;
+
+	snprintf(lockfile, sizeof lockfile, "%s%d", XEN_LOCK_DIR "/xenconsole.", domid);
+
+	while (true) {
+		fd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
+		if (fd < 0)
+			err(errno, "Could not open %s", lockfile);
+
+		while (flock(fd, LOCK_EX | LOCK_NB)) {
+			if (errno == EINTR)
+				continue;
+			else
+				err(errno, "Could not lock %s", lockfile);
+		}
+		if (fstat(fd, &fstab))
+			err(errno, "Could not fstat %s", lockfile);
+		if (stat(lockfile, &stab)) {
+			if (errno != ENOENT)
+				err(errno, "Could not stat %s", lockfile);
+		} else {
+			if (stab.st_dev == fstab.st_dev && stab.st_ino == fstab.st_ino)
+				break;
+		}
+
+		close(fd);
+	}
+
+	lockfd = fd;
+	return;
+}
+
+static void console_unlock(void)
+{
+	if (lockfile[0] && lockfd != -1) {
+		unlink(lockfile);
+		close(lockfd);
+	}
+}
+
 int main(int argc, char **argv)
 {
 	struct termios attr;
@@ -382,6 +434,9 @@ int main(int argc, char **argv)
 		exit(EINVAL);
 	}
 
+	console_lock(domid);
+	atexit(console_unlock);
+
 	/* Set a watch on this domain's console pty */
 	if (!xs_watch(xs, path, ""))
 		err(errno, "Can't set watch for console pty");
-- 
2.1.4

Re: [PATCH v3] xenconsole: Ensure exclusive access to console using locks

[Ian Jackson]

Martin Lucina writes ("[PATCH v3] xenconsole: Ensure exclusive access to console using locks"):
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by obtaining an exclusive lock on
> <XEN_LOCK_DIR>/xenconsole.<DOMID>.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH v3] xenconsole: Ensure exclusive access to console using locks

[Martin Lucina]

On Friday, 24.07.2015 at 17:01, Ian Jackson wrote:
> Martin Lucina writes ("[PATCH v3] xenconsole: Ensure exclusive access to console using locks"):
> > If more than one instance of xenconsole is run against the same DOMID
> > then each instance will only get some data. This change ensures
> > exclusive access to the console by obtaining an exclusive lock on
> > <XEN_LOCK_DIR>/xenconsole.<DOMID>.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Can this also make it into the 4.6 release on the grounds of being a bugfix
for xenconsole, or is this change too invasive?

Martin

Re: [PATCH v3] xenconsole: Ensure exclusive access to console using locks

[Wei Liu]

On Mon, Jul 27, 2015 at 02:44:57PM +0200, Martin Lucina wrote:
> On Friday, 24.07.2015 at 17:01, Ian Jackson wrote:
> > Martin Lucina writes ("[PATCH v3] xenconsole: Ensure exclusive access to console using locks"):
> > > If more than one instance of xenconsole is run against the same DOMID
> > > then each instance will only get some data. This change ensures
> > > exclusive access to the console by obtaining an exclusive lock on
> > > <XEN_LOCK_DIR>/xenconsole.<DOMID>.
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Can this also make it into the 4.6 release on the grounds of being a bugfix
> for xenconsole, or is this change too invasive?
> 

I would say this is a bugfix. The locking pattern and implementation are
well understood. The risk is minimal.

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

> Martin

Re: [PATCH v3] xenconsole: Ensure exclusive access to console using locks

[Ian Campbell]

On Mon, 2015-07-27 at 14:29 +0100, Wei Liu wrote:
> On Mon, Jul 27, 2015 at 02:44:57PM +0200, Martin Lucina wrote:
> > On Friday, 24.07.2015 at 17:01, Ian Jackson wrote:
> > > Martin Lucina writes ("[PATCH v3] xenconsole: Ensure exclusive access 
> > > to console using locks"):
> > > > If more than one instance of xenconsole is run against the same 
> > > > DOMID
> > > > then each instance will only get some data. This change ensures
> > > > exclusive access to the console by obtaining an exclusive lock on
> > > > <XEN_LOCK_DIR>/xenconsole.<DOMID>.
> > > 
> > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > Can this also make it into the 4.6 release on the grounds of being a 
> > bugfix
> > for xenconsole, or is this change too invasive?
> > 
> 
> I would say this is a bugfix. The locking pattern and implementation are
> well understood. The risk is minimal.
> 
> Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Applied, thanks.