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

Ben Pfaff blp at ovn.org
Tue Apr 17 15:31:40 UTC 2018


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 at 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/


More information about the dev mailing list