On Mon, May 18, 2015 at 04:10:24PM -0700, Jarno Rajahalme wrote:
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
You know, we could make evictions reversible, since after all we have a
way to mark rules as to-be-deleted and then not delete them. I don't
know whether it's worthwhile.
The handling of "conflicts" seems, well, conflicted. I see one comment
that says we check for conflicts at bundle add time:
/* Bundle conflicts. A new message being added into the bundle is deemed to be
* in conflict, if it would delete or modify a flow or a port that was added or
* modified by an earlier message in the bundle.
*
* We check for this at the bundle add time, and rely on the messages not being
* in conflict at the bundle commit time. This simplifies the state management
* requirements for rolling back failing commits.
and another that says we check for them at bundle commit time:
/* Check for conflicts. */
/* The spec says to return OFPBFC_MSG_CONFLICT when processing the
* BUNDLE_ADD, but with big bundles that may be very expensive. It may be
* a lot cheaper to check that mods/deletes do not target any of the rules
* added/modded by the current bundle at the commit time. */
but I couldn't find any actual code that returns OFPBFC_MSG_CONFLICT
anywhere.
Worse, I couldn't find any good definition of a "conflict" in the
OpenFlow specification. It just says:
If the message in the request is incompatible with another message
already stored in the bun dle, the switch must refuse to add the
message to the bundle and send an ofp_error_msg with
OFPET_BUNDLE_FAILED type and OFPBFC_MSG_CONFLICT code.
I think we might have to ask for clarification.
I wouldn't use ovs_assert() like this:
ovs_assert(classifier_remove(&table->cls, &new_rule->cr));
because defining NDEBUG will then delete this code. Instead:
if (!classifier_remove(&table->cls, &new_rule->cr)) {
OVS_NOT_REACHED();
}
I guess that the following special case in handle_bundle_control() and
handle_flow_mod__() could be moved into ofputil_decode_flow_mod(), so
that we could assume at execution time that the command is a valid one:
+ default:
+ if (fm->command > 0xff) {
+ VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
+ "flow_mod_table_id extension is not enabled",
+ ofproto->name);
+ }
+ error = OFPERR_OFPFMFC_BAD_COMMAND;
+ break;
I think that some code duplication could be eliminated by writing
handle_flow_mod__() in terms of do_bundle_flow_mod_begin() and
do_bundle_flow_mod_finish().
Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
