On Wed, Oct 09, 2013 at 01:47:50PM -0700, Jarno Rajahalme wrote:
> Use the offset of the last member in struct flow instead of the
> struct size to help catch changes in the declaration.
> 
> Add flow_random_hash_fields() used for testing in places where
> struct flow was used without zero initialization before.
> 
> With these changes we do not need to keep updating explicit padding
> in struct flow as fields are added or deleted.
> 
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>

...

> @@ -603,7 +604,11 @@ flow_wildcards_init_catchall(struct flow_wildcards *wc)
>  }
>  
>  /* Initializes 'wc' as an exact-match set of wildcards; that is, 'wc' does 
> not
> - * wildcard any bits or fields. */
> + * wildcard any bits or fields.
> + * Note that also bits that are always zeroes (padding, extra bits in fields
> + * like IPv6 flow label), and fields that are mutually exclusive (e.g., IPv4
> + * and IPv6 addresses) are included.
> + */
>  void
>  flow_wildcards_init_exact(struct flow_wildcards *wc)
>  {

I don't like this semantic of initializing pads to 0xff very much.  But
I see that flow_wildcards_init_exact() has only one caller worth
considering, which is this code in rule_dpif_lookup_in_table():

    } else if (frag && ofproto->up.frag_handling == OFPC_FRAG_DROP) {
        cls_rule = &ofproto->drop_frags_rule->up.cr;
        if (wc) {
            flow_wildcards_init_exact(wc);
        }
    } else {

I don't understand why we need to use flow_wildcards_init_exact() when
dropping fragments.  If we didn't use it, then we could probably drop
the function entirely.  Do you have any idea why we use it here?
(Justin?)

> @@ -65,7 +63,11 @@ main(int argc, char *argv[])
>              struct flow_wildcards wc;
>              struct flow flow;
>  
> -            random_bytes(&flow, sizeof flow);
> +            /* The generated flows will have random bits where zeroes would
> +             * normally be required (padding, extra bits in fields like IPv6
> +             * flow label, but this test will never use those bits, so it 
> does
> +             * not matter. */
> +            flow_random_hash_fields(&flow);

Is this comment correct?  It doesn't look to me like
flow_random_hash_fields() puts random bits in the whole flow, only in
selected fields.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to