<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 9, 2015 at 1:18 PM, Lennart Poettering <<a href="mailto:lennart@poettering.net" target="_blank">lennart@poettering.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">On Mon, 09.03.15 13:09, Shawn Landden (<a href="mailto:shawn@churchofgit.com">shawn@churchofgit.com</a>) wrote: > + if (UNIT_DEREF(s->accept_socket)) { > + union sockaddr_union sa; > + socklen_t salen = sizeof(sa); > + > + r = getpeername(s->socket_fd, &<a href="http://sa.sa" target="_blank">sa.sa</a>, &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(&<a href="http://sa.sa" target="_blank">sa.sa</a>, 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. > > -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... <div><div class="h5"> > @@ -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:")); </div></div>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. </blockquote><div><a href="https://tools.ietf.org/html/rfc5952#section-4.3">https://tools.ietf.org/html/rfc5952#section-4.3</a> says lowercase-only</div><div><a href="https://tools.ietf.org/html/rfc5952#section-5">https://tools.ietf.org/html/rfc5952#section-5</a> specifies formatting of ipv4-mapped addresses</div><div> </div><div>and it is significantly longer that way, but sure, I can do it that way. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> > + } 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. <div class=""><div class="h5">Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list <a href="mailto:systemd-devel@lists.freedesktop.org">systemd-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/systemd-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/systemd-devel</a> </div></div></blockquote></div> <div> </div>-- <div class="gmail_signature">Shawn Landden</div> </div></div>