On Thu, Dec 15, 2022 at 07:06:34PM -0800, Andrew Hewus Fresh wrote:
> On Thu, Dec 15, 2022 at 06:31:40AM +0000, Klemens Nanni wrote:
> > 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.
This reads fine, _if is the name/unit style string and _lladdr the MAC.
>
>
> > > _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?
Oh of course, in that case the quotes don't matter.
They aren't needed but also don't here, your call.
>
> $ 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.
Fine with me.
>
>
> >
> > > + 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.
Staying in sync with the installer makes sense, I used awk because it
reads much clearer (simpler to hack down in a show-case diffe like this).
>
> 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.
OK kn functionality wise, but I'd appreaciate sticking to our usual ksh
style, see inline.
No need to resend the diff with those nits fixed.
>
>
> 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
Please use == for comparison.
> + _lladdr=$(ifconfig $_if 2>/dev/null |
> + sed -n 's/^[[:space:]]*lladdr[[:space:]]//p')
> + if [[ -n $_lladdr \
> + && -f /etc/hostname.$_lladdr \
> + && -n $(ifconfig -M "$_lladdr") \
Yes, -n/-f now align nicely, but line breaks look ugly and we have the
logic operator at the end everywhere (C and sh code), so I really prefer
if [[ -n $_lladdr && -n f /etc/hostname.$_lladdr &&
-n $(ifconfig -M $_lladdr) ]]; then
or
if [[ -n $_lladdr &&
-n f /etc/hostname.$_lladdr &&
-n $(ifconfig -M $_lladdr) ]]; then
> + ]]; 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
Comments usually go in imperative tense. You could also say what
ifconfig -M does so this becomes clearer and simpler, e.g.
# Map an interface to its MAC address if it is unique.
> +if_name_to_lladdr() {
> + local _lladdr
Please add an empty line here.
> + _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
Please use == for comparsion.
> + _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
>
