On Tue, Jan 22, 2013 at 10:47:17AM -0800, Ben Pfaff wrote:
> On Fri, Jan 18, 2013 at 09:27:55AM +0900, Simon Horman wrote:
> > This patch implements use-space datapath and non-datapath code
> > to match and use the datapath API set out in Leo Alterman's patch
> > "user-space datapath: Add basic MPLS support to kernel".
>
> I'm still reading, I'll continue later.
Here I go.
I noticed just now that the code added to execute_set_action() in
dpif-netdev.c is indented four spaces too far.
I get one build failure:
cc1: warnings being treated as errors
../lib/ofp-actions.c: In function ‘ofpact_from_nxast’:
../lib/ofp-actions.c:407: error: initialization from incompatible pointer
type
on this line:
struct nx_action_push_mpls *nxapm = (struct nx_action_push *)a;
In packets.c, a lot of the new functions check that the packet is at
least sizeof(struct eth_header) bytes long. I think that we should
simply make this a requirement and not bother checking for it. There
shouldn't be any code passing in packets that are not at least 14
bytes long. (Or if you know of any, let's fix them.)
push_mpls() is unnecessarily confusing because it has variables named
'ethtype' and 'eth_type'. I'd drop the latter entirely and rewrite
if (!eth_type_mpls(eth_type)) {
as
if (!eth_type_mpls(get_ethertype(packet))) {
pop_mpls() has the same variable name confusion, please fix it there
too.
I think that we should try to maintain an invariant that l2_5 is
nonnull if and only if the ethertype is an mpls ethertype. Then we
could simplify some of the code here by checking only for l2_5 !=
NULL and not bothering with ethertypes. Actually it looks like we
already maintain this invariant.
eth_mpls_depth() should take a "const" parameter.
struct mpls_eth_header isn't used anywhere. Do you think it is
useful?
IP_DSCP doesn't properly parenthesize its argument. Also it isn't
used anywhere.
IP6_VERSION and IP6_VER aren't used anywhere.
Why does compose_output_action__() save and restore mpls_lse and
mpls_depth? It saves nw_tos and vlan_tci because the output process
might need to temporarily change those, for output ports with
configured priorities and for VLAN splinters, respectively, but I
don't know of anything similar for MPLS.
Most of the functions called by do_xlate_actions() have "execute" or
"xlate" in their names, so perhaps compose_mpls_push_action() and
compose_mpls_pop_action() should follow this pattern.
In compose_mpls_push_action(), for the case where there's already an
MPLS header, do you know why we're getting the LSE from base_flow?
I'd think we could just keep the current one. For the other case,
this:
+ ctx->flow.mpls_lse &= ~htonl(MPLS_LABEL_MASK | MPLS_TC_MASK |
+ MPLS_TTL_MASK | MPLS_BOS_MASK);
seems equivalent to just "ctx->flow.mpls_lse = htonl(0);". But later
on the code writes to mpls_lse without any intervening read, so maybe
we can just delete the &= statement.
I'm not sure why compose_mpls_pop_action() emits
OVS_ACTION_ATTR_POP_MPLS instead of just manipulating mpls_depth and
other flow fields.
I think I'm done reviewing this patch for now.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
