> -----Original Message----- > From: Ben Pfaff <[email protected]> > Sent: Saturday, January 19, 2019 12:04 AM > To: Vishal Deep Ajmera <[email protected]> > Cc: [email protected]; Ashvin Lakshmikantha > <[email protected]> > Subject: Re: [ovs-dev] [PATCH v2] Support for match & set ICMPv6 reserved and > options type fields > > On Thu, Jan 17, 2019 at 11:14:26AM +0000, Vishal Deep Ajmera wrote: > > Currently OVS supports all ARP protocol fields as OXM match fields to > > implement the relevant ARP procedures for IPv4. This includes support > > for matching copying and setting ARP fields. In IPv6 ARP has been > > replaced by ICMPv6 neighbor discovery (ND) procedures, neighbor > > advertisement and neighbor solicitation. > > > > The support for ICMPv6 fields in OVS is not complete for the use cases > > equivalent to ARP in IPv4. OVS lacks support for matching, copying and > > setting the “ND option type” and “ND reserved” fields. Without these > > user cannot implement all ICMPv6 ND procedures for IPv6 support. > > > > This commit adds additional OXM fields to OVS for ICMPv6 “ND option > > type“ and ICMPv6 “ND reserved” using the OXM extension mechanism. This > > allows support for parsing these fields from an ICMPv6 packet header > > and extending the OpenFlow protocol with specifications for these new > > OXM fields for matching, copying and setting. > > > > Signed-off-by: Vishal Deep Ajmera <[email protected]> > > Co-authored-by: Ashvin Lakshmikantha > > <[email protected]> > > Signed-off-by: Ashvin Lakshmikantha > > <[email protected]> > > Thanks for working to make OVS better! > > It looks like miniflow_extract() calls data_pull() for the RSO flags field > without > first checking to see whether the message is long enough. > This is dangerous. > Thanks Ben for reviewing this patch.
I have refactored this code addressing your comments in V3. > This cast should not be needed: > > rso_flags = (uint32_t *) data_pull(&data, > &size, sizeof(uint32_t)); > > The code for populating opt_type[0] and opt_type[1] into the miniflow is > confusing. It looks like only one of these can be nonzero, and if either one > is > present then we put it in the same spot (in the tcp_flags)? If that's so, > then why > bother distinguishing them during parsing? (And how should flow_compose_l4() > know where to put them? > Currently it doesn't bother with them at all.) > Fixed in V3. > The comments on struct flow should describe the new uses of tcp_flags and > igmp_group_ip4. > Fixed in V3. > Please add an item to describe the new feature in NEWS. > Fixed in V3. > Please add parsing tests to odp.at. > I will add parsing test in subsequent patch V4. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
