| Message ID | 20171031142704.13870-3-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for IPv6 load balancers | expand |
On Tue, Oct 31, 2017 at 09:27:02AM -0500, Mark Michelson wrote: > The ct_lb action previously assumed that any address arguments were > IPv4. This patch expands the parsing, formatting, and encoding of ct_lb > to be amenable to IPv6 addresses as well. > > Signed-off-by: Mark Michelson <mmichels@redhat.com> Thanks for working on this. With this change, I get the following warning from Clang 3.8: ../ovn/lib/actions.c:939:32: error: variable 'dst' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] ../ovn/lib/actions.c:953:30: note: uninitialized use occurs here ../ovn/lib/actions.c:939:28: note: remove the 'if' if its condition is always true ../ovn/lib/actions.c:932:21: error: variable 'dst' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] ../ovn/lib/actions.c:953:30: note: uninitialized use occurs here ../ovn/lib/actions.c:932:17: note: remove the 'if' if its condition is always true ../ovn/lib/actions.c:886:13: note: variable 'dst' is declared here I can't figure out what it's talking about, and I can't figure out a simple way to suppress it. Do you have any idea?
On Wed, Nov 1, 2017 at 4:53 PM Ben Pfaff <blp@ovn.org> wrote: > On Tue, Oct 31, 2017 at 09:27:02AM -0500, Mark Michelson wrote: > > The ct_lb action previously assumed that any address arguments were > > IPv4. This patch expands the parsing, formatting, and encoding of ct_lb > > to be amenable to IPv6 addresses as well. > > > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > > Thanks for working on this. > > With this change, I get the following warning from Clang 3.8: > > ../ovn/lib/actions.c:939:32: error: variable 'dst' is used > uninitialized whenever 'if' condition is false > [-Werror,-Wsometimes-uninitialized] > ../ovn/lib/actions.c:953:30: note: uninitialized use occurs here > ../ovn/lib/actions.c:939:28: note: remove the 'if' if its condition is > always true > ../ovn/lib/actions.c:932:21: error: variable 'dst' is used > uninitialized whenever 'if' condition is false > [-Werror,-Wsometimes-uninitialized] > ../ovn/lib/actions.c:953:30: note: uninitialized use occurs here > ../ovn/lib/actions.c:932:17: note: remove the 'if' if its condition is > always true > ../ovn/lib/actions.c:886:13: note: variable 'dst' is declared here > > I can't figure out what it's talking about, and I can't figure out a > simple way to suppress it. Do you have any idea? > I see what's wrong. There is a declaration of a "dst" variable in an inner scope that shadows the declaration of a "dst" variable in an outer scope. In the case where the if statement is false, the inner scope "dst" gets declared and its parameters set. What we want is for the outer scope "dst" to have its parameters set in that case. The easy solution is just to remove the inner scope declaration of dst. I'll do that and repost the patch.
On Thu, Nov 02, 2017 at 02:57:00PM +0000, Mark Michelson wrote: > On Wed, Nov 1, 2017 at 4:53 PM Ben Pfaff <blp@ovn.org> wrote: > > > On Tue, Oct 31, 2017 at 09:27:02AM -0500, Mark Michelson wrote: > > > The ct_lb action previously assumed that any address arguments were > > > IPv4. This patch expands the parsing, formatting, and encoding of ct_lb > > > to be amenable to IPv6 addresses as well. > > > > > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > > > > Thanks for working on this. > > > > With this change, I get the following warning from Clang 3.8: > > > > ../ovn/lib/actions.c:939:32: error: variable 'dst' is used > > uninitialized whenever 'if' condition is false > > [-Werror,-Wsometimes-uninitialized] > > ../ovn/lib/actions.c:953:30: note: uninitialized use occurs here > > ../ovn/lib/actions.c:939:28: note: remove the 'if' if its condition is > > always true > > ../ovn/lib/actions.c:932:21: error: variable 'dst' is used > > uninitialized whenever 'if' condition is false > > [-Werror,-Wsometimes-uninitialized] > > ../ovn/lib/actions.c:953:30: note: uninitialized use occurs here > > ../ovn/lib/actions.c:932:17: note: remove the 'if' if its condition is > > always true > > ../ovn/lib/actions.c:886:13: note: variable 'dst' is declared here > > > > I can't figure out what it's talking about, and I can't figure out a > > simple way to suppress it. Do you have any idea? > > > > I see what's wrong. There is a declaration of a "dst" variable in an inner > scope that shadows the declaration of a "dst" variable in an outer scope. > In the case where the if statement is false, the inner scope "dst" gets > declared and its parameters set. What we want is for the outer scope "dst" > to have its parameters set in that case. The easy solution is just to > remove the inner scope declaration of dst. I'll do that and repost the > patch. Thanks!
diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 0a04af7aa..63885da3c 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -200,7 +200,11 @@ struct ovnact_ct_nat { }; struct ovnact_ct_lb_dst { - ovs_be32 ip; + int family; + union { + struct in6_addr ipv6; + ovs_be32 ipv4; + }; uint16_t port; }; diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c index c9876436d..3c21eb382 100644 --- a/ovn/lib/actions.c +++ b/ovn/lib/actions.c @@ -883,23 +883,63 @@ parse_ct_lb_action(struct action_context *ctx) if (lexer_match(ctx->lexer, LEX_T_LPAREN)) { while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) { - if (ctx->lexer->token.type != LEX_T_INTEGER - || mf_subvalue_width(&ctx->lexer->token.value) > 32) { - free(dsts); - lexer_syntax_error(ctx->lexer, "expecting IPv4 address"); - return; - } + struct ovnact_ct_lb_dst dst; + if (lexer_match(ctx->lexer, LEX_T_LSQUARE)) { + /* IPv6 address and port */ + if (ctx->lexer->token.type != LEX_T_INTEGER + || ctx->lexer->token.format != LEX_F_IPV6) { + free(dsts); + lexer_syntax_error(ctx->lexer, "expecting IPv6 address"); + return; + } + dst.family = AF_INET6; + dst.ipv6 = ctx->lexer->token.value.ipv6; - /* Parse IP. */ - ovs_be32 ip = ctx->lexer->token.value.ipv4; - lexer_get(ctx->lexer); + lexer_get(ctx->lexer); + if (!lexer_match(ctx->lexer, LEX_T_RSQUARE)) { + free(dsts); + lexer_syntax_error(ctx->lexer, "no closing square " + "bracket"); + return; + } + dst.port = 0; + if (lexer_match(ctx->lexer, LEX_T_COLON) + && !action_parse_port(ctx, &dst.port)) { + free(dsts); + return; + } + } else { + if (ctx->lexer->token.type != LEX_T_INTEGER + || (ctx->lexer->token.format != LEX_F_IPV4 + && ctx->lexer->token.format != LEX_F_IPV6)) { + free(dsts); + lexer_syntax_error(ctx->lexer, "expecting IP address"); + return; + } - /* Parse optional port. */ - uint16_t port = 0; - if (lexer_match(ctx->lexer, LEX_T_COLON) - && !action_parse_port(ctx, &port)) { - free(dsts); - return; + /* Parse IP. */ + struct ovnact_ct_lb_dst dst; + if (ctx->lexer->token.format == LEX_F_IPV4) { + dst.family = AF_INET; + dst.ipv4 = ctx->lexer->token.value.ipv4; + } else { + dst.family = AF_INET6; + dst.ipv6 = ctx->lexer->token.value.ipv6; + } + + lexer_get(ctx->lexer); + dst.port = 0; + if (lexer_match(ctx->lexer, LEX_T_COLON)) { + if (dst.family == AF_INET6) { + free(dsts); + lexer_syntax_error(ctx->lexer, "IPv6 address needs " + "square brackets if port is included"); + return; + } else if (!action_parse_port(ctx, &dst.port)) { + free(dsts); + return; + } + } } lexer_match(ctx->lexer, LEX_T_COMMA); @@ -907,7 +947,7 @@ parse_ct_lb_action(struct action_context *ctx) if (n_dsts >= allocated_dsts) { dsts = x2nrealloc(dsts, &allocated_dsts, sizeof *dsts); } - dsts[n_dsts++] = (struct ovnact_ct_lb_dst) { ip, port }; + dsts[n_dsts++] = dst; } } @@ -929,9 +969,19 @@ format_CT_LB(const struct ovnact_ct_lb *cl, struct ds *s) } const struct ovnact_ct_lb_dst *dst = &cl->dsts[i]; - ds_put_format(s, IP_FMT, IP_ARGS(dst->ip)); - if (dst->port) { - ds_put_format(s, ":%"PRIu16, dst->port); + if (dst->family == AF_INET) { + ds_put_format(s, IP_FMT, IP_ARGS(dst->ipv4)); + if (dst->port) { + ds_put_format(s, ":%"PRIu16, dst->port); + } + } else { + if (dst->port) { + ds_put_char(s, '['); + } + ipv6_format_addr(&dst->ipv6, s); + if (dst->port) { + ds_put_format(s, "]:%"PRIu16, dst->port); + } } } ds_put_char(s, ')'); @@ -991,8 +1041,17 @@ encode_CT_LB(const struct ovnact_ct_lb *cl, BUILD_ASSERT(MFF_LOG_DNAT_ZONE < MFF_REG0 + FLOW_N_REGS); for (size_t bucket_id = 0; bucket_id < cl->n_dsts; bucket_id++) { const struct ovnact_ct_lb_dst *dst = &cl->dsts[bucket_id]; + char ip_addr[INET6_ADDRSTRLEN]; + if (dst->family == AF_INET) { + inet_ntop(AF_INET, &dst->ipv4, ip_addr, sizeof ip_addr); + } else { + inet_ntop(AF_INET6, &dst->ipv6, ip_addr, sizeof ip_addr); + } ds_put_format(&ds, ",bucket=bucket_id=%"PRIuSIZE",weight:100,actions=" - "ct(nat(dst="IP_FMT, bucket_id, IP_ARGS(dst->ip)); + "ct(nat(dst=%s%s%s", bucket_id, + dst->family == AF_INET6 && dst->port ? "[" : "", + ip_addr, + dst->family == AF_INET6 && dst->port ? "]" : ""); if (dst->port) { ds_put_format(&ds, ":%"PRIu16, dst->port); } diff --git a/tests/ovn.at b/tests/ovn.at index 490841c63..67709b55b 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -825,13 +825,19 @@ ct_lb(192.168.1.2, 192.168.1.3, ); formats as ct_lb(192.168.1.2, 192.168.1.3); encodes as group:2 has prereqs ip +ct_lb(fd0f::2, fd0f::3, ); + formats as ct_lb(fd0f::2, fd0f::3); + encodes as group:3 + has prereqs ip ct_lb(192.168.1.2:); Syntax error at `)' expecting port number. ct_lb(192.168.1.2:123456); Syntax error at `123456' expecting port number. ct_lb(foo); - Syntax error at `foo' expecting IPv4 address. + Syntax error at `foo' expecting IP address. +ct_lb([192.168.1.2]); + Syntax error at `192.168.1.2' expecting IPv6 address. # ct_next ct_next;
The ct_lb action previously assumed that any address arguments were IPv4. This patch expands the parsing, formatting, and encoding of ct_lb to be amenable to IPv6 addresses as well. Signed-off-by: Mark Michelson <mmichels@redhat.com> --- include/ovn/actions.h | 6 +++- ovn/lib/actions.c | 99 ++++++++++++++++++++++++++++++++++++++++----------- tests/ovn.at | 8 ++++- 3 files changed, 91 insertions(+), 22 deletions(-)