Re: [Devil-linux-develop] [PATCH] IPv6 and bonding

[Friedrich L. <fl...@fl...>]

Hi!

Arnaud Gomes-do-Vale wrote on 15.01.2004 17:51 MET:
> Here is a cleaner version of my patch introducing IPv6 and bonding
> support in the 1.0 branch. The ifenslave binary is now included on the
> CD. I will try to update the documentation when I can find some time.
> 

I have a comment on your changes.

> @@ -214,6 +218,34 @@
>                                 route add -$CMD ${target} ${netmask:+netmask $netmask} ${gateway:+gw $gateway} dev $DEVICE
>                         done
>                 fi
> +
> +               # IPv6 support
> +               if [ -n "$IPV6ADDR" ]; then
> +                       echo "Configuring IPv6 for interface $IF"
> +                       for v6addr in $IPV6ADDR; do
> +                               ifconfig $DEVICE add $v6addr
> +                       done
> +               fi
> +
> +               if [ -n "$IPV6ROUTE" ]; then

Do you think it makes sense when the user does set IPV6ROUTE but
not IPV6ADDR? If not then please rewrite this section.

> +                       for route_info in ${IPV6ROUTE} ; do
> +                               destination=$(echo $route_info | cut -f1 -d%)
> +                               gateway=$(echo $route_info | cut -f2 -d%)

I suggest we use | instead of % so wie can also change this for the
"normal" routes.

> +
> +                               # filter bogus records
> +                               test -z "$destination" && continue
> +
> +                               # add the route
> +                               echo "   adding route to $destination ${gateway:+ via gateway $gateway} on $DEVICE"
> +                               ip -f inet6 route add $destination ${gateway:+via $gateway} dev $DEVICE
> +                       done
> +               fi
> +
> +               # Bonding.
> +               if [ "$SLAVE" = "yes" -a -n "$MASTER" ]; then
> +                       ifenslave -v "$MASTER" $DEVICE
> +                       ip route flush dev $DEVICE
> +               fi
>         fi
>  }

Now about the start order:

> @@ -283,7 +315,8 @@
>                 # physical interfaces are brought up first
>                 #
>                 for interface in $(cd ${CONFIG_DIR}; ls -1 ${CONFIG_FILE}* 2>/dev/null | sed \
> -                       -e '/.sample/d' -e '/~/d' -e "s|${CONFIG_FILE}||" -e '/^lo$/d' -e '/^vlan/d' -ne '/^eth/p')
> +                       -e '/.sample/d' -e '/~/d' -e "s|${CONFIG_FILE}||" -e '/^lo$/d' -e '/^vlan/d' \
> +                       -ne '/^eth/p' -ne '/^bond/p')
>                 do
>                         # did user choose to only handle just one interface
>                         # this time?

Shouldn't Bonding interfaces be configured after the basic interfaces
are up? Then I think you need to create an extra for loop for
the bonding devices.

And one last thing please make this TWO separate PATCHES, one for
IPV6 support and one for bonding.

The smaller the changes the better they can be checked. THX!

-- 
MfG / Regards
Friedrich Lobenstock
____________________________________________________________________
Friedrich Lobenstock                       Linux Services Lobenstock
URL: http://www.lsl.at/                         Email: fl...@fl...
____________________________________________________________________