Looks much cleaner, thank you. Ethan
On Tue, Dec 27, 2011 at 13:23, Ben Pfaff <[email protected]> wrote: > I'd like to change ->dpif_flow_put() and ->dpif_execute() in the dpif > provider to take the structures of the same names as parameters, instead of > passing them discrete parameters, because this seems like a more sensible > way to do things internally than to have two different ways to pass the > parameters. It might even simplify code slightly. But ->flow_put() and > ->execute() wouldn't want the 'type' (because it's implied by the function > being called) or 'error' (because it would be the same as the return > value). Although of course they could just ignore those members, it seems > slightly cleaner to omit them entirely, as this change allows. > > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/dpif-linux.c | 18 ++++++++---------- > lib/dpif-provider.h | 2 +- > lib/dpif.c | 24 ++++++++++++------------ > lib/dpif.h | 20 +++++++------------- > ofproto/ofproto-dpif.c | 18 ++++++++---------- > 5 files changed, 36 insertions(+), 46 deletions(-) > > diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c > index 317274e..e232278 100644 > --- a/lib/dpif-linux.c > +++ b/lib/dpif-linux.c > @@ -884,7 +884,7 @@ dpif_linux_execute(struct dpif *dpif_, > } > > static void > -dpif_linux_operate(struct dpif *dpif_, union dpif_op **ops, size_t n_ops) > +dpif_linux_operate(struct dpif *dpif_, struct dpif_op **ops, size_t n_ops) > { > struct dpif_linux *dpif = dpif_linux_cast(dpif_); > struct nl_transaction **txnsp; > @@ -894,10 +894,10 @@ dpif_linux_operate(struct dpif *dpif_, union dpif_op > **ops, size_t n_ops) > txns = xmalloc(n_ops * sizeof *txns); > for (i = 0; i < n_ops; i++) { > struct nl_transaction *txn = &txns[i]; > - union dpif_op *op = ops[i]; > + struct dpif_op *op = ops[i]; > > if (op->type == DPIF_OP_FLOW_PUT) { > - struct dpif_flow_put *put = &op->flow_put; > + struct dpif_flow_put *put = &op->u.flow_put; > struct dpif_linux_flow request; > > dpif_linux_init_flow_put(dpif_, put->flags, put->key, > put->key_len, > @@ -909,7 +909,7 @@ dpif_linux_operate(struct dpif *dpif_, union dpif_op > **ops, size_t n_ops) > txn->request = ofpbuf_new(1024); > dpif_linux_flow_to_ofpbuf(&request, txn->request); > } else if (op->type == DPIF_OP_EXECUTE) { > - struct dpif_execute *execute = &op->execute; > + struct dpif_execute *execute = &op->u.execute; > > txn->request = dpif_linux_encode_execute( > dpif->dp_ifindex, execute->key, execute->key_len, > @@ -930,10 +930,10 @@ dpif_linux_operate(struct dpif *dpif_, union dpif_op > **ops, size_t n_ops) > > for (i = 0; i < n_ops; i++) { > struct nl_transaction *txn = &txns[i]; > - union dpif_op *op = ops[i]; > + struct dpif_op *op = ops[i]; > > if (op->type == DPIF_OP_FLOW_PUT) { > - struct dpif_flow_put *put = &op->flow_put; > + struct dpif_flow_put *put = &op->u.flow_put; > int error = txn->error; > > if (!error && put->stats) { > @@ -944,11 +944,9 @@ dpif_linux_operate(struct dpif *dpif_, union dpif_op > **ops, size_t n_ops) > dpif_linux_flow_get_stats(&reply, put->stats); > } > } > - put->error = error; > + op->error = error; > } else if (op->type == DPIF_OP_EXECUTE) { > - struct dpif_execute *execute = &op->execute; > - > - execute->error = txn->error; > + op->error = txn->error; > } else { > NOT_REACHED(); > } > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index 429cc9d..154b8a6 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -299,7 +299,7 @@ struct dpif_class { > * > * This function is optional. It is only worthwhile to implement it if > * 'dpif' can perform operations in batch faster than individually. */ > - void (*operate)(struct dpif *dpif, union dpif_op **ops, size_t n_ops); > + void (*operate)(struct dpif *dpif, struct dpif_op **ops, size_t n_ops); > > /* Retrieves 'dpif''s "listen mask" into '*listen_mask'. A 1-bit of value > * 2**X set in '*listen_mask' indicates that 'dpif' will receive messages > diff --git a/lib/dpif.c b/lib/dpif.c > index 9cbf877..43189e2 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -983,7 +983,7 @@ dpif_execute(struct dpif *dpif, > * This function exists because some datapaths can perform batched operations > * faster than individual operations. */ > void > -dpif_operate(struct dpif *dpif, union dpif_op **ops, size_t n_ops) > +dpif_operate(struct dpif *dpif, struct dpif_op **ops, size_t n_ops) > { > size_t i; > > @@ -993,25 +993,25 @@ dpif_operate(struct dpif *dpif, union dpif_op **ops, > size_t n_ops) > } > > for (i = 0; i < n_ops; i++) { > - union dpif_op *op = ops[i]; > + struct dpif_op *op = ops[i]; > struct dpif_flow_put *put; > struct dpif_execute *execute; > > switch (op->type) { > case DPIF_OP_FLOW_PUT: > - put = &op->flow_put; > - put->error = dpif_flow_put(dpif, put->flags, > - put->key, put->key_len, > - put->actions, put->actions_len, > - put->stats); > + put = &op->u.flow_put; > + op->error = dpif_flow_put(dpif, put->flags, > + put->key, put->key_len, > + put->actions, put->actions_len, > + put->stats); > break; > > case DPIF_OP_EXECUTE: > - execute = &op->execute; > - execute->error = dpif_execute(dpif, execute->key, > execute->key_len, > - execute->actions, > - execute->actions_len, > - execute->packet); > + execute = &op->u.execute; > + op->error = dpif_execute(dpif, execute->key, execute->key_len, > + execute->actions, > + execute->actions_len, > + execute->packet); > break; > > default: > diff --git a/lib/dpif.h b/lib/dpif.h > index 0ff2389..6782f29 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -181,8 +181,6 @@ enum dpif_op_type { > }; > > struct dpif_flow_put { > - enum dpif_op_type type; /* Always DPIF_OP_FLOW_PUT. */ > - > /* Input. */ > enum dpif_flow_put_flags flags; /* DPIF_FP_*. */ > const struct nlattr *key; /* Flow to put. */ > @@ -192,30 +190,26 @@ struct dpif_flow_put { > > /* Output. */ > struct dpif_flow_stats *stats; /* Optional flow statistics. */ > - int error; /* 0 or positive errno value. */ > }; > > struct dpif_execute { > - enum dpif_op_type type; /* Always DPIF_OP_EXECUTE. */ > - > - /* Input. */ > const struct nlattr *key; /* Partial flow key (only for metadata). > */ > size_t key_len; /* Length of 'key' in bytes. */ > const struct nlattr *actions; /* Actions to execute on packet. */ > size_t actions_len; /* Length of 'actions' in bytes. */ > const struct ofpbuf *packet; /* Packet to execute. */ > - > - /* Output. */ > - int error; /* 0 or positive errno value. */ > }; > > -union dpif_op { > +struct dpif_op { > enum dpif_op_type type; > - struct dpif_flow_put flow_put; > - struct dpif_execute execute; > + int error; > + union { > + struct dpif_flow_put flow_put; > + struct dpif_execute execute; > + } u; > }; > > -void dpif_operate(struct dpif *, union dpif_op **ops, size_t n_ops); > +void dpif_operate(struct dpif *, struct dpif_op **ops, size_t n_ops); > > /* Upcalls. */ > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 56c3baf..1cd0492 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2390,7 +2390,7 @@ struct flow_miss { > }; > > struct flow_miss_op { > - union dpif_op dpif_op; > + struct dpif_op dpif_op; > struct subfacet *subfacet; > }; > > @@ -2575,10 +2575,10 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct > flow_miss *miss, > subfacet->actions, > subfacet->actions_len, packet, true)) { > struct flow_miss_op *op = &ops[(*n_ops)++]; > - struct dpif_execute *execute = &op->dpif_op.execute; > + struct dpif_execute *execute = &op->dpif_op.u.execute; > > op->subfacet = subfacet; > - execute->type = DPIF_OP_EXECUTE; > + op->dpif_op.type = DPIF_OP_EXECUTE; > execute->key = miss->key; > execute->key_len = miss->key_len; > execute->actions > @@ -2592,10 +2592,10 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct > flow_miss *miss, > > if (facet->may_install && subfacet->key_fitness != ODP_FIT_TOO_LITTLE) { > struct flow_miss_op *op = &ops[(*n_ops)++]; > - struct dpif_flow_put *put = &op->dpif_op.flow_put; > + struct dpif_flow_put *put = &op->dpif_op.u.flow_put; > > op->subfacet = subfacet; > - put->type = DPIF_OP_FLOW_PUT; > + op->dpif_op.type = DPIF_OP_FLOW_PUT; > put->flags = DPIF_FP_CREATE | DPIF_FP_MODIFY; > put->key = miss->key; > put->key_len = miss->key_len; > @@ -2643,7 +2643,7 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, > struct dpif_upcall *upcalls, > struct dpif_upcall *upcall; > struct flow_miss *miss, *next_miss; > struct flow_miss_op flow_miss_ops[FLOW_MISS_MAX_BATCH * 2]; > - union dpif_op *dpif_ops[FLOW_MISS_MAX_BATCH * 2]; > + struct dpif_op *dpif_ops[FLOW_MISS_MAX_BATCH * 2]; > struct hmap todo; > size_t n_ops; > size_t i; > @@ -2713,11 +2713,10 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, > struct dpif_upcall *upcalls, > for (i = 0; i < n_ops; i++) { > struct flow_miss_op *op = &flow_miss_ops[i]; > struct dpif_execute *execute; > - struct dpif_flow_put *put; > > switch (op->dpif_op.type) { > case DPIF_OP_EXECUTE: > - execute = &op->dpif_op.execute; > + execute = &op->dpif_op.u.execute; > if (op->subfacet->actions != execute->actions) { > free((struct nlattr *) execute->actions); > } > @@ -2725,8 +2724,7 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, > struct dpif_upcall *upcalls, > break; > > case DPIF_OP_FLOW_PUT: > - put = &op->dpif_op.flow_put; > - if (!put->error) { > + if (!op->dpif_op.error) { > op->subfacet->installed = true; > } > break; > -- > 1.7.2.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
