On Thu, Apr 28, 2011 at 04:56:31PM +0200, Werner Fink wrote:
> Ensure a proper session on the terminal line, that is do a
> vhangup() and become the controlling terminal. After this
Is it really necessary? login(1) calls vhangup(), wouldn't be enough
to make the vhangup() call optional?
> + sa.sa_handler = SIG_IGN;
> + sa.sa_flags = SA_RESTART;
> + sigemptyset (&sa.sa_mask);
> + sigaction(SIGHUP, &sa, &sa_hup);
> + sigaction(SIGQUIT, &sa, &sa_quit);
> + sigaction(SIGINT, &sa, &sa_int);
Everyone who will read this your patch will not understand why sa_int
and sa_quit are necessary, it's used in another (next) patch...
It would be nice to have patches more consistent and standalone.
> };
> - static const struct option longopts[] = {
> + const struct option longopts[] = {
> + { "nohangup", no_argument, &nipple, F_NOHANGUP },
Ah... here is the "nipple" from the previous patch :-)
We usually keep the struct option read-only (static const) and the
each option has "case :" block in the options parser. I think it's
more readable and more effective that if the large "struct option" is
on the stack.
> + if (((tid = tcgetsid(fd)) < 0) || (pid != tid)) {
> + if (ioctl (fd, TIOCSCTTY, 1) == -1)
> + error("/dev/%s: cannot get controlling tty: %m", tty);
> + }
> +
> + if ((op->flags & F_NOHANGUP) == 0) {
> + /*
> + * vhangup() will replace all open file descriptors in the kernel
> + * that point to our controlling tty by a dummy that will deny
> + * further reading/writing to our device. It will also reset the
> + * tty to sane defaults, so we don't have to modify the tty device
Hmm.. "reset the tty to sane defaults" .. is it always expected
behavior? I can imagine environment where the tty setting is already
modified before getty is called.
For example login(1) saves and restores tty attributes when vhangup()
is called.
For serial lines is this gray zone :-) I'd like to be very
conservative... with vhangup() we are changing the current default
behavior. It would be really better to have --hangup (or
--control-tty) rather than --nohangup to be backwardly compatible.
> + * for sane settings. We also get a SIGHUP/SIGCONT.
> + */
> + if (vhangup())
> + error("/dev/%s: vhangup() failed: %m", tty);
> + (void)ioctl(fd, TIOCNOTTY);
> + }
> +
> + (void) close(fd);
> close(STDIN_FILENO);
> errno = 0;
>
> debug("open(2)\n");
> - if (open(tty, O_RDWR | O_NONBLOCK, 0) != 0)
> - error(_("/dev/%s: cannot open as standard input: %m"),
> - tty);
> + if (open(buf, O_RDWR|O_NOCTTY|O_NONBLOCK, 0) != 0)
> + error(_("/dev/%s: cannot open as standard input: %m"), tty);
> + if (((tid = tcgetsid(0)) < 0) || (pid != tid)) {
> + if (ioctl (0, TIOCSCTTY, 1) == -1)
> + error("/dev/%s: cannot get controlling tty: %m", tty);
> + }
> } else {
> +
> /*
> * Standard input should already be connected to an open port. Make
> * sure it is open for read/write.
> */
> +
> if ((fcntl(STDIN_FILENO, F_GETFL, 0) & O_RDWR) != O_RDWR)
> error(_("%s: not open for read/write"), tty);
> +
> }
>
> + if (tcsetpgrp(STDIN_FILENO, pid))
> + error("/dev/%s: cannot set process group: %m", tty);
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
