[PATCH] weston-launch: Fixed TTY switching

[Bryce Harrington]

On Wed, Apr 01, 2015 at 08:10:44AM +0100, mateuszx.potrola at intel.com wrote:
> From: Mateusz Polrola <mateuszx.potrola at intel.com>
> 
> After weston-launch is executing weston it cannot close TTY file,
> because it is still required to properly handle SIGUSR1 and SIGUSR2
> signals that are used for switching TTY.
> 
> Additionally after opening TTY it has to be activated, so that user
> don't have to manually switch to TTY used by weston first to be able to switch
> to other TTY.
> During shutdown TTY file has to be reopened, as device was already deinitialize by child
> process, but it is still required to cleanup VT handling and leave
> system in sane state.
> 
> Signed-off-by: Mateusz Polrola <mateuszx.potrola at intel.com>
> ---
>  src/weston-launch.c |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/weston-launch.c b/src/weston-launch.c
> index 10c66de..312b955 100644
> --- a/src/weston-launch.c
> +++ b/src/weston-launch.c
> @@ -46,6 +46,7 @@
>  #include <linux/vt.h>
>  #include <linux/major.h>
>  #include <linux/kd.h>
> +#include <linux/limits.h>
>  
>  #include <pwd.h>
>  #include <grp.h>
> @@ -105,6 +106,7 @@ struct weston_launch {
>  	pid_t child;
>  	int verbose;
>  	char *new_user;
> +	char tty_path[PATH_MAX];
>  };
>  
>  union cmsg_data { unsigned char b[4]; int fd; };
> @@ -419,6 +421,13 @@ quit(struct weston_launch *wl, int status)
>  		pam_end(wl->ph, err);
>  	}
>  
> +	/* tty will be deinitialized by child process, so it has to be
> +	 * opened again to correctly cleanup vt handling. */
> +	if (wl->tty != STDIN_FILENO) {
> +		close(wl->tty);
> +		wl->tty = open(wl->tty_path, O_RDWR | O_NOCTTY);
> +	}
> +
>  	if (ioctl(wl->tty, KDSKBMUTE, 0) &&
>  	    ioctl(wl->tty, KDSKBMODE, wl->kb_mode))
>  		fprintf(stderr, "failed to restore keyboard mode: %m\n");
> @@ -521,8 +530,10 @@ setup_tty(struct weston_launch *wl, const char *tty)
>  		t = ttyname(STDIN_FILENO);
>  		if (t && strcmp(t, tty) == 0)
>  			wl->tty = STDIN_FILENO;
> -		else
> +		else {
>  			wl->tty = open(tty, O_RDWR | O_NOCTTY);
> +			strcpy(wl->tty_path, tty);
> +		}

I'm sure this is not going to ever be a problem since tty filenames and
paths are on the short side, but since the tty string is an input
parameter to this routine, it would be better defensive programming to
use strncpy.

>  	} else {
>  		int tty0 = open("/dev/tty0", O_WRONLY | O_CLOEXEC);
>  		char filename[16];
> @@ -535,6 +546,7 @@ setup_tty(struct weston_launch *wl, const char *tty)
>  
>  		snprintf(filename, sizeof filename, "/dev/tty%d", wl->ttynr);
>  		wl->tty = open(filename, O_RDWR | O_NOCTTY);
> +		strcpy(wl->tty_path, filename);

Since filename is set to a fixed length, I'm less worried about the
strcpy here, but for code consistency you might use strncpy here as
well.

>  		close(tty0);
>  	}
>  
> @@ -555,6 +567,10 @@ setup_tty(struct weston_launch *wl, const char *tty)
>  		wl->ttynr = minor(buf.st_rdev);
>  	}
>  
> +	/* Activate tty, otherwise tty switches won't work right away. */
> +	ioctl(wl->tty, VT_ACTIVATE, wl->ttynr);
> +	ioctl(wl->tty, VT_WAITACTIVE, wl->ttynr);
> +

Check the ioctl returns and issue perror() on failure.

Googling VT_WAITACTIVE shows misc. reports about it causing hangs in
years past.  No idea if that's at all likely here in Wayland.  But if it
is, it might be better to use a timed wait, checking the active tty each
cycle, like was done in this fix:

  http://permalink.gmane.org/gmane.linux.kernel.suspend.devel/7119

>  	if (ioctl(wl->tty, KDGKBMODE, &wl->kb_mode))
>  		error(1, errno, "failed to get current keyboard mode: %m\n");
>  
> @@ -744,8 +760,6 @@ main(int argc, char *argv[])
>  		launch_compositor(&wl, argc - optind, argv + optind);
>  
>  	close(wl.sock[1]);
> -	if (wl.tty != STDIN_FILENO)
> -		close(wl.tty);

I'm having some trouble following the move of the close logic.  I trust
what you've done is correct but it's not evident to me why?

>  	while (1) {
>  		struct pollfd fds[2];
> -- 
> 1.7.7.6
> 
> --------------------------------------------------------------
> Intel Shannon Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> Business address: Dromore House, East Park, Shannon, Co. Clare
> 
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel