Hi Lars, Thank you for your comments, I'm going to answer to your comments below, and if you have further comments I would greatly appreciate it.
2012/10/16 Lars Ellenberg <[email protected]>: > > Again, apollogies for not having this sent out when I wrote it, > I'm unsure why it "hibernated" in my Draft folder for five month, > but it was not the only one :( > > I realize the pull request has meanwhile been closed, > and we do have a findif.sh implementation in current git. > > Still I'll just send these comments as I wrote them back then, > maybe some of them still apply. > > At the end, there is a bit of comment how to maybe re-implement > the ipcheck and ifcheck functions without grep and awk. > > Feel free to ignore for now, though. > I'll try to review again whats in git now, > and send a proper git diff, once I find the time > > ;-) > > On Wed, May 30, 2012 at 11:20:03PM -0700, Keisuke MORI wrote: >> This is a proposal enhancement of IPaddr2 to support IPv6 as well as IPv4. >> >> I would appreciate your comments and suggestions for merging this into >> the upstream. >> >> NOTE: This pull request is meant for reviewing the code and >> discussions, and not intended to be merged as is at this moment. > > Github pull request comments are IMO not the best place to discuss these > things, so I send to the linux-ha-dev mailing list as well. > >> ## Benefits: >> >> * Unify the usage, behavior and the code maintenance between IPv4 and IPv6 >> on Linux. >> >> The usage of IPaddr2 and IPv6addr are similar but they have >> different parameters and different behaviors. In particular, they >> may choose a different interface depending on your configuration >> even if you provided similar parameters in the past. >> >> IPv6addr is written in C and rather hard to make improvements. As >> /bin/ip already supports both IPv4 and IPv6, we can share the most >> of the code of IPaddr2 written in bash. > > > IPv6addr is supposed to run on non-Linux as well. > So we better not deprecate it, as long as all the world is not Linux. Agreed. > >> * usable for LVS on IPv6. >> >> IPv6addr does not support lvs_support=true and unfortunately there >> is no possible way to use LVS on IPv6 right now. >> >> IPaddr2(/bin/ip) works for LVS configurations without enabling >> lvs_support both for IPv4 and IPv6. >> >> (You don't have to remove an address on the loopback interface if >> the virtual address is assigned by using /bin/ip.) >> >> See also: >> http://www.gossamer-threads.com/lists/linuxha/dev/76429#76429 >> >> * retire the old 'findif' binary. >> >> 'findif' binary is replaced by a shell script version of findif, >> originally developed by lge. See ClusterLabs/resource-agents#53 : >> findif could be rewritten in shell >> >> * easier support for other pending issues >> >> These pending issues can be fix based on this new IPaddr2. * >> ClusterLabs/resource-agents#68 : Allow ipv6addr to mark new address >> as deprecated * ClusterLabs/resource-agents#77 : New RA that >> controls IPv6 address in loopback interface >> >> >> ## Notes / Changes: >> >> * findif semantics changes >> >> There are some incompatibility in deciding which interface to be >> used when your configuration is ambiguous. But in reality it should >> not be a problem as long as it's configured properly. >> >> The changes mostly came from fixing a bug in the findif binary >> (returns a wrong broadcast) or merging the difference between >> (old)IPaddr2 and IPv6addr. See the ofct test cases for details. >> (case No.6, No.9, No.10, No.12, No.15 in IPaddr2v4 test cases) >> >> Other notable changes are described below. >> >> * "broadcast" parameter for IPv4 >> >> "broadcast" parameter may be required along with "cidr_netmask" when >> you want use a different subnet mask from the static IP address. >> It's because doing such calculation is difficult in the shell script >> version of findif. >> >> See the ofct test cases for details. (case No.11, No.14, No.16, >> No.17 in IPaddr2v4 test cases) >> >> This limitation may be eliminated if we would remove brd options >> from the /bin/ip command line. > > If we do not specify the broadcast at all, ip will do the "right thing" > by default, I think. We should only use it on the ip command line, if > it is in the input parameters. > I don't really have a use case for the broadcast address to not be the > "default", so I would be ok with dropping it completely. It has been fixed and the latest code in the repo now should work like you said. > >> * loopback(lo) now requires cidr_netmask or broadcast. >> >> See the ofct test case in the IPaddr2 ocft script. The reason is >> similar to the previous one. > > We really need to avoid breaking existing configurations. > So we need to fix this. > If we find nothing better, with some heuristic. It has also been fixed now and loopback can be used as same as before. > >> * loose error check for "nic" for a IPv6 link-local address. >> >> IPv6addr was able to check this, but in the shell script it is hard >> to determine a link-local address (requires bitmask calculation). I >> do not think it's worth to implement it in shell. > > There may even be use cases for link-local addresses. > So maybe that is a feature, not a bug ;-) This check is done by simply matching fe80:: prefix as Alan's suggestion and I think it is just enough. > > We could always have a few helpers in C, still. > Or see if we can use existing helpers, if present at runtime. > (ipcalc, sipcalc, there may be more). > >> * send_ua: a new binary >> >> We need one new binary as a replacement of send_arp for IPv6 >> support. IPv6addr.c is reused to make this command. >> >> >> Note that IPv6addr RA is still there and you can continue to use it >> for the backward compatibility. >> >> >> ## Acknowledgement >> >> Thanks to Tomo Nozawa-san for his hard work for writing and testing >> this patch. >> >> Thanks to Lars Ellenberg for the first findif.sh implementation. >> >> >> >> You can merge this Pull Request by running: >> >> git pull https://github.com/kskmori/resource-agents >> IPaddr2-dualstack-devel >> >> Or you can view, comment on it, or merge it online at: >> >> https://github.com/ClusterLabs/resource-agents/pull/97 >> >> -- Commit Summary -- >> >> * [RFC] IPaddr2: Proposal patch to support the dual stack of IPv4 and >> IPv6. >> >> -- File Changes -- >> >> M heartbeat/IPaddr2 (59) M heartbeat/IPv6addr.c (89) M >> heartbeat/Makefile.am (9) A heartbeat/findif.sh (184) M >> tools/ocft/IPaddr2 (24) A tools/ocft/IPaddr2v4 (315) A >> tools/ocft/IPaddr2v6 (257) M tools/ocft/Makefile.am (2) >> >> -- Patch Links -- >> >> https://github.com/ClusterLabs/resource-agents/pull/97.patch > > diff --git a/heartbeat/findif.sh b/heartbeat/findif.sh > new file mode 100644 > index 0000000..6a2807c > --- /dev/null > +++ b/heartbeat/findif.sh > @@ -0,0 +1,184 @@ > +#!/bin/sh > +ipcheck_ipv4() { > local r1_to_255="([1-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-5])" > local r0_to_255="([0-9][0-9]?|1[0-9][0-9]|2[0-4][0-9]|25[0-5])" > local r_ipv4="^$r1_to_255\.$r0_to_255\.$r0_to_255\.$r0_to_255$" > echo "$1" | grep -q -Ee "$r_ipv4" > } > > +ipcheck_ipv6() { > ! echo "$1" | grep -qs "[^0-9:a-fA-F]" > } > > +ifcheck_ipv4() { > + local ifcheck=$1 > + local ifstr > + local counter=0 > + local procfile="/proc/net/dev" > + while read LINE > + do > + if [ $counter -ge 2 ] ; then > + ifstr=`echo $LINE | cut -d ':' -f 1` > + if [ "$ifstr" = "$ifcheck" ] ; then > + return 0 > + fi > + fi > + counter=`expr $counter + 1` > + done < $procfile > > local ifstr rest > while read ifstr rest ; do > [ "$ifstr" = "$ifcheck:" ] && return 0 > done > return 1 > > > + return 1 > +} > +ifcheck_ipv6() { > + local ifcheck="$1" > + local ifstr > + local procfile="/proc/net/if_inet6" > + while read LINE > + do > + ifstr=`echo $LINE | awk -F ' ' '{print $6}'` > + if [ "$ifstr" = "$ifcheck" ] ; then > + return 0 > + fi > + done < $procfile > > # btw, in bash, I tend to use _ as dummy > # much more readable > local tmp ifstr > while read tmp tmp tmp tmp tmp ifstr ; do > [ "$ifstr" = "$ifcheck" ] && return 0 > done > > + return 1 > +} > _______________________________________________________ > Linux-HA-Dev: [email protected] > http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev > Home Page: http://linux-ha.org/ Regards, -- Keisuke MORI _______________________________________________________ Linux-HA-Dev: [email protected] http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/
