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 <lgira...@nicira.com>
> Reported-by: Paul Ingram <p...@nicira.com>
> Signed-off-by: Justin Pettit <jpet...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
