Re: [PATCH v7 4/4] ipv6: Enhance kdump to support ipv6

[Baoquan He]

On 02/11/15 at 11:14am, Minfei Huang wrote:
> Hi, Bao!
>
> Thanks for your review.
>
> Maybe I should re-construct the patchset to make it more clear. For
> simple patch, it is more confortable to review or modify.

Yes, totally agree.

There isn't a standard on how to split patch. I personally think if you
can write what you have done in patch log and reviewer can understand it
only from patch log, then this should be a well split patch.
> Thanks
> Minfei
>
> On 02/11/15 at 10:23am, Baoquan He wrote:
> > On 01/15/15 at 07:28pm, Minfei Huang wrote:
> > > Currently kdump doesn't support ipv6 nfs/ssh dump. Ipv6 is the latest version
> > > of the Internet Protocal. So it is a significant feture for kdump to enhance
> > > to support ipv6.
> > >
> > > What is the main difference in userspace between ipv4 and ipv6 is the ip
> > > address format.
> > > For ipv6 nfs dump:
> > >     if ipv6 address type is link scope, /etc/kdump.conf should be edited like
> > > "nfs [fe80::5054:ff:fe48:ca80%eth0]:/mnt"
> > >     else /etc/kdump.conf should be edited like "nfs [2001:db8:0:f101::2]:/mnt"
> > > For ipv6 ssh dump
> > >     if ipv6 address type is link scope, /etc/kdump.conf should be edited like
> > > "ssh root at fe80::5054:ff:fe48:ca80%eth0"
> > >     else /etc/kdump.conf should be edited like "ssh root at 2001:db8:0:f101::2"
> > >
> > > How to create a ipv6 enviromnet.
> > >     1): Reserving two beaker machine with family fedora.
> > >     2): Choosing a beaker machine as a nfs/ssh server and delete it's ipv4 address
> > >         by "ip address del ipv4-address dev nicname"
> > >     3): Configuring the /etc/kdump.conf like mentioned above.
> > > Signed-off-by: Minfei Huang
> > >
> > > dracut-kdump.sh        |   4 +-
> > >  dracut-module-setup.sh | 143 ++++++++++++++++++++++++++++++++++---------------
> > >  2 files changed, 103 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/dracut-kdump.sh b/dracut-kdump.sh
> > > index e062665..7e6b810 100755
> > > --- a/dracut-kdump.sh
> > > +++ b/dracut-kdump.sh
> > > @@ -121,9 +121,9 @@ get_host_ip()
> > >      then
> > >          kdumpnic=$(getarg kdumpnic=)
> > >          [ -z "$kdumpnic" ] && echo "kdump: failed to get kdumpnic!" && return 1
> > >    _host=`ip addr show dev $kdumpnic|grep 'inet '`
> > >    _host=`ip addr show dev $kdumpnic|grep 'inet'`
> > >    [ $? -ne 0 ] && echo "kdump: wrong kdumpnic: $kdumpnic" && return 1
> > >    _host="${_host##*inet }"
> > >    _host=`echo $_host | cut -d' ' -f2`
> >
> > Why is above change?
> >
> > Hi Minfei,
> >
> > Don't you think this patch is too big?  It's not easy to review. For me,
> > I will make a patch for above change, then we can skip it next time if we
> > think it's OK this time. From patch log, I didn't get hint how to review
> > it, just get a general information that it's a ipv6 enhancement.
> >
> > I have personal suggestion in below inline comment.
> > >      _host="${_host%%/*}"
> > >      [ -z "$_host" ] && echo "kdump: wrong kdumpnic: $kdumpnic" && return 1
> > >      HOST_IP=$_host
> > >
> > > diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> > > index 1e81b42..6a7da8c 100755
> > > --- a/dracut-module-setup.sh
> > > +++ b/dracut-module-setup.sh
> > > @@ -70,14 +70,25 @@ kdump_setup_dns() {
> > >  #$2: srcaddr
> > >  #if it use static ip echo it, or echo null
> >
> > For static info checking and handling, it should be separated as a
> > patch.
> > > kdump_static_ip() {
> > > local _netmask _gateway
> > > local _netmask _gateway _ipaddr
> > > local _netdev="$1" _srcaddr="$2"
> > > local _ipaddr=$(ip addr show dev $_netdev permanent | \
> > > if is_ipv6_address $_srcaddr; then
> > >    _ipaddr=$(ip -6 addr show dev $_netdev permanent | \
> > >                awk "/ $_srcaddr\/.* /{print \$2}")
> > >    if [ -n "$_ipaddr" ]; then
> > >        _netmask=${_ipaddr#*\/}
> > >        _gateway=$(ip -6 route list dev $_netdev | awk '/^default /{print $3}')
> > >        echo -n "[${_srcaddr}]::[${_gateway}]:${_netmask}::"
> > >    fi
> > > else
> > >    _ipaddr=$(ip addr show dev $_netdev permanent | \
> > >                awk "/ $_srcaddr\/.* $_netdev\$/{print \$2}")
> > > if [ -n "$_ipaddr" ]; then
> > >   _netmask=$(ipcalc -m $_ipaddr | cut -d'=' -f2)
> > >   _gateway=$(ip route list dev $_netdev | awk '/^default /{print $3}')
> > >   echo -n "${_srcaddr}::${_gateway}:${_netmask}::"
> > >    if [ -n "$_ipaddr" ]; then
> > >        _netmask=$(ipcalc -m $_ipaddr | cut -d'=' -f2)
> > >        _gateway=$(ip route list dev $_netdev | awk '/^default /{print $3}')
> > >        echo -n "${_srcaddr}::${_gateway}:${_netmask}::"
> > >    fi
> > > fi
> > >
> > > }
> > >
> > > @@ -191,27 +202,45 @@ kdump_setup_znet() {
> > >      echo rd.znet=${NETTYPE},${SUBCHANNELS}${_options} > ${initdir}/etc/cmdline.d/30znet.conf
> > >  }
> > >
> > > +#
> > > +# For the same subnet region, following is the route format
> > > +#   ipv4:
> > > +#        _route='192.168.200.137 dev eth1  src 192.168.200.129
> > > +#               cache '
> > > +#   ipv6:
> > > +#       _route='2001:11::11f from :: dev eth1  src 2001:11::120  metric 0
> > > +#               cache'
> > > +# For the different subnet region, following is the route format
> > > +#   ipv4:
> > > +#       _route='192.168.201.215 via 192.168.200.137 dev eth1  src 192.168.200.129
> > > +#               cache '
> > > +#   ipv6:
> > > +#       _route='2001:10::120 from :: via 2001:11::11f dev eth1  src 2001:11::120  metric 0
> > > +#               cache'
> >
> > route handling either.
> > > get_routes() {
> > >      local _netdev="$1" _target="$2"
> > >      local _route _nexthop
> > > _route=`/sbin/ip route get to $_target 2>&1`
> > >
> > > -#
> > > -# in the same subnet region, following is the route format
> > > -# _route='192.168.200.137 dev eth1  src 192.168.200.129
> > > -#   cache '
> > > -#
> > > -# in the different subnet region, following is the route format
> > > -# _route='192.168.201.215 via 192.168.200.137 dev eth1  src 192.168.200.129
> > > -#   cache '
> > > -#
> > > if `echo $_route | grep -q "via"`; then
> > >    # route going to a different subnet via a router
> > >    _nexthop=`echo $_route | awk '{print $3}'`
> > > fi
> > > _netdev=$(kdump_setup_ifname $_netdev)
> > > if is_ipv6_address $_target; then
> > >    _route=`/sbin/ip -6 route get to $_target 2>&1`
> > >    _netdev=$(kdump_setup_ifname $_netdev)
> > >    if `echo $_route | grep -q "via"`; then
> > >        # route going to a different subnet via a router
> > >        _nexthop=`echo $_route | awk '{print $5}'`
> > >        echo "rd.route=[$_target]:[$_nexthop]:$_netdev" >> ${initdir}/etc/cmdline.d/45route-static.conf
> > >    else
> > >        echo "rd.route=[$_target]::$_netdev" >> ${initdir}/etc/cmdline.d/45route-static.conf
> > >    fi
> > > else
> > >    _route=`/sbin/ip route get to $_target 2>&1`
> > >    if `echo $_route | grep -q "via"`; then
> > >        # route going to a different subnet via a router
> > >        _nexthop=`echo $_route | awk '{print $3}'`
> > >    fi
> > >    _netdev=$(kdump_setup_ifname $_netdev)
> > >
> > > echo "rd.route=$_target:$_nexthop:$_netdev" >> ${initdir}/etc/cmdline.d/45route-static.conf
> > >    echo "rd.route=$_target:$_nexthop:$_netdev" >> ${initdir}/etc/cmdline.d/45route-static.conf
> > > fi
> > >
> > > }
> > >
> > > # Setup dracut to bringup a given network interface
> >
> > Please take below change as a single patch or two.
> > > @@ -268,24 +297,36 @@ 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`
> > > _server=`get_remote_host $config_val`
> > > _netdev=`/sbin/ip route get to $_server 2>&1`
> > > [ $? != 0 ] && echo "Bad kdump location: $config_val" && exit 1
> > > is_hostname $_server && _server=`getent ahosts $_server|cut -d' ' -f1`
> > > #the field in the ip output changes if we go to another subnet
> > > 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`
> > > if is_ipv6_address $_server; then
> > >    _netdev=`/sbin/ip -6 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
> > >        # we are going to a different subnet
> > >        _srcaddr=`echo $_netdev|awk '{print $9}'|head -n 1`
> > >        _netdev=`echo $_netdev|awk '{print $7;}'|head -n 1`
> > >    else
> > >        # we are on the same subnet
> > >        _srcaddr=`echo $_netdev|awk '{print $7}'|head -n 1`
> > >        _netdev=`echo $_netdev|awk '{print $5}'|head -n 1`
> > >    fi
> > > else
> > >    # we are on the same subnet
> > >    _srcaddr=`echo $_netdev|awk '{print $5}'|head -n 1`
> > >    _netdev=`echo $_netdev|awk '{print $3}'|head -n 1`
> > >    _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
> > >        # 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`
> > >    else
> > >        # we are on the same subnet
> > >        _srcaddr=`echo $_netdev|awk '{print $5}'|head -n 1`
> > >        _netdev=`echo $_netdev|awk '{print $3}'|head -n 1`
> > >    fi
> > >
> > > fi
> > >
> > > kdump_setup_netdev "${_netdev}" "${_srcaddr}" "${_server}"
> > >
> > > @@ -439,6 +480,7 @@ kdump_get_iscsi_initiator() {
> >
> > And iscsu handling.
> > > kdump_setup_iscsi_device() {
> > >      local path=$1
> > >      local tgt_name; local tgt_ipaddr;
> > > local _srcaddr _prefix _netdev
> > > local username; local password; local userpwd_str;
> > > local username_in; local password_in; local userpwd_in_str;
> > > local netdev
> > >
> > > @@ -476,19 +518,34 @@ 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')
> > > if is_ipv6_address $tgt_ipaddr; then
> > >
> > > +# filer out the network deivce if the target ip address is scope link address of ipv6
> > >    netdev=$(/sbin/ip -6 route get to $(get_remote_host $tgt_ipaddr) | \
> > >        sed 's|.*dev \(.*\).*|\1|g')
> > > else
> > >    netdev=$(/sbin/ip route get to ${tgt_ipaddr} | \
> > >        sed 's|.*dev \(.*\).*|\1|g')
> > > fi
> > > srcaddr=$(echo $netdev | awk '{ print $3; exit }')
> > > netdev=$(echo $netdev | awk '{ print $1; exit }')
> > > kdump_setup_netdev $netdev $srcaddr $tgt_ipaddr
> > >
> > > kdump_setup_netdev $netdev $srcaddr $(get_remote_host $tgt_ipaddr)
> > >
> > > # prepare netroot= command line
> > >
> > > # FIXME: IPV6 addresses require explicit [] around $tgt_ipaddr
> > > # FIXME: Do we need to parse and set other parameters like protocol, port
> > > #        iscsi_iface_name, netdev_name, LUN etc.
> > >
> > > netroot_str="netroot=iscsi:${userpwd_str}${userpwd_in_str}@$tgt_ipaddr::::$tgt_name"
> > >
> > > if is_ipv6_address $tgt_ipaddr; then
> > >    if `echo $tgt_ipaddr | grep -q "%"`; then
> > >        _prefix=${tgt_ipaddr%\%*}
> > >        _netdev=${tgt_ipaddr#*\%}
> > >        _netdev=$(kdump_setup_ifname $_netdev)
> > >        tgt_ipaddr=$_prefix%$_netdev
> > >    fi
> > >    netroot_str="netroot=iscsi:${userpwd_str}${userpwd_in_str}@[$tgt_ipaddr]::::$tgt_name"
> > >
> > > else
> > >    netroot_str="netroot=iscsi:${userpwd_str}${userpwd_in_str}@$tgt_ipaddr::::$tgt_name"
> > >
> > > fi
> > >
> > > [[ -f $netroot_conf ]] || touch $netroot_conf
> >
> > And this. You can merge it with other general handling or a single
> > patch.
> >
> > At last, it's least we need add some description in document, after all
> > it's a new feature.
> >
> > Please try to split it into several several patches according to
> > functionality block. Each time I want to review this patch, I have you
> > apply them and read all code to understand. With small patches, it's
> > easier to you write the patch log, and you needn't take care of it each
> > time as soon as reviewers think it's OK and offer a ACK.
> >
> > Persona opinion.
> >
> > Thanks
> > Baoquan
> > > @@ -621,6 +678,8 @@ install() {
> > >      inst "/bin/dd" "/bin/dd"
> > >      inst "/bin/tail" "/bin/tail"
> > >      inst "/bin/date" "/bin/date"
> > > inst "/bin/getent" "/bin/getent"
> > > inst "/bin/head" "/bin/head"
> > > inst "/bin/sync" "/bin/sync"
> > > inst "/bin/cut" "/bin/cut"
> > > inst "/sbin/makedumpfile" "/sbin/makedumpfile"
> > >
> > > -- 
> > > 1.9.3
> > >
> > > kexec mailing list
> > > [e-mail]
> > > https://lists.fedoraproject.org/mailman/listinfo/kexec