2015-03-09 23:09 GMT+01:00 Shawn Landden <sh...@churchofgit.com>: > 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.
Maybe by using a temporary variable like this: char *env_str; [...] env_str = strappend("REMOTE_ADDR", addr); if (!env_str) { r = -ENOMEM; goto fail; } our_env[n_env++] = env_str; And the same with the other one ? >> >> > >> > -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 > _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel