[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