It's not a huge deal if we skip looping; I'd rather have that happen than abort().
Thanks, Ben. On Fri, Oct 21, 2011 at 06:11:40PM -0700, Ethan Jackson wrote: > This looks fine to me. > > If FOR_EACH_MATCHING_TABLE is given an invalid TABLE_ID it simply > skips looping. Would it make sense to force callers to check their > TABLE_ID's by replacing the "return NULL" of the else clause of > first_matching_table() to a NOT_REACHED()? I don't feel strongly > about it. > > Ethan > > > On Thu, Sep 8, 2011 at 12:36, Ben Pfaff <b...@nicira.com> wrote: > > --- > > ?include/openflow/nicira-ext.h | ? ?9 ++++++++- > > ?ofproto/ofproto.c ? ? ? ? ? ? | ? 38 +++++++++++++++++++++++++++++++------- > > ?2 files changed, 39 insertions(+), 8 deletions(-) > > > > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > > index 4fab6f1..fe725be 100644 > > --- a/include/openflow/nicira-ext.h > > +++ b/include/openflow/nicira-ext.h > > @@ -99,7 +99,14 @@ enum nx_bad_request_code { > > ? ? NXBRC_NXM_BAD_PREREQ = 0x104, > > > > ? ? /* A given nxm_type was specified more than once. */ > > - ? ?NXBRC_NXM_DUP_TYPE = 0x105 > > + ? ?NXBRC_NXM_DUP_TYPE = 0x105, > > + > > +/* Other errors. */ > > + > > + ? ?/* A request specified a nonexistent table ID. ?(But > > NXFMFC_BAD_TABLE_ID is > > + ? ? * used instead, when it is appropriate, because that is such a special > > + ? ? * case.) */ > > + ? ?NXBRC_BAD_TABLE_ID = 0x200, > > ?}; > > > > ?/* Additional "code" values for OFPET_FLOW_MOD_FAILED. */ > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > > index 76feb91..f501b41 100644 > > --- a/ofproto/ofproto.c > > +++ b/ofproto/ofproto.c > > @@ -1771,6 +1771,17 @@ calc_flow_duration__(long long int start, uint32_t > > *sec, uint32_t *nsec) > > ? ? *nsec = (msecs % 1000) * (1000 * 1000); > > ?} > > > > +/* Checks whether 'table_id' is 0xff or a valid table ID in 'ofproto'. > > ?Returns > > + * 0 if 'table_id' is OK, otherwise an OpenFlow error code. ?*/ > > +static int > > +check_table_id(const struct ofproto *ofproto, uint8_t table_id) > > +{ > > + ? ?return (table_id == 0xff || table_id < ofproto->n_tables > > + ? ? ? ? ? ?? 0 > > + ? ? ? ? ? ?: ofp_mkerr_nicira(OFPET_BAD_REQUEST, NXBRC_BAD_TABLE_ID)); > > + > > +} > > + > > ?static struct classifier * > > ?first_matching_table(struct ofproto *ofproto, uint8_t table_id) > > ?{ > > @@ -1779,11 +1790,6 @@ first_matching_table(struct ofproto *ofproto, > > uint8_t table_id) > > ? ? } else if (table_id < ofproto->n_tables) { > > ? ? ? ? return &ofproto->tables[table_id]; > > ? ? } else { > > - ? ? ? ?/* It would probably be better to reply with an error but there > > doesn't > > - ? ? ? ? * seem to be any appropriate value, so that might just be > > - ? ? ? ? * confusing. */ > > - ? ? ? ?VLOG_WARN_RL(&rl, "controller asked for invalid table %"PRIu8, > > - ? ? ? ? ? ? ? ? ? ? table_id); > > ? ? ? ? return NULL; > > ? ? } > > ?} > > @@ -1806,8 +1812,9 @@ next_matching_table(struct ofproto *ofproto, > > ?* ? - If TABLE_ID is the number of a table in OFPROTO, then the loop > > iterates > > ?* ? ? only once, for that table. > > ?* > > - * ? - Otherwise, TABLE_ID isn't valid for OFPROTO, so ofproto logs a > > warning > > - * ? ? and does not enter the loop at all. > > + * ? - Otherwise, TABLE_ID isn't valid for OFPROTO, so the loop won't be > > + * ? ? entered at all. ?(Perhaps you should have validated TABLE_ID with > > + * ? ? check_table_id().) > > ?* > > ?* All parameters are evaluated multiple times. > > ?*/ > > @@ -1833,6 +1840,12 @@ collect_rules_loose(struct ofproto *ofproto, uint8_t > > table_id, > > ? ? ? ? ? ? ? ? ? ? struct list *rules) > > ?{ > > ? ? struct classifier *cls; > > + ? ?int error; > > + > > + ? ?error = check_table_id(ofproto, table_id); > > + ? ?if (error) { > > + ? ? ? ?return error; > > + ? ?} > > > > ? ? list_init(rules); > > ? ? FOR_EACH_MATCHING_TABLE (cls, table_id, ofproto) { > > @@ -1869,6 +1882,12 @@ collect_rules_strict(struct ofproto *ofproto, > > uint8_t table_id, > > ? ? ? ? ? ? ? ? ? ? ?struct list *rules) > > ?{ > > ? ? struct classifier *cls; > > + ? ?int error; > > + > > + ? ?error = check_table_id(ofproto, table_id); > > + ? ?if (error) { > > + ? ? ? ?return error; > > + ? ?} > > > > ? ? list_init(rules); > > ? ? FOR_EACH_MATCHING_TABLE (cls, table_id, ofproto) { > > @@ -2176,6 +2195,11 @@ add_flow(struct ofproto *ofproto, struct ofconn > > *ofconn, > > ? ? struct rule *rule; > > ? ? int error; > > > > + ? ?error = check_table_id(ofproto, fm->table_id); > > + ? ?if (error) { > > + ? ? ? ?return error; > > + ? ?} > > + > > ? ? /* Check for overlap, if requested. */ > > ? ? if (fm->flags & OFPFF_CHECK_OVERLAP) { > > ? ? ? ? struct classifier *cls; > > -- > > 1.7.4.4 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev