[ovs-dev] [OpenFlow 0.9 2/7] ofproto: Check overlap, emerg flow cache, and error code sync (OpenFlow 0.9)

[Ben Pfaff]

Justin Pettit <jpettit at nicira.com> writes:

> --- a/include/openflow/openflow.h
> +++ b/include/openflow/openflow.h
> @@ -529,6 +529,13 @@ OFP_ASSERT(sizeof(struct ofp_match) == 40);
>  /* By default, choose a priority in the middle. */
>  #define OFP_DEFAULT_PRIORITY 0x8000
>  
> +enum ofp_flow_mod_flags {
> +    OFPFF_SEND_FLOW_REM = 1 << 0,  /* Send flow removed message when flow
> +                                    * expires or is deleted. */
> +    OFPFF_CHECK_OVERLAP = 1 << 1,  /* Check for overlapping entries first. */
> +    OFPFF_EMERG         = 1 << 2   /* Ramark this is for emergency. */
> +};

"Ramark"?

> @@ -601,7 +609,8 @@ enum ofp_bad_request_code {
>      OFPBRC_BAD_VENDOR,          /* Vendor not supported (in ofp_vendor_header 
>                                   * or ofp_stats_request or ofp_stats_reply). */
>      OFPBRC_BAD_SUBTYPE,         /* Vendor subtype not supported. */
> -    OFPBRC_BAD_LENGTH,          /* Wrong request length for type. */
> +    OFPBRC_EPERM,               /* Permissions error. */
> +    OFPBRC_BAD_LEN,             /* Wrong request length for type. */
>      OFPBRC_BUFFER_EMPTY,        /* Specified buffer has already been used. */
>      OFPBRC_BAD_COOKIE           /* Specified buffer does not exist. */
>  };

Argh, WTF does everything get added in the middle!  It's like the
OpenFlow guys actively *want* to make compatibility hard.  (Unless
the ones at the end are unofficial ones that I added--then it's
my fault.)

> @@ -335,6 +337,43 @@ classifier_find_rule_exactly(const struct classifier *cls,
>      return NULL;
>  }
>  
> +/* Checks if the flow defined by 'target' with 'wildcards' at 'priority' 
> + * overlaps with any other rule at the same priority in the classifier.  
> + * Two rules are considered overlapping if a packet could match both. */
> +bool
> +classifier_rule_overlaps(const struct classifier *cls,
> +                         const flow_t *target, uint32_t wildcards,
> +                         unsigned int priority)
> +{
> +    struct cls_rule target_rule;
> +    const struct hmap *tbl;
> +
> +    if (!wildcards) {
> +        return search_exact_table(cls, flow_hash(target, 0), target) ?
> +            true : false;

This would mean that the rules overlap, yes, but if you insert
'target' then it will replace the rule that it overlaps with.  Is
that kind of thing supposed to fail if OFPFF_CHECK_OVERLAP is
set?

> +    }
> +    cls_rule_from_flow(&target_rule, target, wildcards, priority);
> +
> +    for (tbl = &cls->tables[0]; tbl < &cls->tables[CLS_N_FIELDS]; tbl++) {
> +        struct cls_bucket *bucket;
> +
> +        HMAP_FOR_EACH (bucket, struct cls_bucket, hmap_node, tbl) {
> +            struct cls_rule *rule;
> +
> +            LIST_FOR_EACH (rule, struct cls_rule, node.list,
> +                           &bucket->rules) {
> +                if (rule->priority == priority 
> +                        && rules_match_2wild(rule, &target_rule, 0)) {

Similarly, if it's an exact match (same priority, same wildcards,
same values for non-wildcarded fields) then the new rule would
normally replace the old one.  Is that supposed to fail?  (If
not, maybe the function should be named
classifier_rule_overlaps_inexactly() or some such.)

> +                    return true;
> +                }
> +            }
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  /* Ignores target->priority.
>   *
>   * 'callback' is allowed to delete the rule that is passed as its argument, but