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
