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 <mhuang(a)redhat.com>
> > ---
> > 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
> > kexec(a)lists.fedoraproject.org
> >
https://lists.fedoraproject.org/mailman/listinfo/kexec