[PATCH 1/2] tools/console: Error handling fixes

[PATCH 1/2] tools/console: Error handling fixes

[Ian Jackson]

These two were deferred from 4.4.0.  They should IMO be backported to
4.4.x via staging in due course.

 1/2 tools/console: reset tty when xenconsole fails
 2/2 tools/console: xenconsole tolerate tty errors

[PATCH 1/2] tools/console: reset tty when xenconsole fails

[Ian Jackson]

If xenconsole (the client program) fails, it calls err.  This would
previously neglect to reset the user's terminal to sanity.  Use atexit
to do so.

This routinely happens in Xen 4.4 RC5 with pygrub because something
writes the value "" to the tty xenstore key when using xenconsole.
The cause of this is not yet known, but after this patch it just
results in a harmless error message.

Reported-by: M A Young <m.a.young@durham.ac.uk>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: M A Young <m.a.young@durham.ac.uk>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/console/client/main.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index eb6a1a9..62159f6 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -257,6 +257,13 @@ typedef enum {
        CONSOLE_SERIAL,
 } console_type;
 
+static struct termios stdin_old_attr;
+
+static void restore_term_stdin(void)
+{
+	restore_term(STDIN_FILENO, &stdin_old_attr);
+}
+
 int main(int argc, char **argv)
 {
 	struct termios attr;
@@ -383,9 +390,9 @@ int main(int argc, char **argv)
 	}
 
 	init_term(spty, &attr);
-	init_term(STDIN_FILENO, &attr);
+	init_term(STDIN_FILENO, &stdin_old_attr);
+        atexit(restore_term_stdin); /* if this fails, oh dear */
 	console_loop(spty, xs, path);
-	restore_term(STDIN_FILENO, &attr);
 
 	free(path);
 	free(dom_path);
-- 
1.7.10.4

Re: [PATCH 1/2] tools/console: reset tty when xenconsole fails

[George Dunlap]

On 03/17/2014 04:24 PM, Ian Jackson wrote:
> If xenconsole (the client program) fails, it calls err.  This would
> previously neglect to reset the user's terminal to sanity.  Use atexit
> to do so.
>
> This routinely happens in Xen 4.4 RC5 with pygrub because something
> writes the value "" to the tty xenstore key when using xenconsole.
> The cause of this is not yet known, but after this patch it just
> results in a harmless error message.

Is it the case that the cause of this is still not yet known?  It seemed 
to be explained by the subsequent patch. :-)

In any case:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

>
> Reported-by: M A Young <m.a.young@durham.ac.uk>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: M A Young <m.a.young@durham.ac.uk>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>   tools/console/client/main.c |   11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index eb6a1a9..62159f6 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -257,6 +257,13 @@ typedef enum {
>          CONSOLE_SERIAL,
>   } console_type;
>   
> +static struct termios stdin_old_attr;
> +
> +static void restore_term_stdin(void)
> +{
> +	restore_term(STDIN_FILENO, &stdin_old_attr);
> +}
> +
>   int main(int argc, char **argv)
>   {
>   	struct termios attr;
> @@ -383,9 +390,9 @@ int main(int argc, char **argv)
>   	}
>   
>   	init_term(spty, &attr);
> -	init_term(STDIN_FILENO, &attr);
> +	init_term(STDIN_FILENO, &stdin_old_attr);
> +        atexit(restore_term_stdin); /* if this fails, oh dear */
>   	console_loop(spty, xs, path);
> -	restore_term(STDIN_FILENO, &attr);
>   
>   	free(path);
>   	free(dom_path);

Re: [PATCH 1/2] tools/console: reset tty when xenconsole fails

[Ian Jackson]

George Dunlap writes ("Re: [PATCH 1/2] tools/console: reset tty when xenconsole fails"):
> On 03/17/2014 04:24 PM, Ian Jackson wrote:
> > If xenconsole (the client program) fails, it calls err.  This would
> > previously neglect to reset the user's terminal to sanity.  Use atexit
> > to do so.
> >
> > This routinely happens in Xen 4.4 RC5 with pygrub because something
> > writes the value "" to the tty xenstore key when using xenconsole.
> > The cause of this is not yet known, but after this patch it just
> > results in a harmless error message.
> 
> Is it the case that the cause of this is still not yet known?  It seemed 
> to be explained by the subsequent patch. :-)

Um :-).  I will fix that in the commit message.

> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Thanks,
Ian.

Re: [PATCH 1/2] tools/console: reset tty when xenconsole fails

[Ian Campbell]

On Mon, 2014-03-17 at 16:24 +0000, Ian Jackson wrote:
> @@ -383,9 +390,9 @@ int main(int argc, char **argv)
>  	}
>  
>  	init_term(spty, &attr);
> -	init_term(STDIN_FILENO, &attr);
> +	init_term(STDIN_FILENO, &stdin_old_attr);
> +        atexit(restore_term_stdin); /* if this fails, oh dear */

Hard tab?

Otherwise, with the commit message amended as discussed with George:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

>  	console_loop(spty, xs, path);
> -	restore_term(STDIN_FILENO, &attr);
>  
>  	free(path);
>  	free(dom_path);

[PATCH 2/2] tools/console: xenconsole tolerate tty errors

[Ian Jackson]

Since 28d386fc4341 (XSA-57), libxl writes an empty value for the
console tty node, with read-only permission for the guest, when
setting up pv console "frontends".  (The actual tty value is later set
by xenconsoled.)   Writing an empty node is not strictly necessary to
stop the frontend from writing dangerous values here, but it is a good
belt-and-braces approach.

Unfortunately this confuses xenconsole.  It reads the empty value, and
tries to open it as the tty.  xenconsole then exits.

Fix this by having xenconsole treat an empty value the same way as no
value at all.

Also, make the error opening the tty be nonfatal: we just print a
warning, but do not exit.  I think this is helpful in theoretical
situations where xenconsole is racing with libxl and/or xenconsoled.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/console/client/main.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 62159f6..b882345 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -115,9 +115,11 @@ static int get_pty_fd(struct xs_handle *xs, char *path, int seconds)
 			 * disambiguate: just read the pty path */
 			pty_path = xs_read(xs, XBT_NULL, path, &len);
 			if (pty_path != NULL) {
-				pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
-				if (pty_fd == -1)
-					err(errno, "Could not open tty `%s'", pty_path);
+				if (pty_path[0] != '\0') {
+					pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
+					if (pty_fd == -1)
+						warn("Could not open tty `%s'", pty_path);
+				}
 				free(pty_path);
 			}
 		}
-- 
1.7.10.4

Re: [PATCH 2/2] tools/console: xenconsole tolerate tty errors

[Ian Campbell]

On Mon, 2014-03-17 at 16:24 +0000, Ian Jackson wrote:
> Since 28d386fc4341 (XSA-57), libxl writes an empty value for the
> console tty node, with read-only permission for the guest, when
> setting up pv console "frontends".  (The actual tty value is later set
> by xenconsoled.)   Writing an empty node is not strictly necessary to
> stop the frontend from writing dangerous values here, but it is a good
> belt-and-braces approach.
> 
> Unfortunately this confuses xenconsole.  It reads the empty value, and
> tries to open it as the tty.  xenconsole then exits.
> 
> Fix this by having xenconsole treat an empty value the same way as no
> value at all.
> 
> Also, make the error opening the tty be nonfatal: we just print a
> warning, but do not exit.  I think this is helpful in theoretical
> situations where xenconsole is racing with libxl and/or xenconsoled.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  tools/console/client/main.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 62159f6..b882345 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -115,9 +115,11 @@ static int get_pty_fd(struct xs_handle *xs, char *path, int seconds)
>  			 * disambiguate: just read the pty path */
>  			pty_path = xs_read(xs, XBT_NULL, path, &len);
>  			if (pty_path != NULL) {
> -				pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
> -				if (pty_fd == -1)
> -					err(errno, "Could not open tty `%s'", pty_path);
> +				if (pty_path[0] != '\0') {

Can the top-level condition not be
	if ( pty_path != NULL && pty_path[0] != '\0' )
avoiding the reindent?

Ack on the intent though.

> +					pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
> +					if (pty_fd == -1)
> +						warn("Could not open tty `%s'", pty_path);
> +				}
>  				free(pty_path);
>  			}
>  		}

Re: [PATCH 2/2] tools/console: xenconsole tolerate tty errors

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 2/2] tools/console: xenconsole tolerate tty errors"):
> On Mon, 2014-03-17 at 16:24 +0000, Ian Jackson wrote:
> > Since 28d386fc4341 (XSA-57), libxl writes an empty value for the
> > console tty node, with read-only permission for the guest, when
> > setting up pv console "frontends".  (The actual tty value is later set
> > by xenconsoled.)   Writing an empty node is not strictly necessary to
> > stop the frontend from writing dangerous values here, but it is a good
> > belt-and-braces approach.
...
> >  			if (pty_path != NULL) {
> > -				pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
> > -				if (pty_fd == -1)
> > -					err(errno, "Could not open tty `%s'", pty_path);
> > +				if (pty_path[0] != '\0') {
> 
> Can the top-level condition not be
> 	if ( pty_path != NULL && pty_path[0] != '\0' )
> avoiding the reindent?

That would leak pty_path.  I could move free(pty_path) out of the if,
though.  I think that would be better.

Ian.

[PATCH v2 1/2] tools/console: reset tty when xenconsole fails

[Ian Jackson]

If xenconsole (the client program) fails, it calls err.  This would
previously neglect to reset the user's terminal to sanity.  Use atexit
to do so.

This routinely happens in Xen 4.4 RC5 with pygrub because libxl
writes the value "" to the tty xenstore key when using xenconsole.
After this patch this just results in a harmless error message.

Reported-by: M A Young <m.a.young@durham.ac.uk>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: M A Young <m.a.young@durham.ac.uk>
CC: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
v2: Fix whitespace error (reintroduce hard tab)
    Fix commit message not to claim ignorance about root cause
---
 tools/console/client/main.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index eb6a1a9..7ce4f24 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -257,6 +257,13 @@ typedef enum {
        CONSOLE_SERIAL,
 } console_type;
 
+static struct termios stdin_old_attr;
+
+static void restore_term_stdin(void)
+{
+	restore_term(STDIN_FILENO, &stdin_old_attr);
+}
+
 int main(int argc, char **argv)
 {
 	struct termios attr;
@@ -383,9 +390,9 @@ int main(int argc, char **argv)
 	}
 
 	init_term(spty, &attr);
-	init_term(STDIN_FILENO, &attr);
+	init_term(STDIN_FILENO, &stdin_old_attr);
+	atexit(restore_term_stdin); /* if this fails, oh dear */
 	console_loop(spty, xs, path);
-	restore_term(STDIN_FILENO, &attr);
 
 	free(path);
 	free(dom_path);
-- 
1.7.10.4

[PATCH v2 2/2] tools/console: xenconsole tolerate tty errors

[Ian Jackson]

Since 28d386fc4341 (XSA-57), libxl writes an empty value for the
console tty node, with read-only permission for the guest, when
setting up pv console "frontends".  (The actual tty value is later set
by xenconsoled.)   Writing an empty node is not strictly necessary to
stop the frontend from writing dangerous values here, but it is a good
belt-and-braces approach.

Unfortunately this confuses xenconsole.  It reads the empty value, and
tries to open it as the tty.  xenconsole then exits.

Fix this by having xenconsole treat an empty value the same way as no
value at all.

Also, make the error opening the tty be nonfatal: we just print a
warning, but do not exit.  I think this is helpful in theoretical
situations where xenconsole is racing with libxl and/or xenconsoled.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>

---
v2: Combine two conditions and move the free
---
 tools/console/client/main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 7ce4f24..f4c783b 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -114,12 +114,12 @@ static int get_pty_fd(struct xs_handle *xs, char *path, int seconds)
 			/* We only watch for one thing, so no need to 
 			 * disambiguate: just read the pty path */
 			pty_path = xs_read(xs, XBT_NULL, path, &len);
-			if (pty_path != NULL) {
+			if (pty_path != NULL && pty_path[0] != '\0') {
 				pty_fd = open(pty_path, O_RDWR | O_NOCTTY);
 				if (pty_fd == -1)
-					err(errno, "Could not open tty `%s'", pty_path);
-				free(pty_path);
+					warn("Could not open tty `%s'", pty_path);
 			}
+			free(pty_path);
 		}
 	} while (pty_fd == -1 && (now = time(NULL)) < start + seconds);
 
-- 
1.7.10.4

Re: [PATCH v2 2/2] tools/console: xenconsole tolerate tty errors

[Ian Campbell]

On Tue, 2014-03-18 at 18:00 +0000, Ian Jackson wrote:
> Since 28d386fc4341 (XSA-57), libxl writes an empty value for the
> console tty node, with read-only permission for the guest, when
> setting up pv console "frontends".  (The actual tty value is later set
> by xenconsoled.)   Writing an empty node is not strictly necessary to
> stop the frontend from writing dangerous values here, but it is a good
> belt-and-braces approach.
> 
> Unfortunately this confuses xenconsole.  It reads the empty value, and
> tries to open it as the tty.  xenconsole then exits.
> 
> Fix this by having xenconsole treat an empty value the same way as no
> value at all.
> 
> Also, make the error opening the tty be nonfatal: we just print a
> warning, but do not exit.  I think this is helpful in theoretical
> situations where xenconsole is racing with libxl and/or xenconsoled.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

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

Re: [PATCH v2 2/2] tools/console: xenconsole tolerate tty errors

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH v2 2/2] tools/console: xenconsole tolerate tty errors"):
> On Tue, 2014-03-18 at 18:00 +0000, Ian Jackson wrote:
> > Since 28d386fc4341 (XSA-57), libxl writes an empty value for the
> > console tty node, with read-only permission for the guest, when
...
> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

Thanks, I have pushed both of these.

Ian.

Re: [PATCH v2 1/2] tools/console: reset tty when xenconsole fails

[Anthony Liguori]

On Tue, Mar 18, 2014 at 11:00 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> If xenconsole (the client program) fails, it calls err.  This would
> previously neglect to reset the user's terminal to sanity.  Use atexit
> to do so.
>
> This routinely happens in Xen 4.4 RC5 with pygrub because libxl
> writes the value "" to the tty xenstore key when using xenconsole.
> After this patch this just results in a harmless error message.
>
> Reported-by: M A Young <m.a.young@durham.ac.uk>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: M A Young <m.a.young@durham.ac.uk>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> ---
> v2: Fix whitespace error (reintroduce hard tab)
>     Fix commit message not to claim ignorance about root cause
> ---
>  tools/console/client/main.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index eb6a1a9..7ce4f24 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -257,6 +257,13 @@ typedef enum {
>         CONSOLE_SERIAL,
>  } console_type;
>
> +static struct termios stdin_old_attr;
> +
> +static void restore_term_stdin(void)
> +{
> +       restore_term(STDIN_FILENO, &stdin_old_attr);
> +}
> +
>  int main(int argc, char **argv)
>  {
>         struct termios attr;
> @@ -383,9 +390,9 @@ int main(int argc, char **argv)
>         }
>
>         init_term(spty, &attr);
> -       init_term(STDIN_FILENO, &attr);
> +       init_term(STDIN_FILENO, &stdin_old_attr);
> +       atexit(restore_term_stdin); /* if this fails, oh dear */

You may want to consider using a signal handler too.  atexit() isn't
called when exiting due to SIGTERM or SIGSEGV.

Regards,

Anthony Liguori

>         console_loop(spty, xs, path);
> -       restore_term(STDIN_FILENO, &attr);
>
>         free(path);
>         free(dom_path);
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2 1/2] tools/console: reset tty when xenconsole fails

[Ian Jackson]

Anthony Liguori writes ("Re: [Xen-devel] [PATCH v2 1/2] tools/console: reset tty when xenconsole fails"):
> On Tue, Mar 18, 2014 at 11:00 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote:
...
> >         init_term(spty, &attr);
> > -       init_term(STDIN_FILENO, &attr);
> > +       init_term(STDIN_FILENO, &stdin_old_attr);
> > +       atexit(restore_term_stdin); /* if this fails, oh dear */
> 
> You may want to consider using a signal handler too.  atexit() isn't
> called when exiting due to SIGTERM or SIGSEGV.

That's would be quite exciting.  I doubt tcsetattr is async-signal
safe.  We could prat about with sigprocmask and pselect perhaps, but
that's more than I want to do right now (or, indeed, soon).

In practice, the shell will normally restore the terminal in this
situation.

Ian.