On Tue, Nov 22, 2011 at 07:04:51PM -0800, Ethan Jackson wrote:
> I'm wondering how we are going to QA this. I assume you are implementing this
> patch now because the vlan splinters patch will break the invariant that
> userspace and the kernel support the same attributes? In the common case this
> won't be true though so I'm wondering if it makes sense to add some sort of
> flag to ovs-vswitchd that requries all subfacets to be perfect fit. This
> might
> help catch bugs which may otherwise be overlooked.
We could declare that that flag is "-vodp_util" and then look for the
DBG messages that indicate imperfect fit.
I wonder whether we should log at least something at level INFO if we
find imperfect fits. Probably the level of detail of the DBG messages
is too high, and we wouldn't want to make them sound scary. Dunno.
> + enum odp_key_fitness encap_fitness;
> + enum odp_key_fitness fitness;
> + __be16 tci;
>
>
> Why not ovs_be16?
Braino. I changed it to ovs_be16.
>
> + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IN_PORT)) {
> + uint32_t in_port = nl_attr_get_u32(attrs[OVS_KEY_ATTR_IN_PORT]);
> + if (in_port >= UINT16_MAX || in_port >= OFPP_MAX) {
> + VLOG_ERR_RL(&rl, "in_port %"PRIu32" out of supported range",
> + in_port);
> + return EINVAL;
> + }
>
>
> I think you should return ODP_FIT_ERROR here instead? Also, can we simply
> check if in_port >= OFPP_MAX? Perhaps a build assertion could guarantee this.
Oops, thanks, I changed EINVAL to ODP_FIT_ERROR here and in a couple
more places.
I'm pretty sure that GCC will optimize that down to a single
comparison.
> + /* Ethernet header. */
> + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERNET)) {
> + const struct ovs_key_ethernet *eth_key;
> +
> + eth_key = nl_attr_get(attrs[OVS_KEY_ATTR_ETHERNET]);
> + memcpy(flow->dl_src, eth_key->eth_src, ETH_ADDR_LEN);
> + memcpy(flow->dl_dst, eth_key->eth_dst, ETH_ADDR_LEN);
> + }
> + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET;
>
>
> Why is this expected_attrs update not in the if block like the other expected
> attrs updates? Is it because we allways expect there to be an ethernet attr?
Yes, that's right.
> static void facet_update_time(struct ofproto_dpif *, struct facet *,
> - long long int used);
> -static void facet_update_stats(struct ofproto_dpif *, struct facet *,
> - const struct dpif_flow_stats *);
> + long long int used);
>
>
> The indentation of facet_update_time()'s prototype is incorrect here.
Thanks, I fixed it.
>
> +/* A dpif flow associated with a facet.
> + *
> + * See also the large comment on struct subfacet. */
>
>
> "on struct facet"
Thanks, fixed.
>
> +struct subfacet {
> + /* Owners. */
> + struct hmap_node hmap_node; /* In struct ofproto_dpif's 'subfacets' map.
> */
>
>
> 'subfacets' -> 'subfacet''s
The member is in fact named "subfacets" so that would not be an
improvement.
It's a list not a map so I corrected that part.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
