On May 21, 2012, at 4:00 PM, Ben Pfaff wrote: > "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.
Fixed. > 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. After many off-line discussions, I think we came up with a solution we both found satisfactory. I'll send an updated version of this patch soon, which contains lots of details on the design. > 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? Agreed. I tried a few different ones and settled on one that was not fantastic. I've changed it to "<used>" and "-" for used and invalid, respectively. > 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. Thanks for catching that. > 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. */ Updated. > 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. Done. > In fte_make_flow_mod(), there should be another space on this line: > fm.cookie_mask= htonll(0); Fixed. > 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? I changed the logic around so that only adds and modifies are allowed to update the cookie without a fatal error. > 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?) The protocol version wasn't available, and I was planning to disambiguate modifications later based on the version. Regardless, the new version changes all of that, and this comment was dropped. Thanks for the review. I'll send out v2 shortly. --Justin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
