On Mon, Oct 22, 2018 at 11:21 PM Mark Michelson <[email protected]> wrote:
> On 10/22/2018 11:49 AM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > The test "ovn-nbctl: LBs - daemon" fails when it runs the command > > "ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80". > ovn-nbctl > > extracts the vip by calling the socket util function > 'inet_parse_active()', > > and this function blocks when it calls dns_resolve(). It blocks because > > networking is disabled with mock rpm build. > > > > To address this issue, this patch adds a new function - > > inet_parse_active_address_and_port() which expects IP:[port] address > > in the 'target_' argument and disables resolving the host. > > > > This new function is now used in ovn-northd and ovn-nbctl. It is fine to > > use this function as load balancer VIP cannot be a hostname. > > I think the change should also be made in ovn-trace.c, parse_lb_option(). > Thanks Timothy for the comments and testing it out and Mark for the comments. I have addressed the review comments and submitted the v2 - https://patchwork.ozlabs.org/patch/988035/ Thanks Numan > > > > Reported-by: Timothy Redaelli <[email protected]> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1641672 > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > lib/socket-util.c | 49 ++++++++++++++++++++++++++++----------- > > lib/socket-util.h | 2 ++ > > ovn/northd/ovn-northd.c | 2 +- > > ovn/utilities/ovn-nbctl.c | 6 ++--- > > 4 files changed, 42 insertions(+), 17 deletions(-) > > > > diff --git a/lib/socket-util.c b/lib/socket-util.c > > index df9b01a9e..fcd1e5dc5 100644 > > --- a/lib/socket-util.c > > +++ b/lib/socket-util.c > > @@ -512,18 +512,9 @@ exit: > > return false; > > } > > > > -/* Parses 'target', which should be a string in the format > "<host>[:<port>]". > > - * <host>, which is required, may be an IPv4 address or an IPv6 address > > - * enclosed in square brackets. If 'default_port' is nonnegative then > <port> > > - * is optional and defaults to 'default_port' (use 0 to make the kernel > choose > > - * an available port, although this isn't usually appropriate for active > > - * connections). If 'default_port' is negative, then <port> is > required. > > - * > > - * On success, returns true and stores the parsed remote address into > '*ss'. > > - * On failure, logs an error, stores zeros into '*ss', and returns > false. */ > > -bool > > -inet_parse_active(const char *target_, int default_port, > > - struct sockaddr_storage *ss) > > +static bool > > +inet_parse_active__(const char *target_, int default_port, > > + struct sockaddr_storage *ss, bool resolve_host) > > { > > char *target = xstrdup(target_); > > char *port, *host; > > @@ -538,7 +529,7 @@ inet_parse_active(const char *target_, int > default_port, > > ok = false; > > } else { > > ok = parse_sockaddr_components(ss, host, port, default_port, > > - target_, true); > > + target_, resolve_host); > > } > > if (!ok) { > > memset(ss, 0, sizeof *ss); > > @@ -547,6 +538,38 @@ inet_parse_active(const char *target_, int > default_port, > > return ok; > > } > > > > +/* Parses 'target', which should be a string in the format > "<host>[:<port>]". > > + * <host>, which is required, may be an IPv4 address or an IPv6 address > > I see that the difference between this and > inet_parse_active_address_and_port() is that you use the term <host> > instead of <IP>. However, the text still only mentions that <host> may > be an IPv4 or IPv6 address. I think also specifying that a hostname is > accepted would make the description more clear. > > > + * enclosed in square brackets. If 'default_port' is nonnegative then > <port> > > + * is optional and defaults to 'default_port' (use 0 to make the kernel > choose > > + * an available port, although this isn't usually appropriate for active > > + * connections). If 'default_port' is negative, then <port> is > required. > > + * > > + * On success, returns true and stores the parsed remote address into > '*ss'. > > + * On failure, logs an error, stores zeros into '*ss', and returns > false. */ > > +bool > > +inet_parse_active(const char *target_, int default_port, > > + struct sockaddr_storage *ss) > > +{ > > + return inet_parse_active__(target_, default_port, ss, true); > > +} > > + > > +/* Parses 'target', which should be a string in the format > "<IP>[:<port>]". > > + * <IP>, which is required, should be an IPv4 address or an IPv6 address > > + * and may be enclosed in square brackets. If 'default_port' is > nonnegative > > + * then <port> is optional and defaults to 'default_port' (use 0 to > make the > > + * kernel choose an available port, although this isn't usually > appropriate for > > + * active connections). If 'default_port' is negative, then <port> is > > + * required. > > + * > > + * On success, returns true and stores the parsed remote address into > '*ss'. > > + * On failure, logs an error, stores zeros into '*ss', and returns > false. */ > > +bool > > +inet_parse_active_address_and_port(const char *target_, int > default_port, > > + struct sockaddr_storage *ss) > > +{ > > + return inet_parse_active__(target_, default_port, ss, false); > > +} > > > > /* Opens a non-blocking IPv4 or IPv6 socket of the specified 'style' > and > > * connects to 'target', which should be a string in the format > > diff --git a/lib/socket-util.h b/lib/socket-util.h > > index 6d386304d..447a12cfc 100644 > > --- a/lib/socket-util.h > > +++ b/lib/socket-util.h > > @@ -50,6 +50,8 @@ void inet_parse_host_port_tokens(char *s, char > **hostp, char **portp); > > void inet_parse_port_host_tokens(char *s, char **portp, char **hostp); > > bool inet_parse_active(const char *target, int default_port, > > struct sockaddr_storage *ssp); > > +bool inet_parse_active_address_and_port(const char *target, int > default_port, > > + struct sockaddr_storage *ssp); > > int inet_open_active(int style, const char *target, int default_port, > > struct sockaddr_storage *ssp, int *fdp, uint8_t > dscp); > > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index 439651f80..ddc696499 100644 > > --- a/ovn/northd/ovn-northd.c > > +++ b/ovn/northd/ovn-northd.c > > @@ -3209,7 +3209,7 @@ ip_address_and_port_from_lb_key(const char *key, > char **ip_address, > > uint16_t *port, int *addr_family) > > { > > struct sockaddr_storage ss; > > - if (!inet_parse_active(key, 0, &ss)) { > > + if (!inet_parse_active_address_and_port(key, 0, &ss)) { > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > VLOG_WARN_RL(&rl, "bad ip address or port for load balancer > key %s", > > key); > > diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c > > index 798c97276..a2ab646fe 100644 > > --- a/ovn/utilities/ovn-nbctl.c > > +++ b/ovn/utilities/ovn-nbctl.c > > @@ -2637,7 +2637,7 @@ nbctl_lb_add(struct ctl_context *ctx) > > } > > > > struct sockaddr_storage ss_vip; > > - if (!inet_parse_active(lb_vip, 0, &ss_vip)) { > > + if (!inet_parse_active_address_and_port(lb_vip, 0, &ss_vip)) { > > ctl_error(ctx, "%s: should be an IP address (or an IP address " > > "and a port number with : as a separator).", lb_vip); > > return; > > @@ -2667,7 +2667,7 @@ nbctl_lb_add(struct ctl_context *ctx) > > struct sockaddr_storage ss_dst; > > > > if (lb_vip_port) { > > - if (!inet_parse_active(token, -1, &ss_dst)) { > > + if (!inet_parse_active_address_and_port(token, -1, > &ss_dst)) { > > ctl_error(ctx, "%s: should be an IP address and a port > " > > "number with : as a separator.", token); > > goto out; > > @@ -2786,7 +2786,7 @@ lb_info_add_smap(const struct nbrec_load_balancer > *lb, > > const struct smap_node *node = nodes[i]; > > > > struct sockaddr_storage ss; > > - if (!inet_parse_active(node->key, 0, &ss)) { > > + if (!inet_parse_active_address_and_port(node->key, 0, &ss)) > { > > continue; > > } > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
