[PATCH] weston-launch: Fixed TTY switching

Wed Apr 8 05:07:39 PDT 2015

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.

Trying to use wl->tty after child process is terminated will return errors from ioctl.
I found when fd is reopened ioctls are working correctly. I'm suspecting that child process closes tty's fd (it's passed to child process via environment variable) when terminates and somehow deinitializes tty , wl->tty fd itself is still valid, but ioctls are failing for some reason.
> >>       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.

I will change to 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?

I noticed that tty setup in weston (launcher-util.c) uses VT_ACTIVATE and VT_WAITACTIVATE and moved it here, but of course if you think that VT_WAITACTIVE is not needed that is fine for me.
> >>       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

Regards,
Mateusz
--------------------------------------------------------------
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.