Thanks, I pushed these to master.
On Fri, Sep 09, 2011 at 12:53:43PM -0700, Hao Zheng wrote:
> Looks good.
>
> On Fri, Sep 9, 2011 at 12:46 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > Hao pointed out out-of-band that this version still wasn't right.
> > Third time's the charm?
> >
> > Additional incremental:
> >
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index c0b3474..15bcd13 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -2844,8 +2844,7 @@ ofoperation_destroy(struct ofoperation *op)
> > * 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, most notably by calling its
> > ->rule_destruct()
> > - * and ->rule_dealloc() function.
> > + * and 7 for the new rule, calling its ->rule_dealloc() function.
> > *
> > * - If 'op' is a "modify flow" operation, ofproto restores the original
> > * actions.
> >
> > Full revised commit:
> >
> > --8<--------------------------cut here-------------------------->8--
> >
> > From: Ben Pfaff <b...@nicira.com>
> > 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 c9d74ee..8284418 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 849a376..15bcd13 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. */
> > --
> > 1.7.4.4
> >
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
