[ovs-dev] [PATCH 1/2] ofctl: This patch add support for setting the first egress table for egress processing.

[Ben Pfaff]

I don't think there's any need to check whether it's an egress table
along that path.  ovs-ofctl can try to insert the flow either way.

Regardless, a global variable is unacceptable.

On Thu, Jan 14, 2016 at 07:25:20PM +0530, niti Rohilla wrote:
> Hi Ben,
> 
> With reference to global variable "egress_table_id", I have a doubt. I can
> send '0' as the default value from ofp_print_flow_mod() to
> ofpact_check__(). Right now, ofpact_check__() access the first egress table
> id from global variable "egress_table_id", if I remove the global variable
> then egress table id has to sent be from the all the functions that
> internally call ofpact_check__(). For example,
> 
> ofctl_add_flow()->ofctl_flow_mod()->parse_ofp_flow_mod_str()->parse_ofp_str()->parse_ofp_str__()->ofpacts_check()->ofpact_check__()
> 
> In this hierarchy, first I have to get the first egress table from the
> switch using fetch_table_features() and egress table id has to be sent from
> ofctl_add_flow() and I need to change the definition of all the functions
> to include one more parameter to send the first egress table id to
> ofpact_check__().
> 
> Functions that modify the flows needs to first query the first egress table
> from the switch and send it to ofpact_check__ and functions that only parse
> flows for e.g ofctl_parse_flows() can send the default value i.e., but
> definition of all the functions included in the hierarchy has to be changed
> to include one more parameter. It requires changing the definition of many
> functions.
> 
> That is why I have used the global variable. Please suggest which approach
> should be followed.
> 
> Thanks
> Niti Rohilla
> 
> On Wed, Jan 13, 2016 at 10:46 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> > On Wed, Jan 13, 2016 at 03:18:02PM +0530, niti Rohilla wrote:
> > > I can incorporate the egress table implemenation in OF1.3 table features
> > > code and will remove the implementation added for OF1.5.
> >
> > Good.
> >
> > > I will replace the pad[5] field in of13_table_features structure with
> > > command and features field as specified in OF1.5.1 specifications. I can
> > > also remove the ofp15_table_feature_prop_type enums, OF1.5 protocol
> > > definition and code implementation added for the same. Kindly suggest is
> > > this approach is fine.
> >
> > OK.
> >
> > > I can replace the hard coded values for OFPAT_ with OFPACT_OUTPUT and
> > > OFPACT_GROUP.
> >
> > OK.
> >
> > > I have created a global variable "egress_table_id" that contain the table
> > > id of first egress table. This variable is declared as global because I
> > > need the first egress table id to add the checks in ofp-action.c file (in
> > > ofpact_check__()) to prevent adding the output and group action in action
> > > set of egress tables and to disallow the ingress flow tables to direct
> > the
> > > packet to egress flow tables via GOTO_TABLE instruction.
> > >
> > > We need access to the ofproto structure in ofpact_check__().
> > > ofpact_check__() is called from various places. One scenraio is,
> > > ofp_print_flow_mod() -> ofputil_decode_flow_mod() ->
> > > ofpacts_check_consistency() -> ofpacts_check() -> ofpact_check__(). We
> > > don't have access to ofproto strcuture in ofp_print_flow_mod(). Please
> > > suggest if there is any other possible way through which I can get the
> > > first egress table id (stored in ofproto strcuture) in ofp-actions.c
> > file.
> >
> > ofp_print_flow_mod() doesn't need to know the real egress_table_id, so
> > you can use some default that will accept all flows.
> >
> > > I want to clarify one more doubt. In the 2nd patch, when a packet start
> > > egress processing then output port is added to the action_set and we
> > should
> > > also set the actset_output variable. So can we directly add the output
> > port
> > > in action_set and actset_output or we should call xlate_write_action() to
> > > add the output port in action_set and actset_output variable. Please
> > > suggest which approach should be followed for this.
> >
> > I would add it directly.
> >