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

Ben Pfaff blp at nicira.com
Tue May 29 17:34:00 UTC 2012


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.

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.

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

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) {
> +            /* Flow additions may only set a new cookie, not match an
> +             * existing cookie. */
> +            return OFPERR_NXBRC_NXM_INVALID;
>          }
> +        fm->new_cookie = nfm->cookie;
>          fm->idle_timeout = ntohs(nfm->idle_timeout);
>          fm->hard_timeout = ntohs(nfm->hard_timeout);
>          fm->buffer_id = ntohl(nfm->buffer_id);

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

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.

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

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

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

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.



More information about the dev mailing list