Seems fine to me. I would be inclined to have inet_parse_passive() return an error code instead of a bool. That way you wouldn't swallow the error from lookup_ip(). Seems cleaner I think.
Your call, looks fine otherwise. Ethan On Mon, Jul 25, 2011 at 12:29, Ben Pfaff <[email protected]> wrote: > --- > lib/socket-util.c | 115 > +++++++++++++++++++++++++++++++---------------------- > lib/socket-util.h | 3 + > 2 files changed, 70 insertions(+), 48 deletions(-) > > diff --git a/lib/socket-util.c b/lib/socket-util.c > index 935e747..07687ba 100644 > --- a/lib/socket-util.c > +++ b/lib/socket-util.c > @@ -554,9 +554,7 @@ exit: > return error; > } > > -/* Opens a non-blocking IPv4 socket of the specified 'style', binds to > - * 'target', and listens for incoming connections. 'target' should be a > string > - * in the format "[<port>][:<ip>]": > +/* Parses 'target', which should be a string in the format "[<port>][:<ip>]": > * > * - If 'default_port' is -1, then <port> is required. Otherwise, if > * <port> is omitted, then 'default_port' is used instead. > @@ -566,106 +564,127 @@ exit: > * > * - If <ip> is omitted then the IP address is wildcarded. > * > - * 'style' should be SOCK_STREAM (for TCP) or SOCK_DGRAM (for UDP). > - * > - * For TCP, the socket will have SO_REUSEADDR turned on. > - * > - * On success, returns a non-negative file descriptor. On failure, returns a > - * negative errno value. > - * > - * If 'sinp' is non-null, then on success the bound address is stored into > - * '*sinp'. */ > -int > -inet_open_passive(int style, const char *target_, int default_port, > - struct sockaddr_in *sinp) > + * If successful, stores the address into '*sinp' and returns true; otherwise > + * zeros '*sinp' and returns false. */ > +bool > +inet_parse_passive(const char *target_, uint16_t default_port, > + struct sockaddr_in *sinp) > { > char *target = xstrdup(target_); > char *string_ptr = target; > - struct sockaddr_in sin; > const char *host_name; > const char *port_string; > - int fd = 0, error, port; > - unsigned int yes = 1; > + bool ok = false; > + int port; > > /* Address defaults. */ > - memset(&sin, 0, sizeof sin); > - sin.sin_family = AF_INET; > - sin.sin_addr.s_addr = htonl(INADDR_ANY); > - sin.sin_port = htons(default_port); > + memset(sinp, 0, sizeof *sinp); > + sinp->sin_family = AF_INET; > + sinp->sin_addr.s_addr = htonl(INADDR_ANY); > + sinp->sin_port = htons(default_port); > > /* Parse optional port number. */ > port_string = strsep(&string_ptr, ":"); > if (port_string && str_to_int(port_string, 10, &port)) { > - sin.sin_port = htons(port); > + sinp->sin_port = htons(port); > } else if (default_port < 0) { > VLOG_ERR("%s: port number must be specified", target_); > - error = EAFNOSUPPORT; > goto exit; > } > > /* Parse optional bind IP. */ > host_name = strsep(&string_ptr, ":"); > - if (host_name && host_name[0]) { > - error = lookup_ip(host_name, &sin.sin_addr); > - if (error) { > - goto exit; > - } > + if (host_name && host_name[0] && lookup_ip(host_name, &sinp->sin_addr)) { > + goto exit; > + } > + > + ok = true; > + > +exit: > + if (!ok) { > + memset(sinp, 0, sizeof *sinp); > + } > + free(target); > + return ok; > +} > + > + > +/* Opens a non-blocking IPv4 socket of the specified 'style', binds to > + * 'target', and listens for incoming connections. Parses 'target' in the > same > + * way was inet_parse_passive(). > + * > + * 'style' should be SOCK_STREAM (for TCP) or SOCK_DGRAM (for UDP). > + * > + * For TCP, the socket will have SO_REUSEADDR turned on. > + * > + * On success, returns a non-negative file descriptor. On failure, returns a > + * negative errno value. > + * > + * If 'sinp' is non-null, then on success the bound address is stored into > + * '*sinp'. */ > +int > +inet_open_passive(int style, const char *target, int default_port, > + struct sockaddr_in *sinp) > +{ > + struct sockaddr_in sin; > + int fd = 0, error; > + unsigned int yes = 1; > + > + if (!inet_parse_passive(target, default_port, &sin)) { > + return EAFNOSUPPORT; > } > > /* Create non-blocking socket, set SO_REUSEADDR. */ > fd = socket(AF_INET, style, 0); > if (fd < 0) { > error = errno; > - VLOG_ERR("%s: socket: %s", target_, strerror(error)); > - goto exit; > + VLOG_ERR("%s: socket: %s", target, strerror(error)); > + return error; > } > error = set_nonblocking(fd); > if (error) { > - goto exit_close; > + goto error; > } > if (style == SOCK_STREAM > && setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof yes) < 0) { > error = errno; > - VLOG_ERR("%s: setsockopt(SO_REUSEADDR): %s", target_, > strerror(error)); > - goto exit_close; > + VLOG_ERR("%s: setsockopt(SO_REUSEADDR): %s", target, > strerror(error)); > + goto error; > } > > /* Bind. */ > if (bind(fd, (struct sockaddr *) &sin, sizeof sin) < 0) { > error = errno; > - VLOG_ERR("%s: bind: %s", target_, strerror(error)); > - goto exit_close; > + VLOG_ERR("%s: bind: %s", target, strerror(error)); > + goto error; > } > > /* Listen. */ > if (listen(fd, 10) < 0) { > error = errno; > - VLOG_ERR("%s: listen: %s", target_, strerror(error)); > - goto exit_close; > + VLOG_ERR("%s: listen: %s", target, strerror(error)); > + goto error; > } > > if (sinp) { > socklen_t sin_len = sizeof sin; > if (getsockname(fd, (struct sockaddr *) &sin, &sin_len) < 0){ > error = errno; > - VLOG_ERR("%s: getsockname: %s", target_, strerror(error)); > - goto exit_close; > + VLOG_ERR("%s: getsockname: %s", target, strerror(error)); > + goto error; > } > if (sin.sin_family != AF_INET || sin_len != sizeof sin) { > - VLOG_ERR("%s: getsockname: invalid socket name", target_); > - goto exit_close; > + VLOG_ERR("%s: getsockname: invalid socket name", target); > + goto error; > } > *sinp = sin; > } > > - error = 0; > - goto exit; > + return fd; > > -exit_close: > +error: > close(fd); > -exit: > - free(target); > - return error ? -error : fd; > + return error; > } > > /* Returns a readable and writable fd for /dev/null, if successful, otherwise > diff --git a/lib/socket-util.h b/lib/socket-util.h > index 03f4449..10f821e 100644 > --- a/lib/socket-util.h > +++ b/lib/socket-util.h > @@ -40,6 +40,9 @@ bool inet_parse_active(const char *target, uint16_t > default_port, > struct sockaddr_in *sinp); > int inet_open_active(int style, const char *target, uint16_t default_port, > struct sockaddr_in *sinp, int *fdp); > + > +bool inet_parse_passive(const char *target, uint16_t default_port, > + struct sockaddr_in *sinp); > int inet_open_passive(int style, const char *target, int default_port, > struct sockaddr_in *sinp); > > -- > 1.7.4.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
