On Sat, Nov 26, 2022 at 03:23:03PM -0800, Andrew Hewus Fresh wrote:
> On Sat, Nov 26, 2022 at 07:12:12PM +0000, Klemens Nanni wrote:
> > On Sat, Nov 26, 2022 at 10:44:05AM -0800, Andrew Hewus Fresh wrote:
> > > I think this is the complete diff, with the man page bits mostly from
> > > Martijn. Now that the ifconfig -M bits are in, this should be easier
> > > to test. I was able to test the installer bits, and they worked for me.
> >
> > This reads straight forward, but see inline.
> >
> <SNIP>
> > > + # If the _if looks like an lladdr and look it up.
> >
> > That sentence is not proper english.
> > "Look up the interface if _if seems like an lladdr."
> > or
> > "Obtain the interface if _if is a MAC"
> > ?
>
> I realized the comment wasn't particularly useful, so I deleted it.
>
>
> > > + if [[ $_if = ??:??:??:??:??:?? ]]; then
> >
> > Please use == for comparison; I know = works but it reads off.
> > Base should use == consistently, imho.
>
> Somehow I thought the right side being a glob match only worked with "="
> not "==", but turns out I was wrong. Fixed.
>
> >
> > > + if ! _line="$( ifconfig -M "$_if" )"; then
> >
> > Here you quote the subshell and the variable...
>
> That is a heck of a habit I have. Apparently I can't avoid typing
> those. Fixed.
>
> >
> > > + print -u2 "${0##*/}: unique if for lladdr $_if not
> > > found."
> >
> > Maybe just "$_if is not unique"?
>
> I like it! Thanks!
>
> <SNIP>
> > >
> > > + if [[ $_if = ??:??:??:??:??:?? ]]; then
> > > + _if=$(ifconfig -M $_if)
> >
> > ... here you quote neither subshell nor variable.
>
> I was trying to match the style like this, but not quoting things is
> hard for me to do.
>
> > I think that's fine, both install.sub and netstart already use unquoted
> > $_if and the subshell does not have to be quoted, anyway, since word
> > spliting does not occur:
> >
> > $ v=$(echo 1 2 3)
> > $ echo $v
> > 1 2 3
>
> It does split though, echo just re-assembles it back the way it started:
>
> $ v=$(printf "%s\n" 1 2 3 )
> $ # without the quotes , it gets tokenized again and the newlines disappear
> $ echo "$v"
> 1
> 2
> 3
Yes sorry, my example was flawed. You still don't have to quote the
subshell during the variable assignment, though, that's what I was
trying to clarify.
>
>
> <SNIP>
> > > + elif [[ $_if = ??:??:??:??:??:?? ]]; then
> >
> > ==, please.
>
> Can do. Too used to writing portable things for /bin/sh I guess.
>
> <SNIP>
> > > +Additionally the interface name can be replaced with the interface's
> > > lladdr,
> > > +given that the lladdr is unique across the system and no
> > > +.Nm
> > > +exists for the interface name.
> >
> > Isn't that too vague?
> >
> > .Nm is hostname.if but you meant to say that .fxp0 should not exist when
> > .MAC
> > already exists, no?
>
> I'm not sure, it makes sense to me. Hopefully jmc provides an excellent
> option, but I'll think on wording.
>
> Here's another revision, not really any changes to the man page, but
> your suggestions got applied, I think.
Looking great so far, thanks.
One nit inline, no need to resend the diff.
>
>
> Index: etc/netstart
> ===================================================================
> RCS file: /cvs/src/etc/netstart,v
> retrieving revision 1.229
> diff -u -p -r1.229 netstart
> --- etc/netstart 5 Nov 2022 12:06:05 -0000 1.229
> +++ etc/netstart 26 Nov 2022 21:02:08 -0000
> @@ -135,6 +135,22 @@ ifstart() {
> local _if=$1 _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."
> + return
> + fi
> +
> + [[ -z "$_line" ]] && return
These quotes aren't needed, either; [[ doesn't do word splitting.
Base consistently omits quotes unless they're needed to join variables,
e.g. [[ -z "${foo} spaces.. ${bar}" ]]
> + _if=$_line
> + _line=
> +
> + if [[ -e /etc/hostname.$_if ]]; then
> + print -u2 "${0##*/}: $_hn: /etc/hostname.$_if overrides"
> + 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
> @@ -183,14 +199,16 @@ ifmstart() {
> local _sifs=$1 _xifs=$2 _hn _if _sif _xif
>
> for _sif in ${_sifs:-ALL}; do
> - for _hn in /etc/hostname.+([[:alpha:]])+([[:digit:]]); do
> + for _hn in
> /etc/hostname.@(+([[:alpha:]])+([[:digit:]])|??:??:??:??:??:??); do
> [[ -f $_hn ]] || continue
> _if=${_hn#/etc/hostname.}
>
> - # Skip unwanted ifs.
> - for _xif in $_xifs; do
> - [[ $_xif == ${_if%%[0-9]*} ]] && continue 2
> - done
> + if [[ $_if == +([[:alpha:]])+([[:digit:]]) ]]; then
> + # Skip unwanted ifs.
> + for _xif in $_xifs; do
> + [[ $_xif == ${_if%%[0-9]*} ]] &&
> continue 2
> + done
> + fi
>
> # Start wanted ifs.
> [[ $_sif == @(ALL|${_if%%[0-9]*}) ]] && ifstart $_if
> Index: distrib/miniroot/install.sub
> ===================================================================
> RCS file: /cvs/src/distrib/miniroot/install.sub,v
> retrieving revision 1.1214
> diff -u -p -r1.1214 install.sub
> --- distrib/miniroot/install.sub 6 Nov 2022 21:32:54 -0000 1.1214
> +++ distrib/miniroot/install.sub 26 Nov 2022 21:02:08 -0000
> @@ -2431,6 +2431,12 @@ ifstart() {
> local _if=$1 _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
> + fi
> +
> # Create interface if it does not yet exist.
> { ifconfig $_if || ifconfig $_if create; } >/dev/null 2>&1 || return
>
> @@ -2472,6 +2478,9 @@ enable_ifs() {
> svlan) _svlans="$_svlans $_if";;
> vlan) _vlans="$_vlans $_if";;
> esac
> + elif [[ $_if == ??:??:??:??:??:?? ]]; then
> + # start by lladdr
> + ifstart $_if
> else
> # 'Real' interfaces (if available) are done now.
> ifconfig $_if >/dev/null 2>&1 && ifstart $_if
> Index: share/man/man5/hostname.if.5
> ===================================================================
> RCS file: /cvs/src/share/man/man5/hostname.if.5,v
> retrieving revision 1.79
> diff -u -p -r1.79 hostname.if.5
> --- share/man/man5/hostname.if.5 26 Jul 2022 00:36:54 -0000 1.79
> +++ share/man/man5/hostname.if.5 26 Nov 2022 21:02:08 -0000
> @@ -41,9 +41,14 @@ The
> .Nm hostname.*\&
> files contain information regarding the configuration of each network
> interface.
> One file should exist for each interface that is to be configured, such as
> -.Pa hostname.fxp0
> +.Pa hostname.fxp0 ,
> +.Pa hostname.12:34:56:78:90:ab ,
> or
> .Pa hostname.bridge0 .
> +Alternatively the interface name can be replaced with the interface's lladdr,
> +provided the lladdr is unique across the system and no
> +.Nm
> +exists for the interface name.
> A configuration file is not needed for lo0.
> .Pp
> The configuration information is expressed in a line-by-line packed format
>
