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

[Lennart Poettering]

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. 

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