> On Nov 25, 2015, at 11:23 AM, Jarno Rajahalme <ja...@ovn.org> wrote: > >> >> On Nov 25, 2015, at 11:11 AM, Jarno Rajahalme <ja...@ovn.org >> <mailto:ja...@ovn.org>> wrote: >> >> >>> On Nov 25, 2015, at 10:52 AM, Joe Stringer <j...@ovn.org >>> <mailto:j...@ovn.org>> wrote: >>> >>> On 25 November 2015 at 10:31, Jarno Rajahalme <ja...@ovn.org >>> <mailto:ja...@ovn.org>> wrote: >>>> >>>>> On Nov 24, 2015, at 5:02 PM, Joe Stringer <j...@ovn.org >>>>> <mailto:j...@ovn.org>> wrote: >>>>> >>>>> On 24 November 2015 at 13:41, Jarno Rajahalme <ja...@ovn.org >>>>> <mailto: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 <mailto: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) >>>> >>>> No reasoning for missing those, I just did not notice them. Thanks for >>>> pointing them out. >>> >>> OK, I thought it may have been something like "expected errors" vs. >>> "unexpected errors". >> >> Looking into these I noticed this to be the case. Must discern whether to >> fail just the individual action v.s. the whole pipeline. >> > > How about this incremental to cover two cases here (rest are “expected > errors” IMO): > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 36a6fbc..2908339 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -336,6 +336,10 @@ const char *xlate_strerror(enum xlate_error error) > return "Recirculation conflict"; > case XLATE_TOO_MANY_MPLS_LABELS: > return "Too many MPLS labels"; > + case XLATE_BUCKET_CHAINING_TOO_DEEP: > + return "Bucket chaining too deep"; > + case XLATE_NO_INPUT_BUNDLE: > + return "No input bundle"; > } > return "Unknown error"; > } > @@ -1444,10 +1448,9 @@ bucket_is_alive(const struct xlate_ctx *ctx, > struct ofputil_bucket *bucket, int depth) > { > if (depth >= MAX_LIVENESS_RECURSION) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - > - VLOG_WARN_RL(&rl, "bucket chaining exceeded %d links", > - MAX_LIVENESS_RECURSION); > + XLATE_REPORT_ERROR(ctx, "bucket chaining exceeded %d links", > + MAX_LIVENESS_RECURSION); > + ctx->error = XLATE_BUCKET_CHAINING_TOO_DEEP; > return false; > } > > @@ -2323,7 +2326,8 @@ xlate_normal(struct xlate_ctx *ctx) > in_xbundle = lookup_input_bundle(ctx->xbridge, flow->in_port.ofp_port, > ctx->xin->packet != NULL, &in_port); > if (!in_xbundle) { > - xlate_report(ctx, "no input bundle, dropping"); > + XLATE_REPORT_ERROR(ctx, "no input bundle, dropping"); > + ctx->error = XLATE_NO_INPUT_BUNDLE; > return; > } > > > The last one is debatable, as setting the error fails the whole translation > rather than just the normal action. But this is most likely an configuration > error, so maybe failing the whole pipeline (and installing a drop flow) is > the right thing to do here? >
I’d like to push this patch today, either with or without the above incremental, if possible, and then post a new version of the NAT series re-based on this. Ben already Acked the 2nd patch. Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
