Thanks Ben. Some of the changes were taken care but unfortunately missed to be
included in the latest patch as it's a bit confusion to me right now which
patch has what changes due to integration testing I do with vlan/vlan-qinq/mpls
and I frequently rebase to latest. Patch with 2nd code review comments is ready
and I will hold onto it for additional comments. Let me know if there is a way
to version these patches via git? I am planning to manually name them V1, V2...
and note down what it has to avoid missing code changes. For now
MPLS -- V2
VLAN qinq -- V1
Some queries below for the ones not addressed
lib/flow.c
----------
In flow_extract(), I don't think the cast is necessary:
+ packet->l2_5 = (uint32_t *)b.data;
<rk> fixed it.
format_odp_action() and parse_odp_action() use different syntax for
push_mpls and pop_mpls (one includes "tpid=", the other doesn't).
Please make them consistent.
(Is 'tpid' correct terminology for MPLS? I thought it was
VLAN-specific.)
<rk> code is driven from vlan and hence got carried over. Moreover, ovs-dpctl
output showed mpls(0x8847)... and hence didn't catch my attention. Fixed it now
with mpls(eth_type=0x8847/0x8848).
Code like this appears in a few places. Should we create a helper
function?
+ htonl((mpls_label << MPLS_LABEL_SHIFT) |
+ (mpls_tc << MPLS_TC_SHIFT) |
+ (mpls_ttl << MPLS_TTL_SHIFT) |
+ (mpls_stack << MPLS_STACK_SHIFT)));
<rk> Have added inline function for this.
The set_mpls_lse_values() function could also be implemented in terms
of such a helper.
<rk> set_mpls_lse_values itself is a helper function which then invokes api's
to set individual fields in mpls lse. Writing another helper function for this
would be kind of overkill. Not sure I understood your comments?
I'd like to see commit_mpls_action() split into a function to pop an
MPLS header and a function to push an MPLS header. The current
organization is confusing and I'm not convinced that it's 100%
correct.
<rk> It is written to catch error eth_types in push/pop, reason being, none of
the functions leading to commit_mpls_action has a return code which would
address this. I have split the function and added a VLOG_ERR for error case.
lib/packets.c
-------------
I think the logic in dec_mpls_ttl() only works because the TTL is the
low 8 bits of mpls_lse. Conceptually there's a missing shift.
<rk> It is done deliberately that way knowing that ttl is the lower 8bits and
it won't change. All invocations of set_mpls_lse_ttl has ttl not shifted except
one in which it handles other fields of label entry and shift is present just
to keep the code aligned. Knowing it's a no-op, I will add shifts to all.
copy_mpls_ttl_in() and copy_mpls_ttl_out() don't appear to check that
packet data is actually present.
<rk> when these functions are called, packet->data points to l2. Would there be
a possibility that packet->l2 will be NULL, however, packet->l2_5 and
packet->l3 will not? Or you are referring to checking for eth_type which is
handled in the version I have in my repository.
Also in those functions, missing {} here:
+ if (mh == NULL || nh == NULL)
+ return;
<rk>fixed.
push_mpls() and push_mpls_lse() look like they don't check that IPv4
packet data is actually present.
<rk> Code is written to take care of non-IP cases though spec isn't clear on
that. I haven't tested non-IP case.
I think it's time to add ofpbuf_*() functions to insert and remove
data in the middle of a packet. It's hard to see whether open-coded
moves (there are a few in this file in addition to the ones you added
for MPLS) are correct, so it'd be nice to get it right once and reuse
it.
<rk> agreed. Do you want me to write an api for this?
ofproto/ofproto-dpif.c
----------------------
facet_account() has some stray VLOG_DBG calls.
<rk> fixed it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
