Hi
> On Wed, Apr 8, 2015 at 2:03 AM, Bryce Harrington
> wrote:
> > On Wed, Apr 01, 2015 at 08:10:44AM +0100, mateuszx.potr...@intel.com
> wrote:
> >> From: Mateusz Polrola
> >>
> >> 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
> >> ---
> >> 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
> >> #include
> >> #include
> >> +#include
> >>
> >> #include
> >> #include
> >> @@ -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 backgroun
