[PATCH] weston-launch: Fixed TTY switching

[David Herrmann]

Hi

On Wed, Apr 8, 2015 at 2:03 AM, Bryce Harrington <bryce at osg.samsung.com> wrote:
> 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);
>> +     }
>> +

Why this? I don't get why wl->tty is not good enough and you need to
reopen the fd.

>>       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

Why do this at all? There is no reason to wait for the VT to become
active. Just issue the VT_ACTIVATE and continue, it's async and that's
good.

Btw., I'm no big fan of activating a VT when starting a compositor.
Xorg requires this as it cannot initialize in background, but new
compositors should really support this. Imagine you're started by a
login-manager. You really want the compositor to initialize in
background and wait to be switched to by the login manager.

Yes, weston-launch is special, as it is its own login-manager. So I'd
be fine with the VT_ACTIVATE. But i cannot see why we'd need
VT_WAITACTIVE?

>>       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?

This is weird, indeed. No idea why the TTY is closed. But this fix is
independent of all the other hunks in this patch, so I'd apply it
separately.

Thanks
David