From a6a62132df2741914c1e2564cffe33ef2f5ec421 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 9 Sep 2011 12:45:15 -0700 Subject: [PATCH] ofproto: Document that ->rule_construct() should uninitialize victim rules. The comments didn't say how this should work, so this clarifies it. --- ofproto/ofproto-provider.h | 11 ++++++++--- ofproto/ofproto.c | 25 +++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index c9d74eeb7..82844186f 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -686,7 +686,8 @@ struct ofproto_class { * * - 'rule' is replacing an existing rule in its flow table that had the * same matching criteria and priority. In this case, - * ofoperation_get_victim(rule) returns the rule being replaced. + * ofoperation_get_victim(rule) returns the rule being replaced (the + * "victim" rule). * * ->rule_construct() should set the following in motion: * @@ -706,9 +707,13 @@ struct ofproto_class { * - If the rule is valid, update the datapath flow table, adding the new * rule or replacing the existing one. * + * - If 'rule' is replacing an existing rule, uninitialize any derived + * state for the victim rule, as in step 5 in the "Life Cycle" + * described above. + * * (On failure, the ofproto code will roll back the insertion from the flow - * table, either removing 'rule' or replacing it by the flow that was - * originally in its place.) + * table, either removing 'rule' or replacing it by the victim rule if + * there is one.) * * ->rule_construct() must act in one of the following ways: * diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 849a376ae..15bcd1350 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -2828,8 +2828,29 @@ ofoperation_destroy(struct ofoperation *op) * indicate success or an OpenFlow error code (constructed with * e.g. ofp_mkerr()). * - * If 'op' is a "delete flow" operation, 'error' must be 0. That is, flow - * deletions are not allowed to fail. + * If 'error' is 0, indicating success, the operation will be committed + * permanently to the flow table. There is one interesting subcase: + * + * - If 'op' is an "add flow" operation that is replacing an existing rule in + * the flow table (the "victim" rule) by a new one, then the caller must + * have uninitialized any derived state in the victim rule, as in step 5 in + * the "Life Cycle" in ofproto/ofproto-provider.h. ofoperation_complete() + * performs steps 6 and 7 for the victim rule, most notably by calling its + * ->rule_dealloc() function. + * + * If 'error' is nonzero, then generally the operation will be rolled back: + * + * - If 'op' is an "add flow" operation, ofproto removes the new rule or + * restores the original rule. The caller must have uninitialized any + * derived state in the new rule, as in step 5 of in the "Life Cycle" in + * ofproto/ofproto-provider.h. ofoperation_complete() performs steps 6 and + * and 7 for the new rule, calling its ->rule_dealloc() function. + * + * - If 'op' is a "modify flow" operation, ofproto restores the original + * actions. + * + * - 'op' must not be a "delete flow" operation. Removing a rule is not + * allowed to fail. It must always succeed. * * Please see the large comment in ofproto/ofproto-provider.h titled * "Asynchronous Operation Support" for more information. */ -- 2.43.0