[ovs-dev] [PATCH 2/2] ofproto: Document that ->rule_construct() should uninitialize victim rules.

[Hao Zheng]

Looks good.

On Fri, Sep 9, 2011 at 12:46 PM, Ben Pfaff <blp at 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 <blp at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20110909/54690f0c/attachment-0003.html>