[ovs-dev] [PATCH 2/3] ofproto: Do not add flow on flow mod if new_cookie is UINT64_MAX

[Ben Pfaff]

On Fri, Oct 12, 2012 at 04:31:52PM +0900, Isaku Yamahata wrote:
> On Fri, Oct 12, 2012 at 09:26:28AM +0900, Simon Horman wrote:
> > Internally a new_cookie value UINT64_MAX is used for
> > an OpenFlow 1.2 flow mod whose command is not Add.
> > Open Flow 1.2 does not permit adding flows for such commands.
> > Also, UINT64_MAX is a prohibited cookie value, and the
> > existing code created a flow with that value as for the cookie.
> 
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> > ---
> >  ofproto/ofproto.c |   15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 97e00ab..c712bf9 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -3081,6 +3081,17 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
> >      return error;
> >  }
> >  
> > +static enum ofperr
> > +modify_flows_add(struct ofproto *ofproto, struct ofconn *ofconn,
> > +                 const struct ofputil_flow_mod *fm,
> > +                 const struct ofp_header *request)
> > +{
> > +    if (fm->cookie_mask != htonll(0) || fm->new_cookie == htonll(UINT64_MAX)) {
> > +        return 0;
> > +    }
> 
> This depends on how to interpret the spec...
> 
> The OF1.2 says A.3.4.1
> > The value -1 (0xffffffffffffffff) is reserved and must not be used.
> 
> return an error when UINT64_MAX?
> But I can't find any suitable error code, but OFPFMFC_UNKNOWN.
> 
> Do we need to check if cookie mask is non zero?
> The spec says
> > This field is ignored by OFPC_ADD messages.

Simon's change only affects OpenFlow 1.2 OFPFC_MODIFY and
OFPFC_MODIFY_STRICT requests that do not match any existing flow in
the flow table.  For those requests, fm->new_cookie will always be
UINT64_MAX (it doesn't come from the OpenFlow message in this case).
The behavior that Simon implements is correct for OpenFlow 1.2, which
says that a modify request that matches no flows has no effect and is
not an error.

Simon's patch doesn't have any effect on OFPFC_ADD, because
modify_flows_add() won't be called for OFPFC_ADD.

I applied this to master.