On Wed, Jul 23, 2014 at 10:10:36AM +0800, WANG Chao wrote:
On 07/22/14 at 11:14am, Vivek Goyal wrote:
> On Tue, Jul 22, 2014 at 05:10:14PM +0800, WANG Chao wrote:
>
> [..]
> > kdump_setup_bridge() {
> > local _netdev=$1
> > - local _brif _dev
> > + local _brif _dev _mac _kdumpdev
> > for _dev in `ls /sys/class/net/$_netdev/brif/`; do
> > if kdump_is_bond "$_dev"; then
> > kdump_setup_bond "$_dev"
> > @@ -108,20 +124,30 @@ kdump_setup_bridge() {
> > elif kdump_is_vlan "$_dev"; then
> > kdump_setup_vlan "$_dev"
> > else
> > - echo -n " ifname=$_dev:$(kdump_get_mac_addr $_dev)"
>> ${initdir}/etc/cmdline.d/41bridge.conf
> > + _mac=$(kdump_get_mac_addr $_dev)
> > + _kdumpdev=$(kdump_setup_ifname $_dev)
> > + echo -n " ifname=$_kdumpdev:$_mac" >>
${initdir}/etc/cmdline.d/41bridge.conf
> > + fi
> > + if [ -n "$_kdumpdev" ]; then
> > + _brif+="$_kdumpdev,"
> > + else
> > + _brif+="$_dev,"
> > fi
> > - _brif+="$_dev,"
> > + _kdumpdev=""
>
> When will _kdumpdev be zero? IOW, as per your logic kdumpdev will either
> be same as _dev or kdump-$dev. So why are you putting this extra if loop?
>
> IOW, why not simply do.
>
> _brif+="$_kdumpdev"
Because it is if-else statement to handle different types of devices.
For bond/team/vlan, we don't modify their names, so we use $_dev. For
ethernet interfaces, we modify their names, so we use $_kdumpdev.
And we need to unset $_kdumpdev in the end of each loop, otherwise it
retains the value from last loop.
Ok, so how about following structure. I think this is easier to read. And
there are no additional if-else constructs.
for _dev in `ls /sys/class/net/$_netdev/brif/`; do
_kdumpdev=$_dev
if kdmp_is_bond
...
if kdump_is_team
...
.....
....
else
_kdumpdev=$(kdump_setup_ifname $_dev)
fi
_brif+="$_kdumpdev,"
Thanks
Vivek