On Mon, Sep 23, 2013 at 09:17:32PM -0700, Justin Pettit wrote:
>
> On Sep 23, 2013, at 8:39 PM, Ben Pfaff <[email protected]> wrote:
>
> > [resend with Luca's email address fixed]
> >
> > On Mon, Sep 23, 2013 at 08:35:05PM -0700, Ben Pfaff wrote:
> >> Commit 97f63e57a8 (ofproto: Remove soon-to-be-invalid optimizations.)
> >> changed ofproto_delete_flow() to use the general-purpose flow_mod
> >> implementation. However, the general-purpose flow_mod never matches hidden
> >> flows, which are exactly the flows that ofproto_delete_flow()'s callers
> >> want to delete.
> >>
> >> This commit fixes the problem by allowing flow_mods that specify a priority
> >> that can only be for a hidden flow to delete a hidden flow. (This doesn't
> >> allow OpenFlow clients to meddle with hidden flows because OpenFlow uses
> >> only a 16-bit field to specify priority.)
> >>
> >> I verified that, without this commit, if I change from one controller to
> >> another with "ovs-vsctl set-controller", then the in-band rules for the
> >> old controller remain. With this commit, the old rules are removed.
> >>
> >> Reported-by: YAMAMOTO Takashi <[email protected]>
> >> Bug #19821.
> >> Reported-by: Luca Giraudo <[email protected]>
> >> Bug #19888.
> >> Reported-by: Ying Chen <[email protected]>
> >> Signed-off-by: Ben Pfaff <[email protected]>
> >> ---
> >> ofproto/ofproto.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> >> index cfe8d2a..fcb3bca 100644
> >> --- a/ofproto/ofproto.c
> >> +++ b/ofproto/ofproto.c
> >> @@ -3239,7 +3239,7 @@ collect_rule(struct rule *rule, const struct
> >> rule_criteria *c,
> >> struct rule_collection *rules)
> >> OVS_REQUIRES(ofproto_mutex)
> >> {
> >> - if (ofproto_rule_is_hidden(rule)) {
> >> + if (ofproto_rule_is_hidden(rule) && c->cr.priority <= UINT16_MAX) {
>
> It might be nice to add a comment that basically rephrases the second
> paragraph in your commit message about why you want to do this and why
> regular OpenFlow clients can't match hidden flows. Without it, I
> think the code looks a little surprising.
That's reasonable, thanks. I added:
/* We ordinarily want to skip hidden rules, but there has to be a way for
* code internal to OVS to modify and delete them, so if the criteria
* specify a priority that can only be for a hidden flow, then allow hidden
* rules to be selected. (This doesn't allow OpenFlow clients to meddle
* with hidden flows because OpenFlow uses only a 16-bit field to specify
* priority.) */
> Acked-by: Justin Pettit <[email protected]>
Thanks for the review. I'll apply this to master and branch-2.0 soon.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
