[ovs-dev] [PATCH 2/2] ofproto: Make OFPFC_MODIFY and OFPFC_MODIFY_STRICT add a flow if no match.

Justin Pettit jpettit at nicira.com
Wed Mar 24 22:57:03 UTC 2010


On Mar 19, 2010, at 12:49 PM, Ben Pfaff wrote:

This is a pretty big oversight on OpenFlow compatibility.  Thanks for addressing it!

> +/* Implements OFPFC_MODIFY.  Returns 0 on success or an OpenFlow error code as
> + * encoded by ofp_mkerr() on failure. */
> +static int
> +modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm,
> +                   size_t n_actions)
> +{
> +    struct modify_flows_cbdata cbdata;
> +    struct cls_rule target;
> +
> +    cbdata.ofproto = p;
> +    cbdata.ofm = ofm;
> +    cbdata.n_actions = n_actions;
> +    cbdata.matched = false;
> +
> +    cls_rule_from_match(&target, &ofm->match, 0);
> +
> +    classifier_for_each_match(&p->cls, &target, CLS_INC_ALL,
> +                              modify_flows_cb, &cbdata);
> +    if (!cbdata.matched) {
> +        return add_flow(p, NULL, ofm, n_actions);

I think this assumes that Modify commands never have a buffer ID, but I don't think that's true.

> +/* Implements OFPFC_MODIFY_STRICT.  Returns 0 on success or an OpenFlow error
> + * code as encoded by ofp_mkerr() on failure. */
> +static int
> +modify_flow_strict(struct ofproto *p,
> +                   struct ofp_flow_mod *ofm, size_t n_actions)
> +{
> +    struct rule *rule = find_flow_strict(p, ofm);
> +    return (rule && !rule_is_hidden(rule)
> +            ? modify_flow(p, ofm, n_actions, rule)
> +            : add_flow(p, NULL, ofm, n_actions));

Same here, I think Modify Strict commands can have a valid buffer id.

> +static void delete_flows_cb(struct cls_rule *, void *cbdata_);
> +static void delete_flow(struct ofproto *, struct rule *, uint16_t out_port);
> +
> +/* Implements OFPFC_DELETE.  Returns 0 on success or an OpenFlow error code as
> + * encoded by ofp_mkerr() on failure. */
> static int
> -modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm,
> -                   size_t n_actions, uint16_t command)
> +delete_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm)
> {
> ...
> +
> +    return 0;
> +}

I don't see how this will return an OpenFlow error, which the description implies it might.

> +/* Implements OFPFC_DELETE_STRICT.  Returns 0 on success or an OpenFlow error
> + * code as encoded by ofp_mkerr() on failure. */
> +static int
> +delete_flow_strict(struct ofproto *p, struct ofp_flow_mod *ofm)
> +{
> +    struct rule *rule = find_flow_strict(p, ofm);
> +    delete_flow(p, rule, ofm->out_port);

I think "rule" can be NULL if it's not found, which could cause problems for delete_flow().

>     return 0;
> }

Once again, I don't see how this will return an OpenFlow error, which the description implies it might.

> +/* Implements core of OFPFC_DELETE and OFPFC_DELETE_STRICT where 'rule' has
> + * been identified as a flow to delete from 'p''s flow table, by deleting the
> + * flow and sending out a OFPT_FLOW_REMOVED message to any interested
> + * controller.
> + *
> + * Will not delete 'rule' if it is hidden or, if 'out_port' is not
> + * htons(OFPP_NONE), if it does not output to port 'out_port'. */

This description was kind of confusing to me.  How about something along the lines of:

   Will not delete 'rule' if it is hidden or if outputting to 'out_port' is not 
   among the actions when 'out_port' is not htons(OFPP_NONE).

--Justin






More information about the dev mailing list