[ovs-dev] [error reporting 4/6] ofproto: Issue OpenFlow error for bad table IDs.

[Ben Pfaff]

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 <blp at 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 at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >