Re: [CFT] bsdinstall and zfsboot enhancements

On Sun, Jan 05, 2014 at 04:04:03PM -0500, Nathan Whitehorn wrote:
> On 12/01/13 07:34, Jilles Tjoelker wrote:
> > On Sat, Nov 30, 2013 at 04:36:18PM -0600, Nathan Whitehorn wrote:
> >> This took much longer than I'd anticipated, but the patch to init is
> >> attached. I chose not to make the changes to init rather than
> >> getttyent() and friends in libc, which I am open to revisiting.
> > lib/libpam/modules/pam_securetty/pam_securetty.c calls getttynam(3) and
> > will not allow root login on a "fake" TTY that getttynam() does not
> > know. This module is enabled by default for the "login" service.

> > So it is probably better to patch libc rather than init.

> OK, here's a revised patch. This one is shorter and works by introducing
> an "auto" flag (ideas for names appreciated) that means "on" if the line
> is an active console and "off" otherwise. Note that the behavior is now:
> - ttys marked "off" stay off
> - ttys marked "on" stay on
> - ttys marked "auto" are enabled iff they are console devices
> - ttys not present in /etc/ttys stay off

> This behavior change is much easier to implement when doing it in libc
> for various structural reasons and allows the terminal type, etc. to be
> specified in the usual way.

Instead of "auto" you could use "ifconsole".

This looks sensible to me.

> > As a preparatory patch, you could remove se_index and session_index from
> > init. They are only used to warn about a changed slot number in utmp(5)
> > which is irrelevant with utmpx. This noise warning would also appear
> > in most cases when changing from a "fake" console entry to a real line
> > in /etc/ttys. Also, if you do decide to fake ttys entries in init rather
> > than libc, the patch to init will be simpler.

> With the new patch, this is indeed the case: no changes to init are
> necessary at all. This does not change any behavior unless explicitly
> requested in /etc/ttys, so unless there are any objections in the next
> couple days, I will commit it.

I have some small remarks inline below.

I would like to remove se_index and session_index from init soon if it
does not conflict with other work.

> Index: include/ttyent.h
> ===================================================================
> --- include/ttyent.h	(revision 260331)
> +++ include/ttyent.h	(working copy)
> _at__at_ -37,6 +37,7 _at__at_
>  
>  #define	_TTYS_OFF	"off"
>  #define	_TTYS_ON	"on"
> +#define	_TTYS_AUTO	"auto"
>  #define	_TTYS_SECURE	"secure"
>  #define	_TTYS_INSECURE	"insecure"
>  #define	_TTYS_WINDOW	"window"
> Index: lib/libc/gen/getttyent.c
> ===================================================================
> --- lib/libc/gen/getttyent.c	(revision 260331)
> +++ lib/libc/gen/getttyent.c	(working copy)
> _at__at_ -39,6 +39,9 _at__at_
>  #include <ctype.h>
>  #include <string.h>
>  
> +#include <sys/types.h>
> +#include <sys/sysctl.h>
> +
>  static char zapchar;
>  static FILE *tf;
>  static size_t lbsize;
> _at__at_ -64,6 +67,32 _at__at_
>  	return (t);
>  }
>  
> +static int
> +auto_tty_status(const char *ty_name)
> +{
> +	size_t len;
> +	char *buf, *cons, *nextcons;
> +
> +	/* Check if this is an enabled kernel console line */
> +	buf = NULL;
> +	if (sysctlbyname("kern.console", NULL, &len, NULL, 0) == -1)
> +		return (0); /* Errors mean don't enable */
> +	buf = malloc(len);
> +	if (sysctlbyname("kern.console", buf, &len, NULL, 0) == -1)
> +		return (0);

conscontrol also does this, but is it possible that the length increases
between the checks?

> +
> +	if ((cons = strchr(buf, '/')) == NULL)
> +		return (0);
> +	*cons = '\0';
> +	nextcons = buf;
> +	while ((cons = strsep(&nextcons, ",")) != NULL && strlen(cons) != 0) {
> +		if (strcmp(cons, ty_name) == 0)
> +			return (TTY_ON);
> +	}
> +
> +	return (0);
> +}
> +
>  struct ttyent *
>  getttyent(void)
>  {
> _at__at_ -126,6 +155,8 _at__at_
>  			tty.ty_status &= ~TTY_ON;
>  		else if (scmp(_TTYS_ON))
>  			tty.ty_status |= TTY_ON;
> +		else if (scmp(_TTYS_AUTO))
> +			tty.ty_status |= auto_tty_status(tty.ty_name);
>  		else if (scmp(_TTYS_SECURE))
>  			tty.ty_status |= TTY_SECURE;
>  		else if (scmp(_TTYS_INSECURE))
> Index: libexec/getty/ttys.5
> ===================================================================
> --- libexec/getty/ttys.5	(revision 260331)
> +++ libexec/getty/ttys.5	(working copy)
> _at__at_ -102,8 +102,11 _at__at_
>  .Pp
>  As flag values, the strings ``on'' and ``off'' specify that
>  .Xr init 8
> -should (should not) execute the command given in the second field,
> -while ``secure'' (if ``on'' is also specified) allows users with a
> +should (should not) execute the command given in the second field.
> +``auto'' will cause this line to be enabled if and only if it is
> +an active kernel console device (it is equivalent to ``on'' in this
> +case).
> +The flag ``secure'' (if ``on'' is also specified) allows users with a

Please change "if ``on'' is also specified" to something like "if the
line is enabled". A line may also be enabled if ``auto'' is specified.

>  uid of 0 to login on
>  this line.
>  The flag ``dialin'' indicates that a tty entry describes a dialin

This (mostly obsolete) flag seems to have no effect now, because almost
12 years ago the condition for the DIALUP log message was accidentally
changed to require a remote hostname (-h) as well as a dialup tty (it
was used to require a dialup tty and no remote hostname).

-- 
Jilles Tjoelker