On Apr 6, 2012, at 12:56 PM, Bruce Dubbs wrote: > I've tried moving some things around on my development system and would like > some feedback. > > For a bridge, br0, I have: > > ifconfig.br0 > > ONBOOT=yes > IFACE=br0 > SERVICE="bridge ipv4-static" > IP=192.168.0.22 > GATEWAY=192.168.0.1 > PREFIX=24 > BROADCAST=192.168.0.255 > INTERFACES=eth0 # Add to IFACE > IP_FORWARD=true > #MTU=9000 > > I changed /sbin/ifup to do: > > for S in ${SERVICE}; do > IFCONFIG=${file} /lib/services/${S} ${IFACE} up > done > > # Bring up all interfaces > for I in $IFACE $INTERFACES; do up $I; done > > # Set MTU if requested. Check if MTU has a "good" value. > if test -n "${MTU}"; then > if [[ ${MTU} =~ ^[0-9]+$ ]] && [[ $MTU -ge 68 ]] ; then > for I in $IFACE $INTERFACES; do ip link set dev $I mtu $MTU; done > else > log_info_msg2 "Invalid MTU $MTU" > fi > fi > > if [ -n "${GATEWAY}" ]; then > if ip route | grep -q default; then > log_warning_msg "\nGateway already setup; skipping." > else > log_info_msg "Setting up default gateway..." > ip route add default via ${GATEWAY} dev ${1} > evaluate_retval > fi > fi > > This initializes all services first and then sets them to UP. > It also sets the MTU to all parts of the interface if requested. > Finally, I moved the GATEWAY portion from the ipv4_static service to ifup. > > The script does not use the CHECK_LINK parameter any more. > > --------- > The bridge service 'up' portion now does: > > log_info_msg "Creating the ${1} interface..." > brctl addbr ${1} > evaluate_retval > > for I in ${INTERFACES}; do > log_info_msg "Adding ${I} to ${1}..." > brctl addif ${1} ${I} > evaluate_retval > done > > if is_true ${IP_FORWARD}; then > sysctl -w net.ipv4.ip_forward=1 > /dev/null > log_success_msg "Setting net.ipv4.ip_forward = 1" > fi > > The point is that the bridge only does three things: > 1. Create the bridge > 2. Add any interfaces > 3. Set ip_forward if requested > > ------- > > There is a new function in init-functions. is_true, that tests if a parameter > is any of 1, y, t, yes, true. > > --------- > The ipv4-static service 'up' portion now merely does: > > ip addr add ${args} dev ${1} > > where the args are generally "$IP/$PREFIX broadcast ${BROADCAST}" > > --------- > > What these scripts to *not* do is process multiple IP addresses if there is > more than 1 ethx interface. The full scripts with messages and error > checking are attached. They work for me, but I need to see what others think.
1) Why is ifup bringing up the virtual interface? That is clearly a service-level responsibility. In fact, in the case of a bridge, that interface doesn't even exist until you create it with brctl. What purpose does it serve in ifup? 2) Why does ifup manage any routing at all (including gw)? That's a service-level issue. 3) Why does bridge handle setting IP_FORWARD? Why isn't that in ipv4-static? Bridging shouldn't care about IP issues like routing, and that's the wrong place to set it. 4) Why pay any attention to the MTU value? Report an error from ip if a bad MTU value is set; I don't think anyone needs to rely on the ifup-&-co. scripts to check for valid values. ip will report those values. AFAIC--and I was the one who put a numeric check in the first place--it doesn't even need to check if it's a number. (Another minor point...Why specify BROADCAST? Isn't it unusual to have an address/prefix that didn't produce the right broadcast address.) 5) What does this mean? "What these scripts to *not* do is process multiple IP addresses if there is more than 1 ethx interface." Are you talking about a bridge with multiple slaves (e.g., eth0 -- br0 -- eth1)? Or virtual NIC definitions (e.g., eth1:3)? Seems to me a big part of the issue was making sure the scripts support virtual NICs. Supporting multiple slaves for a bridge in trivial. Again; it's not a multi-homed IP setup. A bridge, regardless of the number of enslaved ports, has only a single logical interface (e.g., br0). 6) Also, do we just dislike PHYS as a variable name? That seems preferable to two variables with similar and somewhat confusing names (IFACE vs INTERFACES). 7) As for cleanly separating logic...The general guideline should be that ifup should not use IFACE. In fact, only ifdown does, and that's just a reference-counting shortcut. The other guideline is that the service scripts should refer to IFACE, and only use PHYS (what you call INTERFACES) as necessary (for example, in the case of a bridge enslaving ports). Stuff like ifup handling gateways, bridge handling IP-related sysctls, and ifup bringing up bridge interfaces all seem misplaced. * * * It's good to take time to look at this, but can you reframe this in terms of the problems you're solving? AFAIK, there was only one remaining issue to look at, which is how to properly support virtual NICS. I thought I had resolved the issues with MTU, multiple PHYS per bridge, and cleanly separating the service/physical logic issues. It just seems like a lot of changes here are pushing the wrong logic into the wrong scripts. Q > -- Bruce > > > #!/bin/sh > ######################################################################## > # Begin /lib/services/bridge > # > # Description : Bridge Boot Script > # > # Authors : Nathan Coulson - nat...@linuxfromscratch.org > # Bruce Dubbs - bdu...@linuxfromscratch.org > # > # Version : LFS-7.0 > # > ######################################################################## > > . /lib/lsb/init-functions > . ${IFCONFIG} > > if [ -n "${INTERFACE}" ]; then > log_failure_msg "INTERFACES variable missing from ${IFCONFIG}" > exit 1 > fi > > case "${2}" in > up) > log_info_msg2 "\n" > log_info_msg "Creating the ${1} interface..." > brctl addbr ${1} > evaluate_retval > > for I in ${INTERFACES}; do > log_info_msg "Adding ${I} to ${1}..." > brctl addif ${1} ${I} > evaluate_retval > done > > if is_true ${IP_FORWARD}; then > sysctl -w net.ipv4.ip_forward=1 > /dev/null > log_success_msg "Setting net.ipv4.ip_forward = 1" > fi > ;; > > down) > for I in ${INTERFACES}; do > log_info_msg "Removing ${I} from ${1}..." > ip link set ${I} down && > brctl delif ${1} ${I} > evaluate_retval > done > > log_info_msg "Bringing down the ${1} interface..." > ip link set ${1} down > brctl delbr ${1} > evaluate_retval > ;; > > *) > echo "Usage: ${0} [interface] {up|down}" > exit 1 > ;; > esac > > # End /lib/services/bridge > > #!/bin/sh > ######################################################################## > # Begin /sbin/ifup > # > # Description : Interface Up > # > # Authors : Nathan Coulson - nat...@linuxfromscratch.org > # Kevin P. Fleming - kpflem...@linuxfromscratch.org > # Update : Bruce Dubbs - bdu...@linuxfromscratch.org > # > # Version : LFS 7.0 > # > # Notes : The IFCONFIG variable is passed to the SERVICE script > # in the /lib/services directory, to indicate what file the > # service should source to get interface specifications. > # > ######################################################################## > > > up() > { > if ip link show $1 > /dev/null 2>&1; then > link_status=`ip link show $1` > > if [ -n "${link_status}" ]; then > if ! echo "${link_status}" | grep -q UP; then > ip link set $1 up > fi > fi > > else > log_failure_msg "\nInterface ${IFACE} doesn't exist." > exit 1 > fi > } > > RELEASE="7.0" > > USAGE="Usage: $0 [ -hV ] [--help] [--version] interface" > VERSTR="LFS ifup, version ${RELEASE}" > > while [ $# -gt 0 ]; do > case "$1" in > --help | -h) help="y"; break ;; > > --version | -V) echo "${VERSTR}"; exit 0 ;; > > -*) echo "ifup: ${1}: invalid option" >&2 > echo "${USAGE}" >& 2 > exit 2 ;; > > *) break ;; > esac > done > > if [ -n "$help" ]; then > echo "${VERSTR}" > echo "${USAGE}" > echo > cat << HERE_EOF > ifup is used to bring up a network interface. The interface > parameter, e.g. eth0 or eth0:2, must match the trailing part of the > interface specifications file, e.g. /etc/sysconfig/ifconfig.eth0:2. > > HERE_EOF > exit 0 > fi > > file=/etc/sysconfig/ifconfig.${1} > > # Skip backup files > [ "${file}" = "${file%""~""}" ] || exit 0 > > . /lib/lsb/init-functions > > log_info_msg "Bringing up the ${1} interface... " > > if [ ! -r "${file}" ]; then > log_failure_msg "\n${file} is missing or cannot be accessed." > exit 1 > fi > > . $file > > if [ "$IFACE" = "" ]; then > log_failure_msg "\n${file} does not define an interface [IFACE]." > exit 1 > fi > > # Do not process this service if started by boot, and ONBOOT > # is not set to yes > if [ "${IN_BOOT}" = "1" -a "${ONBOOT}" != "yes" ]; then > log_info_msg2 "skipped\n" > exit 0 > fi > > for S in ${SERVICE}; do > if [ ! -x "/lib/services/${S}" ]; then > MSG="\nUnable to process ${file}. Either " > MSG="${MSG}the SERVICE '${S} was not present " > MSG="${MSG}or cannot be executed." > log_failure_msg "$MSG" > exit 1 > fi > done > > for S in ${SERVICE}; do > IFCONFIG=${file} /lib/services/${S} ${IFACE} up > done > > # Bring up all interfaces > for I in $IFACE $INTERFACES; do up $I; done > > # Set MTU if requested. Check if MTU has a "good" value. > if test -n "${MTU}"; then > if [[ ${MTU} =~ ^[0-9]+$ ]] && [[ $MTU -ge 68 ]] ; then > for I in $IFACE $INTERFACES; do ip link set dev $I mtu $MTU; done > else > log_info_msg2 "Invalid MTU $MTU" > fi > fi > > if [ -n "${GATEWAY}" ]; then > if ip route | grep -q default; then > log_warning_msg "\nGateway already setup; skipping." > else > log_info_msg "Setting up default gateway..." > ip route add default via ${GATEWAY} dev ${1} > evaluate_retval > fi > fi > > # End /sbin/ifup > #!/bin/sh > ######################################################################## > # Begin /lib/services/ipv4-static > # > # Description : IPV4 Static Boot Script > # > # Authors : Nathan Coulson - nat...@linuxfromscratch.org > # Kevin P. Fleming - kpflem...@linuxfromscratch.org > # Update : Bruce Dubbs - bdu...@linuxfromscratch.org > # > # Version : LFS 7.0 > # > ######################################################################## > > . /lib/lsb/init-functions > . ${IFCONFIG} > > if [ -z "${IP}" ]; then > log_failure_msg "\nIP variable missing from ${IFCONFIG}, cannot continue." > exit 1 > fi > > if [ -z "${PREFIX}" -a -z "${PEER}" ]; then > log_warning_msg "\nPREFIX variable missing from ${IFCONFIG}, assuming 24." > PREFIX=24 > args="${args} ${IP}/${PREFIX}" > > elif [ -n "${PREFIX}" -a -n "${PEER}" ]; then > log_failure_msg "\nPREFIX and PEER both specified in ${IFCONFIG}, cannot > continue." > exit 1 > > elif [ -n "${PREFIX}" ]; then > args="${args} ${IP}/${PREFIX}" > > elif [ -n "${PEER}" ]; then > args="${args} ${IP} peer ${PEER}" > fi > > if [ -n "${BROADCAST}" ]; then > args="${args} broadcast ${BROADCAST}" > fi > > case "${2}" in > up) > if [ "$(ip addr show ${1} 2>/dev/null | grep ${IP})" == "" ]; then > > # Cosmetic output not needed for multiple services > if ! $(echo ${SERVICE} | grep -q " "); then > log_info_msg2 "\n" # Terminate the previous message > fi > > log_info_msg "Adding IPv4 address ${IP} to the ${1} interface..." > ip addr add ${args} dev ${1} > evaluate_retval > > #if [ -n "${GATEWAY}" ]; then > # if ip route | grep -q default; then > # log_warning_msg "\nGateway already setup; skipping." > # else > # log_info_msg "Setting up default gateway..." > # ip route add default via ${GATEWAY} dev ${1} > # evaluate_retval > # fi > #fi > else > log_warning_msg "Cannot add IPv4 address ${IP} to ${1}. Already > present." > fi > ;; > > down) > if [ "$(ip addr show ${1} 2>/dev/null | grep ${IP})" != "" ]; then > log_info_msg "Removing IPv4 address ${IP} from the ${1} interface..." > ip addr del ${args} dev ${1} > evaluate_retval > fi > > if [ -n "${GATEWAY}" ]; then > # Only remove the gateway if ther are no remaining ipv4 addresses > if [ "$(ip addr show ${1} 2>/dev/null | grep 'inet ')" != "" ]; then > log_info_msg "Removing default gateway..." > ip route del default > evaluate_retval > fi > fi > ;; > > *) > echo "Usage: ${0} [interface] {up|down}" > exit 1 > ;; > esac > > # End /lib/services/ipv4-static > -- > http://linuxfromscratch.org/mailman/listinfo/lfs-dev > FAQ: http://www.linuxfromscratch.org/faq/ > Unsubscribe: See the above information page -- http://linuxfromscratch.org/mailman/listinfo/lfs-dev FAQ: http://www.linuxfromscratch.org/faq/ Unsubscribe: See the above information page