diff mbox series

[ovs-dev] ovsdb: Fix timeout type for wait operation.

Message ID 20200525170702.761482-1-i.maximets@ovn.org
State Accepted
Headers show
Series [ovs-dev] ovsdb: Fix timeout type for wait operation. | expand

Commit Message

Ilya Maximets May 25, 2020, 5:07 p.m. UTC 
According to RFC 7047, 'timeout' is an integer field:

 5.2.6.  Wait
   The "wait" object contains the following members:
      "op": "wait"                        required
      "timeout": <integer>                optional
      ...

For some reason initial implementation treated it as a real number.

This causes a build issue with clang that complains that LLONG_MAX
could not be represented as double:

 ovsdb/execution.c:733:32: error: implicit conversion from 'long long'
                           to 'double' changes value from
                           9223372036854775807 to 9223372036854775808
            timeout_msec = MIN(LLONG_MAX, json_real(timeout));
                           ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 /usr/include/sys/limits.h:69:19: note: expanded from macro 'LLONG_MAX'
 #define LLONG_MAX       __LLONG_MAX     /* max for a long long */
                        ^~~~~~~~~~~
 /usr/include/x86/_limits.h:74:21: note: expanded from macro '__LLONG_MAX'
 #define __LLONG_MAX     0x7fffffffffffffffLL    /* max value for a long long */
                        ^~~~~~~~~~~~~~~~~~~~
 ./lib/util.h:90:21: note: expanded from macro 'MIN'
 #define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
                     ^  ~

Fix that by changing parser to treat 'timeout' as integer.
Fixes clang build on FreeBSD 12.1 in CirrusCI.

Fixes: f85f8ebbfac9 ("Initial implementation of OVSDB.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 ovsdb/execution.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Han Zhou May 28, 2020, 6:06 p.m. UTC | #1 
On Mon, May 25, 2020 at 10:07 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> According to RFC 7047, 'timeout' is an integer field:
>
>  5.2.6.  Wait
>    The "wait" object contains the following members:
>       "op": "wait"                        required
>       "timeout": <integer>                optional
>       ...
>
> For some reason initial implementation treated it as a real number.
>
> This causes a build issue with clang that complains that LLONG_MAX
> could not be represented as double:
>
>  ovsdb/execution.c:733:32: error: implicit conversion from 'long long'
>                            to 'double' changes value from
>                            9223372036854775807 to 9223372036854775808
>             timeout_msec = MIN(LLONG_MAX, json_real(timeout));
>                            ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  /usr/include/sys/limits.h:69:19: note: expanded from macro 'LLONG_MAX'
>  #define LLONG_MAX       __LLONG_MAX     /* max for a long long */
>                         ^~~~~~~~~~~
>  /usr/include/x86/_limits.h:74:21: note: expanded from macro '__LLONG_MAX'
>  #define __LLONG_MAX     0x7fffffffffffffffLL    /* max value for a long
long */
>                         ^~~~~~~~~~~~~~~~~~~~
>  ./lib/util.h:90:21: note: expanded from macro 'MIN'
>  #define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
>                      ^  ~
>
> Fix that by changing parser to treat 'timeout' as integer.
> Fixes clang build on FreeBSD 12.1 in CirrusCI.
>
> Fixes: f85f8ebbfac9 ("Initial implementation of OVSDB.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  ovsdb/execution.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ovsdb/execution.c b/ovsdb/execution.c
> index e45f3d679..3a0dad5d0 100644
> --- a/ovsdb/execution.c
> +++ b/ovsdb/execution.c
> @@ -712,7 +712,7 @@ ovsdb_execute_wait(struct ovsdb_execution *x, struct
ovsdb_parser *parser,
>      long long int timeout_msec = 0;
>      size_t i;
>
> -    timeout = ovsdb_parser_member(parser, "timeout", OP_NUMBER |
OP_OPTIONAL);
> +    timeout = ovsdb_parser_member(parser, "timeout", OP_INTEGER |
OP_OPTIONAL);
>      where = ovsdb_parser_member(parser, "where", OP_ARRAY);
>      columns_json = ovsdb_parser_member(parser, "columns",
>                                         OP_ARRAY | OP_OPTIONAL);
> @@ -730,7 +730,7 @@ ovsdb_execute_wait(struct ovsdb_execution *x, struct
ovsdb_parser *parser,
>      }
>      if (!error) {
>          if (timeout) {
> -            timeout_msec = MIN(LLONG_MAX, json_real(timeout));
> +            timeout_msec = json_integer(timeout);
>              if (timeout_msec < 0) {
>                  error = ovsdb_syntax_error(timeout, NULL,
>                                             "timeout must be
nonnegative");
> --
> 2.25.4
>

This looks good to me. (I didn't test myself)

Acked-by: Han Zhou <hzhou@ovn.org>
Numan Siddique May 29, 2020, 6:27 a.m. UTC | #2 
On Thu, May 28, 2020 at 11:37 PM Han Zhou <hzhou@ovn.org> wrote:

> On Mon, May 25, 2020 at 10:07 AM Ilya Maximets <i.maximets@ovn.org> wrote:
> >
> > According to RFC 7047, 'timeout' is an integer field:
> >
> >  5.2.6.  Wait
> >    The "wait" object contains the following members:
> >       "op": "wait"                        required
> >       "timeout": <integer>                optional
> >       ...
> >
> > For some reason initial implementation treated it as a real number.
> >
> > This causes a build issue with clang that complains that LLONG_MAX
> > could not be represented as double:
> >
> >  ovsdb/execution.c:733:32: error: implicit conversion from 'long long'
> >                            to 'double' changes value from
> >                            9223372036854775807 to 9223372036854775808
> >             timeout_msec = MIN(LLONG_MAX, json_real(timeout));
> >                            ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  /usr/include/sys/limits.h:69:19: note: expanded from macro 'LLONG_MAX'
> >  #define LLONG_MAX       __LLONG_MAX     /* max for a long long */
> >                         ^~~~~~~~~~~
> >  /usr/include/x86/_limits.h:74:21: note: expanded from macro
> '__LLONG_MAX'
> >  #define __LLONG_MAX     0x7fffffffffffffffLL    /* max value for a long
> long */
> >                         ^~~~~~~~~~~~~~~~~~~~
> >  ./lib/util.h:90:21: note: expanded from macro 'MIN'
> >  #define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
> >                      ^  ~
> >
> > Fix that by changing parser to treat 'timeout' as integer.
> > Fixes clang build on FreeBSD 12.1 in CirrusCI.
> >
> > Fixes: f85f8ebbfac9 ("Initial implementation of OVSDB.")
> > Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> > ---
> >  ovsdb/execution.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ovsdb/execution.c b/ovsdb/execution.c
> > index e45f3d679..3a0dad5d0 100644
> > --- a/ovsdb/execution.c
> > +++ b/ovsdb/execution.c
> > @@ -712,7 +712,7 @@ ovsdb_execute_wait(struct ovsdb_execution *x, struct
> ovsdb_parser *parser,
> >      long long int timeout_msec = 0;
> >      size_t i;
> >
> > -    timeout = ovsdb_parser_member(parser, "timeout", OP_NUMBER |
> OP_OPTIONAL);
> > +    timeout = ovsdb_parser_member(parser, "timeout", OP_INTEGER |
> OP_OPTIONAL);
> >      where = ovsdb_parser_member(parser, "where", OP_ARRAY);
> >      columns_json = ovsdb_parser_member(parser, "columns",
> >                                         OP_ARRAY | OP_OPTIONAL);
> > @@ -730,7 +730,7 @@ ovsdb_execute_wait(struct ovsdb_execution *x, struct
> ovsdb_parser *parser,
> >      }
> >      if (!error) {
> >          if (timeout) {
> > -            timeout_msec = MIN(LLONG_MAX, json_real(timeout));
> > +            timeout_msec = json_integer(timeout);
> >              if (timeout_msec < 0) {
> >                  error = ovsdb_syntax_error(timeout, NULL,
> >                                             "timeout must be
> nonnegative");
> > --
> > 2.25.4
> >
>
> This looks good to me. (I didn't test myself)
>
> Acked-by: Han Zhou <hzhou@ovn.org>
>

Applied the patch and tested it.

Acked-by: Numan Siddique <numans@ovn.org>

Thanks
Numan


> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Ilya Maximets June 1, 2020, 11:31 a.m. UTC | #3 
On 5/29/20 8:27 AM, Numan Siddique wrote:
> 
> 
> On Thu, May 28, 2020 at 11:37 PM Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> wrote:
> 
>     On Mon, May 25, 2020 at 10:07 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
>     >
>     > According to RFC 7047, 'timeout' is an integer field:
>     >
>     >  5.2.6.  Wait
>     >    The "wait" object contains the following members:
>     >       "op": "wait"                        required
>     >       "timeout": <integer>                optional
>     >       ...
>     >
>     > For some reason initial implementation treated it as a real number.
>     >
>     > This causes a build issue with clang that complains that LLONG_MAX
>     > could not be represented as double:
>     >
>     >  ovsdb/execution.c:733:32: error: implicit conversion from 'long long'
>     >                            to 'double' changes value from
>     >                            9223372036854775807 to 9223372036854775808
>     >             timeout_msec = MIN(LLONG_MAX, json_real(timeout));
>     >                            ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     >  /usr/include/sys/limits.h:69:19: note: expanded from macro 'LLONG_MAX'
>     >  #define LLONG_MAX       __LLONG_MAX     /* max for a long long */
>     >                         ^~~~~~~~~~~
>     >  /usr/include/x86/_limits.h:74:21: note: expanded from macro '__LLONG_MAX'
>     >  #define __LLONG_MAX     0x7fffffffffffffffLL    /* max value for a long
>     long */
>     >                         ^~~~~~~~~~~~~~~~~~~~
>     >  ./lib/util.h:90:21: note: expanded from macro 'MIN'
>     >  #define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
>     >                      ^  ~
>     >
>     > Fix that by changing parser to treat 'timeout' as integer.
>     > Fixes clang build on FreeBSD 12.1 in CirrusCI.
>     >
>     > Fixes: f85f8ebbfac9 ("Initial implementation of OVSDB.")
>     > Signed-off-by: Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>>
>     > ---
>     >  ovsdb/execution.c | 4 ++--
>     >  1 file changed, 2 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/ovsdb/execution.c b/ovsdb/execution.c
>     > index e45f3d679..3a0dad5d0 100644
>     > --- a/ovsdb/execution.c
>     > +++ b/ovsdb/execution.c
>     > @@ -712,7 +712,7 @@ ovsdb_execute_wait(struct ovsdb_execution *x, struct
>     ovsdb_parser *parser,
>     >      long long int timeout_msec = 0;
>     >      size_t i;
>     >
>     > -    timeout = ovsdb_parser_member(parser, "timeout", OP_NUMBER |
>     OP_OPTIONAL);
>     > +    timeout = ovsdb_parser_member(parser, "timeout", OP_INTEGER |
>     OP_OPTIONAL);
>     >      where = ovsdb_parser_member(parser, "where", OP_ARRAY);
>     >      columns_json = ovsdb_parser_member(parser, "columns",
>     >                                         OP_ARRAY | OP_OPTIONAL);
>     > @@ -730,7 +730,7 @@ ovsdb_execute_wait(struct ovsdb_execution *x, struct
>     ovsdb_parser *parser,
>     >      }
>     >      if (!error) {
>     >          if (timeout) {
>     > -            timeout_msec = MIN(LLONG_MAX, json_real(timeout));
>     > +            timeout_msec = json_integer(timeout);
>     >              if (timeout_msec < 0) {
>     >                  error = ovsdb_syntax_error(timeout, NULL,
>     >                                             "timeout must be
>     nonnegative");
>     > --
>     > 2.25.4
>     >
> 
>     This looks good to me. (I didn't test myself)
> 
>     Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>>
> 
> 
> Applied the patch and tested it.
> 
> Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>>

Thanks, Han and Numan!
Applied to master and backported down to 2.5.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/ovsdb/execution.c b/ovsdb/execution.c
index e45f3d679..3a0dad5d0 100644
--- a/ovsdb/execution.c
+++ b/ovsdb/execution.c
@@ -712,7 +712,7 @@  ovsdb_execute_wait(struct ovsdb_execution *x, struct ovsdb_parser *parser,
     long long int timeout_msec = 0;
     size_t i;
 
-    timeout = ovsdb_parser_member(parser, "timeout", OP_NUMBER | OP_OPTIONAL);
+    timeout = ovsdb_parser_member(parser, "timeout", OP_INTEGER | OP_OPTIONAL);
     where = ovsdb_parser_member(parser, "where", OP_ARRAY);
     columns_json = ovsdb_parser_member(parser, "columns",
                                        OP_ARRAY | OP_OPTIONAL);
@@ -730,7 +730,7 @@  ovsdb_execute_wait(struct ovsdb_execution *x, struct ovsdb_parser *parser,
     }
     if (!error) {
         if (timeout) {
-            timeout_msec = MIN(LLONG_MAX, json_real(timeout));
+            timeout_msec = json_integer(timeout);
             if (timeout_msec < 0) {
                 error = ovsdb_syntax_error(timeout, NULL,
                                            "timeout must be nonnegative");