ofproto: Make ->construct() and ->destruct() more symmetrical.

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@nicira.com>
CC: Hao Zheng <hzheng@nicira.com>

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4cad7af..e94cd89 100644 (file)
--- 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 (file)
--- 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 6203676..4d4c232 100644 (file)
--- 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);