On Sun, Jul 03, 2022 at 09:05:28AM +0200, Alexander Hall wrote: > On Sat, Jul 02, 2022 at 08:12:29PM +0000, Klemens Nanni wrote: > > On Sat, Jul 02, 2022 at 03:00:00PM +0200, Alexander Hall wrote: > > > On Thu, Jun 30, 2022 at 03:35:05PM +0000, Klemens Nanni wrote: > > > > On Tue, Dec 07, 2021 at 08:15:41PM +0000, Klemens Nanni wrote: > > > > > On Tue, Nov 23, 2021 at 01:17:14AM +0000, Klemens Nanni wrote: > > > > > > On Tue, Nov 16, 2021 at 11:09:40PM +0000, Klemens Nanni wrote: > > > > > > > Run on boot without arguments, netstart(8) creates all virtual > > > > > > > interfaces *for which hostname.if files exist* before configuring > > > > > > > them. > > > > > > > > > > > > > > This prevents ordering problems with bridges and its members, as > > > > > > > dlg's > > > > > > > commit message from 2018 reminds us. > > > > > > > > > > > > > > But it also helps interface types like pair(4) which pair one > > > > > > > another > > > > > > > in whatever way the user says: > > > > > > > > > > > > > > $ cat /etc/hostname.pair1 > > > > > > > patch pair2 > > > > > > > $ cat /etc/hostname.pair2 > > > > > > > rdomain 1 > > > > > > > > > > > > > > On boot this works, but `sh /etc/netstart pair1 pair2' won't work > > > > > > > because pair2 does not exist a creation time of pair1 because > > > > > > > netstart > > > > > > > does not create virtual interfaces upfront. > > > > > > > > > > > > > > I just hit this exact use case when setting up gelatod(8) (see > > > > > > > ports@). > > > > > > > > > > > > > > To fix this, pass the list of interfaces to vifscreate() and make > > > > > > > it > > > > > > > create only those iff given. > > > > > > > > > > > > > > Regular boot, i.e. `sh /etc/netstart', stays uneffected by this > > > > > > > and > > > > > > > selective runs as shown work as expected without requring users > > > > > > > to know > > > > > > > the order in which netstart creates/configures interfaces. > > > > > > > > > > > > > > The installer's internal version of netstart doesn't need this at > > > > > > > all; > > > > > > > neither does it have the selective semantic nor does vifscreate() > > > > > > > exist. > > > > > > > > > > > > Anyone? > > > > > > > > > > > > It seems only logical to treat subsets of interfaces the same way as > > > > > > a full `sh /etc/netstart'. > > > > > > > > > > > > A pair of pair(4) is one example, I'm certain there are more > > > > > > scenarios > > > > > > where you craft interfaces with `ifconfig ...' in the shell, then > > > > > > set up > > > > > > the hostname.* files and test them with `sh /etc/netstart bridge0 > > > > > > ...' > > > > > > where pseudo interfaces are involved. > > > > > > > > > > Anyone? > > > > > > > > > > This is really practical and fixes things at least for me when I > > > > > destroy > > > > > interfaces, reconfigure and recreate them together, for example like > > > > > so: > > > > > > > > > > # ifconfig pair2 destroy > > > > > # ifconfig pair1 destroy > > > > > ... edit hostname.* > > > > > # sh /etc/netstart pair1 pair2 > > > > > ifconfig: patch pair2: No such file or directory > > > > > add net default: gateway 192.0.0.1 > > > > > > > > > > (redoing it because who knows what failed due to the order problem and > > > > > what didn't...) > > > > > > > > > > # ifconfig pair2 destroy > > > > > # ifconfig pair1 destroy > > > > > # sh /usr/src/etc/netstart pair1 pair2 > > > > > add net default: gateway 192.0.0.1 > > > > > > > > > > Feedback? Objection? OK? > > > > > > > > One last ping with the same diff on top of -CURRENT. > > > > > > > > > > > > Index: etc/netstart > > > > =================================================================== > > > > RCS file: /cvs/src/etc/netstart,v > > > > retrieving revision 1.218 > > > > diff -u -p -r1.218 netstart > > > > --- etc/netstart 26 Jun 2022 09:36:13 -0000 1.218 > > > > +++ etc/netstart 30 Jun 2022 14:48:46 -0000 > > > > @@ -94,9 +94,11 @@ ifcreate() { > > > > } > > > > > > > > # Create interfaces for network pseudo-devices referred to by > > > > hostname.if files. > > > > -# Usage: vifscreate > > > > +# Optionally, limit creation to given interfaces only. > > > > +# Usage: vifscreate [if ...] > > > > vifscreate() { > > > > - local _vif _hn _if > > > > + local _vif _hn _if _ifs > > > > + set -A _ifs -- "$@" > > > > > > > > for _vif in $(ifconfig -C); do > > > > for _hn in /etc/hostname.${_vif}+([[:digit:]]); do > > > > @@ -106,6 +108,9 @@ vifscreate() { > > > > # loopback for routing domain is created by > > > > kernel > > > > [[ -n ${_if##lo[1-9]*} ]] || continue > > > > > > > > + ((${#_ifs[*]} > 0)) && [[ ${_ifs[*]} != > > > > *${_if}* ]] && > > > > + continue > > > > > > My gut feeling says this is wrong. > > > I suspect `netstart vlan0` will create an0. > > > > Sorry, I don't follow; how would it chop leading chars? > > > > Maybe you meant that somehow `sh /etc/netstart an0' would attempt > > creating an0 since *an0* would match e.g. "lo0 em0 vlan0" or so? > > Yeah, along those lines, but my example was bogus as $_if would never > be "an0" here.
Correct. > However, `sh /etc/vetstart egre0 # (or even whatevergre0)` would create > gre0 if /etc/hostname.gre0 existed. > > Unless I'm mistaken again. Either way, it feels fragile. I think you're mistaken, but we're not going that direction anyway. > > If so, my diff would change nothing in this regard and netstart already > > behaves... off for bogus interfaces, with and without my diff: > > > > # echo up > /etc/hostname.vlan0 > > # echo up > /etc/hostname.an0 > > # sh /etc/netstart -n an0 ; echo $? > > WARNING: /etc/hostname.an0 is insecure, fixing permissions. > > { ifconfig an0 || ifconfig an0 create; } > > ifconfig an0 up > > 0 > > # sh /etc/netstart an0 ; echo $? > > 0 > > > > > You could probably do > > > > > > ((${#_ifs[*]} > 0)) && [[ " ${_ifs[*]} " != *" ${_if} "* ]] && > > > > > > but then it starts looking even worse. :-P > > > > Agreed. > > > > Here's a better diff at the end reusing isin() from install.sub. > > This way the purely theoretical false-positive globbing goes away and > > cryptic ksh code is traded for known, mnemonic ksh code. > > > > > > + > > > > if ! ifcreate $_if; then > > > > print -u2 "${0##*/}: create for '$_if' > > > > failed." > > > > fi > > > > @@ -314,6 +319,7 @@ $PRINT_ONLY || [[ ! -f /etc/soii.key ]] > > > > # If we were invoked with a list of interface names, just reconfigure > > > > these > > > > # interfaces (or bridges), add default routes and return. > > > > if (($# > 0)); then > > > > + vifscreate "$@" > > > > for _if; do ifstart $_if; done > > > > defaultroute > > > > return > > > > > > Would it be a problem just creating all pinpointed interfaces, be they > > > virtual or not, upfront? > > > > That'll fix my use case but I'd need more time to reason about this > > approach. > > > > ifstart()'s creation comes to mind: > > > > 139 Check for ifconfig'able interface, except if -n option is > > specified. > > 140 ifcreate $_if || return > > > > Will that trip or can it go? > > Given its > > { ifconfig $_if || ifconfig $_if create; } >/dev/null 2>&1 > > , I'd say it's safe. > > > With your diff it'd be purely redundant. > > It would only be redundant for the case when you pass specific interface > names to netstart. > > However, my loop would attempt to create any specified interface, regardless > of whether a matching hostname.if file existed... Oh and it would attempt to create physical interfaces multiple times, which is an obvious bug or rather redundancy. -CURRENT netstart does that as well, simply as part of ifstart(): # sh /etc/netstart -n em0 lo0 { ifconfig em0 || ifconfig em0 create; } ifconfig em0 up netstart: /etc/hostname.lo0: No such file or directory. But your diff does it more than once for physical interfaces and also does it for loopback devices which are skipped properly in vifscreate(): # sh /etc/netstart -n em0 lo0 { ifconfig em0 || ifconfig em0 create; } { ifconfig lo0 || ifconfig lo0 create; } { ifconfig em0 || ifconfig em0 create; } ifconfig em0 up netstart: /etc/hostname.lo0: No such file or directory. > > > diff --git a/etc/netstart b/etc/netstart > > > index 33e9689a819..62ca64803d8 100644 > > > --- a/etc/netstart > > > +++ b/etc/netstart > > > @@ -314,6 +314,7 @@ $PRINT_ONLY || [[ ! -f /etc/soii.key ]] || > > > # If we were invoked with a list of interface names, just reconfigure > > > these > > > # interfaces (or bridges), add default routes and return. > > > if (($# > 0)); then > > > + for _if; do ifcreate $_if; done > > > for _if; do ifstart $_if; done > > > defaultroute > > > return So I'd disregard your approach. > > > > > > Index: netstart > > =================================================================== > > RCS file: /cvs/src/etc/netstart,v > > retrieving revision 1.218 > > diff -u -p -r1.218 netstart > > --- netstart 26 Jun 2022 09:36:13 -0000 1.218 > > +++ netstart 2 Jul 2022 20:01:16 -0000 > > @@ -11,6 +11,17 @@ usage() { > > exit 1 > > } > > > > +# Test the first argument against the remaining ones, return success on a > > match. > > +isin() { > > + local _a=$1 _b > > + > > + shift > > + for _b; do > > + [[ $_a == "$_b" ]] && return 0 > > + done > > + return 1 > > +} > > + > > # Echo file $1 to stdout. Skip comment lines. Strip leading and trailing > > # whitespace if IFS is set. > > # Usage: stripcom /path/to/file > > @@ -94,9 +105,11 @@ ifcreate() { > > } > > > > # Create interfaces for network pseudo-devices referred to by hostname.if > > files. > > -# Usage: vifscreate > > +# Optionally, limit creation to given interfaces only. > > +# Usage: vifscreate [if ...] > > vifscreate() { > > - local _vif _hn _if > > + local _vif _hn _if _ifs > > + set -A _ifs -- "$@" > > > > for _vif in $(ifconfig -C); do > > for _hn in /etc/hostname.${_vif}+([[:digit:]]); do > > @@ -106,6 +119,10 @@ vifscreate() { > > # loopback for routing domain is created by kernel > > [[ -n ${_if##lo[1-9]*} ]] || continue > > > > + if ((${#_ifs[*]} > 0)) && ! isin $_if "${_ifs[@]}"; then > > + continue > > + fi > > + > > Ok, I concur, and isin() is way cleaner. I do think the _ifs dance looks > messy though, and would suggest just using $@ as-is; Fine with me and actually better, thanks. _ifs seems like idiomatic way in our shell code, i.e. assign function arguments to local variables, but $@ is a bit special. In fact, "$@" is `set -u' clean whereas "${_ifs[@]}" is not. netstart does not use `set -u', but it's still an argument for the $@: $ set -u $ set -A _ifs -- "$@" $ echo "$@" $ echo "${_ifs[@]}" ksh: _ifs[@]: parameter not set This works like a charm for me and behaviour only differs if I actually pass interfaces: # sh /etc/netstart -n > old # sh /usr/src/etc/netstart -n > new # diff -u old new ; echo $? 0 # sh /etc/netstart -n pair1 pair2 > old-ifs # sh /usr/src/etc/netstart -n pair1 pair2 > new-ifs # diff -u old-ifs new-ifs --- old-ifs Sun Jul 3 13:54:51 2022 +++ new-ifs Sun Jul 3 13:54:45 2022 @@ -1,4 +1,6 @@ { ifconfig pair1 || ifconfig pair1 create; } +{ ifconfig pair2 || ifconfig pair2 create; } +{ ifconfig pair1 || ifconfig pair1 create; } ifconfig pair1 inet 192.0.0.4/29 ifconfig pair1 patch pair2 { ifconfig pair2 || ifconfig pair2 create; } Feedback? Objection? OK? Index: netstart =================================================================== RCS file: /cvs/src/etc/netstart,v retrieving revision 1.218 diff -u -p -r1.218 netstart --- netstart 26 Jun 2022 09:36:13 -0000 1.218 +++ netstart 3 Jul 2022 09:46:05 -0000 @@ -11,6 +11,17 @@ usage() { exit 1 } +# Test the first argument against the remaining ones, return success on a match. +isin() { + local _a=$1 _b + + shift + for _b; do + [[ $_a == "$_b" ]] && return 0 + done + return 1 +} + # Echo file $1 to stdout. Skip comment lines. Strip leading and trailing # whitespace if IFS is set. # Usage: stripcom /path/to/file @@ -94,7 +105,8 @@ ifcreate() { } # Create interfaces for network pseudo-devices referred to by hostname.if files. -# Usage: vifscreate +# Optionally, limit creation to given interfaces only. +# Usage: vifscreate [if ...] vifscreate() { local _vif _hn _if @@ -106,6 +118,10 @@ vifscreate() { # loopback for routing domain is created by kernel [[ -n ${_if##lo[1-9]*} ]] || continue + if (($# > 0)) && ! isin $_if "$@"; then + continue + fi + if ! ifcreate $_if; then print -u2 "${0##*/}: create for '$_if' failed." fi @@ -313,7 +329,11 @@ $PRINT_ONLY || [[ ! -f /etc/soii.key ]] # If we were invoked with a list of interface names, just reconfigure these # interfaces (or bridges), add default routes and return. +# Create virtual interfaces upfront to make ifconfig commands depending on +# other interfaces, e.g. "patch", work regardless of in which order interface +# names were specified. if (($# > 0)); then + vifscreate "$@" for _if; do ifstart $_if; done defaultroute return