[ovs-dev] [PATCH V2] ofproto-dpif-xlate: Incorrect handling of errors in group action processing

[Vishal Deep Ajmera]

Thanks Ben for reviewing the patch. I have sent V3 patch after rebasing with master.

Warm Regards,
Vishal Ajmera

-----Original Message-----
From: Ben Pfaff [mailto:blp at ovn.org] 
Sent: Friday, January 05, 2018 2:29 AM
To: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
Cc: dev at openvswitch.org; Keshav Gupta <keshav.gupta at ericsson.com>
Subject: Re: [PATCH V2] ofproto-dpif-xlate: Incorrect handling of errors in group action processing

On Mon, Dec 11, 2017 at 11:54:33AM +0000, Vishal Deep Ajmera wrote:
> As per OpenFlow v1.3 specification, when an action list contains a 
> group action a copy of the packet is passed to the group for 
> processing by the group. This means that if there is an error 
> encountered during group processing, only the copy of packet should be 
> dropped, but subsequent actions in the action list should be executed on the original packet.
> 
> Additionally, if the group type is "ALL", each action bucket of the 
> group should process a copy of the packet. If there is an error while 
> processing one bucket other buckets should still be processed.
> 
> Example 1:
> table=0,in_port=tap0 actions=output:tap1,group:10,output:tap2
> 
> Even if any error is encountered while processing the group action, 
> the packet should still be forwarded to ports tap1 and tap2.
> 
> Example 2:
> group_id=1,type=all,bucket=actions=output:tap1,bucket=actions=encap(et
> h)
> 
> Even if processing the action in the second bucket fails because the 
> packet already has an Ethernet header, the other copy of the packet 
> should still be processed by the first bucket and output to port tap1.
> 
> Currently the error handling in OVS does not comply with those rules. 
> When any group bucket execution fails the error is recorded in the 
> so-called "translation context" which is global for the processing of 
> the original packet. Once an error is recorded, OVS skips processing 
> subsequent buckets and installs a drop action in the datapath even if 
> parts of the action list were previously processed successfully.
> 
> This patch clears the error flag after any bucket of a group is executed.
> This way the error encountered in processing any bucket of the group 
> will not impact other actions of action-list or other buckets of the group.
> 
> Errors which are system limits to protect translation from taking too 
> long time or too much space are not cleared. Instead drop action is 
> installed for them.
> 
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Signed-off-by: Keshav Gupta <keshav.gupta at ericsson.com>

Seems reasonable to me, but it fails to apply due to white space damage.  Can you resubmit a clean copy?

Thanks,

Ben.