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 <[email protected]>
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.
Ideally the code would also reject NXM_NX_COOKIE in the corner case of
flow adds.
I think it's about time to add a function-level comment to
nx_pull_match(). 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.
In the initial assignment to cookie and cookie_mask in
nx_pull_match(), I'd use htonll(0) instead of plain 0.
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.
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.
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.
Simiilarly in collect_rules_strict().
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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
