<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 9, 2015 at 9:22 AM, Lennart Poettering <<a href="mailto:lennart@poettering.net" target="_blank">lennart@poettering.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, 08.03.15 16:24, Shawn Landden (<a href="mailto:shawn@churchofgit.com">shawn@churchofgit.com</a>) wrote: > <varlistentry> > diff --git a/src/core/service.c b/src/core/service.c > index cc4ea19..6a690ac 100644 > --- a/src/core/service.c > +++ b/src/core/service.c > @@ -22,6 +22,7 @@ > #include <errno.h> > #include <signal.h> > #include <unistd.h> > +#include <arpa/inet.h> > > #include "async.h" > #include "manager.h" > @@ -1119,6 +1120,52 @@ static int service_spawn( > goto fail; > } > > + if (s->accept_socket.unit) { Please use UNIT_DEREF() for this. > + union sockaddr_union sa; > + socklen_t salen = sizeof(sa); > + _cleanup_free_ char *remote_addr = NULL; > + char a[MAX(INET6_ADDRSTRLEN, INET_ADDRSTRLEN)]; > + > + 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) { > + 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) { > + r = -errno; > + goto fail; > + } Hmm, so we already have sockaddr_pretty() in socket-util.c. Can't we fold this logic into that? Maybe add a boolean param that indicates whether to append the port number or not. I'd rather not have code for this all over the place. > + > + if (asprintf(our_env + n_env++, > + "REMOTE_ADDR=%s", > + /* musl and glibc inet_ntop() present v4-mapped addresses in ::ffff:a.b.c.d form */ > + sa.sa.sa_family == AF_INET6 && strchr(a, '.') ? > + strempty(startswith(a, "::ffff:")) : > + a) < 0) { > + r = -ENOMEM; > + goto fail; > + } sockaddr_pretty() already has propery code for this, please use that. Also, we don't care about non-glibc libcs anyway, hence please no reference to that. </blockquote><div>How about doing it this way in sockaddr_pretty() instead of rewriting inet_ntop() to the fact that the man page does not say that glibc does this?</div><div>Otherwise I agree with everything in this review. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Then, we try to avoid asprintf() if we just want to concat strings, please use strappend() or strjoin() for that. > + > + if (asprintf(our_env + n_env++, > + "REMOTE_PORT=%u", > + ntohs(sa.sa.sa_family == AF_INET6 ? > + sa.in6.sin6_port : > + sa.in.sin_port)) < 0) { > + r = -ENOMEM; > + goto fail; To make this easy I think a new sockaddr_port() call in socket-util.c would make sense that returns an "int", in which it either encodes an error or the converted port number. 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> </blockquote></div> <div> </div>-- <div class="gmail_signature">Shawn Landden</div> </div></div>