[systemd-devel] [PATCH] add REMOTE_ADDR and REMOTE_PORT for Accept=yes

[Shawn Landden]

On Mon, Mar 9, 2015 at 9:22 AM, Lennart Poettering <lennart at poettering.net>
wrote:

> On Sun, 08.03.15 16:24, Shawn Landden (shawn at churchofgit.com) 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
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>



-- 
Shawn Landden
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150309/746f04fa/attachment.html>