> On Nov 4, 2015, at 11:31 AM, Joe Stringer <[email protected]> wrote:
>
> On 28 October 2015 at 20:07, Jarno Rajahalme <[email protected]
> <mailto:[email protected]>> wrote:
>> Sometimes xlate_actions() fails due to too deep recursion, too many
>> MPLS labels, or missing recirculation context. Make xlate_actions()
>> fail in these circumstances, so that we can:
>>
>> - not install a datapath flow if xlate_actions() fails, and
>>
>> - delete an existing datapath flow, rather than replacing it with an
>> incomplete flow, when the revalidation fails due to failing
>> xlate_actions.
>>
>> Before this action 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 <[email protected]>
>
> Could you also describe how this affects upcall translation? (Does it
> just fail to install the flow, or will it install a new flow drops the
> traffic which hits the offending parts of translation?)
>
In the current form it just fails the upcall processing, which means no flow is
created and the packet is dropped. In master, a flow with partially translated
actions (up to the point of failure) is executed and installed. I see that it
may be more prudent to install a drop flow instead to reduce stress on upcalls.
>
>> @@ -1066,9 +1067,14 @@ upcall_xlate(struct udpif *udpif, struct upcall
>> *upcall,
>>
>> upcall->dump_seq = seq_read(udpif->dump_seq);
>> upcall->reval_seq = seq_read(udpif->reval_seq);
>> - xlate_actions(&xin, &upcall->xout);
>> + error = xlate_actions(&xin, &upcall->xout);
>> upcall->xout_initialized = true;
>>
>> + if (error) {
>> + ofpbuf_init(&upcall->put_actions, 0);
>> + return error;
>> + }
>> +
>> /* Special case for fail-open mode.
>> *
>> * If we are in fail-open mode, but we are connected to a controller too,
>
> Isn't upcall->put_actions initialized in upcall_receive()?
>
I missed this due to another instance of put_actions init a bit further down
(which I now replaced with a comment).
>
>> @@ -4988,7 +4991,8 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct
>> flow *flow,
>> struct ds *ds)
>> {
>> struct trace_ctx trace;
>> -
>> + int error;
>> +
>
> Whitespace.
>
Fixed.
>
>> @@ -5007,29 +5011,32 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct
>> flow *flow,
>> trace.xin.resubmit_hook = trace_resubmit;
>> trace.xin.report_hook = trace_report_valist;
>>
>> - xlate_actions(&trace.xin, &trace.xout);
>> -
>> - ds_put_char(ds, '\n');
>> - trace_format_flow(ds, 0, "Final flow", &trace);
>> - trace_format_megaflow(ds, 0, "Megaflow", &trace);
>> + error = xlate_actions(&trace.xin, &trace.xout);
>> + if (error) {
>> + ds_put_format(ds, "\nTranslation failed with errno %d!", error);
>> + } else {
>> + ds_put_char(ds, '\n');
>> + trace_format_flow(ds, 0, "Final flow", &trace);
>> + trace_format_megaflow(ds, 0, "Megaflow", &trace);
>
> Does this completely lose the context about where the translation
> failed? Is it possible/worthwhile to still try to print as much info
> as we can, up until the point that translation failed?
>
> The errno could also be translated using ovs_strerror().
I changed this to check the error only after printing the final flow and
megaflow. I also now use the report_hook to report the error to the trace
output as it happens, in addition to using the ovs_strerror().
I’ll post a new version with the NAT series, as it depends on this.
Thanks for the review!
Jarno
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
