On 01/05/15 at 10:31am, Dave Young wrote:
On 01/04/15 at 06:59pm, Minfei Huang wrote:
> On 12/29/14 at 03:57pm, Dave Young wrote:
> > On 12/15/14 at 11:20am, Minfei Huang wrote:
> > > We have to get the network config by using ip route when setup the net
> > > or iscsi. So wrap the similar functional code to the function to make
> > > the kdump code clear.
> > >
> > > Call the function kdump_setup_currect_net, we can setup the net or iscsi
> > > whichever you want.
> > >
> > > Signed-off-by: Minfei Huang <mhuang(a)redhat.com>
> > > ---
> > > dracut-module-setup.sh | 73
+++++++++++++++++++++++++-------------------------
> > > 1 file changed, 36 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> > > index c704a65..10f7c83 100755
> > > --- a/dracut-module-setup.sh
> > > +++ b/dracut-module-setup.sh
> > > @@ -261,24 +261,15 @@ kdump_setup_netdev() {
> > > kdump_setup_dns "$_netdev"
> > > }
> > >
> > > -#Function:kdump_install_net
> > > -#$1: config values of net line in kdump.conf
> > > -#$2: srcaddr of network device
> > > -kdump_install_net() {
> > > - local _server _netdev _srcaddr
> > > - local config_val="$1"
> > > -
> > > - _server=`echo $config_val | sed 's/.*@//' | cut -d':'
-f1`
> > > -
> > > - _need_dns=`echo $_server|grep "[a-zA-Z]"`
> > > - [ -n "$_need_dns" ] && _server=`getent hosts
$_server|cut -d' ' -f1`
> > > +kdump_setup_currect_net() {
> > > + local _server=$1 _netdev _srcaddr
> > > + local _type=$2
> > >
> > > _netdev=`/sbin/ip route get to $_server 2>&1`
> > > [ $? != 0 ] && echo "Bad kdump location:
$config_val" && exit 1
> > >
> > > #the field in the ip output changes if we go to another subnet
> > > - if [ -n "`echo $_netdev | grep via`" ]
> > > - then
> > > + if [ -n "`echo $_netdev | grep via`" ]; then
> > > # we are going to a different subnet
> > > _srcaddr=`echo $_netdev|awk '{print $7}'|head -n 1`
> > > _netdev=`echo $_netdev|awk '{print $5;}'|head -n 1`
> > > @@ -290,20 +281,36 @@ kdump_install_net() {
> > >
> > > kdump_setup_netdev "${_netdev}" "${_srcaddr}"
"${_server}"
> > >
> > > - #save netdev used for kdump as cmdline
> > > - # Whoever calling kdump_install_net() is setting up the default
gateway,
> > > - # ie. bootdev/kdumpnic. So don't override the setting if calling
> > > - # kdump_install_net() for another time. For example, after setting
eth0 as
> > > - # the default gate way for network dump, eth1 in the fence kdump path
will
> > > - # call kdump_install_net again and we don't want eth1 to be the
default
> > > - # gateway.
> > > - if [ ! -f ${initdir}${initdir}/etc/cmdline.d/60kdumpnic.conf ]
&&
> > > - [ ! -f ${initdir}/etc/cmdline.d/70bootdev.conf ]; then
> > > - echo "kdumpnic=$(kdump_setup_ifname $_netdev)" >
${initdir}/etc/cmdline.d/60kdumpnic.conf
> > > - echo "bootdev=$(kdump_setup_ifname $_netdev)" >
${initdir}/etc/cmdline.d/70bootdev.conf
> > > + if [ $_type = "net" ]; then
> >
> > I do not understand the _type, what is it for? is there other type which is not
net?
> >
> > If this is new code, please split the patch to two patches, one is cleanup and
> > add common function, the other is for adding new functionalities.
> >
>
> For some networks, we would specify the config after setuping the network
> device. Here the kdump code, we can make the parameter _type "net" or
> "iscsi".
>
> > > + #save netdev used for kdump as cmdline
> > > + # Whoever calling kdump_install_net() is setting up the default
gateway,
> > > + # ie. bootdev/kdumpnic. So don't override the setting if
calling
> > > + # kdump_install_net() for another time. For example, after
setting eth0 as
> > > + # the default gate way for network dump, eth1 in the fence kdump
path will
> > > + # call kdump_install_net again and we don't want eth1 to be
the default
> > > + # gateway.
> > > + if [ ! -f ${initdir}${initdir}/etc/cmdline.d/60kdumpnic.conf ]
&&
> > > + [ ! -f ${initdir}/etc/cmdline.d/70bootdev.conf ]; then
> > > + echo "kdumpnic=$(kdump_setup_ifname $_netdev)" >
${initdir}/etc/cmdline.d/60kdumpnic.conf
> > > + echo "bootdev=$(kdump_setup_ifname $_netdev)" >
${initdir}/etc/cmdline.d/70bootdev.conf
> > > + fi
> > > fi
> > > }
> > >
> > > +#Function:kdump_install_net
> > > +#$1: config values of net line in kdump.conf
> > > +#$2: srcaddr of network device
> >
> > It is only using one argument, so since you are modifying it please
> > update the comment. Also: s/net line/ssh|nfs line
> >
>
> Will comment it here.
>
> > > +kdump_install_net() {
> > > + local config_val="$1" _server _need_dns
> > > +
> > > + _server=`echo $config_val | sed 's/.*@//' | cut -d':'
-f1`
> > > +
> > > + _need_dns=`echo $_server|grep "[a-zA-Z]"`
> >
> > In case ipv6 addresses which contains [a-zA-Z] it does not need dns
convertion...
> >
>
> It does not matter if we filte ipv6 address again. Otherwise we should
s/filte/file?
The format, ipv4 xxx.xxx.xxx(x is the digit) and ipv6
xxxx:xxxx:xxxx:xxxx:xxxx:xxxx(x is the character), matchs the syntax
that ipv4 only contains the digit and dot, ipv6 only contains the
character a-zA-Z and colon.
In other words, we can filte the domain/hostname out by using "grep
[a-zA-Z]" and grep ".".
Use following filter
_need_dns=`echo $_server|grep "[a-zA-Z]"|grep "."`
can make sense.
> varify the $_server value, domain/hostname or not.
But as for this function, it is not good, a variable _need_dns depends on
code in other functions will twist the code. Make the variable defination clear
is better to avoid bugs.
>
> > > + [ -n "$_need_dns" ] && _server=`getent hosts
$_server|cut -d' ' -f1`
> >
> > I remember getent hosts was replaced by getent ahosts, right?
>
> Yes. will modify it.
> >
> > > +
> > > + kdump_setup_currect_net $_server "net"
> >
> > kdump-setup_currect_net looks not good, I assume you means 'correct'?
> > How about _kdump_setup_net or just kdump_setup_net()?
> >
> > I'm confused with the second argument, see previous comment.
> >
>
> Will name the function properly.
>
> > > +}
> > > +
> > > default_dump_target_install_conf()
> > > {
> > > local _target _fstype
> > > @@ -434,13 +441,10 @@ kdump_get_iscsi_initiator() {
> > > # No ibft handling yet.
> > > kdump_setup_iscsi_device() {
> > > local path=$1
> > > - local tgt_name; local tgt_ipaddr;
> > > - local username; local password; local userpwd_str;
> > > - local username_in; local password_in; local userpwd_in_str;
> > > - local netdev
> > > - local srcaddr
> > > - local idev
> > > - local netroot_str ; local initiator_str;
> > > + local tgt_name tgt_ipaddr
> > > + local username password userpwd_str
> > > + local username_in password_in userpwd_in_str
> > > + local netroot_str initiator_str
> > > local netroot_conf="${initdir}/etc/cmdline.d/50iscsi.conf"
> > > local initiator_conf="/etc/iscsi/initiatorname.iscsi"
> > >
> > > @@ -472,12 +476,7 @@ kdump_setup_iscsi_device() {
> > >
> > > [ -n "$username_in" ] &&
userpwd_in_str=":$username_in:$password_in"
> > >
> > > - netdev=$(/sbin/ip route get to ${tgt_ipaddr} | \
> > > - sed 's|.*dev \(.*\).*|\1|g')
> > > - srcaddr=$(echo $netdev | awk '{ print $3; exit }')
> > > - netdev=$(echo $netdev | awk '{ print $1; exit }')
> > > -
> > > - kdump_setup_netdev $netdev $srcaddr $tgt_ipaddr
> > > + kdump_setup_currect_net $tgt_ipaddr "iscsi"
> >
> > Hmm, looks like the type becomes iscsi here, if this is the case then how
about
> > doing something like below:
> >
> > iscsi path:
> > -> kdump_setup_net() $tgt_ipaddr
> >
> > net path:
> > -> kdump_setup_net() $_server
> > -> kdump_setup_kdumpnic_bootdev() _server
> >
>
> In net path, we need the $_netdev value which gets in the
> kdump_setup_currect_net. So it is better wrap in one function.
I understand your purpose to add a common function, but it introduced more confuses
the two types makes the logic more complicated.
There was a discussion with Bao and Chao before wrapping the common code
to a function. We want to cleanup the setup network code in purpose,
especial for getting ip route info. Maybe we still need discuss it.
>
> > >
> > > # prepare netroot= command line
> > > # FIXME: IPV6 addresses require explicit [] around $tgt_ipaddr
> > > --
> > > 1.8.3.1
> > >
> > > _______________________________________________
> > > kexec mailing list
> > > kexec(a)lists.fedoraproject.org
> > >
https://lists.fedoraproject.org/mailman/listinfo/kexec