[ovs-dev] ovn: Clean up log() action parsing errors.
diff mbox series

Message ID 20180731060319.27558-1-jpettit@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] ovn: Clean up log() action parsing errors.
Related show

Commit Message

Justin Pettit July 31, 2018, 6:03 a.m. UTC
This also add some OVN action parsing tests.

Suggested-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 ovn/lib/actions.c |  9 ++++++++-
 tests/ovn.at      | 19 +++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

Comments

0-day Robot July 31, 2018, 6:55 a.m. UTC | #1
Bleep bloop.  Greetings Justin Pettit, 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:
depbase=`echo ovn/controller/lflow.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
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 -Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT ovn/controller/lflow.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/lflow.o ovn/controller/lflow.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ovn/controller/lport.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
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 -Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT ovn/controller/lport.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/lport.o ovn/controller/lport.c &&\
mv -f $depbase.Tpo $depbase.Po
depbase=`echo ovn/controller/ofctrl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
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 -Wno-null-pointer-arithmetic -Werror -Werror   -g -O2 -MT ovn/controller/ofctrl.o -MD -MP -MF $depbase.Tpo -c -o ovn/controller/ofctrl.o ovn/controller/ofctrl.c &&\
mv -f $depbase.Tpo $depbase.Po
ovn/controller/ofctrl.c: In function ‘ofctrl_put’:
ovn/controller/ofctrl.c:1086:9: error: missing initializer for field ‘flags’ of ‘struct ofputil_meter_config’ [-Werror=missing-field-initializers]
         };
         ^
In file included from ovn/controller/ofctrl.c:35:0:
./include/openvswitch/ofp-meter.h:53:14: note: ‘flags’ declared here
     uint16_t flags;
              ^
ovn/controller/ofctrl.c: At top level:
cc1: error: unrecognized command line option "-Wno-null-pointer-arithmetic" [-Werror]
cc1: all warnings being treated as errors
make[2]: *** [ovn/controller/ofctrl.o] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


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

Thanks,
0-day Robot
Mark Michelson July 31, 2018, 12:21 p.m. UTC | #2
The 0-day Robot needs to lay off the pipe.

Acked-by: Mark Michelson <mmichels@redhat.com>

On 07/31/2018 02:03 AM, Justin Pettit wrote:
> This also add some OVN action parsing tests.
> 
> Suggested-by: Ben Pfaff <blp@ovn.org>
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> ---
>   ovn/lib/actions.c |  9 ++++++++-
>   tests/ovn.at      | 19 +++++++++++++++++++
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index 2126fd57551e..bc1a20040cff 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -2071,7 +2071,8 @@ parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
>           } else if (lexer_match_id(ctx->lexer, "allow")) {
>               log->verdict = LOG_VERDICT_ALLOW;
>           } else {
> -            lexer_syntax_error(ctx->lexer, "unknown acl verdict");
> +            lexer_syntax_error(ctx->lexer, "unknown verdict");
> +            return;
>           }
>       } else if (lexer_match_id(ctx->lexer, "name")) {
>           if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> @@ -2103,6 +2104,9 @@ parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
>                   log->severity = severity;
>                   lexer_get(ctx->lexer);
>                   return;
> +            } else {
> +                lexer_syntax_error(ctx->lexer, "unknown severity");
> +                return;
>               }
>           }
>           lexer_syntax_error(ctx->lexer, "expecting severity");
> @@ -2142,6 +2146,9 @@ parse_LOG(struct action_context *ctx)
>               lexer_match(ctx->lexer, LEX_T_COMMA);
>           }
>       }
> +    if (log->verdict == LOG_VERDICT_UNKNOWN) {
> +        lexer_syntax_error(ctx->lexer, "expecting verdict");
> +    }
>   }
>   
>   static void
> diff --git a/tests/ovn.at b/tests/ovn.at
> index df80b98d64b4..163f5c590516 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1224,6 +1224,25 @@ set_meter(100, 1000, );
>   set_meter(4294967295, 4294967295);
>       encodes as meter:3
>   
> +# log
> +log(verdict=allow, severity=warning);
> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.00.04)
> +log(name="test1", verdict=drop, severity=info);
> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31)
> +log(verdict=drop, severity=info, meter="meter1");
> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06,meter_id=4)
> +log(name="test1", verdict=drop, severity=info, meter="meter1");
> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31,meter_id=4)
> +log(verdict=drop);
> +    formats as log(verdict=drop, severity=info);
> +    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06)
> +log(verdict=bad_verdict, severity=info);
> +    Syntax error at `bad_verdict' unknown verdict.
> +log(verdict=drop, severity=bad_severity);
> +    Syntax error at `bad_severity' unknown severity.
> +log(severity=notice);
> +    Syntax error at `;' expecting verdict.
> +
>   # put_nd_ra_opts
>   reg1[0] = put_nd_ra_opts(addr_mode = "slaac", mtu = 1500, prefix = aef0::/64, slla = ae:01:02:03:04:05);
>       encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.00.ff.ff.00.00.00.00.00.00.00.00.05.01.00.00.00.00.05.dc.03.04.40.c0.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.05,pause)
>
Justin Pettit July 31, 2018, 4:13 p.m. UTC | #3
> On Jul 31, 2018, at 5:21 AM, Mark Michelson <mmichels@redhat.com> wrote:
> 
> The 0-day Robot needs to lay off the pipe.

Yeah, I'm not sure what was happening there.  I emailed Aaron about it.

> Acked-by: Mark Michelson <mmichels@redhat.com>

Thanks!  I pushed this to master and branch-2.10.

--Justin
Ben Pfaff July 31, 2018, 4:58 p.m. UTC | #4
On Tue, Jul 31, 2018 at 09:13:37AM -0700, Justin Pettit wrote:
> 
> > On Jul 31, 2018, at 5:21 AM, Mark Michelson <mmichels@redhat.com> wrote:
> > 
> > The 0-day Robot needs to lay off the pipe.
> 
> Yeah, I'm not sure what was happening there.  I emailed Aaron about it.

It's just a GCC warning.  Fix:
https://patchwork.ozlabs.org/patch/951704/
Justin Pettit July 31, 2018, 5 p.m. UTC | #5
> On Jul 31, 2018, at 9:58 AM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Tue, Jul 31, 2018 at 09:13:37AM -0700, Justin Pettit wrote:
>> 
>>> On Jul 31, 2018, at 5:21 AM, Mark Michelson <mmichels@redhat.com> wrote:
>>> 
>>> The 0-day Robot needs to lay off the pipe.
>> 
>> Yeah, I'm not sure what was happening there.  I emailed Aaron about it.
> 
> It's just a GCC warning.  Fix:
> https://patchwork.ozlabs.org/patch/951704/

Interesting.  I don't see it with gcc 7.3.0.  Thanks for the fix.

--Justin
Ben Pfaff July 31, 2018, 5:16 p.m. UTC | #6
On Tue, Jul 31, 2018 at 10:00:42AM -0700, Justin Pettit wrote:
> 
> > On Jul 31, 2018, at 9:58 AM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > On Tue, Jul 31, 2018 at 09:13:37AM -0700, Justin Pettit wrote:
> >> 
> >>> On Jul 31, 2018, at 5:21 AM, Mark Michelson <mmichels@redhat.com> wrote:
> >>> 
> >>> The 0-day Robot needs to lay off the pipe.
> >> 
> >> Yeah, I'm not sure what was happening there.  I emailed Aaron about it.
> > 
> > It's just a GCC warning.  Fix:
> > https://patchwork.ozlabs.org/patch/951704/
> 
> Interesting.  I don't see it with gcc 7.3.0.  Thanks for the fix.

Older GCC had some weird warnings.
Aaron Conole July 31, 2018, 8:25 p.m. UTC | #7
Justin Pettit <jpettit@ovn.org> writes:

>> On Jul 31, 2018, at 5:21 AM, Mark Michelson <mmichels@redhat.com> wrote:
>> 
>> The 0-day Robot needs to lay off the pipe.
>
> Yeah, I'm not sure what was happening there.  I emailed Aaron about it.

Sorry about that.  I'll work on protecting the list for when the robot
indulges in compile-altering substances.  It's a young bot -
experimentation is to be expected.

>> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> Thanks!  I pushed this to master and branch-2.10.
>
> --Justin
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ben Pfaff July 31, 2018, 8:36 p.m. UTC | #8
On Tue, Jul 31, 2018 at 04:25:32PM -0400, Aaron Conole wrote:
> Justin Pettit <jpettit@ovn.org> writes:
> 
> >> On Jul 31, 2018, at 5:21 AM, Mark Michelson <mmichels@redhat.com> wrote:
> >> 
> >> The 0-day Robot needs to lay off the pipe.
> >
> > Yeah, I'm not sure what was happening there.  I emailed Aaron about it.
> 
> Sorry about that.  I'll work on protecting the list for when the robot
> indulges in compile-altering substances.  It's a young bot -
> experimentation is to be expected.

For what it's worth, I appreciate the bot.  It needs a little training
but that's OK.

(And I still giggle a little every time I see the "Bleep, bloop"
greeting.)

Patch
diff mbox series

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 2126fd57551e..bc1a20040cff 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -2071,7 +2071,8 @@  parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
         } else if (lexer_match_id(ctx->lexer, "allow")) {
             log->verdict = LOG_VERDICT_ALLOW;
         } else {
-            lexer_syntax_error(ctx->lexer, "unknown acl verdict");
+            lexer_syntax_error(ctx->lexer, "unknown verdict");
+            return;
         }
     } else if (lexer_match_id(ctx->lexer, "name")) {
         if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
@@ -2103,6 +2104,9 @@  parse_log_arg(struct action_context *ctx, struct ovnact_log *log)
                 log->severity = severity;
                 lexer_get(ctx->lexer);
                 return;
+            } else {
+                lexer_syntax_error(ctx->lexer, "unknown severity");
+                return;
             }
         }
         lexer_syntax_error(ctx->lexer, "expecting severity");
@@ -2142,6 +2146,9 @@  parse_LOG(struct action_context *ctx)
             lexer_match(ctx->lexer, LEX_T_COMMA);
         }
     }
+    if (log->verdict == LOG_VERDICT_UNKNOWN) {
+        lexer_syntax_error(ctx->lexer, "expecting verdict");
+    }
 }
 
 static void
diff --git a/tests/ovn.at b/tests/ovn.at
index df80b98d64b4..163f5c590516 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1224,6 +1224,25 @@  set_meter(100, 1000, );
 set_meter(4294967295, 4294967295);
     encodes as meter:3
 
+# log
+log(verdict=allow, severity=warning);
+    encodes as controller(userdata=00.00.00.07.00.00.00.00.00.04)
+log(name="test1", verdict=drop, severity=info);
+    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31)
+log(verdict=drop, severity=info, meter="meter1");
+    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06,meter_id=4)
+log(name="test1", verdict=drop, severity=info, meter="meter1");
+    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06.74.65.73.74.31,meter_id=4)
+log(verdict=drop);
+    formats as log(verdict=drop, severity=info);
+    encodes as controller(userdata=00.00.00.07.00.00.00.00.01.06)
+log(verdict=bad_verdict, severity=info);
+    Syntax error at `bad_verdict' unknown verdict.
+log(verdict=drop, severity=bad_severity);
+    Syntax error at `bad_severity' unknown severity.
+log(severity=notice);
+    Syntax error at `;' expecting verdict.
+
 # put_nd_ra_opts
 reg1[0] = put_nd_ra_opts(addr_mode = "slaac", mtu = 1500, prefix = aef0::/64, slla = ae:01:02:03:04:05);
     encodes as controller(userdata=00.00.00.08.00.00.00.00.00.01.de.10.00.00.00.40.86.00.00.00.ff.00.ff.ff.00.00.00.00.00.00.00.00.05.01.00.00.00.00.05.dc.03.04.40.c0.ff.ff.ff.ff.ff.ff.ff.ff.00.00.00.00.ae.f0.00.00.00.00.00.00.00.00.00.00.00.00.00.00.01.01.ae.01.02.03.04.05,pause)