diff mbox series

[ovs-dev,v3,1/2] Datapath: New MPLS actions for layer 2 tunnelling

Message ID 7d1b81bf82976cbc894763fe628a3a281a392e09.1588869591.git.martin.varghese@nokia.com
State Superseded
Headers show
Series Encap & Decap actions for MPLS packet type | expand

Commit Message

Martin Varghese May 8, 2020, 4:31 a.m. UTC 
From: Martin Varghese <martin.varghese@nokia.com>

Upstream commit:

commit f66b53fdbb22ced1a323b22b9de84a61aacd8d18
Author: Martin Varghese <martin.varghese@nokia.com>
Date:   Sat Dec 21 08:50:46 2019 +0530

    openvswitch: New MPLS actions for layer 2 tunnelling
    The existing PUSH MPLS action inserts MPLS header between ethernet
    header and the IP header. Though this behaviour is fine for L3 VPN where an
    IP  packet is encapsulated inside a MPLS tunnel, it does not suffice the
    L2 VPN (l2 tunnelling) requirements. In L2 VPN the MPLS header should
    encapsulate the ethernet packet.

    The new mpls action ADD_MPLS inserts MPLS header at the start of the
    packet or at the start of the l3 header depending on the value of l3
    tunnel flag in the ADD_MPLS arguments.

    POP_MPLS action is extended to support ethertype 0x6558.

    Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
    Acked-by: Pravin B Shelar <pshelar@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
 datapath/actions.c                                | 42 ++++++++++++++++++-----
 datapath/flow_netlink.c                           | 33 ++++++++++++++++++
 datapath/linux/compat/include/linux/openvswitch.h | 35 +++++++++++++++++--
 3 files changed, 98 insertions(+), 12 deletions(-)

Comments

0-day Robot May 8, 2020, 4:59 a.m. UTC | #1 
Bleep bloop.  Greetings Martin Varghese, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/dpctl.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpctl.lo lib/dpctl.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpctl.lo -MD -MP -MF lib/.deps/dpctl.Tpo -c lib/dpctl.c -o lib/dpctl.o
depbase=`echo lib/dp-packet.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/dp-packet.lo -MD -MP -MF $depbase.Tpo -c -o lib/dp-packet.lo lib/dp-packet.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dp-packet.lo -MD -MP -MF lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o lib/dp-packet.o
depbase=`echo lib/dpif-netdev-lookup-generic.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev-lookup-generic.lo lib/dpif-netdev-lookup-generic.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF lib/.deps/dpif-netdev-lookup-generic.Tpo -c lib/dpif-netdev-lookup-generic.c -o lib/dpif-netdev-lookup-generic.o
depbase=`echo lib/dpif-netdev.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netdev.lo lib/dpif-netdev.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o lib/dpif-netdev.o
lib/dpif-netdev.c: In function 'dp_execute_cb':
lib/dpif-netdev.c:7160:5: error: enumeration value 'OVS_ACTION_ATTR_ADD_MPLS' not handled in switch [-Werror=switch]
     switch ((enum ovs_action_attr)type) {
     ^
cc1: all warnings being treated as errors
make[2]: *** [lib/dpif-netdev.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Martin Varghese May 8, 2020, 5:38 a.m. UTC | #2 
It is already handled in path. Did i miss something here?

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 51c8885..39a38cc 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7496,6 +7496,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
*packets_,
     case OVS_ACTION_ATTR_CT_CLEAR:
     case OVS_ACTION_ATTR_CHECK_PKT_LEN:
     case OVS_ACTION_ATTR_DROP:
+    case OVS_ACTION_ATTR_ADD_MPLS:
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
     }


On 5/8/20, 0-day Robot <robot@bytheb.org> wrote:
> Bleep bloop.  Greetings Martin Varghese, I am a robot and I have tried out
> your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> build:
> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
> -g -O2 -MT lib/dpctl.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpctl.lo
> lib/dpctl.c &&\
> mv -f $depbase.Tpo $depbase.Plo
> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpctl.lo -MD
> -MP -MF lib/.deps/dpctl.Tpo -c lib/dpctl.c -o lib/dpctl.o
> depbase=`echo lib/dp-packet.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
> -g -O2 -MT lib/dp-packet.lo -MD -MP -MF $depbase.Tpo -c -o lib/dp-packet.lo
> lib/dp-packet.c &&\
> mv -f $depbase.Tpo $depbase.Plo
> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dp-packet.lo
> -MD -MP -MF lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o lib/dp-packet.o
> depbase=`echo lib/dpif-netdev-lookup-generic.lo | sed
> 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
> -g -O2 -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF $depbase.Tpo -c -o
> lib/dpif-netdev-lookup-generic.lo lib/dpif-netdev-lookup-generic.c &&\
> mv -f $depbase.Tpo $depbase.Plo
> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT
> lib/dpif-netdev-lookup-generic.lo -MD -MP -MF
> lib/.deps/dpif-netdev-lookup-generic.Tpo -c lib/dpif-netdev-lookup-generic.c
> -o lib/dpif-netdev-lookup-generic.o
> depbase=`echo lib/dpif-netdev.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
> -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o
> lib/dpif-netdev.lo lib/dpif-netdev.c &&\
> mv -f $depbase.Tpo $depbase.Plo
> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netdev.lo
> -MD -MP -MF lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o
> lib/dpif-netdev.o
> lib/dpif-netdev.c: In function 'dp_execute_cb':
> lib/dpif-netdev.c:7160:5: error: enumeration value
> 'OVS_ACTION_ATTR_ADD_MPLS' not handled in switch [-Werror=switch]
>      switch ((enum ovs_action_attr)type) {
>      ^
> cc1: all warnings being treated as errors
> make[2]: *** [lib/dpif-netdev.lo] Error 1
> make[2]: Leaving directory
> `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory
> `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
> make: *** [all] Error 2
>
>
> Please check this out.  If you feel there has been an error, please email
> aconole@redhat.com
>
> Thanks,
> 0-day Robot
>
Ilya Maximets June 4, 2020, 9:48 p.m. UTC | #3 
On 5/8/20 7:38 AM, Martin Varghese wrote:
> It is already handled in path. Did i miss something here?

Robot applied only patch #1 from the series and failed to build.
You need to make sure that it's possible to build and run OVS while
having only first patch applied or combine both patches.

The reason is that if someone will try to bisect some issue he/she
should be able to build and run successfully on any given commit
in a git history.

Best regards, Ilya Maximets.

> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 51c8885..39a38cc 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7496,6 +7496,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
>      case OVS_ACTION_ATTR_CT_CLEAR:
>      case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>      case OVS_ACTION_ATTR_DROP:
> +    case OVS_ACTION_ATTR_ADD_MPLS:
>      case __OVS_ACTION_ATTR_MAX:
>          OVS_NOT_REACHED();
>      }
> 
> 
> On 5/8/20, 0-day Robot <robot@bytheb.org> wrote:
>> Bleep bloop.  Greetings Martin Varghese, I am a robot and I have tried out
>> your patch.
>> Thanks for your contribution.
>>
>> I encountered some error that I wasn't expecting.  See the details below.
>>
>>
>> build:
>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
>> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
>> -g -O2 -MT lib/dpctl.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpctl.lo
>> lib/dpctl.c &&\
>> mv -f $depbase.Tpo $depbase.Plo
>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
>> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
>> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpctl.lo -MD
>> -MP -MF lib/.deps/dpctl.Tpo -c lib/dpctl.c -o lib/dpctl.o
>> depbase=`echo lib/dp-packet.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
>> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
>> -g -O2 -MT lib/dp-packet.lo -MD -MP -MF $depbase.Tpo -c -o lib/dp-packet.lo
>> lib/dp-packet.c &&\
>> mv -f $depbase.Tpo $depbase.Plo
>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
>> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
>> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dp-packet.lo
>> -MD -MP -MF lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o lib/dp-packet.o
>> depbase=`echo lib/dpif-netdev-lookup-generic.lo | sed
>> 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
>> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
>> -g -O2 -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF $depbase.Tpo -c -o
>> lib/dpif-netdev-lookup-generic.lo lib/dpif-netdev-lookup-generic.c &&\
>> mv -f $depbase.Tpo $depbase.Plo
>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
>> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
>> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT
>> lib/dpif-netdev-lookup-generic.lo -MD -MP -MF
>> lib/.deps/dpif-netdev-lookup-generic.Tpo -c lib/dpif-netdev-lookup-generic.c
>> -o lib/dpif-netdev-lookup-generic.o
>> depbase=`echo lib/dpif-netdev.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H
>> -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes
>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror
>> -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o
>> lib/dpif-netdev.lo lib/dpif-netdev.c &&\
>> mv -f $depbase.Tpo $depbase.Plo
>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I
>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
>> -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes
>> -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers
>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netdev.lo
>> -MD -MP -MF lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o
>> lib/dpif-netdev.o
>> lib/dpif-netdev.c: In function 'dp_execute_cb':
>> lib/dpif-netdev.c:7160:5: error: enumeration value
>> 'OVS_ACTION_ATTR_ADD_MPLS' not handled in switch [-Werror=switch]
>>      switch ((enum ovs_action_attr)type) {
>>      ^
>> cc1: all warnings being treated as errors
>> make[2]: *** [lib/dpif-netdev.lo] Error 1
>> make[2]: Leaving directory
>> `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
>> make[1]: *** [all-recursive] Error 1
>> make[1]: Leaving directory
>> `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
>> make: *** [all] Error 2
>>
>>
>> Please check this out.  If you feel there has been an error, please email
>> aconole@redhat.com
>>
>> Thanks,
>> 0-day Robot
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Eelco Chaudron Nov. 12, 2020, 10:44 a.m. UTC | #4 
On 4 Jun 2020, at 23:48, Ilya Maximets wrote:

> On 5/8/20 7:38 AM, Martin Varghese wrote:
>> It is already handled in path. Did i miss something here?
>
> Robot applied only patch #1 from the series and failed to build.
> You need to make sure that it's possible to build and run OVS while
> having only first patch applied or combine both patches.
>
> The reason is that if someone will try to bisect some issue he/she
> should be able to build and run successfully on any given commit
> in a git history.

Martin, are you planning to fix this soon, and sent a new patch?
I would like to review this patch, as it’s been out there a while.

//Eelco

>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 51c8885..39a38cc 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -7496,6 +7496,7 @@ dp_execute_cb(void *aux_, struct 
>> dp_packet_batch
>> *packets_,
>>      case OVS_ACTION_ATTR_CT_CLEAR:
>>      case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>>      case OVS_ACTION_ATTR_DROP:
>> +    case OVS_ACTION_ATTR_ADD_MPLS:
>>      case __OVS_ACTION_ATTR_MAX:
>>          OVS_NOT_REACHED();
>>      }
>>
>>
>> On 5/8/20, 0-day Robot <robot@bytheb.org> wrote:
>>> Bleep bloop.  Greetings Martin Varghese, I am a robot and I have 
>>> tried out
>>> your patch.
>>> Thanks for your contribution.
>>>
>>> I encountered some error that I wasn't expecting.  See the details 
>>> below.
>>>
>>>
>>> build:
>>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 
>>> -DHAVE_CONFIG_H
>>> -I.    -I ./include -I ./include -I ./lib -I ./lib    
>>> -Wstrict-prototypes
>>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
>>> -Wformat-security
>>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror 
>>> -Werror
>>> -g -O2 -MT lib/dpctl.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpctl.lo
>>> lib/dpctl.c &&\
>>> mv -f $depbase.Tpo $depbase.Plo
>>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include 
>>> -I
>>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security 
>>> -Wswitch-enum
>>> -Wunused-parameter -Wbad-function-cast -Wcast-align 
>>> -Wstrict-prototypes
>>> -Wold-style-definition -Wmissing-prototypes 
>>> -Wmissing-field-initializers
>>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT 
>>> lib/dpctl.lo -MD
>>> -MP -MF lib/.deps/dpctl.Tpo -c lib/dpctl.c -o lib/dpctl.o
>>> depbase=`echo lib/dp-packet.lo | sed 
>>> 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 
>>> -DHAVE_CONFIG_H
>>> -I.    -I ./include -I ./include -I ./lib -I ./lib    
>>> -Wstrict-prototypes
>>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
>>> -Wformat-security
>>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror 
>>> -Werror
>>> -g -O2 -MT lib/dp-packet.lo -MD -MP -MF $depbase.Tpo -c -o 
>>> lib/dp-packet.lo
>>> lib/dp-packet.c &&\
>>> mv -f $depbase.Tpo $depbase.Plo
>>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include 
>>> -I
>>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security 
>>> -Wswitch-enum
>>> -Wunused-parameter -Wbad-function-cast -Wcast-align 
>>> -Wstrict-prototypes
>>> -Wold-style-definition -Wmissing-prototypes 
>>> -Wmissing-field-initializers
>>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT 
>>> lib/dp-packet.lo
>>> -MD -MP -MF lib/.deps/dp-packet.Tpo -c lib/dp-packet.c -o 
>>> lib/dp-packet.o
>>> depbase=`echo lib/dpif-netdev-lookup-generic.lo | sed
>>> 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 
>>> -DHAVE_CONFIG_H
>>> -I.    -I ./include -I ./include -I ./lib -I ./lib    
>>> -Wstrict-prototypes
>>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
>>> -Wformat-security
>>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror 
>>> -Werror
>>> -g -O2 -MT lib/dpif-netdev-lookup-generic.lo -MD -MP -MF 
>>> $depbase.Tpo -c -o
>>> lib/dpif-netdev-lookup-generic.lo lib/dpif-netdev-lookup-generic.c 
>>> &&\
>>> mv -f $depbase.Tpo $depbase.Plo
>>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include 
>>> -I
>>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security 
>>> -Wswitch-enum
>>> -Wunused-parameter -Wbad-function-cast -Wcast-align 
>>> -Wstrict-prototypes
>>> -Wold-style-definition -Wmissing-prototypes 
>>> -Wmissing-field-initializers
>>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT
>>> lib/dpif-netdev-lookup-generic.lo -MD -MP -MF
>>> lib/.deps/dpif-netdev-lookup-generic.Tpo -c 
>>> lib/dpif-netdev-lookup-generic.c
>>> -o lib/dpif-netdev-lookup-generic.o
>>> depbase=`echo lib/dpif-netdev.lo | sed 
>>> 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
>>> /bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 
>>> -DHAVE_CONFIG_H
>>> -I.    -I ./include -I ./include -I ./lib -I ./lib    
>>> -Wstrict-prototypes
>>> -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat 
>>> -Wformat-security
>>> -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
>>> -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
>>> -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror 
>>> -Werror
>>> -g -O2 -MT lib/dpif-netdev.lo -MD -MP -MF $depbase.Tpo -c -o
>>> lib/dpif-netdev.lo lib/dpif-netdev.c &&\
>>> mv -f $depbase.Tpo $depbase.Plo
>>> libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include 
>>> -I
>>> ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra
>>> -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security 
>>> -Wswitch-enum
>>> -Wunused-parameter -Wbad-function-cast -Wcast-align 
>>> -Wstrict-prototypes
>>> -Wold-style-definition -Wmissing-prototypes 
>>> -Wmissing-field-initializers
>>> -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT 
>>> lib/dpif-netdev.lo
>>> -MD -MP -MF lib/.deps/dpif-netdev.Tpo -c lib/dpif-netdev.c -o
>>> lib/dpif-netdev.o
>>> lib/dpif-netdev.c: In function 'dp_execute_cb':
>>> lib/dpif-netdev.c:7160:5: error: enumeration value
>>> 'OVS_ACTION_ATTR_ADD_MPLS' not handled in switch [-Werror=switch]
>>>      switch ((enum ovs_action_attr)type) {
>>>      ^
>>> cc1: all warnings being treated as errors
>>> make[2]: *** [lib/dpif-netdev.lo] Error 1
>>> make[2]: Leaving directory
>>> `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
>>> make[1]: *** [all-recursive] Error 1
>>> make[1]: Leaving directory
>>> `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
>>> make: *** [all] Error 2
>>>
>>>
>>> Please check this out.  If you feel there has been an error, please 
>>> email
>>> aconole@redhat.com
>>>
>>> Thanks,
>>> 0-day Robot
>>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/datapath/actions.c b/datapath/actions.c
index fbf4457..65266e4 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -185,7 +185,7 @@  static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
 }
 
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
-		     const struct ovs_action_push_mpls *mpls)
+		      __be32 mpls_lse, __be16 mpls_ethertype, __u16 mac_len)
 {
 	struct mpls_shim_hdr *new_mpls_lse;
 
@@ -197,26 +197,30 @@  static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 		return -ENOMEM;
 
 	if (!ovs_skb_get_inner_protocol(skb)) {
-		skb_set_inner_network_header(skb, skb->mac_len);
+		skb_set_inner_network_header(skb, skb_network_offset(skb));
 		ovs_skb_set_inner_protocol(skb, skb->protocol);
 	}
 
 	skb_push(skb, MPLS_HLEN);
 	memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
-		skb->mac_len);
+		mac_len);
 	skb_reset_mac_header(skb);
 #ifdef MPLS_HEADER_IS_L3
-	skb_set_network_header(skb, skb->mac_len);
+	skb_set_network_header(skb, mac_len);
 #endif
+	skb_reset_mac_len(skb);
 
 	new_mpls_lse = mpls_hdr(skb);
-	new_mpls_lse->label_stack_entry = mpls->mpls_lse;
+	new_mpls_lse->label_stack_entry = mpls_lse;
 
 	skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
 
+	if (!mac_len)
+		key->mac_proto = MAC_PROTO_NONE;
+
 	if (ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET)
-		update_ethertype(skb, eth_hdr(skb), mpls->mpls_ethertype);
-	skb->protocol = mpls->mpls_ethertype;
+		update_ethertype(skb, eth_hdr(skb), mpls_ethertype);
+	skb->protocol = mpls_ethertype;
 
 	invalidate_flow_key(key);
 	return 0;
@@ -252,6 +256,9 @@  static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	if (eth_p_mpls(skb->protocol))
 		skb->protocol = ethertype;
 
+	if (ethertype == htons(ETH_P_TEB))
+		key->mac_proto = MAC_PROTO_ETHERNET;
+
 	invalidate_flow_key(key);
 	return 0;
 }
@@ -1309,9 +1316,26 @@  static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			execute_hash(skb, key, a);
 			break;
 
-		case OVS_ACTION_ATTR_PUSH_MPLS:
-			err = push_mpls(skb, key, nla_data(a));
+		case OVS_ACTION_ATTR_PUSH_MPLS: {
+			struct ovs_action_push_mpls *mpls = nla_data(a);
+
+			err = push_mpls(skb, key, mpls->mpls_lse,
+					mpls->mpls_ethertype, skb->mac_len);
 			break;
+		}
+
+		case OVS_ACTION_ATTR_ADD_MPLS: {
+		       struct ovs_action_add_mpls *mpls = nla_data(a);
+		       __u16 mac_len = 0;
+
+		       if (mpls->tun_flags & OVS_MPLS_L3_TUNNEL_FLAG_MASK)
+			       mac_len = skb->mac_len;
+
+		       err = push_mpls(skb, key, mpls->mpls_lse,
+				       mpls->mpls_ethertype, mac_len);
+		       break;
+	       }
+
 
 		case OVS_ACTION_ATTR_POP_MPLS:
 			err = pop_mpls(skb, key, nla_get_be16(a));
diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index d3fd771..3d54d4d 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -94,6 +94,7 @@  static bool actions_may_change_flow(const struct nlattr *actions)
 		case OVS_ACTION_ATTR_SET_MASKED:
 		case OVS_ACTION_ATTR_METER:
 		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+		case OVS_ACTION_ATTR_ADD_MPLS:
 		default:
 			return true;
 		}
@@ -3002,6 +3003,7 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			[OVS_ACTION_ATTR_METER] = sizeof(u32),
 			[OVS_ACTION_ATTR_CLONE] = (u32)-1,
 			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
+			[OVS_ACTION_ATTR_ADD_MPLS] = sizeof(struct ovs_action_add_mpls),
 		};
 		const struct ovs_action_push_vlan *vlan;
 		int type = nla_type(a);
@@ -3069,6 +3071,33 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 		case OVS_ACTION_ATTR_RECIRC:
 			break;
 
+		case OVS_ACTION_ATTR_ADD_MPLS: {
+			const struct ovs_action_add_mpls *mpls = nla_data(a);
+
+			if (!eth_p_mpls(mpls->mpls_ethertype))
+				return -EINVAL;
+
+			if (mpls->tun_flags & OVS_MPLS_L3_TUNNEL_FLAG_MASK) {
+				if (vlan_tci & htons(VLAN_CFI_MASK) ||
+				    (eth_type != htons(ETH_P_IP) &&
+				     eth_type != htons(ETH_P_IPV6) &&
+				     eth_type != htons(ETH_P_ARP) &&
+				     eth_type != htons(ETH_P_RARP) &&
+				     !eth_p_mpls(eth_type)))
+					return -EINVAL;
+				mpls_label_count++;
+			} else {
+				if (mac_proto == MAC_PROTO_ETHERNET) {
+					mpls_label_count = 1;
+					mac_proto = MAC_PROTO_NONE;
+				} else {
+					mpls_label_count++;
+				}
+		       }
+		       eth_type = mpls->mpls_ethertype;
+		       break;
+		}
+
 		case OVS_ACTION_ATTR_PUSH_MPLS: {
 			const struct ovs_action_push_mpls *mpls = nla_data(a);
 
@@ -3108,6 +3137,10 @@  static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			proto = nla_get_be16(a);
 			mpls_label_count--;
 
+			if (proto == htons(ETH_P_TEB) &&
+			    mac_proto != MAC_PROTO_NONE)
+				return -EINVAL;
+
 			if (!eth_p_mpls(proto) || !mpls_label_count)
 				eth_type = htons(0);
 			else
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index f7c3b2e..d5723ea 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -798,8 +798,32 @@  struct ovs_action_push_tnl {
 };
 #endif
 
-/**
- * enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action.
+/* struct ovs_action_add_mpls - %OVS_ACTION_ATTR_ADD_MPLS action
+ * argument.
+ * @mpls_lse: MPLS label stack entry to push.
+ * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame.
+ * @tun_flags: MPLS tunnel attributes.
+ *
+ * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and
+ * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected.
+ */
+struct ovs_action_add_mpls {
+	__be32 mpls_lse;
+	__be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
+	__u16 tun_flags;
+};
+
+#define OVS_MPLS_L3_TUNNEL_FLAG_MASK  (1 << 0) /* Flag to specify the place of
+						* insertion of MPLS header.
+						* When false, the MPLS header
+						* will be inserted at the start
+						* of the packet.
+						* When true, the MPLS header
+						* will be inserted at the start
+						* of the l3 header.
+						*/
+
+/* enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action.
  * @OVS_CT_ATTR_COMMIT: If present, commits the connection to the conntrack
  * table. This allows future packets for the same connection to be identified
  * as 'established' or 'related'. The flow key for the current packet will
@@ -989,7 +1013,11 @@  struct check_pkt_len_arg {
  * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set
  * of actions if greater than the specified packet length, else execute
  * another set of actions.
- * @OVS_ACTION_ATTR_DROP: Explicit drop action.
+ * @OVS_ACTION_ATTR_ADD_MPLS: Push a new MPLS label stack entry at the
+ * start of the packet or at the start of the l3 header depending on the value
+ * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
+ * argument.
+  * @OVS_ACTION_ATTR_DROP: Explicit drop action.
  */
 
 enum ovs_action_attr {
@@ -1018,6 +1046,7 @@  enum ovs_action_attr {
 	OVS_ACTION_ATTR_METER,        /* u32 meter number. */
 	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
 	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
+	OVS_ACTION_ATTR_ADD_MPLS,     /* struct ovs_action_add_mpls. */
 
 #ifndef __KERNEL__
 	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/