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

[Ethan Jackson]

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
>