[ovs-dev] [PATCH] ofp-util: Clean up cookie handling.

[Justin Pettit]

On May 29, 2012, at 10:34 AM, Ben Pfaff wrote:

> On Fri, May 25, 2012 at 06:48:40PM -0700, Justin Pettit wrote:
>> Commit e72e793 (Add ability to restrict flow mods and flow stats
>> requests to cookies.) modified cookie handling.  Some of its behavior
>> was unintuitive and there was at least one bug (described below).
>> Commit f66b87d (DESIGN: Document uses for flow cookies.) attempted to
>> document a clean design for cookie handling.  This commit updates the
>> DESIGN document and brings the implementation in line with it.
>> 
>> In commit e72e793, the code that handled processing OpenFlow flow
>> modification requests set the cookie mask to exact-match.  This seems
>> reasonable for adding flows, but is not correct for matching, since
>> OpenFlow 1.0 doesn't support matching based on the cookie.  This commit
>> changes to cookie mask to fully wildcarded, which is the correct
>> behavior for modifications and deletions.  It doesn't cause any problems
>> for flow additions, since the mask is ignored for that operation.
>> 
>> Bug #9742
>> 
>> Reported-by: Luca Giraudo <lgiraudo at nicira.com>
>> Reported-by: Paul Ingram <paul at nicira.com>
>> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> 
> Thank you.  I only have small comments.

Oh, thank Christ.  :-)

> In DESIGN below, I think that the new described behavior is actually
> the same as OpenFlow 1.1, just described in different words:
>> +        - But, unlike OpenFlow 1.1, OFPC_MODIFY and OFPFC_MODIFY_STRICT
>> +          add a new flow if there is no match and the mask is zero (or
>> +          not given).  This provides OpenFlow 1.0-like behavior when a
>> +          mask is not specified.

You're right.  I still want to highlight that you don't need to specify a mask, so I've reworded it to the following:

        - Like OpenFlow 1.1, OFPC_MODIFY and OFPFC_MODIFY_STRICT add a
          new flow if there is no match and the mask is zero (or not
          given).

> In ofp_print_flow_mod(), the old version printed fm.cookie, the new
> version prints fm.new_cookie.  Shouldn't we print both (potentially)?

How about the following right after we print fm.new_cookie?

    if (fm.cookie_mask != htonll(0)) {
        ds_put_format(s, "cookie:0x%"PRIx64"/0x%"PRIx64" ",
                ntohll(fm.cookie), ntohll(fm.cookie_mask));
    }    

> I think that the precedence of == is higher than &, so that we need
> parentheses around "command & 0xff" below:
>> +        if (command & 0xff == OFPFC_ADD && fm->cookie_mask) {

Whoops.  Thank you.

> Should add "\" before each "-" below (not a new problem):
>> +supplied for the \fBdel-flows\fR, \fBmod-flows\fR, \fBdump-flows\fR, and
>> +\fBdump-aggregate\fR commands to limit matching cookies.  A 1-bit in
>> +\fImask\fR indicates that the corresponding bit in \fIcookie\fR must

Done.

> Also in "-1" here (though not in "0-bit"):
>> +match exactly, and a 0-bit wildcards that bit.  A mask of -1 may be used
>> +to exactly match a cookie.

Done.

> Also here:
>> +The \fBmod-flows\fR command can update the cookies of flows that
>> +match a cookie by specifying the \fIcookie\fR field twice (once with a
>> +mask for matching and once without to indicate the new value):
>> +.RS

Done.

> Also here:
>> +.IP \fBcookie=1/-1,cookie=2\fR
>> +Update cookies with a value of 1 to 2.

Done.

> The following example is rather confusing.  "cookie=0/0" is a no-op
> AFAIK, is there any reason to ever write that explicitly?  Also the
> wording is ambiguous.  I'd say "Change all flows' cookies to 1." or
> something else that is equally clear:
>> +.IP \fBcookie=0/0,cookie=1\fR
>> +Update all flows with a cookie of 1.
>> +.RE

I did it to provide more examples, but it probably was just confusing.  I removed "cookie=0/0" and reordered the examples, which I think makes things clearer.  I also removed the ambiguity.

> It might be wise to give complete ovs-ofctl commands above, instead of
> just excerpts, and to describe the full effect of the command instead
> of just the effect on cookies (e.g. a complete "mod-flows" would also
> change the flows' actions).  Otherwise readers might still end up
> misunderstanding.

Okay, combined with the previous comment, it now looks like this:

.IP "\fBovs\-ofctl mod\-flows br0 cookie=1,action=normal\fR"
Change all flows' cookies to 1 and change their actions to \fBnormal\fR.
.IP "\fBovs\-ofctl mod\-flows br0 cookie=1/\-1,cookie=2,action=normal\fR"
Update cookies with a value of 1 to 2 and change their actions to 
\fBnormal\fR.

If all those changes look reasonable, I'll push it.

--Justin