[ovs-dev] [PATCH 2/2] Add ability to restrict flow mods and flow stats requests to cookies.
Justin Pettit
jpettit at nicira.com
Wed Dec 28 05:14:35 UTC 2011
On Dec 26, 2011, at 3:49 PM, Ben Pfaff wrote:
> On Fri, Dec 23, 2011 at 01:53:14PM -0800, Justin Pettit wrote:
>> With this commit, it is possible to limit flow deletions and
>> modifications to specific cookies. It also provides the ability to
>> dump flows based on their cookies.
>>
>> Signed-off-by: Justin Pettit <jpettit at nicira.com>
>
> This looks very complete. I have several comments but they are all
> very minor.
>
> The comment on NXM_NX_COOKIE in nicira-ext.h says that "This match is
> otherwise ignored" but the implementation rejects it in situations
> where it isn't understood. I think it would be better to improve the
> comment than to change the implementation.
Okay. I pointed out that they're not allowed.
> Ideally the code would also reject NXM_NX_COOKIE in the corner case of
> flow adds.
Sounds reasonable. Done.
> I think it's about time to add a function-level comment to
> nx_pull_match().
Done. Here's what I wrote:
/* Parses the nx_match formatted match description in 'b' with length
* 'match_len'. The results are stored in 'rule', which is initialized
* with 'priority'. If 'cookie' and 'cookie_mask' contain valid
* pointers, then the cookie and mask will be stored in them if a
* "NXM_NX_COOKIE*" match is defined. Otherwise, 0 is stored in both.
*
* Returns 0 if successful, otherwise an OpenFlow error code.
*/
> I think that nx_pull_match() should probably ensure
> that its cookie and cookie_mask arguments are both null or both
> nonnull with an assertion like
> assert((cookie != NULL) == (cookie_mask != NULL));
> and then just check one of them later on.
Sounds good.
> In the initial assignment to cookie and cookie_mask in
> nx_pull_match(), I'd use htonll(0) instead of plain 0.
Okay.
> The changes to parse_nxm_field_name() look wrong. I think it will
> accept, e.g., "NXM" or "N" or even "" as a substitute for
> "NXM_NX_COOKIE". Also, I think that the new "if" shouldn't be inside
> the loop.
Good points. Cleaned up.
> I think that ofputil_decode_flow_mod handles OFPFC_ADD inconsistently
> for OF1.0 and NXM: for OF1.0, it sets cookie_mask to 0, for NXM it
> sets cookie_mask to UINT64_MAX. I think that a consistent value would
> make more sense; I don't think the particular value matters.
Okay, I went with UINT64_MAX, since it seems more conceptually correct.
> In collect_rules_loose(), elsewhere we tend to write
> && (rule->flow_cookie & cookie_mask)
> == (cookie & cookie_mask)) {
> as:
> && !((rule->flow_cookie ^ cookie) & cookie_mask)
> in theory saving one bit-op.
It also takes one less line. Thanks.
> Simiilarly in collect_rules_strict().
Updated.
> The documentation says:
> An opaque identifier called a cookie can be associated with a
> flow:
> but in fact every flow has a cookie, it's just 0 by default, so maybe
> some other wording would be better. But I don't think that the
> wording later:
> If this field is omitted, a default cookie value of 0 is used.
> is that great, because just before that we're talking about "querying,
> modifying, and deleting flows" and there's no default cookie value at
> all for any of those operations, only for adding flows. It might be a
> good idea to talk about the meaning of the cookie for add operations
> separately from the meaning for other operations, since in fact
> they're quite different.
That's true. I updated the documentation to the following:
An opaque identifier called a cookie can be used as a handle to iden‐
tify a set of flows:
cookie=value[/mask]
A cookie can be associated with a flow using the add-flow and
add-flows commands. value can be any 64-bit number and need not
be unique among flows. If this field is omitted, a default
cookie value of 0 is used.
When using NXM, the cookie can be used as a handle for querying,
modifying, and deleting flows. In addition to value, an
optional mask may be supplied for the del-flows, mod-flows,
dump-flows, and dump-aggregate commands to limit matching cook‐
ies. A 1-bit in mask indicates that the corresponding bit in
cookie must match exactly, and a 0-bit wildcards that bit.
Thanks for the feedback! I've updated the NEWS section and pushed.
--Justin
More information about the dev
mailing list