From 073e2a6f2383cf7c17f78b8fa29e6e5293f60ba0 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 28 Jul 2011 14:19:52 -0700 Subject: [PATCH] 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 CC: Hao Zheng --- ofproto/ofproto-dpif.c | 7 ++----- ofproto/ofproto-provider.h | 38 +++++++++++++++++++++----------------- ofproto/ofproto.c | 12 ++++++++++-- 3 files changed, 33 insertions(+), 24 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 4cad7af09..e94cd8952 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 be1e4de6c..985f1121f 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 6203676e2..4d4c23201 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); -- 2.43.0