On Mon, Mar 9, 2015 at 9:22 AM, Lennart Poettering <[email protected]> wrote:
> On Sun, 08.03.15 16:24, Shawn Landden ([email protected]) 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, &sa.sa, &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. > 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? Otherwise I agree with everything in this review. > > 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/systemd-devel > -- Shawn Landden
_______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
