[ovs-dev] [PATCH 3/3] ofproto: Centralize action checking, doing it at decode time.

Ben Pfaff blp at nicira.com
Sat Nov 2 05:16:47 UTC 2013


On Fri, Nov 01, 2013 at 03:58:52PM -0700, Jarno Rajahalme wrote:
> 
> On Nov 1, 2013, at 9:43 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > Jarno pointed out that modify_flows__() didn't really need to check every
> > instance of the flow separately.  After some further investigation I
> > decided that this was even more of an improvement.
> > 
> 
> Indeed! Check the actions right after they are decoded rather than
> multiple places afterwards.
> 
> One question below, otherwise:
> 
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

Thanks!

> > @@ -1700,7 +1701,7 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
> >     case OFPACT_WRITE_ACTIONS: {
> >         struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
> >         return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
> > -                             flow, max_ports, table_id, false);
> > +                             flow, false, max_ports, table_id, n_tables);
> >     }
> > 
> >     case OFPACT_WRITE_METADATA:
> > @@ -1714,11 +1715,14 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
> >         return 0;
> >     }
> > 
> > -    case OFPACT_GOTO_TABLE:
> > -        if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
> > +    case OFPACT_GOTO_TABLE: {
> > +        uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
> > +        if ((table_id != 255 && goto_table <= table_id)
> 
> Can the table id really be 255 at this point? I just learned that 255
> is not allowed in OF 1.1, and in OF1.2(+) it is allowed only on
> deletes, which should have no actions.

I think there's a bug elsewhere that might allow it to squeak through.
When parse_ofp_str__() parses a flow it can't always tell what version
of OpenFlow is going to be used in advance, so it can provide table ==
255 even with a flow that contains a Goto-Table.
(ofputil_encode_flow_mod() will later squash 255 into 0 for
transmission.)  I guess we can or should fix that.



More information about the dev mailing list