Acked-by: Jarno Rajahalme <jrajaha...@nicira.com> I would have expected clang to flag different annotations as a warning… Did you look if we have similar situations with other functions? Would it be possible to automate such a check somehow?
I’ll review the rest of the series as well, Jarno > On Jul 1, 2015, at 2:06 PM, Ben Pfaff <b...@nicira.com> wrote: > > Jarno, this patch is probably a good one for you to look at. It's a > possible important bug fix and I know that you're knowledgeable about > the mutex in question. > > (If you wanted to look at the rest of the series that would be nice too > but this patch in particular may be important.) > > On Wed, Jun 24, 2015 at 10:57:01AM -0700, Ben Pfaff wrote: >> ofproto_enable_eviction() and ofproto_disable_eviction() require >> ofproto_mutex (and they were even annotated that way, though not on their >> prototypes but only at definition), but it wasn't being held. This fixes >> the problem. >> >> Found by inspection. >> >> Signed-off-by: Ben Pfaff <b...@nicira.com> >> --- >> ofproto/ofproto.c | 21 ++++++++++++--------- >> 1 file changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 08ba043..278c97f 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -83,10 +83,12 @@ static void oftable_set_name(struct oftable *, const >> char *name); >> >> static enum ofperr evict_rules_from_table(struct oftable *) >> OVS_REQUIRES(ofproto_mutex); >> -static void oftable_disable_eviction(struct oftable *); >> +static void oftable_disable_eviction(struct oftable *) >> + OVS_REQUIRES(ofproto_mutex); >> static void oftable_enable_eviction(struct oftable *, >> const struct mf_subfield *fields, >> - size_t n_fields); >> + size_t n_fields) >> + OVS_REQUIRES(ofproto_mutex); >> >> /* A set of rules within a single OpenFlow table (oftable) that have the same >> * values for the oftable's eviction_fields. A rule to be evicted, when one >> is >> @@ -1419,13 +1421,6 @@ ofproto_configure_table(struct ofproto *ofproto, int >> table_id, >> return; >> } >> >> - if (s->groups) { >> - oftable_enable_eviction(table, s->groups, s->n_groups); >> - } else { >> - oftable_disable_eviction(table); >> - } >> - >> - table->max_flows = s->max_flows; >> >> if (classifier_set_prefix_fields(&table->cls, >> s->prefix_fields, s->n_prefix_fields)) { >> @@ -1433,6 +1428,12 @@ ofproto_configure_table(struct ofproto *ofproto, int >> table_id, >> } >> >> ovs_mutex_lock(&ofproto_mutex); >> + if (s->groups) { >> + oftable_enable_eviction(table, s->groups, s->n_groups); >> + } else { >> + oftable_disable_eviction(table); >> + } >> + table->max_flows = s->max_flows; >> evict_rules_from_table(table); >> ovs_mutex_unlock(&ofproto_mutex); >> } >> @@ -7431,7 +7432,9 @@ static void >> oftable_destroy(struct oftable *table) >> { >> ovs_assert(classifier_is_empty(&table->cls)); >> + ovs_mutex_lock(&ofproto_mutex); >> oftable_disable_eviction(table); >> + ovs_mutex_unlock(&ofproto_mutex); >> classifier_destroy(&table->cls); >> free(table->name); >> } >> -- >> 2.1.3 >> _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev