> On Nov 24, 2015, at 5:02 PM, Joe Stringer <j...@ovn.org> wrote: > > On 24 November 2015 at 13:41, Jarno Rajahalme <ja...@ovn.org> wrote: >> Sometimes xlate_actions() fails due to too deep recursion, too many >> MPLS labels, or missing recirculation context. Make xlate_actions() >> clear out the produced odp actions in these cases to make it easy for >> the caller to install a drop flow (instead or installing a flow with >> partially translated actions). Also, return a specific error code, so >> that the error can be properly propagated where meaningful. >> >> Before this patch it was possible that the revalidation installed a >> flow with a recirculation ID with an invalid recirc ID (== 0), due to >> the introduction of in-place modification in commit 43b2f131a229 >> (ofproto: Allow in-place modifications of datapath flows). >> >> Signed-off-by: Jarno Rajahalme <ja...@ovn.org> > > Should this also set the error when receiving packets on a mirror port > in xlate_actions()? Or when receiving tagged VLAN traffic that doesn't > correspond to the port's vlan tag? Or when a group has no live bucket? > Are there any other cases that should also be covered? (I just scanned > across ofproto/ofproto-dpif-xlate.c looking for cases where we're > already logging that we drop the packet, but maybe there's a reasoning > behind not including these - if so, please enlighten me)
There may be a qualitative difference between a translation error and some of these. - Group has no live bucket: For example, all the links happen to be down. This is not a translation error, but a (hopefully transient) event in the network. However, bucket chaining exceeding the limit probably is an (internal) error, in the same way as any recursion depth is. - Mirror port and/or bundle not found: Not sure of this. There are three cases: - During xcache stats push: There is no translation context, so we can’t use this error mechanism here. This should be avoided, if we make the original translation causing this to fail, though. - Within normal action: According to the comments it most likely is a transient configuration error, but there is value in installing a drop flow instead of a possibly partially translated flow in this case too. Also, getting an explicit error response during tracing would be useful. There is a difference on failing just the normal action or the whole pipeline, though. - In mirror_ingress_packet(): It could be argued that the packet processing should continue even when ingress mirroring fails. I’ll post an incremental patch for these to make reviewing easier. Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
