[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