[ovs-dev] [multiple tables 4/8] ofproto: Make ->construct() and ->destruct() more symmetrical.

[Ethan Jackson]

Looks good,
Ethan

On Thu, Aug 4, 2011 at 16:29, Ben Pfaff <blp at nicira.com> wrote:
> The ofproto_provider's ->construct() was required to allocate and
> initialize the flow tables, but ->destruct() was not allowed to
> uninitialize and free them.  This arrangement is oddly asymmetrical (and
> undocumented), so this commit changes the code so that the client is
> responsible for both allocation and freeing.
>
> Reported-by: Hao Zheng <hzheng at nicira.com>
> CC: Hao Zheng <hzheng at nicira.com>
> ---
>  ofproto/ofproto-dpif.c     |    7 ++-----
>  ofproto/ofproto-provider.h |   36 ++++++++++++++++++++----------------
>  ofproto/ofproto.c          |   12 ++++++++++--
>  3 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2aea4e1..137d1f9 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -419,7 +419,7 @@ dealloc(struct ofproto *ofproto_)
>  }
>
>  static int
> -construct(struct ofproto *ofproto_)
> +construct(struct ofproto *ofproto_, int *n_tablesp)
>  {
>     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>     const char *name = ofproto->up.name;
> @@ -464,14 +464,11 @@ construct(struct ofproto *ofproto_)
>
>     list_init(&ofproto->completions);
>
> -    ofproto->up.tables = xmalloc(sizeof *ofproto->up.tables);
> -    classifier_init(&ofproto->up.tables[0]);
> -    ofproto->up.n_tables = 1;
> -
>     ofproto_dpif_unixctl_init();
>
>     ofproto->has_bundle_action = false;
>
> +    *n_tablesp = 1;
>     return 0;
>  }
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index be1e4de..985f112 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -255,17 +255,20 @@ struct ofproto_class {
>      * Construction
>      * ============
>      *
> -     * ->construct() should not modify most base members of the ofproto.  In
> -     * particular, the client will initialize the ofproto's 'ports' member
> -     * after construction is complete.
> -     *
> -     * ->construct() should initialize the base 'n_tables' member to the number
> -     * of flow tables supported by the datapath (between 1 and 255, inclusive),
> -     * initialize the base 'tables' member with space for one classifier per
> -     * table, and initialize each classifier with classifier_init.  Each flow
> -     * table should be initially empty, so ->construct() should delete flows
> -     * from the underlying datapath, if necessary, rather than populating the
> -     * tables.
> +     * ->construct() should not modify any base members of the ofproto.  The
> +     * client will initialize the ofproto's 'ports' and 'tables' members after
> +     * construction is complete.
> +     *
> +     * When ->construct() is called, the client does not yet know how many flow
> +     * tables the datapath supports, so ofproto->n_tables will be 0 and
> +     * ofproto->tables will be NULL.  ->construct() should store the number of
> +     * flow tables supported by the datapath (between 1 and 255, inclusive)
> +     * into '*n_tables'.  After a successful return, the client will initialize
> +     * the base 'n_tables' member to '*n_tables' and allocate and initialize
> +     * the base 'tables' member as the specified number of empty flow tables.
> +     * Each flow table will be initially empty, so ->construct() should delete
> +     * flows from the underlying datapath, if necessary, rather than populating
> +     * the tables.
>      *
>      * Only one ofproto instance needs to be supported for any given datapath.
>      * If a datapath is already open as part of one "ofproto", then another
> @@ -279,15 +282,16 @@ struct ofproto_class {
>      * Destruction
>      * ===========
>      *
> -     * ->destruct() must do at least the following:
> +     * If 'ofproto' has any pending asynchronous operations, ->destruct()
> +     * must complete all of them by calling ofoperation_complete().
>      *
> -     *   - If 'ofproto' has any pending asynchronous operations, ->destruct()
> -     *     must complete all of them by calling ofoperation_complete().
> -     *
> -     *   - If 'ofproto' has any rules left in any of its flow tables, ->
> +     * ->destruct() must also destroy all remaining rules in the ofproto's
> +     * tables, by passing each remaining rule to ofproto_rule_destroy().  The
> +     * client will destroy the flow tables themselves after ->destruct()
> +     * returns.
>      */
>     struct ofproto *(*alloc)(void);
> -    int (*construct)(struct ofproto *ofproto);
> +    int (*construct)(struct ofproto *ofproto, int *n_tables);
>     void (*destruct)(struct ofproto *ofproto);
>     void (*dealloc)(struct ofproto *ofproto);
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index f40f995..79c85a0 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -290,7 +290,9 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>  {
>     const struct ofproto_class *class;
>     struct ofproto *ofproto;
> +    int n_tables;
>     int error;
> +    int i;
>
>     *ofprotop = NULL;
>
> @@ -337,14 +339,20 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>     list_init(&ofproto->pending);
>     hmap_init(&ofproto->deletions);
>
> -    error = ofproto->ofproto_class->construct(ofproto);
> +    error = ofproto->ofproto_class->construct(ofproto, &n_tables);
>     if (error) {
>         VLOG_ERR("failed to open datapath %s: %s",
>                  datapath_name, strerror(error));
>         ofproto_destroy__(ofproto);
>         return error;
>     }
> -    assert(ofproto->n_tables > 0);
> +
> +    assert(n_tables >= 1 && n_tables <= 255);
> +    ofproto->n_tables = n_tables;
> +    ofproto->tables = xmalloc(n_tables * sizeof *ofproto->tables);
> +    for (i = 0; i < n_tables; i++) {
> +        classifier_init(&ofproto->tables[i]);
> +    }
>
>     ofproto->datapath_id = pick_datapath_id(ofproto);
>     VLOG_INFO("using datapath ID %016"PRIx64, ofproto->datapath_id);
> --
> 1.7.4.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>