[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