Looks good to me.
I had a thought but I don't feel strongly about it. What if we made a
new hook for ofproto implementations:
enum ofperr (*validate_actions)(const struct ofpact ofpacts[], size_t
ofpacts_len, const struct flow *, int max_ports).
In the ofproto-dpif implementation this would simply pass through to
ofpacts_check(), but in other implementations it could reject action
lists which are not implementable by their dpif. This would allow us
to make *packet_out() and *rule_construct() assume that the actions
are both valid and implementable. Then they could return void instead
of an ofperr. Also if we did this, rule_modify_actions() may be
completely unnecessary. The complete_operation() calls could be
pulled into ofproto.
At any rate, this seems marginally cleaner to me, but you may feel
strongly in the other direction in which case the current approach is
fine.
Ethan
On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 58 ++++++++++++++-----------------------------
> ofproto/ofproto-provider.h | 14 ++++-------
> ofproto/ofproto.c | 16 +++++++++---
> 3 files changed, 36 insertions(+), 52 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index f1d2ae0..44cbd17 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4592,13 +4592,6 @@ rule_construct(struct rule *rule_)
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct rule_dpif *victim;
> uint8_t table_id;
> - enum ofperr error;
> -
> - error = ofpacts_check(rule->up.ofpacts, rule->up.ofpacts_len,
> - &rule->up.cr.flow, ofproto->up.max_ports);
> - if (error) {
> - return error;
> - }
>
> rule->packet_count = 0;
> rule->byte_count = 0;
> @@ -4701,15 +4694,6 @@ static void
> rule_modify_actions(struct rule *rule_)
> {
> struct rule_dpif *rule = rule_dpif_cast(rule_);
> - struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> - enum ofperr error;
> -
> - error = ofpacts_check(rule->up.ofpacts, rule->up.ofpacts_len,
> - &rule->up.cr.flow, ofproto->up.max_ports);
> - if (error) {
> - ofoperation_complete(rule->up.pending, error);
> - return;
> - }
>
> complete_operation(rule);
> }
> @@ -6359,36 +6343,32 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf
> *packet,
> const struct ofpact *ofpacts, size_t ofpacts_len)
> {
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> - enum ofperr error;
> + struct odputil_keybuf keybuf;
> + struct dpif_flow_stats stats;
>
> - error = ofpacts_check(ofpacts, ofpacts_len, flow, ofproto->up.max_ports);
> - if (!error) {
> - struct odputil_keybuf keybuf;
> - struct dpif_flow_stats stats;
> + struct ofpbuf key;
>
> - struct ofpbuf key;
> + struct action_xlate_ctx ctx;
> + uint64_t odp_actions_stub[1024 / 8];
> + struct ofpbuf odp_actions;
>
> - struct action_xlate_ctx ctx;
> - uint64_t odp_actions_stub[1024 / 8];
> - struct ofpbuf odp_actions;
> + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> + odp_flow_key_from_flow(&key, flow);
>
> - ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> - odp_flow_key_from_flow(&key, flow);
> + dpif_flow_stats_extract(flow, packet, &stats);
>
> - dpif_flow_stats_extract(flow, packet, &stats);
> + action_xlate_ctx_init(&ctx, ofproto, flow, flow->vlan_tci, NULL,
> + packet_get_tcp_flags(packet, flow), packet);
> + ctx.resubmit_stats = &stats;
>
> - action_xlate_ctx_init(&ctx, ofproto, flow, flow->vlan_tci, NULL,
> - packet_get_tcp_flags(packet, flow), packet);
> - ctx.resubmit_stats = &stats;
> + ofpbuf_use_stub(&odp_actions,
> + odp_actions_stub, sizeof odp_actions_stub);
> + xlate_actions(&ctx, ofpacts, ofpacts_len, &odp_actions);
> + dpif_execute(ofproto->dpif, key.data, key.size,
> + odp_actions.data, odp_actions.size, packet);
> + ofpbuf_uninit(&odp_actions);
>
> - ofpbuf_use_stub(&odp_actions,
> - odp_actions_stub, sizeof odp_actions_stub);
> - xlate_actions(&ctx, ofpacts, ofpacts_len, &odp_actions);
> - dpif_execute(ofproto->dpif, key.data, key.size,
> - odp_actions.data, odp_actions.size, packet);
> - ofpbuf_uninit(&odp_actions);
> - }
> - return error;
> + return 0;
> }
>
> /* NetFlow. */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 2f62473..80cd55f 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -803,11 +803,7 @@ struct ofproto_class {
> * registers, then it is an error if 'rule->cr' does not wildcard all
> * registers.
> *
> - * - Validate that 'rule->ofpacts' is a sequence of well-formed actions
> - * that the datapath can correctly implement. If your ofproto
> - * implementation only implements a subset of the actions that Open
> - * vSwitch understands, then you should implement your own action
> - * validation.
> + * - Validate that the datapath can correctly implement
> 'rule->ofpacts'.
> *
> * - If the rule is valid, update the datapath flow table, adding the
> new
> * rule or replacing the existing one.
> @@ -889,8 +885,8 @@ struct ofproto_class {
> *
> * ->rule_modify_actions() should set the following in motion:
> *
> - * - Validate that the actions now in 'rule' are well-formed OpenFlow
> - * actions that the datapath can correctly implement.
> + * - Validate that the datapath can correctly implement the actions now
> + * in 'rule'.
> *
> * - Update the datapath flow table with the new actions.
> *
> @@ -943,8 +939,8 @@ struct ofproto_class {
> * The caller retains ownership of 'packet' and of 'ofpacts', so
> * ->packet_out() should not modify or free them.
> *
> - * This function must validate that it can implement 'ofpacts'. If not,
> - * then it should return an OpenFlow error code.
> + * This function must validate that it can correctly implement 'ofpacts'.
> + * If not, then it should return an OpenFlow error code.
> *
> * 'flow' reflects the flow information for 'packet'. All of the
> * information in 'flow' is extracted from 'packet', except for
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index cbcf0d2..17131ca 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2138,10 +2138,13 @@ handle_packet_out(struct ofconn *ofconn, const struct
> ofp_packet_out *opo)
> ofpbuf_use_const(payload, po.packet, po.packet_len);
> }
>
> - /* Send out packet. */
> + /* Verify actions against packet, then send packet if successful. */
> flow_extract(payload, 0, 0, po.in_port, &flow);
> - error = p->ofproto_class->packet_out(p, payload, &flow,
> - po.ofpacts, po.ofpacts_len);
> + error = ofpacts_check(po.ofpacts, po.ofpacts_len, &flow, p->max_ports);
> + if (!error) {
> + error = p->ofproto_class->packet_out(p, payload, &flow,
> + po.ofpacts, po.ofpacts_len);
> + }
> ofpbuf_delete(payload);
>
> exit_free_ofpacts:
> @@ -3237,7 +3240,12 @@ handle_flow_mod(struct ofconn *ofconn, const struct
> ofp_header *oh)
> * dropped from OpenFlow in the near future. There is no good error
> * code, so just state that the flow table is full. */
> error = OFPERR_OFPFMFC_ALL_TABLES_FULL;
> - } else {
> + }
> + if (!error) {
> + error = ofpacts_check(fm.ofpacts, fm.ofpacts_len,
> + &fm.cr.flow, ofproto->max_ports);
> + }
> + if (!error) {
> error = handle_flow_mod__(ofconn_get_ofproto(ofconn), ofconn, &fm,
> oh);
> }
> if (error) {
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
