[ovs-dev] [wdp error reporting 3/5] ofproto: Propagate OpenFlow OFPT_FLOW_MOD errors to controller.

Justin Pettit jpettit at nicira.com
Mon Aug 16 21:31:57 UTC 2010


Looks good.

--Justin


On Jul 26, 2010, at 3:44 PM, Ben Pfaff wrote:

> Errors can occur during flow table modifications requested via OpenFlow
> OFPT_FLOW_MOD commands, but ofproto was in most cases failing to report
> these errors back to the controller using OpenFlow error codes.
> 
> Reported-by: Tom Everman <teverman at google.com>
> ---
> ofproto/ofproto.c |   71 ++++++++++++++++++++++++++++++----------------------
> ofproto/wdp.h     |    6 +++-
> 2 files changed, 45 insertions(+), 32 deletions(-)
> 
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 3c13273..e5bc8d2 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2033,12 +2033,12 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
>         error = pktbuf_retrieve(ofconn->pktbuf, ntohl(ofm->buffer_id),
>                                 &packet, &in_port);
>         if (!error) {
> -            wdp_flow_inject(p->wdp, rule, in_port, packet);
> +            error = wdp_flow_inject(p->wdp, rule, in_port, packet);
>             ofpbuf_delete(packet);
>         }
>     }
> 
> -    return 0;
> +    return error;
> }
> 
> static struct wdp_rule *
> @@ -2072,10 +2072,10 @@ send_buffered_packet(struct ofproto *ofproto, struct ofconn *ofconn,
>         return error;
>     }
> 
> -    wdp_flow_inject(ofproto->wdp, rule, in_port, packet);
> +    error = wdp_flow_inject(ofproto->wdp, rule, in_port, packet);
>     ofpbuf_delete(packet);
> 
> -    return 0;
> +    return error;
> }
> 
> /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
> @@ -2088,7 +2088,8 @@ struct modify_flows_cbdata {
> };
> 
> static int modify_flow(struct ofproto *, const struct ofp_flow_mod *,
> -                       size_t n_actions, struct wdp_rule *);
> +                       size_t n_actions, struct wdp_rule *)
> +    WARN_UNUSED_RESULT;
> static int modify_flows_cb(struct wdp_rule *, void *cbdata_);
> 
> /* Implements OFPFC_ADD and OFPFC_MODIFY.  Returns 0 on success or an OpenFlow
> @@ -2103,6 +2104,7 @@ modify_flows_loose(struct ofproto *p, struct ofconn *ofconn,
>     struct modify_flows_cbdata cbdata;
>     uint8_t table_id;
>     flow_t target;
> +    int error;
> 
>     cbdata.ofproto = p;
>     cbdata.ofm = ofm;
> @@ -2113,14 +2115,18 @@ modify_flows_loose(struct ofproto *p, struct ofconn *ofconn,
>                     &target);
> 
>     table_id = flow_mod_table_id(ofconn, ofm);
> -    wdp_flow_for_each_match(p->wdp, &target, table_id_to_include(table_id),
> -                            modify_flows_cb, &cbdata);
> +    error = wdp_flow_for_each_match(p->wdp, &target,
> +                                    table_id_to_include(table_id),
> +                                    modify_flows_cb, &cbdata);
> +    if (error) {
> +        return error;
> +    }
> +
>     if (cbdata.match) {
>         /* This credits the packet to whichever flow happened to match last.
>          * That's weird.  Maybe we should do a lookup for the flow that
>          * actually matches the packet?  Who knows. */
> -        send_buffered_packet(p, ofconn, cbdata.match, ofm);
> -        return 0;
> +        return send_buffered_packet(p, ofconn, cbdata.match, ofm);
>     } else {
>         return add_flow(p, ofconn, ofm, n_actions);
>     }
> @@ -2137,8 +2143,8 @@ modify_flow_strict(struct ofproto *p, struct ofconn *ofconn,
> {
>     struct wdp_rule *rule = find_flow_strict(p, ofconn, ofm);
>     if (rule && !rule_is_hidden(rule)) {
> -        modify_flow(p, ofm, n_actions, rule);
> -        return send_buffered_packet(p, ofconn, rule, ofm);
> +        int error = modify_flow(p, ofm, n_actions, rule);
> +        return error ? error : send_buffered_packet(p, ofconn, rule, ofm);
>     } else {
>         return add_flow(p, ofconn, ofm, n_actions);
>     }
> @@ -2152,7 +2158,8 @@ modify_flows_cb(struct wdp_rule *rule, void *cbdata_)
> 
>     if (!rule_is_hidden(rule)) {
>         cbdata->match = rule;
> -        modify_flow(cbdata->ofproto, cbdata->ofm, cbdata->n_actions, rule);
> +        return modify_flow(cbdata->ofproto, cbdata->ofm, cbdata->n_actions,
> +                           rule);
>     }
>     return 0;
> }
> @@ -2195,11 +2202,11 @@ struct delete_flows_cbdata {
> };
> 
> static int delete_flows_cb(struct wdp_rule *, void *cbdata_);
> -static void delete_flow_core(struct ofproto *, struct wdp_rule *,
> +static int delete_flow_core(struct ofproto *, struct wdp_rule *,
>                              uint16_t out_port);
> 
> /* Implements OFPFC_DELETE. */
> -static void
> +static int
> delete_flows_loose(struct ofproto *p, const struct ofconn *ofconn,
>                    const struct ofp_flow_mod *ofm)
> {
> @@ -2214,19 +2221,21 @@ delete_flows_loose(struct ofproto *p, const struct ofconn *ofconn,
>                     &target);
>     table_id = flow_mod_table_id(ofconn, ofm);
> 
> -    wdp_flow_for_each_match(p->wdp, &target, table_id_to_include(table_id),
> -                            delete_flows_cb, &cbdata);
> +    return wdp_flow_for_each_match(p->wdp, &target,
> +                                   table_id_to_include(table_id),
> +                                   delete_flows_cb, &cbdata);
> }
> 
> /* Implements OFPFC_DELETE_STRICT. */
> -static void
> +static int
> delete_flow_strict(struct ofproto *p, const struct ofconn *ofconn,
>                    struct ofp_flow_mod *ofm)
> {
>     struct wdp_rule *rule = find_flow_strict(p, ofconn, ofm);
>     if (rule) {
> -        delete_flow_core(p, rule, ofm->out_port);
> +        return delete_flow_core(p, rule, ofm->out_port);
>     }
> +    return 0;
> }
> 
> /* Callback for delete_flows_loose(). */
> @@ -2235,7 +2244,7 @@ delete_flows_cb(struct wdp_rule *rule, void *cbdata_)
> {
>     struct delete_flows_cbdata *cbdata = cbdata_;
> 
> -    delete_flow_core(cbdata->ofproto, rule, cbdata->out_port);
> +    return delete_flow_core(cbdata->ofproto, rule, cbdata->out_port);
> }
> 
> /* Implements core of OFPFC_DELETE and OFPFC_DELETE_STRICT where 'rule' has
> @@ -2246,18 +2255,18 @@ delete_flows_cb(struct wdp_rule *rule, void *cbdata_)
>  * Will not delete 'rule' if it is hidden.  Will delete 'rule' only if
>  * 'out_port' is htons(OFPP_NONE) or if 'rule' actually outputs to the
>  * specified 'out_port'. */
> -static void
> +static int
> delete_flow_core(struct ofproto *p, struct wdp_rule *rule, uint16_t out_port)
> {
>     if (rule_is_hidden(rule)) {
> -        return;
> +        return 0;
>     }
> 
>     if (out_port != htons(OFPP_NONE) && !rule_has_out_port(rule, out_port)) {
> -        return;
> +        return 0;
>     }
> 
> -    delete_flow(p, rule, OFPRR_DELETE);
> +    return delete_flow(p, rule, OFPRR_DELETE);
> }
> 
> static int
> @@ -2334,12 +2343,10 @@ handle_flow_mod(struct ofproto *p, struct ofconn *ofconn,
>         return modify_flow_strict(p, ofconn, ofm, n_actions);
> 
>     case OFPFC_DELETE:
> -        delete_flows_loose(p, ofconn, ofm);
> -        return 0;
> +        return delete_flows_loose(p, ofconn, ofm);
> 
>     case OFPFC_DELETE_STRICT:
> -        delete_flow_strict(p, ofconn, ofm);
> -        return 0;
> +        return delete_flow_strict(p, ofconn, ofm);
> 
>     default:
>         return ofp_mkerr(OFPET_FLOW_MOD_FAILED, OFPFMFC_BAD_COMMAND);
> @@ -2648,7 +2655,7 @@ compose_flow_removed(struct ofproto *p, const struct wdp_rule *rule,
>     return buf;
> }
> 
> -static void
> +static int
> delete_flow(struct ofproto *p, struct wdp_rule *rule, uint8_t reason)
> {
>     /* We limit the maximum number of queued flow expirations it by accounting
> @@ -2660,6 +2667,7 @@ delete_flow(struct ofproto *p, struct wdp_rule *rule, uint8_t reason)
>     struct ofproto_rule *ofproto_rule = ofproto_rule_cast(rule);
>     struct wdp_flow_stats stats;
>     struct ofpbuf *buf;
> +    int error;
> 
>     if (ofproto_rule->send_flow_removed) {
>         /* Compose most of the ofp_flow_removed before 'rule' is destroyed. */
> @@ -2668,8 +2676,9 @@ delete_flow(struct ofproto *p, struct wdp_rule *rule, uint8_t reason)
>         buf = NULL;
>     }
> 
> -    if (wdp_flow_delete(p->wdp, rule, &stats)) {
> -        return;
> +    error = wdp_flow_delete(p->wdp, rule, &stats);
> +    if (error) {
> +        return error;
>     }
> 
>     if (buf) {
> @@ -2697,6 +2706,8 @@ delete_flow(struct ofproto *p, struct wdp_rule *rule, uint8_t reason)
>         }
>     }
>     free(ofproto_rule);
> +
> +    return 0;
> }
> 
> /* pinsched callback for sending 'packet' on 'ofconn'. */
> diff --git a/ofproto/wdp.h b/ofproto/wdp.h
> index 11ab028..1a89fed 100644
> --- a/ofproto/wdp.h
> +++ b/ofproto/wdp.h
> @@ -187,9 +187,11 @@ struct wdp_flow_put {
> 
> int wdp_flow_put(struct wdp *, struct wdp_flow_put *,
>                    struct wdp_flow_stats *old_stats,
> -                   struct wdp_rule **rulep);
> +                   struct wdp_rule **rulep)
> +    WARN_UNUSED_RESULT;
> int wdp_flow_delete(struct wdp *, struct wdp_rule *,
> -                      struct wdp_flow_stats *final_stats);
> +                      struct wdp_flow_stats *final_stats)
> +    WARN_UNUSED_RESULT;
> 
> /* Sending packets in flows. */
> int wdp_flow_inject(struct wdp *, struct wdp_rule *,
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org





More information about the dev mailing list