[ovs-dev,06/11] Make <host>:<port> parsing uniform treewide.

Message ID 20180413172655.31638-6-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev,01/11] socket-util: Fix error in comment on ss_format_address().
Related show

Commit Message

Ben Pfaff April 13, 2018, 5:26 p.m.
I didn't realize until now that the tree had two different ways of parsing
strings in the form <host>:<port> and <port>:<host>.  There are the
long-standing inet_parse_active() and inet_parse_passive() functions, and
more recently the ipv46_parse() function.  This commit eliminates the
latter and changes the code to use the former.

The two implementations interpreted some input differently.  In particular,
the older functions required IPv6 addresses to be [bracketed], but the
newer ones did not.  I'd prefer the brackets to be required, because of
ambiguous cases like "::1:2:3:4:80" (is :80 part of the IPv6 address or a
port number?) but for compatibility this patch changes the merged code to
use the more liberal interpretation.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/packets.c             |  76 -------------------------
 lib/packets.h             |  10 ----
 lib/socket-util.c         | 140 +++++++++++++++++++++++++++-------------------
 lib/socket-util.h         |   3 +-
 ovn/utilities/ovn-nbctl.c |  43 +++-----------
 ovsdb/raft.c              |   5 +-
 tests/ovn-nbctl.at        |  56 +++++++++----------
 7 files changed, 125 insertions(+), 208 deletions(-)

Comments

Mark Michelson April 16, 2018, 7:28 p.m. | #1
Hi Ben,

The cleanup of IPv6 address parsing is greatly appreciated. I haven't 
yet reviewed the meat of this patchset, but I figured I'd weigh in on 
the commit message on this patch.

I wrote the newer parsing code when extending load balancers to work for 
IPv6. The issues I ran into were:

* Load balancer addresses in OVN have optional ports.
* Other user commands in OVN that use IPv6 addresses do not require 
square brackets around the IPv6 address (e.g. `ovn-nbctl lsp-set-addresses`)

Interestingly, there had not yet been any user commands that would 
accept an IPv6 address with an optional port. The choices were either to 
require square brackets around all IPv6 addresses used in load 
balancers, or allow bracket-less addresses when no port was specified. 
To me, it would seem weird as a user if I had to put square brackets 
around IPv6 addresses for some commands but not for others. Also in 
other programs I've used, I am not used to including brackets on an IPv6 
address when there is no port present. I elected to go with the method 
that allowed for brackets to be required only when a port is present.

Unfortunately, no such function existed that would allow for this. 
inet_parse_active() was close, but it required square brackets around 
IPv6 addresses. So that's why I wrote the new function.

To address your ambiguity concern, "::1:2:3:4:80" is an IPv6 address 
with no port. With no square brackets present, that's the only sensible 
way to interpret it.

I'll have a deeper look at the entire patchset and give more feedback on 
it later.

Mark!

On 04/13/2018 12:26 PM, Ben Pfaff wrote:
> I didn't realize until now that the tree had two different ways of parsing
> strings in the form <host>:<port> and <port>:<host>.  There are the
> long-standing inet_parse_active() and inet_parse_passive() functions, and
> more recently the ipv46_parse() function.  This commit eliminates the
> latter and changes the code to use the former.
> 
> The two implementations interpreted some input differently.  In particular,
> the older functions required IPv6 addresses to be [bracketed], but the
> newer ones did not.  I'd prefer the brackets to be required, because of
> ambiguous cases like "::1:2:3:4:80" (is :80 part of the IPv6 address or a
> port number?) but for compatibility this patch changes the merged code to
> use the more liberal interpretation.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>   lib/packets.c             |  76 -------------------------
>   lib/packets.h             |  10 ----
>   lib/socket-util.c         | 140 +++++++++++++++++++++++++++-------------------
>   lib/socket-util.h         |   3 +-
>   ovn/utilities/ovn-nbctl.c |  43 +++-----------
>   ovsdb/raft.c              |   5 +-
>   tests/ovn-nbctl.at        |  56 +++++++++----------
>   7 files changed, 125 insertions(+), 208 deletions(-)
> 
> diff --git a/lib/packets.c b/lib/packets.c
> index 8ebae8cc8fe8..38bfb6015b9e 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -652,82 +652,6 @@ ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
>       return error;
>   }
>   
> -/* Parses the string into an IPv4 or IPv6 address.
> - * The port flags act as follows:
> - * * PORT_OPTIONAL: A port may be present but is not required
> - * * PORT_REQUIRED: A port must be present
> - * * PORT_FORBIDDEN: A port must not be present
> - */
> -char * OVS_WARN_UNUSED_RESULT
> -ipv46_parse(const char *s, enum port_flags flags, struct sockaddr_storage *ss)
> -{
> -    char *error = NULL;
> -
> -    char *copy;
> -    copy = xstrdup(s);
> -
> -    char *addr;
> -    char *port;
> -    if (*copy == '[') {
> -        char *end;
> -
> -        addr = copy + 1;
> -        end = strchr(addr, ']');
> -        if (!end) {
> -            error = xasprintf("No closing bracket on address %s", s);
> -            goto finish;
> -        }
> -        *end++ = '\0';
> -        if (*end == ':') {
> -            port = end + 1;
> -        } else {
> -            port = NULL;
> -        }
> -    } else {
> -        addr = copy;
> -        port = strchr(copy, ':');
> -        if (port) {
> -            if (strchr(port + 1, ':')) {
> -                port = NULL;
> -            } else {
> -                *port++ = '\0';
> -            }
> -        }
> -    }
> -
> -    if (port && !*port) {
> -        error = xasprintf("Port is an empty string");
> -        goto finish;
> -    }
> -
> -    if (port && flags == PORT_FORBIDDEN) {
> -        error = xasprintf("Port forbidden in address %s", s);
> -        goto finish;
> -    } else if (!port && flags == PORT_REQUIRED) {
> -        error = xasprintf("Port required in address %s", s);
> -        goto finish;
> -    }
> -
> -    struct addrinfo hints = {
> -        .ai_flags = AI_NUMERICHOST | AI_NUMERICSERV,
> -        .ai_family = AF_UNSPEC,
> -    };
> -    struct addrinfo *res;
> -    int status;
> -    status = getaddrinfo(addr, port, &hints, &res);
> -    if (status) {
> -        error = xasprintf("Error parsing address %s: %s",
> -                s, gai_strerror(status));
> -        goto finish;
> -    }
> -    memcpy(ss, res->ai_addr, res->ai_addrlen);
> -    freeaddrinfo(res);
> -
> -finish:
> -    free(copy);
> -    return error;
> -}
> -
>   /* Parses string 's', which must be an IPv6 address.  Stores the IPv6 address
>    * into '*ip'.  Returns true if successful, otherwise false. */
>   bool
> diff --git a/lib/packets.h b/lib/packets.h
> index 9a71aa3abbdb..b2bf70697e90 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1362,16 +1362,6 @@ struct in6_addr ipv6_create_mask(int mask);
>   int ipv6_count_cidr_bits(const struct in6_addr *netmask);
>   bool ipv6_is_cidr(const struct in6_addr *netmask);
>   
> -enum port_flags {
> -    PORT_OPTIONAL,
> -    PORT_REQUIRED,
> -    PORT_FORBIDDEN,
> -};
> -
> -char *ipv46_parse(const char *s, enum port_flags flags,
> -                  struct sockaddr_storage *ss)
> -    OVS_WARN_UNUSED_RESULT;
> -
>   bool ipv6_parse(const char *s, struct in6_addr *ip);
>   char *ipv6_parse_masked(const char *s, struct in6_addr *ipv6,
>                           struct in6_addr *mask);
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 223e3780ba5f..0964a015e3f9 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -333,39 +333,89 @@ guess_netmask(ovs_be32 ip_)
>               : htonl(0));                          /* ??? */
>   }
>   
> -/* This is like strsep() except:
> - *
> - *    - The separator string is ":".
> - *
> - *    - Square brackets [] quote ":" separators and are removed from the
> - *      tokens. */
> -char *
> -inet_parse_token(char **pp)
> +static char *
> +unbracket(char *s)
> +{
> +    if (*s == '[') {
> +        s++;
> +
> +        char *end = strchr(s, '\0');
> +        if (end[-1] == ']') {
> +            end[-1] = '\0';
> +        }
> +    }
> +    return s;
> +}
> +
> +/* 'host_index' is 0 if the host precedes the port within 's', 1 otherwise. */
> +static void
> +inet_parse_tokens__(char *s, int host_index, char **hostp, char **portp)
>   {
> -    char *p = *pp;
> -
> -    if (p == NULL) {
> -        return NULL;
> -    } else if (*p == '\0') {
> -        *pp = NULL;
> -        return p;
> -    } else if (*p == '[') {
> -        char *start = p + 1;
> -        char *end = start + strcspn(start, "]");
> -        *pp = (*end == '\0' ? NULL
> -               : end[1] == ':' ? end + 2
> -               : end + 1);
> -        *end = '\0';
> -        return start;
> +    char *colon = NULL;
> +    bool in_brackets = false;
> +    int n_colons = 0;
> +    for (char *p = s; *p; p++) {
> +        if (*p == '[') {
> +            in_brackets = true;
> +        } else if (*p == ']') {
> +            in_brackets = false;
> +        } else if (*p == ':' && !in_brackets) {
> +            n_colons++;
> +            colon = p;
> +        }
> +    }
> +
> +    *hostp = *portp = NULL;
> +    if (n_colons > 1) {
> +        *hostp = s;
>       } else {
> -        char *start = p;
> -        char *end = start + strcspn(start, ":");
> -        *pp = *end == '\0' ? NULL : end + 1;
> -        *end = '\0';
> -        return start;
> +        char **tokens[2];
> +        tokens[host_index] = hostp;
> +        tokens[!host_index] = portp;
> +
> +        if (colon) {
> +            *colon = '\0';
> +            *tokens[1] = unbracket(colon + 1);
> +        }
> +        *tokens[0] = unbracket(s);
>       }
>   }
>   
> +/* Parses 's', a string in the form "<host>[:<port>]", into its (required) host
> + * and (optional) port components, and stores pointers to them in '*hostp' and
> + * '*portp' respectively.  Always sets '*hostp' nonnull, although possibly to
> + * an empty string empty string.  Can set '*portp' to the null string.
> + *
> + * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> + * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> + * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> + * is a host but "[::1:2:3:4]:80" is a host and a port.
> + *
> + * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> + */
> +void
> +inet_parse_host_port_tokens(char *s, char **hostp, char **portp)
> +{
> +    inet_parse_tokens__(s, 0, hostp, portp);
> +}
> +
> +/* Parses 's', a string in the form "<port>[:<host>]", into its port and host
> + * components, and stores pointers to them in '*portp' and '*hostp'
> + * respectively.  Both '*portp' and '*hostp' can end up null.
> + *
> + * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> + * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> + * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> + * is a host but "[::1:2:3:4]:80" is a host and a port.
> + *
> + * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> + */
> +void
> +inet_parse_port_host_tokens(char *s, char **portp, char **hostp)
> +{
> +    inet_parse_tokens__(s, 1, hostp, portp);
> +}
> +
>   static bool
>   parse_sockaddr_components(struct sockaddr_storage *ss,
>                             char *host_s,
> @@ -441,23 +491,16 @@ inet_parse_active(const char *target_, int default_port,
>                     struct sockaddr_storage *ss)
>   {
>       char *target = xstrdup(target_);
> -    const char *port;
> -    char *host;
> -    char *p;
> +    char *port, *host;
>       bool ok;
>   
> -    p = target;
> -    host = inet_parse_token(&p);
> -    port = inet_parse_token(&p);
> +    inet_parse_host_port_tokens(target, &host, &port);
>       if (!host) {
>           VLOG_ERR("%s: host must be specified", target_);
>           ok = false;
>       } else if (!port && default_port < 0) {
>           VLOG_ERR("%s: port must be specified", target_);
>           ok = false;
> -    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> -        VLOG_ERR("%s: unexpected characters follow host and port", target_);
> -        ok = false;
>       } else {
>           ok = parse_sockaddr_components(ss, host, port, default_port, target_);
>       }
> @@ -571,20 +614,13 @@ inet_parse_passive(const char *target_, int default_port,
>                      struct sockaddr_storage *ss)
>   {
>       char *target = xstrdup(target_);
> -    const char *port;
> -    char *host;
> -    char *p;
> +    char *port, *host;
>       bool ok;
>   
> -    p = target;
> -    port = inet_parse_token(&p);
> -    host = inet_parse_token(&p);
> +    inet_parse_port_host_tokens(target, &port, &host);
>       if (!port && default_port < 0) {
>           VLOG_ERR("%s: port must be specified", target_);
>           ok = false;
> -    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> -        VLOG_ERR("%s: unexpected characters follow port and host", target_);
> -        ok = false;
>       } else {
>           ok = parse_sockaddr_components(ss, host, port, default_port, target_);
>       }
> @@ -707,16 +743,8 @@ bool
>   inet_parse_address(const char *target_, struct sockaddr_storage *ss)
>   {
>       char *target = xstrdup(target_);
> -    char *p = target;
> -    char *host = inet_parse_token(&p);
> -    bool ok = false;
> -    if (!host) {
> -        VLOG_ERR("%s: host must be specified", target_);
> -    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> -        VLOG_ERR("%s: unexpected characters follow host", target_);
> -    } else {
> -        ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
> -    }
> +    char *host = unbracket(target);
> +    bool ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
>       if (!ok) {
>           memset(ss, 0, sizeof *ss);
>       }
> diff --git a/lib/socket-util.h b/lib/socket-util.h
> index d927d67a0e1b..239d3f22041c 100644
> --- a/lib/socket-util.h
> +++ b/lib/socket-util.h
> @@ -46,7 +46,8 @@ int check_connection_completion(int fd);
>   void drain_fd(int fd, size_t n_packets);
>   ovs_be32 guess_netmask(ovs_be32 ip);
>   
> -char *inet_parse_token(char **);
> +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);
>   int inet_open_active(int style, const char *target, int default_port,
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 7efc5065c962..16698de1f049 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -1798,10 +1798,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>       }
>   
>       struct sockaddr_storage ss_vip;
> -    char *error;
> -    error = ipv46_parse(lb_vip, PORT_OPTIONAL, &ss_vip);
> -    if (error) {
> -        free(error);
> +    if (!inet_parse_active(lb_vip, 0, &ss_vip)) {
>           ctl_fatal("%s: should be an IP address (or an IP address "
>                     "and a port number with : as a separator).", lb_vip);
>       }
> @@ -1848,17 +1845,13 @@ nbctl_lb_add(struct ctl_context *ctx)
>               token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
>           struct sockaddr_storage ss_dst;
>   
> -        error = ipv46_parse(token, is_vip_with_port
> -                ? PORT_REQUIRED
> -                : PORT_FORBIDDEN,
> -                &ss_dst);
> -
> -        if (error) {
> -            free(error);
> -            if (is_vip_with_port) {
> +        if (is_vip_with_port) {
> +            if (!inet_parse_active(token, -1, &ss_dst)) {
>                   ctl_fatal("%s: should be an IP address and a port "
> -                        "number with : as a separator.", token);
> -            } else {
> +                          "number with : as a separator.", token);
> +            }
> +        } else {
> +            if (!inet_parse_address(token, &ss_dst)) {
>                   ctl_fatal("%s: should be an IP address.", token);
>               }
>           }
> @@ -1954,36 +1947,18 @@ lb_info_add_smap(const struct nbrec_load_balancer *lb,
>   {
>       struct ds key = DS_EMPTY_INITIALIZER;
>       struct ds val = DS_EMPTY_INITIALIZER;
> -    char *error, *protocol;
>   
>       const struct smap_node **nodes = smap_sort(&lb->vips);
>       if (nodes) {
>           for (int i = 0; i < smap_count(&lb->vips); i++) {
>               const struct smap_node *node = nodes[i];
> -            protocol = lb->protocol;
>   
>               struct sockaddr_storage ss;
> -            error = ipv46_parse(node->key, PORT_OPTIONAL, &ss);
> -            if (error) {
> -                VLOG_WARN("%s", error);
> -                free(error);
> +            if (!inet_parse_active(node->key, 0, &ss)) {
>                   continue;
>               }
>   
> -            if (ss.ss_family == AF_INET) {
> -                struct sockaddr_in *sin = ALIGNED_CAST(struct sockaddr_in *,
> -                                                       &ss);
> -                if (!sin->sin_port) {
> -                    protocol = "tcp/udp";
> -                }
> -            } else {
> -                struct sockaddr_in6 *sin6 = ALIGNED_CAST(struct sockaddr_in6 *,
> -                                                         &ss);
> -                if (!sin6->sin6_port) {
> -                    protocol = "tcp/udp";
> -                }
> -            }
> -
> +            char *protocol = ss_get_port(&ss) ? lb->protocol : "tcp/udp";
>               i == 0 ? ds_put_format(&val,
>                           UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
>                           UUID_ARGS(&lb->header_.uuid),
> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> index edb22f2e7b60..c0c1e98977b9 100644
> --- a/ovsdb/raft.c
> +++ b/ovsdb/raft.c
> @@ -335,9 +335,8 @@ raft_make_address_passive(const char *address_)
>           return xasprintf("p%s", address_);
>       } else {
>           char *address = xstrdup(address_);
> -        char *p = strchr(address, ':') + 1;
> -        char *host = inet_parse_token(&p);
> -        char *port = inet_parse_token(&p);
> +        char *host, *port;
> +        inet_parse_host_port_tokens(strchr(address, ':') + 1, &host, &port);
>   
>           struct ds paddr = DS_EMPTY_INITIALIZER;
>           ds_put_format(&paddr, "p%.3s:%s:", address, port);
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index f07b47fd20ea..514e7e7d2d49 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -460,46 +460,46 @@ AT_SETUP([ovn-nbctl - LBs])
>   OVN_NBCTL_TEST_START
>   
>   dnl Add two LBs.
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
>   [ovn-nbctl: 30.0.0.10:80a: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
>   [ovn-nbctl: 30.0.0.10:a80: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> -[ovn-nbctl: 30.0.0.10:: should be an IP address (or an IP address and a port number with : as a separator).
> -])
> -
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
>   [ovn-nbctl: 192.168.10.20: should be an IP address and a port number with : as a separator.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
>   [ovn-nbctl: 30.0.0.1a: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
> -[ovn-nbctl: 192.168.10.10:80: should be an IP address.
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
> +[ovn-nbctl: 30.0.0: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
>   [ovn-nbctl: 192.168.10.20:80: should be an IP address.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
>   [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
>   [ovn-nbctl: 192.168.10.10:: should be an IP address.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
>   [ovn-nbctl: 192.168.10.1a: should be an IP address.
>   ])
>   
> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> +[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +])
> +
>   AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>   [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>   ])
> @@ -685,56 +685,56 @@ AT_SETUP([ovn-nbctl - LBs IPv6])
>   OVN_NBCTL_TEST_START
>   
>   dnl A bunch of commands that should fail
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
>   [ovn-nbctl: [[ae0f::10]]:80a: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
>   [ovn-nbctl: [[ae0f::10]]:a80: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> -[ovn-nbctl: [[ae0f::10]]:: should be an IP address (or an IP address and a port number with : as a separator).
> -])
> -
> -
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
>   [ovn-nbctl: fd0f::20: should be an IP address and a port number with : as a separator.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
>   [ovn-nbctl: ae0f::10fff: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
>   [ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
>   [ovn-nbctl: [[fd0f::20]]:80: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
>   [ovn-nbctl: [[fd0f::10]]:a80: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
>   [ovn-nbctl: [[fd0f::10]]:: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
>   [ovn-nbctl: fd0f::1001a: should be an IP address.
>   ])
>   
>   
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> +[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +])
> +
> +
>   AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10 tcp], [1], [],
>   [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>   ])
>
Ben Pfaff April 16, 2018, 7:59 p.m. | #2
Hi Mark.

I hope my commit message did not come across as critical.  I understand
why the new API was added.  If criticism is warranted, it should be
directed toward me as having failed to originally catch the redundancy
in the review process.

Feedback is welcome (and needed for review).  I'll look forward to it.

On Mon, Apr 16, 2018 at 02:28:29PM -0500, Mark Michelson wrote:
> Hi Ben,
> 
> The cleanup of IPv6 address parsing is greatly appreciated. I haven't yet
> reviewed the meat of this patchset, but I figured I'd weigh in on the commit
> message on this patch.
> 
> I wrote the newer parsing code when extending load balancers to work for
> IPv6. The issues I ran into were:
> 
> * Load balancer addresses in OVN have optional ports.
> * Other user commands in OVN that use IPv6 addresses do not require square
> brackets around the IPv6 address (e.g. `ovn-nbctl lsp-set-addresses`)
> 
> Interestingly, there had not yet been any user commands that would accept an
> IPv6 address with an optional port. The choices were either to require
> square brackets around all IPv6 addresses used in load balancers, or allow
> bracket-less addresses when no port was specified. To me, it would seem
> weird as a user if I had to put square brackets around IPv6 addresses for
> some commands but not for others. Also in other programs I've used, I am not
> used to including brackets on an IPv6 address when there is no port present.
> I elected to go with the method that allowed for brackets to be required
> only when a port is present.
> 
> Unfortunately, no such function existed that would allow for this.
> inet_parse_active() was close, but it required square brackets around IPv6
> addresses. So that's why I wrote the new function.
> 
> To address your ambiguity concern, "::1:2:3:4:80" is an IPv6 address with no
> port. With no square brackets present, that's the only sensible way to
> interpret it.
> 
> I'll have a deeper look at the entire patchset and give more feedback on it
> later.
> 
> Mark!
> 
> On 04/13/2018 12:26 PM, Ben Pfaff wrote:
> >I didn't realize until now that the tree had two different ways of parsing
> >strings in the form <host>:<port> and <port>:<host>.  There are the
> >long-standing inet_parse_active() and inet_parse_passive() functions, and
> >more recently the ipv46_parse() function.  This commit eliminates the
> >latter and changes the code to use the former.
> >
> >The two implementations interpreted some input differently.  In particular,
> >the older functions required IPv6 addresses to be [bracketed], but the
> >newer ones did not.  I'd prefer the brackets to be required, because of
> >ambiguous cases like "::1:2:3:4:80" (is :80 part of the IPv6 address or a
> >port number?) but for compatibility this patch changes the merged code to
> >use the more liberal interpretation.
> >
> >Signed-off-by: Ben Pfaff <blp@ovn.org>
> >---
> >  lib/packets.c             |  76 -------------------------
> >  lib/packets.h             |  10 ----
> >  lib/socket-util.c         | 140 +++++++++++++++++++++++++++-------------------
> >  lib/socket-util.h         |   3 +-
> >  ovn/utilities/ovn-nbctl.c |  43 +++-----------
> >  ovsdb/raft.c              |   5 +-
> >  tests/ovn-nbctl.at        |  56 +++++++++----------
> >  7 files changed, 125 insertions(+), 208 deletions(-)
> >
> >diff --git a/lib/packets.c b/lib/packets.c
> >index 8ebae8cc8fe8..38bfb6015b9e 100644
> >--- a/lib/packets.c
> >+++ b/lib/packets.c
> >@@ -652,82 +652,6 @@ ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
> >      return error;
> >  }
> >-/* Parses the string into an IPv4 or IPv6 address.
> >- * The port flags act as follows:
> >- * * PORT_OPTIONAL: A port may be present but is not required
> >- * * PORT_REQUIRED: A port must be present
> >- * * PORT_FORBIDDEN: A port must not be present
> >- */
> >-char * OVS_WARN_UNUSED_RESULT
> >-ipv46_parse(const char *s, enum port_flags flags, struct sockaddr_storage *ss)
> >-{
> >-    char *error = NULL;
> >-
> >-    char *copy;
> >-    copy = xstrdup(s);
> >-
> >-    char *addr;
> >-    char *port;
> >-    if (*copy == '[') {
> >-        char *end;
> >-
> >-        addr = copy + 1;
> >-        end = strchr(addr, ']');
> >-        if (!end) {
> >-            error = xasprintf("No closing bracket on address %s", s);
> >-            goto finish;
> >-        }
> >-        *end++ = '\0';
> >-        if (*end == ':') {
> >-            port = end + 1;
> >-        } else {
> >-            port = NULL;
> >-        }
> >-    } else {
> >-        addr = copy;
> >-        port = strchr(copy, ':');
> >-        if (port) {
> >-            if (strchr(port + 1, ':')) {
> >-                port = NULL;
> >-            } else {
> >-                *port++ = '\0';
> >-            }
> >-        }
> >-    }
> >-
> >-    if (port && !*port) {
> >-        error = xasprintf("Port is an empty string");
> >-        goto finish;
> >-    }
> >-
> >-    if (port && flags == PORT_FORBIDDEN) {
> >-        error = xasprintf("Port forbidden in address %s", s);
> >-        goto finish;
> >-    } else if (!port && flags == PORT_REQUIRED) {
> >-        error = xasprintf("Port required in address %s", s);
> >-        goto finish;
> >-    }
> >-
> >-    struct addrinfo hints = {
> >-        .ai_flags = AI_NUMERICHOST | AI_NUMERICSERV,
> >-        .ai_family = AF_UNSPEC,
> >-    };
> >-    struct addrinfo *res;
> >-    int status;
> >-    status = getaddrinfo(addr, port, &hints, &res);
> >-    if (status) {
> >-        error = xasprintf("Error parsing address %s: %s",
> >-                s, gai_strerror(status));
> >-        goto finish;
> >-    }
> >-    memcpy(ss, res->ai_addr, res->ai_addrlen);
> >-    freeaddrinfo(res);
> >-
> >-finish:
> >-    free(copy);
> >-    return error;
> >-}
> >-
> >  /* Parses string 's', which must be an IPv6 address.  Stores the IPv6 address
> >   * into '*ip'.  Returns true if successful, otherwise false. */
> >  bool
> >diff --git a/lib/packets.h b/lib/packets.h
> >index 9a71aa3abbdb..b2bf70697e90 100644
> >--- a/lib/packets.h
> >+++ b/lib/packets.h
> >@@ -1362,16 +1362,6 @@ struct in6_addr ipv6_create_mask(int mask);
> >  int ipv6_count_cidr_bits(const struct in6_addr *netmask);
> >  bool ipv6_is_cidr(const struct in6_addr *netmask);
> >-enum port_flags {
> >-    PORT_OPTIONAL,
> >-    PORT_REQUIRED,
> >-    PORT_FORBIDDEN,
> >-};
> >-
> >-char *ipv46_parse(const char *s, enum port_flags flags,
> >-                  struct sockaddr_storage *ss)
> >-    OVS_WARN_UNUSED_RESULT;
> >-
> >  bool ipv6_parse(const char *s, struct in6_addr *ip);
> >  char *ipv6_parse_masked(const char *s, struct in6_addr *ipv6,
> >                          struct in6_addr *mask);
> >diff --git a/lib/socket-util.c b/lib/socket-util.c
> >index 223e3780ba5f..0964a015e3f9 100644
> >--- a/lib/socket-util.c
> >+++ b/lib/socket-util.c
> >@@ -333,39 +333,89 @@ guess_netmask(ovs_be32 ip_)
> >              : htonl(0));                          /* ??? */
> >  }
> >-/* This is like strsep() except:
> >- *
> >- *    - The separator string is ":".
> >- *
> >- *    - Square brackets [] quote ":" separators and are removed from the
> >- *      tokens. */
> >-char *
> >-inet_parse_token(char **pp)
> >+static char *
> >+unbracket(char *s)
> >+{
> >+    if (*s == '[') {
> >+        s++;
> >+
> >+        char *end = strchr(s, '\0');
> >+        if (end[-1] == ']') {
> >+            end[-1] = '\0';
> >+        }
> >+    }
> >+    return s;
> >+}
> >+
> >+/* 'host_index' is 0 if the host precedes the port within 's', 1 otherwise. */
> >+static void
> >+inet_parse_tokens__(char *s, int host_index, char **hostp, char **portp)
> >  {
> >-    char *p = *pp;
> >-
> >-    if (p == NULL) {
> >-        return NULL;
> >-    } else if (*p == '\0') {
> >-        *pp = NULL;
> >-        return p;
> >-    } else if (*p == '[') {
> >-        char *start = p + 1;
> >-        char *end = start + strcspn(start, "]");
> >-        *pp = (*end == '\0' ? NULL
> >-               : end[1] == ':' ? end + 2
> >-               : end + 1);
> >-        *end = '\0';
> >-        return start;
> >+    char *colon = NULL;
> >+    bool in_brackets = false;
> >+    int n_colons = 0;
> >+    for (char *p = s; *p; p++) {
> >+        if (*p == '[') {
> >+            in_brackets = true;
> >+        } else if (*p == ']') {
> >+            in_brackets = false;
> >+        } else if (*p == ':' && !in_brackets) {
> >+            n_colons++;
> >+            colon = p;
> >+        }
> >+    }
> >+
> >+    *hostp = *portp = NULL;
> >+    if (n_colons > 1) {
> >+        *hostp = s;
> >      } else {
> >-        char *start = p;
> >-        char *end = start + strcspn(start, ":");
> >-        *pp = *end == '\0' ? NULL : end + 1;
> >-        *end = '\0';
> >-        return start;
> >+        char **tokens[2];
> >+        tokens[host_index] = hostp;
> >+        tokens[!host_index] = portp;
> >+
> >+        if (colon) {
> >+            *colon = '\0';
> >+            *tokens[1] = unbracket(colon + 1);
> >+        }
> >+        *tokens[0] = unbracket(s);
> >      }
> >  }
> >+/* Parses 's', a string in the form "<host>[:<port>]", into its (required) host
> >+ * and (optional) port components, and stores pointers to them in '*hostp' and
> >+ * '*portp' respectively.  Always sets '*hostp' nonnull, although possibly to
> >+ * an empty string empty string.  Can set '*portp' to the null string.
> >+ *
> >+ * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> >+ * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> >+ * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> >+ * is a host but "[::1:2:3:4]:80" is a host and a port.
> >+ *
> >+ * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> >+ */
> >+void
> >+inet_parse_host_port_tokens(char *s, char **hostp, char **portp)
> >+{
> >+    inet_parse_tokens__(s, 0, hostp, portp);
> >+}
> >+
> >+/* Parses 's', a string in the form "<port>[:<host>]", into its port and host
> >+ * components, and stores pointers to them in '*portp' and '*hostp'
> >+ * respectively.  Both '*portp' and '*hostp' can end up null.
> >+ *
> >+ * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> >+ * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> >+ * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> >+ * is a host but "[::1:2:3:4]:80" is a host and a port.
> >+ *
> >+ * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> >+ */
> >+void
> >+inet_parse_port_host_tokens(char *s, char **portp, char **hostp)
> >+{
> >+    inet_parse_tokens__(s, 1, hostp, portp);
> >+}
> >+
> >  static bool
> >  parse_sockaddr_components(struct sockaddr_storage *ss,
> >                            char *host_s,
> >@@ -441,23 +491,16 @@ inet_parse_active(const char *target_, int default_port,
> >                    struct sockaddr_storage *ss)
> >  {
> >      char *target = xstrdup(target_);
> >-    const char *port;
> >-    char *host;
> >-    char *p;
> >+    char *port, *host;
> >      bool ok;
> >-    p = target;
> >-    host = inet_parse_token(&p);
> >-    port = inet_parse_token(&p);
> >+    inet_parse_host_port_tokens(target, &host, &port);
> >      if (!host) {
> >          VLOG_ERR("%s: host must be specified", target_);
> >          ok = false;
> >      } else if (!port && default_port < 0) {
> >          VLOG_ERR("%s: port must be specified", target_);
> >          ok = false;
> >-    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> >-        VLOG_ERR("%s: unexpected characters follow host and port", target_);
> >-        ok = false;
> >      } else {
> >          ok = parse_sockaddr_components(ss, host, port, default_port, target_);
> >      }
> >@@ -571,20 +614,13 @@ inet_parse_passive(const char *target_, int default_port,
> >                     struct sockaddr_storage *ss)
> >  {
> >      char *target = xstrdup(target_);
> >-    const char *port;
> >-    char *host;
> >-    char *p;
> >+    char *port, *host;
> >      bool ok;
> >-    p = target;
> >-    port = inet_parse_token(&p);
> >-    host = inet_parse_token(&p);
> >+    inet_parse_port_host_tokens(target, &port, &host);
> >      if (!port && default_port < 0) {
> >          VLOG_ERR("%s: port must be specified", target_);
> >          ok = false;
> >-    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> >-        VLOG_ERR("%s: unexpected characters follow port and host", target_);
> >-        ok = false;
> >      } else {
> >          ok = parse_sockaddr_components(ss, host, port, default_port, target_);
> >      }
> >@@ -707,16 +743,8 @@ bool
> >  inet_parse_address(const char *target_, struct sockaddr_storage *ss)
> >  {
> >      char *target = xstrdup(target_);
> >-    char *p = target;
> >-    char *host = inet_parse_token(&p);
> >-    bool ok = false;
> >-    if (!host) {
> >-        VLOG_ERR("%s: host must be specified", target_);
> >-    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> >-        VLOG_ERR("%s: unexpected characters follow host", target_);
> >-    } else {
> >-        ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
> >-    }
> >+    char *host = unbracket(target);
> >+    bool ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
> >      if (!ok) {
> >          memset(ss, 0, sizeof *ss);
> >      }
> >diff --git a/lib/socket-util.h b/lib/socket-util.h
> >index d927d67a0e1b..239d3f22041c 100644
> >--- a/lib/socket-util.h
> >+++ b/lib/socket-util.h
> >@@ -46,7 +46,8 @@ int check_connection_completion(int fd);
> >  void drain_fd(int fd, size_t n_packets);
> >  ovs_be32 guess_netmask(ovs_be32 ip);
> >-char *inet_parse_token(char **);
> >+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);
> >  int inet_open_active(int style, const char *target, int default_port,
> >diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> >index 7efc5065c962..16698de1f049 100644
> >--- a/ovn/utilities/ovn-nbctl.c
> >+++ b/ovn/utilities/ovn-nbctl.c
> >@@ -1798,10 +1798,7 @@ nbctl_lb_add(struct ctl_context *ctx)
> >      }
> >      struct sockaddr_storage ss_vip;
> >-    char *error;
> >-    error = ipv46_parse(lb_vip, PORT_OPTIONAL, &ss_vip);
> >-    if (error) {
> >-        free(error);
> >+    if (!inet_parse_active(lb_vip, 0, &ss_vip)) {
> >          ctl_fatal("%s: should be an IP address (or an IP address "
> >                    "and a port number with : as a separator).", lb_vip);
> >      }
> >@@ -1848,17 +1845,13 @@ nbctl_lb_add(struct ctl_context *ctx)
> >              token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
> >          struct sockaddr_storage ss_dst;
> >-        error = ipv46_parse(token, is_vip_with_port
> >-                ? PORT_REQUIRED
> >-                : PORT_FORBIDDEN,
> >-                &ss_dst);
> >-
> >-        if (error) {
> >-            free(error);
> >-            if (is_vip_with_port) {
> >+        if (is_vip_with_port) {
> >+            if (!inet_parse_active(token, -1, &ss_dst)) {
> >                  ctl_fatal("%s: should be an IP address and a port "
> >-                        "number with : as a separator.", token);
> >-            } else {
> >+                          "number with : as a separator.", token);
> >+            }
> >+        } else {
> >+            if (!inet_parse_address(token, &ss_dst)) {
> >                  ctl_fatal("%s: should be an IP address.", token);
> >              }
> >          }
> >@@ -1954,36 +1947,18 @@ lb_info_add_smap(const struct nbrec_load_balancer *lb,
> >  {
> >      struct ds key = DS_EMPTY_INITIALIZER;
> >      struct ds val = DS_EMPTY_INITIALIZER;
> >-    char *error, *protocol;
> >      const struct smap_node **nodes = smap_sort(&lb->vips);
> >      if (nodes) {
> >          for (int i = 0; i < smap_count(&lb->vips); i++) {
> >              const struct smap_node *node = nodes[i];
> >-            protocol = lb->protocol;
> >              struct sockaddr_storage ss;
> >-            error = ipv46_parse(node->key, PORT_OPTIONAL, &ss);
> >-            if (error) {
> >-                VLOG_WARN("%s", error);
> >-                free(error);
> >+            if (!inet_parse_active(node->key, 0, &ss)) {
> >                  continue;
> >              }
> >-            if (ss.ss_family == AF_INET) {
> >-                struct sockaddr_in *sin = ALIGNED_CAST(struct sockaddr_in *,
> >-                                                       &ss);
> >-                if (!sin->sin_port) {
> >-                    protocol = "tcp/udp";
> >-                }
> >-            } else {
> >-                struct sockaddr_in6 *sin6 = ALIGNED_CAST(struct sockaddr_in6 *,
> >-                                                         &ss);
> >-                if (!sin6->sin6_port) {
> >-                    protocol = "tcp/udp";
> >-                }
> >-            }
> >-
> >+            char *protocol = ss_get_port(&ss) ? lb->protocol : "tcp/udp";
> >              i == 0 ? ds_put_format(&val,
> >                          UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
> >                          UUID_ARGS(&lb->header_.uuid),
> >diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> >index edb22f2e7b60..c0c1e98977b9 100644
> >--- a/ovsdb/raft.c
> >+++ b/ovsdb/raft.c
> >@@ -335,9 +335,8 @@ raft_make_address_passive(const char *address_)
> >          return xasprintf("p%s", address_);
> >      } else {
> >          char *address = xstrdup(address_);
> >-        char *p = strchr(address, ':') + 1;
> >-        char *host = inet_parse_token(&p);
> >-        char *port = inet_parse_token(&p);
> >+        char *host, *port;
> >+        inet_parse_host_port_tokens(strchr(address, ':') + 1, &host, &port);
> >          struct ds paddr = DS_EMPTY_INITIALIZER;
> >          ds_put_format(&paddr, "p%.3s:%s:", address, port);
> >diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> >index f07b47fd20ea..514e7e7d2d49 100644
> >--- a/tests/ovn-nbctl.at
> >+++ b/tests/ovn-nbctl.at
> >@@ -460,46 +460,46 @@ AT_SETUP([ovn-nbctl - LBs])
> >  OVN_NBCTL_TEST_START
> >  dnl Add two LBs.
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> >  [ovn-nbctl: 30.0.0.10:80a: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> >  [ovn-nbctl: 30.0.0.10:a80: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> >-[ovn-nbctl: 30.0.0.10:: should be an IP address (or an IP address and a port number with : as a separator).
> >-])
> >-
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
> >  [ovn-nbctl: 192.168.10.20: should be an IP address and a port number with : as a separator.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
> >  [ovn-nbctl: 30.0.0.1a: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
> >-[ovn-nbctl: 192.168.10.10:80: should be an IP address.
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
> >+[ovn-nbctl: 30.0.0: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
> >  [ovn-nbctl: 192.168.10.20:80: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
> >  [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
> >  [ovn-nbctl: 192.168.10.10:: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
> >  [ovn-nbctl: 192.168.10.1a: should be an IP address.
> >  ])
> >+AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> >+[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >+])
> >+
> >  AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
> >  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >  ])
> >@@ -685,56 +685,56 @@ AT_SETUP([ovn-nbctl - LBs IPv6])
> >  OVN_NBCTL_TEST_START
> >  dnl A bunch of commands that should fail
> >-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >  [ovn-nbctl: [[ae0f::10]]:80a: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >  [ovn-nbctl: [[ae0f::10]]:a80: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >-[ovn-nbctl: [[ae0f::10]]:: should be an IP address (or an IP address and a port number with : as a separator).
> >-])
> >-
> >-
> >-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> >  [ovn-nbctl: fd0f::20: should be an IP address and a port number with : as a separator.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> >  [ovn-nbctl: ae0f::10fff: should be an IP address (or an IP address and a port number with : as a separator).
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> >  [ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
> >  [ovn-nbctl: [[fd0f::20]]:80: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
> >  [ovn-nbctl: [[fd0f::10]]:a80: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
> >  [ovn-nbctl: [[fd0f::10]]:: should be an IP address.
> >  ])
> >-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
> >  [ovn-nbctl: fd0f::1001a: should be an IP address.
> >  ])
> >+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> >+[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >+])
> >+
> >+
> >  AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10 tcp], [1], [],
> >  [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> >  ])
> >
>
Mark Michelson April 16, 2018, 9:40 p.m. | #3
On 04/13/2018 12:26 PM, Ben Pfaff wrote:
> I didn't realize until now that the tree had two different ways of parsing
> strings in the form <host>:<port> and <port>:<host>.  There are the
> long-standing inet_parse_active() and inet_parse_passive() functions, and
> more recently the ipv46_parse() function.  This commit eliminates the
> latter and changes the code to use the former.
> 
> The two implementations interpreted some input differently.  In particular,
> the older functions required IPv6 addresses to be [bracketed], but the
> newer ones did not.  I'd prefer the brackets to be required, because of
> ambiguous cases like "::1:2:3:4:80" (is :80 part of the IPv6 address or a
> port number?) but for compatibility this patch changes the merged code to
> use the more liberal interpretation.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---
>   lib/packets.c             |  76 -------------------------
>   lib/packets.h             |  10 ----
>   lib/socket-util.c         | 140 +++++++++++++++++++++++++++-------------------
>   lib/socket-util.h         |   3 +-
>   ovn/utilities/ovn-nbctl.c |  43 +++-----------
>   ovsdb/raft.c              |   5 +-
>   tests/ovn-nbctl.at        |  56 +++++++++----------
>   7 files changed, 125 insertions(+), 208 deletions(-)
> 
> diff --git a/lib/packets.c b/lib/packets.c
> index 8ebae8cc8fe8..38bfb6015b9e 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -652,82 +652,6 @@ ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
>       return error;
>   }
>   
> -/* Parses the string into an IPv4 or IPv6 address.
> - * The port flags act as follows:
> - * * PORT_OPTIONAL: A port may be present but is not required
> - * * PORT_REQUIRED: A port must be present
> - * * PORT_FORBIDDEN: A port must not be present
> - */
> -char * OVS_WARN_UNUSED_RESULT
> -ipv46_parse(const char *s, enum port_flags flags, struct sockaddr_storage *ss)
> -{
> -    char *error = NULL;
> -
> -    char *copy;
> -    copy = xstrdup(s);
> -
> -    char *addr;
> -    char *port;
> -    if (*copy == '[') {
> -        char *end;
> -
> -        addr = copy + 1;
> -        end = strchr(addr, ']');
> -        if (!end) {
> -            error = xasprintf("No closing bracket on address %s", s);
> -            goto finish;
> -        }
> -        *end++ = '\0';
> -        if (*end == ':') {
> -            port = end + 1;
> -        } else {
> -            port = NULL;
> -        }
> -    } else {
> -        addr = copy;
> -        port = strchr(copy, ':');
> -        if (port) {
> -            if (strchr(port + 1, ':')) {
> -                port = NULL;
> -            } else {
> -                *port++ = '\0';
> -            }
> -        }
> -    }
> -
> -    if (port && !*port) {
> -        error = xasprintf("Port is an empty string");
> -        goto finish;
> -    }
> -
> -    if (port && flags == PORT_FORBIDDEN) {
> -        error = xasprintf("Port forbidden in address %s", s);
> -        goto finish;
> -    } else if (!port && flags == PORT_REQUIRED) {
> -        error = xasprintf("Port required in address %s", s);
> -        goto finish;
> -    }
> -
> -    struct addrinfo hints = {
> -        .ai_flags = AI_NUMERICHOST | AI_NUMERICSERV,
> -        .ai_family = AF_UNSPEC,
> -    };
> -    struct addrinfo *res;
> -    int status;
> -    status = getaddrinfo(addr, port, &hints, &res);
> -    if (status) {
> -        error = xasprintf("Error parsing address %s: %s",
> -                s, gai_strerror(status));
> -        goto finish;
> -    }
> -    memcpy(ss, res->ai_addr, res->ai_addrlen);
> -    freeaddrinfo(res);
> -
> -finish:
> -    free(copy);
> -    return error;
> -}
> -
>   /* Parses string 's', which must be an IPv6 address.  Stores the IPv6 address
>    * into '*ip'.  Returns true if successful, otherwise false. */
>   bool
> diff --git a/lib/packets.h b/lib/packets.h
> index 9a71aa3abbdb..b2bf70697e90 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1362,16 +1362,6 @@ struct in6_addr ipv6_create_mask(int mask);
>   int ipv6_count_cidr_bits(const struct in6_addr *netmask);
>   bool ipv6_is_cidr(const struct in6_addr *netmask);
>   
> -enum port_flags {
> -    PORT_OPTIONAL,
> -    PORT_REQUIRED,
> -    PORT_FORBIDDEN,
> -};
> -
> -char *ipv46_parse(const char *s, enum port_flags flags,
> -                  struct sockaddr_storage *ss)
> -    OVS_WARN_UNUSED_RESULT;
> -
>   bool ipv6_parse(const char *s, struct in6_addr *ip);
>   char *ipv6_parse_masked(const char *s, struct in6_addr *ipv6,
>                           struct in6_addr *mask);
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 223e3780ba5f..0964a015e3f9 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -333,39 +333,89 @@ guess_netmask(ovs_be32 ip_)
>               : htonl(0));                          /* ??? */
>   }
>   
> -/* This is like strsep() except:
> - *
> - *    - The separator string is ":".
> - *
> - *    - Square brackets [] quote ":" separators and are removed from the
> - *      tokens. */
> -char *
> -inet_parse_token(char **pp)
> +static char *
> +unbracket(char *s)
> +{
> +    if (*s == '[') {
> +        s++;
> +
> +        char *end = strchr(s, '\0');
> +        if (end[-1] == ']') {
> +            end[-1] = '\0';
> +        }
> +    }
> +    return s;
> +}
> +
> +/* 'host_index' is 0 if the host precedes the port within 's', 1 otherwise. */
> +static void
> +inet_parse_tokens__(char *s, int host_index, char **hostp, char **portp)
>   {
> -    char *p = *pp;
> -
> -    if (p == NULL) {
> -        return NULL;
> -    } else if (*p == '\0') {
> -        *pp = NULL;
> -        return p;
> -    } else if (*p == '[') {
> -        char *start = p + 1;
> -        char *end = start + strcspn(start, "]");
> -        *pp = (*end == '\0' ? NULL
> -               : end[1] == ':' ? end + 2
> -               : end + 1);
> -        *end = '\0';
> -        return start;
> +    char *colon = NULL;
> +    bool in_brackets = false;
> +    int n_colons = 0;
> +    for (char *p = s; *p; p++) {
> +        if (*p == '[') {
> +            in_brackets = true;
> +        } else if (*p == ']') {
> +            in_brackets = false;
> +        } else if (*p == ':' && !in_brackets) {
> +            n_colons++;
> +            colon = p;
> +        }
> +    }
> +
> +    *hostp = *portp = NULL;
> +    if (n_colons > 1) {
> +        *hostp = s;
>       } else {
> -        char *start = p;
> -        char *end = start + strcspn(start, ":");
> -        *pp = *end == '\0' ? NULL : end + 1;
> -        *end = '\0';
> -        return start;
> +        char **tokens[2];
> +        tokens[host_index] = hostp;
> +        tokens[!host_index] = portp;
> +
> +        if (colon) {
> +            *colon = '\0';
> +            *tokens[1] = unbracket(colon + 1);
> +        }
> +        *tokens[0] = unbracket(s);
>       }
>   }
>   
> +/* Parses 's', a string in the form "<host>[:<port>]", into its (required) host
> + * and (optional) port components, and stores pointers to them in '*hostp' and
> + * '*portp' respectively.  Always sets '*hostp' nonnull, although possibly to
> + * an empty string empty string.  Can set '*portp' to the null string.

"empty string" is repeated. Also, it's a bit of a nitpick, but if 's' is 
NULL, then *hostp will get set to NULL, which contradicts the comment here.

> + *
> + * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> + * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> + * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> + * is a host but "[::1:2:3:4]:80" is a host and a port.
> + *
> + * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> + */
> +void
> +inet_parse_host_port_tokens(char *s, char **hostp, char **portp)
> +{
> +    inet_parse_tokens__(s, 0, hostp, portp);
> +}
> +
> +/* Parses 's', a string in the form "<port>[:<host>]", into its port and hos
> + * components, and stores pointers to them in '*portp' and '*hostp'
> + * respectively.  Both '*portp' and '*hostp' can end up null.

I'm a bit confused here. inet_parse_tokens__() is called both from 
inet_parse_host_port_tokens() and inet_parse_port_host_tokens(), just 
with the host_index different. My expectation would then be that 
inet_parse_port_host_tokens() should make the same non-NULL guarantee 
for *portp that inet_parse_host_port_tokens() does for *hostp. What am I 
missing?

> + *
> + * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> + * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> + * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> + * is a host but "[::1:2:3:4]:80" is a host and a port.
> + *
> + * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> + */
> +void
> +inet_parse_port_host_tokens(char *s, char **portp, char **hostp)
> +{
> +    inet_parse_tokens__(s, 1, hostp, portp);
> +}
> +
>   static bool
>   parse_sockaddr_components(struct sockaddr_storage *ss,
>                             char *host_s,
> @@ -441,23 +491,16 @@ inet_parse_active(const char *target_, int default_port,
>                     struct sockaddr_storage *ss)
>   {
>       char *target = xstrdup(target_);
> -    const char *port;
> -    char *host;
> -    char *p;
> +    char *port, *host;
>       bool ok;
>   
> -    p = target;
> -    host = inet_parse_token(&p);
> -    port = inet_parse_token(&p);
> +    inet_parse_host_port_tokens(target, &host, &port);
>       if (!host) {
>           VLOG_ERR("%s: host must be specified", target_);
>           ok = false;
>       } else if (!port && default_port < 0) {
>           VLOG_ERR("%s: port must be specified", target_);
>           ok = false;
> -    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> -        VLOG_ERR("%s: unexpected characters follow host and port", target_);
> -        ok = false;
>       } else {
>           ok = parse_sockaddr_components(ss, host, port, default_port, target_);
>       }
> @@ -571,20 +614,13 @@ inet_parse_passive(const char *target_, int default_port,
>                      struct sockaddr_storage *ss)
>   {
>       char *target = xstrdup(target_);
> -    const char *port;
> -    char *host;
> -    char *p;
> +    char *port, *host;
>       bool ok;
>   
> -    p = target;
> -    port = inet_parse_token(&p);
> -    host = inet_parse_token(&p);
> +    inet_parse_port_host_tokens(target, &port, &host);
>       if (!port && default_port < 0) {
>           VLOG_ERR("%s: port must be specified", target_);
>           ok = false;
> -    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> -        VLOG_ERR("%s: unexpected characters follow port and host", target_);
> -        ok = false;
>       } else {
>           ok = parse_sockaddr_components(ss, host, port, default_port, target_);
>       }
> @@ -707,16 +743,8 @@ bool
>   inet_parse_address(const char *target_, struct sockaddr_storage *ss)
>   {
>       char *target = xstrdup(target_);
> -    char *p = target;
> -    char *host = inet_parse_token(&p);
> -    bool ok = false;
> -    if (!host) {
> -        VLOG_ERR("%s: host must be specified", target_);
> -    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
> -        VLOG_ERR("%s: unexpected characters follow host", target_);
> -    } else {
> -        ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
> -    }
> +    char *host = unbracket(target);
> +    bool ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
>       if (!ok) {
>           memset(ss, 0, sizeof *ss);
>       }
> diff --git a/lib/socket-util.h b/lib/socket-util.h
> index d927d67a0e1b..239d3f22041c 100644
> --- a/lib/socket-util.h
> +++ b/lib/socket-util.h
> @@ -46,7 +46,8 @@ int check_connection_completion(int fd);
>   void drain_fd(int fd, size_t n_packets);
>   ovs_be32 guess_netmask(ovs_be32 ip);
>   
> -char *inet_parse_token(char **);
> +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);
>   int inet_open_active(int style, const char *target, int default_port,
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 7efc5065c962..16698de1f049 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -1798,10 +1798,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>       }
>   
>       struct sockaddr_storage ss_vip;
> -    char *error;
> -    error = ipv46_parse(lb_vip, PORT_OPTIONAL, &ss_vip);
> -    if (error) {
> -        free(error);
> +    if (!inet_parse_active(lb_vip, 0, &ss_vip)) {
>           ctl_fatal("%s: should be an IP address (or an IP address "
>                     "and a port number with : as a separator).", lb_vip);
>       }
> @@ -1848,17 +1845,13 @@ nbctl_lb_add(struct ctl_context *ctx)
>               token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
>           struct sockaddr_storage ss_dst;
>   
> -        error = ipv46_parse(token, is_vip_with_port
> -                ? PORT_REQUIRED
> -                : PORT_FORBIDDEN,
> -                &ss_dst);
> -
> -        if (error) {
> -            free(error);
> -            if (is_vip_with_port) {
> +        if (is_vip_with_port) {
> +            if (!inet_parse_active(token, -1, &ss_dst)) {
>                   ctl_fatal("%s: should be an IP address and a port "
> -                        "number with : as a separator.", token);
> -            } else {
> +                          "number with : as a separator.", token);
> +            }
> +        } else {
> +            if (!inet_parse_address(token, &ss_dst)) {
>                   ctl_fatal("%s: should be an IP address.", token);
>               }
>           }
> @@ -1954,36 +1947,18 @@ lb_info_add_smap(const struct nbrec_load_balancer *lb,
>   {
>       struct ds key = DS_EMPTY_INITIALIZER;
>       struct ds val = DS_EMPTY_INITIALIZER;
> -    char *error, *protocol;
>   
>       const struct smap_node **nodes = smap_sort(&lb->vips);
>       if (nodes) {
>           for (int i = 0; i < smap_count(&lb->vips); i++) {
>               const struct smap_node *node = nodes[i];
> -            protocol = lb->protocol;
>   
>               struct sockaddr_storage ss;
> -            error = ipv46_parse(node->key, PORT_OPTIONAL, &ss);
> -            if (error) {
> -                VLOG_WARN("%s", error);
> -                free(error);
> +            if (!inet_parse_active(node->key, 0, &ss)) {
>                   continue;
>               }
>   
> -            if (ss.ss_family == AF_INET) {
> -                struct sockaddr_in *sin = ALIGNED_CAST(struct sockaddr_in *,
> -                                                       &ss);
> -                if (!sin->sin_port) {
> -                    protocol = "tcp/udp";
> -                }
> -            } else {
> -                struct sockaddr_in6 *sin6 = ALIGNED_CAST(struct sockaddr_in6 *,
> -                                                         &ss);
> -                if (!sin6->sin6_port) {
> -                    protocol = "tcp/udp";
> -                }
> -            }
> -
> +            char *protocol = ss_get_port(&ss) ? lb->protocol : "tcp/udp";
>               i == 0 ? ds_put_format(&val,
>                           UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
>                           UUID_ARGS(&lb->header_.uuid),
> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> index edb22f2e7b60..c0c1e98977b9 100644
> --- a/ovsdb/raft.c
> +++ b/ovsdb/raft.c
> @@ -335,9 +335,8 @@ raft_make_address_passive(const char *address_)
>           return xasprintf("p%s", address_);
>       } else {
>           char *address = xstrdup(address_);
> -        char *p = strchr(address, ':') + 1;
> -        char *host = inet_parse_token(&p);
> -        char *port = inet_parse_token(&p);
> +        char *host, *port;
> +        inet_parse_host_port_tokens(strchr(address, ':') + 1, &host, &port);
>   
>           struct ds paddr = DS_EMPTY_INITIALIZER;
>           ds_put_format(&paddr, "p%.3s:%s:", address, port);
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index f07b47fd20ea..514e7e7d2d49 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -460,46 +460,46 @@ AT_SETUP([ovn-nbctl - LBs])
>   OVN_NBCTL_TEST_START
>   
>   dnl Add two LBs.
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
>   [ovn-nbctl: 30.0.0.10:80a: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
>   [ovn-nbctl: 30.0.0.10:a80: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> -[ovn-nbctl: 30.0.0.10:: should be an IP address (or an IP address and a port number with : as a separator).
> -])
> -
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
>   [ovn-nbctl: 192.168.10.20: should be an IP address and a port number with : as a separator.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
>   [ovn-nbctl: 30.0.0.1a: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
> -[ovn-nbctl: 192.168.10.10:80: should be an IP address.
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
> +[ovn-nbctl: 30.0.0: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
>   [ovn-nbctl: 192.168.10.20:80: should be an IP address.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
>   [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
>   [ovn-nbctl: 192.168.10.10:: should be an IP address.
>   ])
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
>   [ovn-nbctl: 192.168.10.1a: should be an IP address.
>   ])
>   
> +AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
> +[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +])
> +
>   AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
>   [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>   ])
> @@ -685,56 +685,56 @@ AT_SETUP([ovn-nbctl - LBs IPv6])
>   OVN_NBCTL_TEST_START
>   
>   dnl A bunch of commands that should fail
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
>   [ovn-nbctl: [[ae0f::10]]:80a: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
>   [ovn-nbctl: [[ae0f::10]]:a80: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> -[ovn-nbctl: [[ae0f::10]]:: should be an IP address (or an IP address and a port number with : as a separator).
> -])
> -
> -
> -AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
>   [ovn-nbctl: fd0f::20: should be an IP address and a port number with : as a separator.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
>   [ovn-nbctl: ae0f::10fff: should be an IP address (or an IP address and a port number with : as a separator).
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
>   [ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
>   [ovn-nbctl: [[fd0f::20]]:80: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
>   [ovn-nbctl: [[fd0f::10]]:a80: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
>   [ovn-nbctl: [[fd0f::10]]:: should be an IP address.
>   ])
>   
>   
> -AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
>   [ovn-nbctl: fd0f::1001a: should be an IP address.
>   ])
>   
>   
> +AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
> +[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
> +])
> +
> +
>   AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10 tcp], [1], [],
>   [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
>   ])
>
Ben Pfaff April 17, 2018, 3:31 p.m. | #4
On Mon, Apr 16, 2018 at 04:40:19PM -0500, Mark Michelson wrote:
> On 04/13/2018 12:26 PM, Ben Pfaff wrote:
> >I didn't realize until now that the tree had two different ways of parsing
> >strings in the form <host>:<port> and <port>:<host>.  There are the
> >long-standing inet_parse_active() and inet_parse_passive() functions, and
> >more recently the ipv46_parse() function.  This commit eliminates the
> >latter and changes the code to use the former.
> >
> >The two implementations interpreted some input differently.  In particular,
> >the older functions required IPv6 addresses to be [bracketed], but the
> >newer ones did not.  I'd prefer the brackets to be required, because of
> >ambiguous cases like "::1:2:3:4:80" (is :80 part of the IPv6 address or a
> >port number?) but for compatibility this patch changes the merged code to
> >use the more liberal interpretation.
> >
> >Signed-off-by: Ben Pfaff <blp@ovn.org>

> >+/* Parses 's', a string in the form "<host>[:<port>]", into its (required) host
> >+ * and (optional) port components, and stores pointers to them in '*hostp' and
> >+ * '*portp' respectively.  Always sets '*hostp' nonnull, although possibly to
> >+ * an empty string empty string.  Can set '*portp' to the null string.
> 
> "empty string" is repeated. 

Oops, will fix.

> Also, it's a bit of a nitpick, but if 's' is NULL, then *hostp will
> get set to NULL, which contradicts the comment here.

If 's' is NULL then the function segfaults when it tries to dereference
it.

> >+ *
> >+ * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
> >+ * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
> >+ * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
> >+ * is a host but "[::1:2:3:4]:80" is a host and a port.
> >+ *
> >+ * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
> >+ */
> >+void
> >+inet_parse_host_port_tokens(char *s, char **hostp, char **portp)
> >+{
> >+    inet_parse_tokens__(s, 0, hostp, portp);
> >+}
> >+
> >+/* Parses 's', a string in the form "<port>[:<host>]", into its port and hos
> >+ * components, and stores pointers to them in '*portp' and '*hostp'
> >+ * respectively.  Both '*portp' and '*hostp' can end up null.
> 
> I'm a bit confused here. inet_parse_tokens__() is called both from
> inet_parse_host_port_tokens() and inet_parse_port_host_tokens(), just with
> the host_index different. My expectation would then be that
> inet_parse_port_host_tokens() should make the same non-NULL guarantee for
> *portp that inet_parse_host_port_tokens() does for *hostp. What am I
> missing?

For inet_parse_host_port_tokens() there are these cases in
inet_parse_tokens__():

        * n_colons > 1: *hostp will be nonnull, *portp will be null.

        * n_colons == 1: *hostp and *portp will be nonnull.

        * n_colons == 0: *hostp will be nonnull, *portp will be null.

For inet_parse_port_host_tokens() there are these cases in
inet_parse_tokens__():

        * n_colons > 1: *hostp will be nonnull, *portp will be null.

        * n_colons == 1: *hostp and *portp will be nonnull.

        * n_colons == 0: *portp will be nonnull, *hostp will be null.

The difference, then, is that when there are no colons, the two
functions interpret the field as a host or a port, respectively, and
that means that in the former only the port can be null but in the
latter either the host or the port (although not both at the same time)
can be null.  I guess the comment could be better phrased.

I forgot to fix this before I applied the series, so I sent a fresh
patch:
        https://patchwork.ozlabs.org/patch/899415/

Patch

diff --git a/lib/packets.c b/lib/packets.c
index 8ebae8cc8fe8..38bfb6015b9e 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -652,82 +652,6 @@  ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
     return error;
 }
 
-/* Parses the string into an IPv4 or IPv6 address.
- * The port flags act as follows:
- * * PORT_OPTIONAL: A port may be present but is not required
- * * PORT_REQUIRED: A port must be present
- * * PORT_FORBIDDEN: A port must not be present
- */
-char * OVS_WARN_UNUSED_RESULT
-ipv46_parse(const char *s, enum port_flags flags, struct sockaddr_storage *ss)
-{
-    char *error = NULL;
-
-    char *copy;
-    copy = xstrdup(s);
-
-    char *addr;
-    char *port;
-    if (*copy == '[') {
-        char *end;
-
-        addr = copy + 1;
-        end = strchr(addr, ']');
-        if (!end) {
-            error = xasprintf("No closing bracket on address %s", s);
-            goto finish;
-        }
-        *end++ = '\0';
-        if (*end == ':') {
-            port = end + 1;
-        } else {
-            port = NULL;
-        }
-    } else {
-        addr = copy;
-        port = strchr(copy, ':');
-        if (port) {
-            if (strchr(port + 1, ':')) {
-                port = NULL;
-            } else {
-                *port++ = '\0';
-            }
-        }
-    }
-
-    if (port && !*port) {
-        error = xasprintf("Port is an empty string");
-        goto finish;
-    }
-
-    if (port && flags == PORT_FORBIDDEN) {
-        error = xasprintf("Port forbidden in address %s", s);
-        goto finish;
-    } else if (!port && flags == PORT_REQUIRED) {
-        error = xasprintf("Port required in address %s", s);
-        goto finish;
-    }
-
-    struct addrinfo hints = {
-        .ai_flags = AI_NUMERICHOST | AI_NUMERICSERV,
-        .ai_family = AF_UNSPEC,
-    };
-    struct addrinfo *res;
-    int status;
-    status = getaddrinfo(addr, port, &hints, &res);
-    if (status) {
-        error = xasprintf("Error parsing address %s: %s",
-                s, gai_strerror(status));
-        goto finish;
-    }
-    memcpy(ss, res->ai_addr, res->ai_addrlen);
-    freeaddrinfo(res);
-
-finish:
-    free(copy);
-    return error;
-}
-
 /* Parses string 's', which must be an IPv6 address.  Stores the IPv6 address
  * into '*ip'.  Returns true if successful, otherwise false. */
 bool
diff --git a/lib/packets.h b/lib/packets.h
index 9a71aa3abbdb..b2bf70697e90 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -1362,16 +1362,6 @@  struct in6_addr ipv6_create_mask(int mask);
 int ipv6_count_cidr_bits(const struct in6_addr *netmask);
 bool ipv6_is_cidr(const struct in6_addr *netmask);
 
-enum port_flags {
-    PORT_OPTIONAL,
-    PORT_REQUIRED,
-    PORT_FORBIDDEN,
-};
-
-char *ipv46_parse(const char *s, enum port_flags flags,
-                  struct sockaddr_storage *ss)
-    OVS_WARN_UNUSED_RESULT;
-
 bool ipv6_parse(const char *s, struct in6_addr *ip);
 char *ipv6_parse_masked(const char *s, struct in6_addr *ipv6,
                         struct in6_addr *mask);
diff --git a/lib/socket-util.c b/lib/socket-util.c
index 223e3780ba5f..0964a015e3f9 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -333,39 +333,89 @@  guess_netmask(ovs_be32 ip_)
             : htonl(0));                          /* ??? */
 }
 
-/* This is like strsep() except:
- *
- *    - The separator string is ":".
- *
- *    - Square brackets [] quote ":" separators and are removed from the
- *      tokens. */
-char *
-inet_parse_token(char **pp)
+static char *
+unbracket(char *s)
+{
+    if (*s == '[') {
+        s++;
+
+        char *end = strchr(s, '\0');
+        if (end[-1] == ']') {
+            end[-1] = '\0';
+        }
+    }
+    return s;
+}
+
+/* 'host_index' is 0 if the host precedes the port within 's', 1 otherwise. */
+static void
+inet_parse_tokens__(char *s, int host_index, char **hostp, char **portp)
 {
-    char *p = *pp;
-
-    if (p == NULL) {
-        return NULL;
-    } else if (*p == '\0') {
-        *pp = NULL;
-        return p;
-    } else if (*p == '[') {
-        char *start = p + 1;
-        char *end = start + strcspn(start, "]");
-        *pp = (*end == '\0' ? NULL
-               : end[1] == ':' ? end + 2
-               : end + 1);
-        *end = '\0';
-        return start;
+    char *colon = NULL;
+    bool in_brackets = false;
+    int n_colons = 0;
+    for (char *p = s; *p; p++) {
+        if (*p == '[') {
+            in_brackets = true;
+        } else if (*p == ']') {
+            in_brackets = false;
+        } else if (*p == ':' && !in_brackets) {
+            n_colons++;
+            colon = p;
+        }
+    }
+
+    *hostp = *portp = NULL;
+    if (n_colons > 1) {
+        *hostp = s;
     } else {
-        char *start = p;
-        char *end = start + strcspn(start, ":");
-        *pp = *end == '\0' ? NULL : end + 1;
-        *end = '\0';
-        return start;
+        char **tokens[2];
+        tokens[host_index] = hostp;
+        tokens[!host_index] = portp;
+
+        if (colon) {
+            *colon = '\0';
+            *tokens[1] = unbracket(colon + 1);
+        }
+        *tokens[0] = unbracket(s);
     }
 }
 
+/* Parses 's', a string in the form "<host>[:<port>]", into its (required) host
+ * and (optional) port components, and stores pointers to them in '*hostp' and
+ * '*portp' respectively.  Always sets '*hostp' nonnull, although possibly to
+ * an empty string empty string.  Can set '*portp' to the null string.
+ *
+ * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
+ * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
+ * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
+ * is a host but "[::1:2:3:4]:80" is a host and a port.
+ *
+ * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
+ */
+void
+inet_parse_host_port_tokens(char *s, char **hostp, char **portp)
+{
+    inet_parse_tokens__(s, 0, hostp, portp);
+}
+
+/* Parses 's', a string in the form "<port>[:<host>]", into its port and host
+ * components, and stores pointers to them in '*portp' and '*hostp'
+ * respectively.  Both '*portp' and '*hostp' can end up null.
+ *
+ * Supports both IPv4 and IPv6.  IPv6 addresses may be quoted with square
+ * brackets.  Resolves ambiguous cases that might represent an IPv6 address or
+ * an IPv6 address and a port as representing just a host, e.g. "::1:2:3:4:80"
+ * is a host but "[::1:2:3:4]:80" is a host and a port.
+ *
+ * Modifies 's' and points '*hostp' and '*portp' (if nonnull) into it.
+ */
+void
+inet_parse_port_host_tokens(char *s, char **portp, char **hostp)
+{
+    inet_parse_tokens__(s, 1, hostp, portp);
+}
+
 static bool
 parse_sockaddr_components(struct sockaddr_storage *ss,
                           char *host_s,
@@ -441,23 +491,16 @@  inet_parse_active(const char *target_, int default_port,
                   struct sockaddr_storage *ss)
 {
     char *target = xstrdup(target_);
-    const char *port;
-    char *host;
-    char *p;
+    char *port, *host;
     bool ok;
 
-    p = target;
-    host = inet_parse_token(&p);
-    port = inet_parse_token(&p);
+    inet_parse_host_port_tokens(target, &host, &port);
     if (!host) {
         VLOG_ERR("%s: host must be specified", target_);
         ok = false;
     } else if (!port && default_port < 0) {
         VLOG_ERR("%s: port must be specified", target_);
         ok = false;
-    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
-        VLOG_ERR("%s: unexpected characters follow host and port", target_);
-        ok = false;
     } else {
         ok = parse_sockaddr_components(ss, host, port, default_port, target_);
     }
@@ -571,20 +614,13 @@  inet_parse_passive(const char *target_, int default_port,
                    struct sockaddr_storage *ss)
 {
     char *target = xstrdup(target_);
-    const char *port;
-    char *host;
-    char *p;
+    char *port, *host;
     bool ok;
 
-    p = target;
-    port = inet_parse_token(&p);
-    host = inet_parse_token(&p);
+    inet_parse_port_host_tokens(target, &port, &host);
     if (!port && default_port < 0) {
         VLOG_ERR("%s: port must be specified", target_);
         ok = false;
-    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
-        VLOG_ERR("%s: unexpected characters follow port and host", target_);
-        ok = false;
     } else {
         ok = parse_sockaddr_components(ss, host, port, default_port, target_);
     }
@@ -707,16 +743,8 @@  bool
 inet_parse_address(const char *target_, struct sockaddr_storage *ss)
 {
     char *target = xstrdup(target_);
-    char *p = target;
-    char *host = inet_parse_token(&p);
-    bool ok = false;
-    if (!host) {
-        VLOG_ERR("%s: host must be specified", target_);
-    } else if (p && p[strspn(p, " \t\r\n")] != '\0') {
-        VLOG_ERR("%s: unexpected characters follow host", target_);
-    } else {
-        ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
-    }
+    char *host = unbracket(target);
+    bool ok = parse_sockaddr_components(ss, host, NULL, 0, target_);
     if (!ok) {
         memset(ss, 0, sizeof *ss);
     }
diff --git a/lib/socket-util.h b/lib/socket-util.h
index d927d67a0e1b..239d3f22041c 100644
--- a/lib/socket-util.h
+++ b/lib/socket-util.h
@@ -46,7 +46,8 @@  int check_connection_completion(int fd);
 void drain_fd(int fd, size_t n_packets);
 ovs_be32 guess_netmask(ovs_be32 ip);
 
-char *inet_parse_token(char **);
+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);
 int inet_open_active(int style, const char *target, int default_port,
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 7efc5065c962..16698de1f049 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1798,10 +1798,7 @@  nbctl_lb_add(struct ctl_context *ctx)
     }
 
     struct sockaddr_storage ss_vip;
-    char *error;
-    error = ipv46_parse(lb_vip, PORT_OPTIONAL, &ss_vip);
-    if (error) {
-        free(error);
+    if (!inet_parse_active(lb_vip, 0, &ss_vip)) {
         ctl_fatal("%s: should be an IP address (or an IP address "
                   "and a port number with : as a separator).", lb_vip);
     }
@@ -1848,17 +1845,13 @@  nbctl_lb_add(struct ctl_context *ctx)
             token != NULL; token = strtok_r(NULL, ",", &save_ptr)) {
         struct sockaddr_storage ss_dst;
 
-        error = ipv46_parse(token, is_vip_with_port
-                ? PORT_REQUIRED
-                : PORT_FORBIDDEN,
-                &ss_dst);
-
-        if (error) {
-            free(error);
-            if (is_vip_with_port) {
+        if (is_vip_with_port) {
+            if (!inet_parse_active(token, -1, &ss_dst)) {
                 ctl_fatal("%s: should be an IP address and a port "
-                        "number with : as a separator.", token);
-            } else {
+                          "number with : as a separator.", token);
+            }
+        } else {
+            if (!inet_parse_address(token, &ss_dst)) {
                 ctl_fatal("%s: should be an IP address.", token);
             }
         }
@@ -1954,36 +1947,18 @@  lb_info_add_smap(const struct nbrec_load_balancer *lb,
 {
     struct ds key = DS_EMPTY_INITIALIZER;
     struct ds val = DS_EMPTY_INITIALIZER;
-    char *error, *protocol;
 
     const struct smap_node **nodes = smap_sort(&lb->vips);
     if (nodes) {
         for (int i = 0; i < smap_count(&lb->vips); i++) {
             const struct smap_node *node = nodes[i];
-            protocol = lb->protocol;
 
             struct sockaddr_storage ss;
-            error = ipv46_parse(node->key, PORT_OPTIONAL, &ss);
-            if (error) {
-                VLOG_WARN("%s", error);
-                free(error);
+            if (!inet_parse_active(node->key, 0, &ss)) {
                 continue;
             }
 
-            if (ss.ss_family == AF_INET) {
-                struct sockaddr_in *sin = ALIGNED_CAST(struct sockaddr_in *,
-                                                       &ss);
-                if (!sin->sin_port) {
-                    protocol = "tcp/udp";
-                }
-            } else {
-                struct sockaddr_in6 *sin6 = ALIGNED_CAST(struct sockaddr_in6 *,
-                                                         &ss);
-                if (!sin6->sin6_port) {
-                    protocol = "tcp/udp";
-                }
-            }
-
+            char *protocol = ss_get_port(&ss) ? lb->protocol : "tcp/udp";
             i == 0 ? ds_put_format(&val,
                         UUID_FMT "    %-20.16s%-11.7s%-*.*s%s",
                         UUID_ARGS(&lb->header_.uuid),
diff --git a/ovsdb/raft.c b/ovsdb/raft.c
index edb22f2e7b60..c0c1e98977b9 100644
--- a/ovsdb/raft.c
+++ b/ovsdb/raft.c
@@ -335,9 +335,8 @@  raft_make_address_passive(const char *address_)
         return xasprintf("p%s", address_);
     } else {
         char *address = xstrdup(address_);
-        char *p = strchr(address, ':') + 1;
-        char *host = inet_parse_token(&p);
-        char *port = inet_parse_token(&p);
+        char *host, *port;
+        inet_parse_host_port_tokens(strchr(address, ':') + 1, &host, &port);
 
         struct ds paddr = DS_EMPTY_INITIALIZER;
         ds_put_format(&paddr, "p%.3s:%s:", address, port);
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index f07b47fd20ea..514e7e7d2d49 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -460,46 +460,46 @@  AT_SETUP([ovn-nbctl - LBs])
 OVN_NBCTL_TEST_START
 
 dnl Add two LBs.
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80a 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
 [ovn-nbctl: 30.0.0.10:80a: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:a80 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
 [ovn-nbctl: 30.0.0.10:a80: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
-[ovn-nbctl: 30.0.0.10:: should be an IP address (or an IP address and a port number with : as a separator).
-])
-
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10:80 192.168.10.10:80,192.168.10.20 tcp], [1], [],
 [ovn-nbctl: 192.168.10.20: should be an IP address and a port number with : as a separator.
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.1a 192.168.10.10:80,192.168.10.20:80], [1], [],
 [ovn-nbctl: 30.0.0.1a: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
-[ovn-nbctl: 192.168.10.10:80: should be an IP address.
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0 192.168.10.10:80,192.168.10.20:80], [1], [],
+[ovn-nbctl: 30.0.0: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10,192.168.10.20:80], [1], [],
 [ovn-nbctl: 192.168.10.20:80: should be an IP address.
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:a80], [1], [],
 [ovn-nbctl: 192.168.10.10:a80: should be an IP address.
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.10:], [1], [],
 [ovn-nbctl: 192.168.10.10:: should be an IP address.
 ])
 
-AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 30.0.0.10 192.168.10.1a], [1], [],
 [ovn-nbctl: 192.168.10.1a: should be an IP address.
 ])
 
+AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10: 192.168.10.10:80,192.168.10.20:80 tcp], [1], [],
+[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
+])
+
 AT_CHECK([ovn-nbctl lb-add lb0 30.0.0.10 192.168.10.10 tcp], [1], [],
 [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
 ])
@@ -685,56 +685,56 @@  AT_SETUP([ovn-nbctl - LBs IPv6])
 OVN_NBCTL_TEST_START
 
 dnl A bunch of commands that should fail
-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80a [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
 [ovn-nbctl: [[ae0f::10]]:80a: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:a80 [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
 [ovn-nbctl: [[ae0f::10]]:a80: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
-[ovn-nbctl: [[ae0f::10]]:: should be an IP address (or an IP address and a port number with : as a separator).
-])
-
-
-AT_CHECK([ovn-nbctl lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]:80 [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
 [ovn-nbctl: fd0f::20: should be an IP address and a port number with : as a separator.
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10fff [[fd0f::10]]:80,fd0f::20 tcp], [1], [],
 [ovn-nbctl: ae0f::10fff: should be an IP address (or an IP address and a port number with : as a separator).
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:80,[[fd0f::20]]:80], [1], [],
 [ovn-nbctl: [[fd0f::10]]:80: should be an IP address.
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::10,[[fd0f::20]]:80], [1], [],
 [ovn-nbctl: [[fd0f::20]]:80: should be an IP address.
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:a80], [1], [],
 [ovn-nbctl: [[fd0f::10]]:a80: should be an IP address.
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 [[fd0f::10]]:], [1], [],
 [ovn-nbctl: [[fd0f::10]]:: should be an IP address.
 ])
 
 
-AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 ae0f::10 fd0f::1001a], [1], [],
 [ovn-nbctl: fd0f::1001a: should be an IP address.
 ])
 
 
+AT_CHECK([ovn-nbctl -vsocket_util:off lb-add lb0 [[ae0f::10]]: [[fd0f::10]]:80,[[fd0f::20]]:80 tcp], [1], [],
+[ovn-nbctl: Protocol is unnecessary when no port of vip is given.
+])
+
+
 AT_CHECK([ovn-nbctl lb-add lb0 ae0f::10 fd0f::10 tcp], [1], [],
 [ovn-nbctl: Protocol is unnecessary when no port of vip is given.
 ])