On Mon, Mar 9, 2015 at 1:18 PM, Lennart Poettering <lenn...@poettering.net> wrote:
> On Mon, 09.03.15 13:09, Shawn Landden (sh...@churchofgit.com) wrote: > > > + if (UNIT_DEREF(s->accept_socket)) { > > + union sockaddr_union sa; > > + socklen_t salen = sizeof(sa); > > + > > + r = getpeername(s->socket_fd, &sa.sa, &salen); > > + if (r < 0) { > > + r = -errno; > > + goto fail; > > + } > > + > > + if (sa.sa.sa_family == AF_INET || > > + sa.sa.sa_family == AF_INET6) { > > + _cleanup_free_ char *addr = NULL; > > + uint16_t port = (uint16_t)sockaddr_port(&sa); > > We try to avoid invoking functions in the same lines as we declare > variables. > > Also, even though this cannot fail in this case, I think it would be > nicer, to make port an int, and check for < 0 and return an error in > that case... > > > + > > + r = sockaddr_pretty(&sa.sa, salen, true, > false, &addr); > > + if (r < 0) > > + goto fail; > > + > > + if (!(our_env[n_env++] = > strappend("REMOTE_ADDR=", addr))) { > > In newer code we try to avoid making assignments and doing if checks > in the same line. > > Taking this out would be pretty gruesome because of use of the post-increment operator. > > > > -int sockaddr_pretty(const struct sockaddr *_sa, socklen_t salen, bool > translate_ipv6, char **ret) { > > +int sockaddr_port(const union sockaddr_union *sa) { > > + assert_return(sa->sa.sa_family == AF_INET6 || > > + sa->sa.sa_family == AF_INET, > > + -ENOTSUP); > > assert_return() is a macro to use only for cases where there's a clear > programming error of the caller. But I am pretty sure that this is not > the case here. If it gets called on an non-AF_INET/AF_INET6 socket > then this should fail without logging. > > Also I think an error code of EAFNOSUPPORT would be more appropriate. > > Hmm, and I think this should probably take an actual "struct > sockaddr", rather than a union sockaddr_union... > > > @@ -475,42 +485,40 @@ int sockaddr_pretty(const struct sockaddr *_sa, > socklen_t salen, bool translate_ > > > > switch (sa->sa.sa_family) { > > > > - case AF_INET: { > > - uint32_t a; > > - > > - a = ntohl(sa->in.sin_addr.s_addr); > > - > > - if (asprintf(&p, > > - "%u.%u.%u.%u:%u", > > - a >> 24, (a >> 16) & 0xFF, (a >> 8) & > 0xFF, a & 0xFF, > > - ntohs(sa->in.sin_port)) < 0) > > - return -ENOMEM; > > - > > - break; > > - } > > - > > + case AF_INET: > > case AF_INET6: { > > - static const unsigned char ipv4_prefix[] = { > > - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xFF, 0xFF > > - }; > > - > > - if (translate_ipv6 && memcmp(&sa->in6.sin6_addr, > ipv4_prefix, sizeof(ipv4_prefix)) == 0) { > > - const uint8_t *a = sa->in6.sin6_addr.s6_addr+12; > > + char a[MAX(INET6_ADDRSTRLEN, INET_ADDRSTRLEN)]; > > + const char *addr; > > + bool ipv4_mapped = false; > > + > > + if (inet_ntop(sa->sa.sa_family, > > + /* this field of the API is kinda > braindead, > > + * should take head of struct so it can > be passed the union...*/ > > + sa->sa.sa_family == AF_INET6 ? > > + &sa->in6.sin6_addr : > > + &sa->in.sin_addr, > > + a, sizeof(a)) == NULL) > > + return -ENOMEM; > > + > > + /* glibc inet_ntop() presents v4-mapped addresses in > ::ffff:a.b.c.d form */ > > + if (translate_ipv6 && sa->sa.sa_family == AF_INET6 && > strchr(a, '.')) { > > + ipv4_mapped = true; > > + addr = strempty(startswith(a, "::ffff:")); > > I think it would be a lot nicer to check the raw IP prefix as before, > and only then format things, then first formatting and then looking at > the string again. > > We shouldn't bind us too closely to the precise formatting. I mean, if > one day libc decides to format the thing as ipv6 again, or with > uppercase hex chars, we shouldn't break. > > > + } else > > + addr = &a[0]; > > + > > + if (include_port) { > > + uint16_t port = (uint16_t)sockaddr_port(sa); > > No function calls in variable declarations, please. Same rule as above > applies. > Lennart > > -- > Lennart Poettering, Red Hat > _______________________________________________ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel > -- Shawn Landden
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel