On Mon, May 21, 2012 at 11:29:47AM -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 there was at least one bug (described below). Commit
> f66b87d (DESIGN: Document uses for flow cookies.) attempted 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>
"git am" says:
Applying: ofp-util: Clean up cookie handling.
/home/blp/ovs/.git/rebase-apply/patch:189: trailing whitespace.
/* Wildcard matching of the cookie is only supported through NXM.
*/
warning: 1 line adds whitespace errors.
This commit updates DESIGN, but it doesn't make the change that I
expected, which was to remove the paragraph that starts out "However,
unlike OpenFlow 1.1, ..." as suggested before at:
http://openvswitch.org/pipermail/dev/2012-March/016103.html
I think that, to remove that paragraph in DESIGN, we would have to
make these changes in ofproto.c:
- In modify_flows__(), set rule->flow_cookie to
fm->new_cookie only if the latter is nonzero.
- Update modify_flows_loose() and modify_flows_strict() to
call add_flows() if there was no match only if cookie_mask
was 0.
and that afterward the design would seem a little less haphazard.
The table in the comment ofp-util.h is nice, but I'd use something
other than "-" to indicate that a field is used, because I usually
expect "-" in a table to mean that a value *isn't* applicable. Maybe
you could write "used" or "<used>" in those fields?
In ofputil_decode_flow_mod(), the test of 'command' against OFPFC_ADD
should test "command & 0xff", otherwise it will fail for insertion of
a flow into a table other than 0 when the NXT_FLOW_MOD_TABLE_ID
extension is enabled.
I'm not sure that this comment in ofputil_decode_flow_mod() is as
clear as can be:
/* The "NXM_NX_COOKIE*" matches are not valid for flow
* additions. Additions must use the "cookie" field of
* the "nx_flow_mod" structure. */
Maybe:
/* Flow additions may only set a new cookie, not match
* on an existing cookie. */
Also the comment in ofputil_flow_mod_usable_protocols():
/* Wildcard matching of the cookie is only supported through NXM. */
would be improved by removing "Wildcard", I think, because OF1.0
doesn't even support exact matching of cookies. Also the line
following the comment looks like it is indented one more space than it
should be.
In fte_make_flow_mod(), there should be another space on this line:
fm.cookie_mask= htonll(0);
In parse_ofp_str(), flow dumps (command == -1) and flow deletions
can't specify a new cookie. We disallow matching on cookie for add,
should we disallow specifying a new cookie for those commands? On a
related note I'm not sure I understand this comment:
/* Set both the matching cookie and new cookie. We'll
* let later logic determine whether either don't apply
* based on the command. */
since it seems that we already know the command in use. (What later
logic does it refer to?)
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
