Looks good, Ethan
On Thu, Aug 4, 2011 at 16:29, Ben Pfaff <[email protected]> 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 <[email protected]> > CC: Hao Zheng <[email protected]> > --- > 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
