On Sat, Feb 14, 2015 at 10:33 AM, Simon Horman <ho...@verge.net.au> wrote:
> On Thu, Dec 18, 2014 at 10:31:43AM -0800, Pravin Shelar wrote:
>> On Tue, Dec 16, 2014 at 3:10 PM, Jarno Rajahalme <jrajaha...@nicira.com>
>> wrote:
>> > LGTM,
>> >
>> > With some comments below:
>> >
>> > Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> >
>> > Jarno
>> >
>> > On Dec 15, 2014, at 12:37 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> >
>> >> vSwitchd does not generate correct MPLS actions for multiple
>> >> MPLS push or pop action.
>> >> Datapath can handle multiple push action for in single action list.
>> >> But for after first MPLS pop it needs to recirculate packet to
>> >> refill packet key. Following patch fixes it accordingly.
>> >>
>> >> Reported-by: Stefano Salsano <stefano.sals...@uniroma2.it>
>> >> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
>> >> ---
>> >> lib/flow.c | 11 ++++++---
>> >> ofproto/ofproto-dpif-xlate.c | 24 ++++++++++----------
>> >> tests/automake.mk | 1 +
>> >> tests/mpls-xlate.at | 50
>> >> ++++++++++++++++++++++++++++++++++++++++++
>> >> tests/odp.at | 2 -
>> >> tests/ofproto-dpif.at | 42 +++++++++++++++++-----------------
>> >> tests/testsuite.at | 1 +
>> >> 7 files changed, 92 insertions(+), 39 deletions(-)
>> >> create mode 100644 tests/mpls-xlate.at
>> >>
>> >> diff --git a/lib/flow.c b/lib/flow.c
>> >> index 521ee82..eb7fdf1 100644
>> >> --- a/lib/flow.c
>> >> +++ b/lib/flow.c
>> >> @@ -1422,18 +1422,21 @@ flow_count_mpls_labels(const struct flow *flow,
>> >> struct flow_wildcards *wc)
>> >> /* dl_type is always masked. */
>> >> if (eth_type_mpls(flow->dl_type)) {
>> >> int i;
>> >> - int len = FLOW_MAX_MPLS_LABELS;
>> >> + int cnt;
>> >>
>> >> - for (i = 0; i < len; i++) {
>> >> + cnt = 0;
>> >> + for (i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
>> >> if (wc) {
>> >> wc->masks.mpls_lse[i] |= htonl(MPLS_BOS_MASK);
>> >> }
>> >> if (flow->mpls_lse[i] & htonl(MPLS_BOS_MASK)) {
>> >> return i + 1;
>> >> }
>> >> + if (flow->mpls_lse[i]) {
>> >> + cnt++;
>> >> + }
>> >
>> > If the intent is to skip potential explicit null labels, then this is
>> > maybe OK. The explicit NULL label for IPv6 has the label value 2, should
>> > they be skipped too?
>> >
>> Current code depends on BOS flag set to search last MPLS label. This
>> does not work in all cases. For example kernel can only parse one MPLS
>> label, so in case of multiple labels it sets outer MPLS only. The
>> outer label does not set BOS flag and we get wrong count. This patch
>> fixes the issue.
>
> I may be missing something obvious but this does not seem right to me
> because I think that 0 is a valid MPLS LSE (label:0, exp:0, bos:0, ttl:0).
>
ok, I will send patch to fix this case.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
