> On Tue, Feb 13, 2018 at 03:43:51PM +0100, Lorenzo Bianconi wrote: >> icmp4 action is used to replace the IPv4 packet been processed with >> an ICMPv4 packet initialized based on incoming IPv4 one. >> Ethernet and IPv4 fields not listed are not changed: >> - ip.proto = 1 (ICMPv4) >> - ip.frag = 0 (not a fragment) >> - icmp4.type = 3 (destination unreachable) >> - icmp4.code = 1 (host unreachable) >> Prerequisite: ip4 >> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com> > > Thanks for the patch! It looks really good.
Hi Ben, thanks for the review. I will add your comments in v2. Just a question inline. Regards, Lorenzo > > I'm appending a few changes I suggest folding in: usually we write hex > numbers preceded by 0x (which the # flag does), and because the two > lines that get deleted aren't really needed. > are you referring to the ethertype value printed in pinctrl_handle_icmp()? > There's some inconsistency in naming between "icmp" and "icmp4". I > guess I like icmp4 better because it seems likely that we'll eventually > add an icmp6. > > Did you consider the tos value passed to packet_set_ipv4()? I'd be > somewhat inclined to use ip_flow->nw_tos. > > The documentation says that IPv4 values not listed are not changed, so > either we should update the documentation or the code regarding TTL and > TOS. > > I'd add some parsing tests to the "ovn -- action parsing" test in > tests/ovn.at. > > Thanks, > > Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
