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?
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.
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?
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
