On Thu, Dec 15, 2022 at 06:31:40AM +0000, Klemens Nanni wrote:
> On Sun, Dec 11, 2022 at 01:05:13PM -0800, Andrew Hewus Fresh wrote:
> > This would be the diff to swap priority of name/lladdr as some folks
> > wanted to see. I still don't recommend having both as you can still
> > have surprising outcomes with more complex configurations.
> >
> > The install.sub includes an "if_name_to_lladdr" function from the
> > diff to support creating a hostname.lladdr during install. If we don't
> > decide to support that, I'd probably in-line that as is done in
> > netstart.
> >
> > I did find that my iwm doesn't like being configured by lladdr at this
> > point of the upgrade because it doesn't have firmware loaded when it's
> > looking to see if it's a valid hostname.lladdr, but the iwm's lladdr is
> > all zeros still.
> >
> >
> > Index: etc/netstart
> > ===================================================================
> > RCS file: /cvs/src/etc/netstart,v
> > retrieving revision 1.230
> > diff -u -p -r1.230 netstart
> > --- etc/netstart 5 Dec 2022 20:12:00 -0000 1.230
> > +++ etc/netstart 11 Dec 2022 20:44:34 -0000
>
> More context for this diff:
> 138 if [[ $_if == ??:??:??:??:??:?? ]]; then
> 139 if ! _line=$( ifconfig -M $_if ); then
> 140 print -u2 "${0##*/}: $_if is not unique."
> 141 return
> 142 fi
>
> > @@ -144,11 +144,14 @@ ifstart() {
> > [[ -z $_line ]] && return
>
> _line is the interface name "em0", please call it _ifname or _if_name.
> Just anything obviously descriptive.
Will do. I ended up just adding _lladdr and moving things around
appropriately. I think it worked out very well.
> > _if=$_line
> > _line=
>
> So the MAC in _if is overwritten with the interface name in _line which
> is being zeroed, i.e. we looked up the interface via lladdr and proceed
> as if the user had placed an .em0 file instead.
>
> With s/_inline/_ifname/ that would be much easier to follow.
>
> > -
> > - if [[ -e /etc/hostname.$_if ]]; then
> > - print -u2 "${0##*/}: $_hn: /etc/hostname.$_if overrides"
> > + else
> > + _line=$(ifconfig $_if | sed -n
> > 's/^[[:space:]]*lladdr[[:space:]]//p')
>
> This will print "_if: no such interface" for nonexistent interfaces and
> must be silenced.
Ahh, yes. fixed.
> > + if [[ -n $_line && -n $(ifconfig -M $_line) \
>
> Pretty sure the -M call needs "$_line" since you can hit existent
> interfaces without lladdr, so _line is empty and `ifconfig -M' would
> print usage while `ifconfig -M ""' is silent as expected.
Wouldn't $_line be blank then and this would short-circuit at "-n $line"
instead of trying to run ifconfig -M with no arguments?
$ x=; [[ -n $x && -n $( print -u2 $x!; ifconfig -M $x ) ]] && echo oops
$
I will wrap it in quotes to be less surprising.
>
> > + && -e /etc/hostname.$_line ]]; then
>
> The same sanity check further below uses -f. Not sure if existence is
> enough here or whether we should only switch files if there is an actual
> file...
We should be consistent in ignoring things that aren't files.
>
> > + print -u2 "${0##*/}: $_hn: /etc/hostname.$_line:
> > overrides"
> > return
> > fi
> > + _line=
> > fi
>
> Otherwise the else block logic makes sense, but now you're using _if as
> an interface name before the input validation, which follows immediately
> afterwards.
>
> Just merge the two like this:
> if [[ $_if == ??:??:??:??:??:?? ]]; then
> ...
> else if [[ $_if == +([[:alpha:]])+([[:digit:]]) ]]; then
> ...
> else
> return
>
> Diff below to make things clear.
> Maybe it'd be better to clean up things a bit and then do the prio swap
> separately?
I tried just adding a variable and doing a bit of tidying first, but it
didn't seem worthwhile.
After seeing your suggestion to combine the logic into an elif, a
lightbulb went on and I reversed the order of the checks so the more
common (name/unit) would be handled first. Overall I think it feels
much nicer. Thanks!
I used many of your suggestions, but I kept sed instead of switching to
awk to keep that the same with install.sub. It is still in a helper
function in install.sub, since I believe we are going to reuse that
logic later when we allow that answer to the question in the installer.
I also added the alpha+digit check in install.sub for consistency.
> If that looks right, we can adapt the insaller bits accordingly.
> I did not go over them now.
Here's the full diff, including the man page rebased onto jmc@'s rework.
Index: etc/netstart
===================================================================
RCS file: /cvs/src/etc/netstart,v
retrieving revision 1.230
diff -u -p -r1.230 netstart
--- etc/netstart 5 Dec 2022 20:12:00 -0000 1.230
+++ etc/netstart 16 Dec 2022 02:54:28 -0000
@@ -132,28 +132,35 @@ vifscreate() {
# Start a single interface.
# Usage: ifstart if1
ifstart() {
- local _if=$1 _hn=/etc/hostname.$1 _cmds _i=0 _line _stat
+ local _if=$1 _lladdr _hn=/etc/hostname.$1 _cmds _i=0 _line _stat
set -A _cmds
- if [[ $_if == ??:??:??:??:??:?? ]]; then
- if ! _line=$( ifconfig -M $_if ); then
- print -u2 "${0##*/}: $_if is not unique."
+ # Interface names must be alphanumeric only. We check to avoid
+ # configuring backup or temp files, and to catch the "*" case.
+ if [[ $_if = +([[:alpha:]])+([[:digit:]]) ]]; then
+ _lladdr=$(ifconfig $_if 2>/dev/null |
+ sed -n 's/^[[:space:]]*lladdr[[:space:]]//p')
+ if [[ -n $_lladdr \
+ && -f /etc/hostname.$_lladdr \
+ && -n $(ifconfig -M "$_lladdr") \
+ ]]; then
+ print -u2 "${0##*/}: $_hn: /etc/hostname.$_lladdr
overrides"
return
fi
- [[ -z $_line ]] && return
- _if=$_line
- _line=
-
- if [[ -e /etc/hostname.$_if ]]; then
- print -u2 "${0##*/}: $_hn: /etc/hostname.$_if overrides"
+ # We also support hostname.lladdr, but it must resolve to be valid
+ elif [[ $_if == ??:??:??:??:??:?? ]]; then
+ _lladdr=$_if
+ _if=$(ifconfig -M $_lladdr)
+ if (($? != 0)); then
+ print -u2 "${0##*/}: $_lladdr is not unique."
return
fi
- fi
- # Interface names must be alphanumeric only. We check to avoid
- # configuring backup or temp files, and to catch the "*" case.
- [[ $_if != +([[:alpha:]])+([[:digit:]]) ]] && return
+ [[ -z $_if ]] && return
+ else
+ return
+ fi
if [[ ! -f $_hn ]]; then
print -u2 "${0##*/}: $_hn: No such file or directory."
Index: distrib/miniroot/install.sub
===================================================================
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1215
diff -u -p -r1.1215 install.sub
--- distrib/miniroot/install.sub 5 Dec 2022 20:12:00 -0000 1.1215
+++ distrib/miniroot/install.sub 16 Dec 2022 02:54:28 -0000
@@ -356,6 +356,15 @@ get_ifs() {
done
}
+# Maps an interface name to lladdr,
+# filtered by whether it's valid for use by ifconfig -M
+if_name_to_lladdr() {
+ local _lladdr
+ _lladdr=$(ifconfig $1 2>/dev/null |
+ sed -n 's/^[[:space:]]*lladdr[[:space:]]//p')
+ [[ -n $_lladdr && -n $(ifconfig -M "$_lladdr") ]] && echo $_lladdr
+}
+
# Return the device name of the disk device $1, which may be a disklabel UID.
get_dkdev_name() {
local _dev=${1#/dev/} _d
@@ -2428,13 +2437,18 @@ parse_hn_line() {
# Start interface using the on-disk hostname.if file passed as argument $1.
# Much of this is gratuitously stolen from /etc/netstart.
ifstart() {
- local _if=$1 _hn=/mnt/etc/hostname.$1 _cmds _i=0 _line
+ local _if=$1 _lladdr _hn=/mnt/etc/hostname.$1 _cmds _i=0 _line
set -A _cmds
- if [[ $_if == ??:??:??:??:??:?? ]]; then
- _if=$(ifconfig -M $_if)
- [[ -z $_if ]] && return # invalid interface
- [[ -e /mnt/etc/hostname.$_if ]] && return # duplicate config
+ if [[ $_if = +([[:alpha:]])+([[:digit:]]) ]]; then
+ _lladdr=$(if_name_to_lladdr $_if)
+ [[ -n $_lladdr && -f /mnt/etc/hostname.$_lladdr ]] && return
+ elif [[ $_if == ??:??:??:??:??:?? ]]; then
+ _lladdr=$_if
+ _if=$(ifconfig -M $_lladdr)
+ [[ -z $_if ]] && return
+ else
+ return
fi
# Create interface if it does not yet exist.
Index: share/man/man5/hostname.if.5
===================================================================
RCS file: /cvs/src/share/man/man5/hostname.if.5,v
retrieving revision 1.81
diff -u -p -r1.81 hostname.if.5
--- share/man/man5/hostname.if.5 15 Dec 2022 06:39:05 -0000 1.81
+++ share/man/man5/hostname.if.5 16 Dec 2022 02:54:28 -0000
@@ -49,7 +49,7 @@ so interfaces can alternatively be refer
their link layer address (lladdr),
such as
.Dq hostname.00:00:5e:00:53:af .
-Priority is given to configuration by interface name/unit over lladdr.
+Priority is given to configuration by interface lladdr over name/unit.
A configuration file is not needed for lo0.
.Pp
The configuration information is expressed in a line-by-line packed format
