On 07/23/14 at 09:26am, Vivek Goyal wrote:
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,"
Right. This is much better! Thanks!
- Chao