The change to execute_set_action(), it implies that OVS_KEY_ATTR_MPLS
got put in a place you don't want it in a previous patch:
parse_remaining_mpls() will read past the end of the packet.
I don't think that flow_extract() checks that the packet is long
enough for an IPv4 or IPv6 header when it sets nw_ttl.
In flow_format(), I think it would be better to output nothing rather
than mpls(0) if there is no MPLS header.
s/encapsualted/encapsulated/ in parse_mpls_onward().
In flow_compose(), I would normally format
inner_dl_type = flow->encap_dl_type == htons(0)
? flow->dl_type
: flow->encap_dl_type;
as
inner_dl_type = (flow->encap_dl_type == htons(0)
? flow->dl_type
: flow->encap_dl_type);
In flow_compose(), I think that the tests like this:
if (flow->dl_type == htons(ETH_TYPE_IP) ||
flow->encap_dl_type == htons(ETH_TYPE_IP)) {
can be written to just check inner_dl_type instead.
In match_set_any_mpls_label(), I think that |= should be &=.
Similarly for the other mpls_set_any_*() functions.
I no longer think that we should add NXM constants for the MPLS
fields. I think when I asked Ravi to do that, we didn't have OXM
support. Instead, let's just use the OXM constants (OXM_OF_MPLS_*).
I don't think that we should support writing to the MPLS BOS field,
that is, 'writable' should be false for OXM_OF_MPLS_BOS. We don't
support writing to other fields that would essentially result in
packet corruption or require us to reinterpret the flow on the fly
(e.g. we don't support writing to the ethertype or IP protocol).
format_mpls_lse() shows a change in name of mpls_lse_to_stack() to
mpls_lse_to_bos(), implying that this function's name was changed at
some point. It would be better to start out with the preferred name.
In format_mpls_lse_values(), we normally put a space after the
'return' keyword. Also, despite its name, this function does not
format anything (it's a parsing helper, not a formatting helper).
format_odp_action() makes a change in its output format (adding a
comma) that seems should have been incorporated into an earlier patch.
In check_expectations_mpls(), please evaluate check_expectations() in
a statement of its own, because MAX evaluates its arguments more than
once.
(Or, we could write a function that simply returns MAX of two enum
odp_key_fitness parameters. We could call it
greatest_expectations().)
But, looking closer, the only value greater than ODP_FIT_TOO_LITTLE is
ODP_FIT_ERROR, and check_expectations() never returns that, so in fact
check_expectations_mpls() is a constant function that always returns
ODP_FIT_TOO_LITTLE.
Is there a circular calling relationship between parse_l3_onward() and
parse_mpls_onward()? Otherwise the stray prototype for
parse_l3_onward seems a little odd.
There's a change in parse_8021q_onward, in the assignment to
encap_fitness, that I think just changes line breaking without making
any semantic changes.
This change deletes a blank line before commit_set_nw_action() that
it should not.
The checks that this patch adds to ofpact_check__() should happen at
action parsing time instead, because this is not necessary to know the
flow or the maximum number of ports to validate them.
The #if 0...#endif block should not get added to ofpact_format().
There are some typos in the comment on ofpact_push: "MPSL" => "MPLS",
"BPP" => "PBB".
My impression is that 0x8847 is much more common than 0x8848, so
perhaps that should be a default Ethertype for the "push_mpls" action.
The comment in ofputil_usable_protocols() says that OF1.1+ supports
matching on the MPLS stack flag, but in fact this was only added in
OF1.3. (OF1.1 supports the other MPLS matches.)
I left off reading this at packets.c, I'll try to get back to it
later.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
