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
