[ovs-dev] [PATCH] ofp-actions: Account for OF version when enforcing action consistency

[Simon Horman]

On Wed, Nov 06, 2013 at 09:11:08AM +0900, Simon Horman wrote:
> On Tue, Nov 05, 2013 at 09:49:45AM -0800, Jarno Rajahalme wrote:
> > 
> > On Nov 5, 2013, at 12:34 AM, Simon Horman <horms at verge.net.au> wrote:
> > 
> > > 3. OpenFlow1.3
> > > 
> > >   This does not have the implicit VLAN tag push behaviour of OpenFlow1.0.
> > > 
> > >   An MPLS push action pushes an MPLS LSE before any VLAN tags that are
> > >   present.
> > > 
> > > ofpact_get_SET_VLAN_VID(a)->push_vlan_if_needed and similar devices
> > > can't be used in order to effect this logic when ofpact_check__()
> > > is called via parse_ofp_str() as they are not filled in for
> > > specific OF versions at that time. Again, for OpenFlow1.0 and
> > > thus push_vlan_if_needed I believe this is cosmetic. But that is not
> > > the case for the MPLS portion of this change.
> > > 
> > > The net effect of this change on run-time is:
> > > 
> > > * To increase logging under some circumstances as ofpacts_check()
> > >  may now be called up to four times instead of up to twice
> > >  for each invocation of parse_ofp_str__()
> > > 
> > > * To disallow MPLS push actions on VLAN flows using OpenFlow1.3.
> > >  These did not comply to the OpenFlow1.3 as they would push an MPLS LSE
> > >  after any VLAN tags that were present. A subsequent patch will support
> > >  them in a manner that complies with the OpenFlow1.3.
> > 
> > Are there still patterns of inconsistent (MPLS) actions after the subsequent
> > patch? I.e., what would be the net effect of this patch after the MPLS push
> > actions on VLAN flows is fixed for OF 1.3?
> 
> >From the point of view of the specification, I think that inconsistency
> detection should be complete for MPLS after this patch. Of course it is
> quite possible there are scenarios that I have not considered.
> 
> There are some restrictions on combinations of push and pop MPLS actions
> with both themselves and other actions. These result from limitations in
> the current implementation and are not treated as inconsistent before or
> after this patch. Rather they are rejected doing translation.
> 
> It is planned for most if not all of these restrictions to be removed in
> the future using recirculation. However, I believe it would be not
> difficult to enhance ofpacts_check__() to detect these and treat them as
> inconsistent if it is desired. Basically duplicating part of the logic in
> do_xlate_actions().
> 
> As these restrictions are not part of the specification my current
> preference is to leave them out of ofpacts_check__().

Hi Jarno, Hi Ben,

I'm wondering if you have had any (further) thoughts on this.