On Wed, Nov 24, 2021 at 02:30:08AM +0100, Klemens Nanni wrote:
> On Tue, Nov 16, 2021 at 09:22:26AM +1000, David Gwynne wrote:
> > On Mon, Nov 15, 2021 at 02:31:42PM +0000, Klemens Nanni wrote:
> > > On Mon, Nov 15, 2021 at 01:37:49PM +0000, Stuart Henderson wrote:
> > > > On 2021/11/15 12:27, Klemens Nanni wrote:
> > > > > On Sun, Nov 14, 2021 at 07:04:42PM -0700, Theo de Raadt wrote:
> > > > > > I think physical interfaces should come up when something is
> > > > > > configured
> > > > > > on them, but virtual interfaces shouldn't -- mostly because the
> > > > > > order of
> > > > > > configuration is often muddled.
> > > > >
> > > > > So "inet6 2001:db8::1" in hostname.em0 will do the trick but
> > > > > hostname.vport0 would need another "up" for the same behaviour:
> > > > > that's
> > > > > rather confusing me as a user.
> > > >
> > > > hostname.* files are orthogonal to this; netstart can process all the
> > > > lines,
> > > > then if it has seen a line doing address configuration and has not seen
> > > > an
> > > > explicit "down", it can bring the interface up automatically at the end.
> > > > (if this changed, it would be a nightmare for users to do anything
> > > > else).
> > >
> > > Yes, netstart can and should deal with this correctly, just like you
> > > describe.
> > >
> > > > Users would need to make sure they have a netstart which does that if
> > > > updating a kernel, but that's just a case of matching kernel+userland
> > > > and is
> > > > nothing new for OpenBSD.
> > > >
> > > > The different behaviour would be apparent with separate runs of
> > > > ifconfig.
> > > > some scripts may need adapting and users might need to run "ifconfig XX
> > > > up"
> > > > themselves but I don't think that would be a problem.
> > >
> > > Agreed.
> > >
> > > Having the implicit-up logic entirely contained in netstart would make
> > > lifer much easier, both for network stack hackers and users, imho.
> >
> > this was my attempt at just that.
>
> Given netstart(8) always pulls interfaces UP or DOWN as the last step,
> surely this wouldn't be all, right?
Put differently: Such netstart change really just tries to abstract a
consistent behaviour across multiple interfaces doing it differently.
> Interfaces/drivers still pull themselves up, thus create route message
> churn, transition from initial DOWN to implicit UP due to address
> configuration to possibly administrative DOWN again due to "down" in
> hostname.*, etc.
So once/iff this change lands, I think all interfaces should do (or not
do) what netstart compensates for.
Once all interfaces avoid implicit state changes and only transition
upon administrative command (and possibly `autoconf'), the netstart
workaround --that's what it really is-- ought to be removed.
> If we decide to handle this in netstart alone, shouldn't all interfaces
> behave like vport(4) and not mess with their state unless explicitly
> requested to do so?
Then, finally, interfaces only go UP if users do `ifconfig ... up'
or hostname.* contain the word "up". Otherwise they stay DOWN.
This would be a dead simple thing to reason.
The netstart workaround itself already changes behaviour and tells users
to add a very specific line to their configuration -- a format which
really shouldn't have such specific requirements but instead should work
like regular `ifconfig' lines on the shell, btw.
So If that already needs attention, the final solution of no interface
doing implicit changes and netstart not doing any implicit stuff would
also eventually result in the simplest and most intuitive form of
configuration: "up" anywhere pulls UP, "down" anywhere pulls DOWN,
neither anywhere leaves interfaces at their creation default of DOWN.
> Not sure if the (now finally documented) implicit `up' in `autoconf'
> should stay after the netstart change, but that's another topic
> (pulling interfaces UP two times won't hurt, I guess).
>
> > the installer has its own netstart though, right?
>
> Yes, diff for that at the end after tweaking yours and adapting it.
>
> > Index: etc/netstart
> > ===================================================================
> > RCS file: /cvs/src/etc/netstart,v
> > retrieving revision 1.216
> > diff -u -p -r1.216 netstart
> > --- etc/netstart 2 Sep 2021 19:38:20 -0000 1.216
> > +++ etc/netstart 15 Nov 2021 23:20:00 -0000
> > @@ -71,6 +71,9 @@ parse_hn_line() {
> > dhcp) _cmds[${#_cmds[*]}]="ifconfig $_if inet autoconf"
> > V4_AUTOCONF=true
> > ;;
> > + down) _c[0]=
>
> This reset seems unneeded, `_c' isn't used afterwards and I don't
> undertand why you do it.
>
> > + _ifup=down
>
> So `_ifup' comes from ifstart() and not parse_hn_line().
>
> We use the "_" prefix to denote local variables...
>
> > + ;;
> > '!'*) _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
> > _cmds[${#_cmds[*]}]="${_cmd#!}"
> > ;;
> > @@ -118,6 +121,7 @@ vifscreate() {
> > ifstart() {
> > local _if=$1 _hn=/etc/hostname.$1 _cmds _i=0 _line _stat
> > set -A _cmds
> > + _ifup=up
>
> except you define a global variable inside a function.
>
> This should be local to ifstart() (deserving its prefix), which makes it
> reach into the scope of parse_hn_line() (as is usual semantic with
> `local' variables in at least ksh and bash).
>
> I've added comments to reflect on this special use, as I'm unaware of
> any other piece of shell code were we actively use function-local
> variables up the call stack.
>
> >
> > # Interface names must be alphanumeric only. We check to avoid
> > # configuring backup or temp files, and to catch the "*" case.
> > @@ -145,6 +149,8 @@ ifstart() {
> > while IFS= read -- _line; do
> > parse_hn_line $_line
> > done <$_hn
> > +
> > + _cmds[${#_cmds[*]}]="ifconfig $_if $_ifup"
>
> Lastly, I'd say `_ifstate' because that equally means "up" and "down".
>
> >
> > # Apply the interface configuration commands stored in _cmds array.
> > while ((_i < ${#_cmds[*]})); do
> > Index: share/man/man5/hostname.if.5
> > ===================================================================
> > RCS file: /cvs/src/share/man/man5/hostname.if.5,v
> > retrieving revision 1.77
> > diff -u -p -r1.77 hostname.if.5
> > --- share/man/man5/hostname.if.5 17 Jul 2021 15:28:31 -0000 1.77
> > +++ share/man/man5/hostname.if.5 15 Nov 2021 23:20:01 -0000
> > @@ -57,6 +57,9 @@ the administrator should not expect magi
> > and the
> > per-driver manual pages to see what arguments are permitted.
> > .Pp
> > +Interfaces are implicitly configured to be brought up and running.
> > +This behaviour can be disabled by adding a line containing down to the
> > file.
>
> Probably best to use `.Dq down' and make clear that it must be on its
> own line without anything else, otherwise users might end up with
> "down ..." or "... down" but netstart would still pull it UP in the end.
>
> > +.Pp
> > Arguments containing either whitespace or single quote
> > characters must be double quoted.
> > For example:
> >
>
> I'm leaving out the manual bits until the discussion settles.
>
>
> Index: distrib/miniroot/install.sub
> ===================================================================
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1184
> diff -u -p -r1.1184 install.sub
> --- distrib/miniroot/install.sub 2 Nov 2021 16:54:01 -0000 1.1184
> +++ distrib/miniroot/install.sub 24 Nov 2021 01:29:31 -0000
> @@ -2414,6 +2414,9 @@ parse_hn_line() {
> _cmds[${#_cmds[*]}]="ifconfig $_if inet autoconf"
> V4_AUTOCONF=true
> ;;
> + down) # used in ifstart()
> + down
> + ;;
> '!'*|bridge)
> # Skip shell commands and bridge in the installer.
> return
> @@ -2430,6 +2433,8 @@ parse_hn_line() {
> ifstart() {
> local _if=$1 _hn=/mnt/etc/hostname.$1 _cmds _i=0 _line
> set -A _cmds
> + # possibly set in parse_hn_line()
> + local _ifstate=up
>
> # Create interface if it does not yet exist.
> { ifconfig $_if || ifconfig $_if create; } >/dev/null 2>&1 || return
> @@ -2442,6 +2447,10 @@ ifstart() {
> while IFS= read -- _line; do
> parse_hn_line $_line
> done <$_hn
> +
> + # Default to implicit UP for all interfaces unless there was an explicit
> + # "down" line.
> + _cmds[${#_cmds[*]}]="ifconfig $_if $_ifstate"
>
> # Apply the interface configuration commands stored in _cmds array.
> while ((_i < ${#_cmds[*]})); do
> Index: etc/netstart
> ===================================================================
> RCS file: /cvs/src/etc/netstart,v
> retrieving revision 1.216
> diff -u -p -r1.216 netstart
> --- etc/netstart 2 Sep 2021 19:38:20 -0000 1.216
> +++ etc/netstart 24 Nov 2021 01:29:32 -0000
> @@ -71,6 +71,9 @@ parse_hn_line() {
> dhcp) _cmds[${#_cmds[*]}]="ifconfig $_if inet autoconf"
> V4_AUTOCONF=true
> ;;
> + down) # used in ifstart()
> + down
> + ;;
> '!'*) _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
> _cmds[${#_cmds[*]}]="${_cmd#!}"
> ;;
> @@ -118,6 +121,8 @@ vifscreate() {
> ifstart() {
> local _if=$1 _hn=/etc/hostname.$1 _cmds _i=0 _line _stat
> set -A _cmds
> + # possibly set in parse_hn_line()
> + local _ifstate=up
>
> # Interface names must be alphanumeric only. We check to avoid
> # configuring backup or temp files, and to catch the "*" case.
> @@ -145,6 +150,10 @@ ifstart() {
> while IFS= read -- _line; do
> parse_hn_line $_line
> done <$_hn
> +
> + # Default to implicit UP for all interfaces unless there was an explicit
> + # "down" line.
> + _cmds[${#_cmds[*]}]="ifconfig $_if $_ifstate"
>
> # Apply the interface configuration commands stored in _cmds array.
> while ((_i < ${#_cmds[*]})); do
