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

2015-03-12 Thread Lennart Poettering 
On Mon, 09.03.15 14:48, Shawn Landden (sh...@churchofgit.com) wrote:


  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.

 https://tools.ietf.org/html/rfc5952#section-4.3 says lowercase-only
 https://tools.ietf.org/html/rfc5952#section-5 specifies formatting of
 ipv4-mapped addresses

Well, but you know how these things go. I mean, you are the one using
musl. I'd not be surprised if some of the implementations don't
strictly follow the rfc in some detail and end up with a slightly
different formatting and the code is hosed...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-03-10 Thread Ronny Chevalier 
2015-03-10 12:41 GMT+01:00 Shawn Landden sh...@churchofgit.com:
 ---
  TODO |  2 -
  man/systemd.socket.xml   |  7 ++-
  src/core/service.c   | 41 -
  src/libsystemd/sd-resolve/test-resolve.c |  2 +-
  src/shared/socket-util.c | 76 
 +++-
  src/shared/socket-util.h |  4 +-
  src/timesync/timesyncd-server.h  |  2 +-
  7 files changed, 106 insertions(+), 28 deletions(-)

 diff --git a/TODO b/TODO
 index ae32388..780084a 100644
 --- a/TODO
 +++ b/TODO
 @@ -164,8 +164,6 @@ Features:
  * as soon as we have kdbus, and sender timestamps, revisit coalescing 
 multiple parallel daemon reloads:

 http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html

 -* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
 doing per-connection socket activation. use format introduced by xinetd or 
 CGI for this
 -
  * the install state probably shouldn't get confused by generated units, 
 think dbus1/kdbus compat!

  * in systemctl list-unit-files: show the install value the presets would 
 suggest for a service in a third column
 diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
 index 3938345..6808179 100644
 --- a/man/systemd.socket.xml
 +++ b/man/systemd.socket.xml
 @@ -357,7 +357,12 @@
  daemons designed for usage with
  
 citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
  to work unmodified with systemd socket
 -activation./para/listitem
 +activation./para
 +
 +paraFor IPv4 and IPv6 connections the 
 varnameREMOTE_ADDR/varname
 +environment variable will contain the remote IP, and 
 varnameREMOTE_PORT/varname
 +will contain the remote port. This is the same as the format used by 
 CGI.
 +For SOCK_RAW the port is the IP protocol./para/listitem
/varlistentry

varlistentry
 diff --git a/src/core/service.c b/src/core/service.c
 index cc4ea19..bcfce96 100644
 --- a/src/core/service.c
 +++ b/src/core/service.c
 @@ -1095,7 +1095,7 @@ static int service_spawn(
  if (r  0)
  goto fail;

 -our_env = new0(char*, 4);
 +our_env = new0(char*, 6);
  if (!our_env) {
  r = -ENOMEM;
  goto fail;
 @@ -1119,6 +1119,45 @@ static int service_spawn(
  goto fail;
  }

 +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 (IN_SET(sa.sa.sa_family, AF_INET, AF_INET6)) {
 +_cleanup_free_ char *addr = NULL;
 +char *t;
 +int port;
 +
 +r = sockaddr_pretty(sa.sa, salen, true, false, 
 addr);
 +if (r  0)
 +goto fail;
 +
 +t = strappend(REMOTE_ADDR=, addr);
 +if (!t) {
 +r = -ENOMEM;
 +goto fail;
 +}
 +our_env[n_env++] = t;
 +
 +port = sockaddr_port(sa.sa);
 +if (port  0) {
 +r = port;
 +goto fail;
 +}
 +
 +if (asprintf((our_env + n_env++), REMOTE_PORT=%u, 
 port)  0) {
 +r = -ENOMEM;
 +goto fail;
 +}
 +}
 +}
 +
  final_env = strv_env_merge(2, UNIT(s)-manager-environment, 
 our_env, NULL);
  if (!final_env) {
  r = -ENOMEM;
 diff --git a/src/libsystemd/sd-resolve/test-resolve.c 
 b/src/libsystemd/sd-resolve/test-resolve.c
 index 3187ce9..354a407 100644
 --- a/src/libsystemd/sd-resolve/test-resolve.c
 +++ b/src/libsystemd/sd-resolve/test-resolve.c
 @@ -46,7 +46,7 @@ static int getaddrinfo_handler(sd_resolve_query *q, int 
 ret, const struct addrin
  for (i = ai; i; i = i-ai_next) {
  _cleanup_free_ char *addr = NULL;

 -assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
 addr) == 0);
 +assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
 true, addr) == 0);
  puts(addr);
  }

 diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
 index 74d90fa..0d87cb1 100644
 --- a/src/shared/socket-util.c
 +++ b/src/shared/socket-util.c
 @@ -297,7 +297,7 @@ int socket_address_print(const SocketAddress *a, char 
 **ret) {

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

2015-03-10 Thread Shawn Landden 
---
 TODO |  2 -
 man/systemd.socket.xml   |  7 ++-
 src/core/service.c   | 41 -
 src/libsystemd/sd-resolve/test-resolve.c |  2 +-
 src/shared/socket-util.c | 76 +++-
 src/shared/socket-util.h |  4 +-
 src/timesync/timesyncd-server.h  |  2 +-
 7 files changed, 106 insertions(+), 28 deletions(-)

diff --git a/TODO b/TODO
index ae32388..780084a 100644
--- a/TODO
+++ b/TODO
@@ -164,8 +164,6 @@ Features:
 * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple 
parallel daemon reloads:
   http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
-* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
doing per-connection socket activation. use format introduced by xinetd or CGI 
for this
-
 * the install state probably shouldn't get confused by generated units, think 
dbus1/kdbus compat!
 
 * in systemctl list-unit-files: show the install value the presets would 
suggest for a service in a third column
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 3938345..6808179 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -357,7 +357,12 @@
 daemons designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation./para
+
+paraFor IPv4 and IPv6 connections the varnameREMOTE_ADDR/varname
+environment variable will contain the remote IP, and 
varnameREMOTE_PORT/varname
+will contain the remote port. This is the same as the format used by 
CGI.
+For SOCK_RAW the port is the IP protocol./para/listitem
   /varlistentry
 
   varlistentry
diff --git a/src/core/service.c b/src/core/service.c
index cc4ea19..bcfce96 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1095,7 +1095,7 @@ static int service_spawn(
 if (r  0)
 goto fail;
 
-our_env = new0(char*, 4);
+our_env = new0(char*, 6);
 if (!our_env) {
 r = -ENOMEM;
 goto fail;
@@ -1119,6 +1119,45 @@ static int service_spawn(
 goto fail;
 }
 
+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 (IN_SET(sa.sa.sa_family, AF_INET, AF_INET6)) {
+_cleanup_free_ char *addr = NULL;
+char *t;
+int port;
+
+r = sockaddr_pretty(sa.sa, salen, true, false, addr);
+if (r  0)
+goto fail;
+
+t = strappend(REMOTE_ADDR=, addr);
+if (!t) {
+r = -ENOMEM;
+goto fail;
+}
+our_env[n_env++] = t;
+
+port = sockaddr_port(sa.sa);
+if (port  0) {
+r = port;
+goto fail;
+}
+
+if (asprintf((our_env + n_env++), REMOTE_PORT=%u, 
port)  0) {
+r = -ENOMEM;
+goto fail;
+}
+}
+}
+
 final_env = strv_env_merge(2, UNIT(s)-manager-environment, our_env, 
NULL);
 if (!final_env) {
 r = -ENOMEM;
diff --git a/src/libsystemd/sd-resolve/test-resolve.c 
b/src/libsystemd/sd-resolve/test-resolve.c
index 3187ce9..354a407 100644
--- a/src/libsystemd/sd-resolve/test-resolve.c
+++ b/src/libsystemd/sd-resolve/test-resolve.c
@@ -46,7 +46,7 @@ static int getaddrinfo_handler(sd_resolve_query *q, int ret, 
const struct addrin
 for (i = ai; i; i = i-ai_next) {
 _cleanup_free_ char *addr = NULL;
 
-assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
addr) == 0);
+assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
true, addr) == 0);
 puts(addr);
 }
 
diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
index 74d90fa..0d87cb1 100644
--- a/src/shared/socket-util.c
+++ b/src/shared/socket-util.c
@@ -297,7 +297,7 @@ int socket_address_print(const SocketAddress *a, char 
**ret) {
 return 0;
 }
 
-return sockaddr_pretty(a-sockaddr.sa, a-size, false, ret);
+return sockaddr_pretty(a-sockaddr.sa, a-size, false, true, ret);
 }
 
 bool

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

2015-03-10 Thread Zbigniew Jędrzejewski-Szmek 
On Tue, Mar 10, 2015 at 01:21:27PM +0100, Ronny Chevalier wrote:
 2015-03-10 12:41 GMT+01:00 Shawn Landden sh...@churchofgit.com:
  ---
   TODO |  2 -
   man/systemd.socket.xml   |  7 ++-
   src/core/service.c   | 41 -
   src/libsystemd/sd-resolve/test-resolve.c |  2 +-
   src/shared/socket-util.c | 76 
  +++-
   src/shared/socket-util.h |  4 +-
   src/timesync/timesyncd-server.h  |  2 +-
   7 files changed, 106 insertions(+), 28 deletions(-)
Applied (with the fix for IN_SET).

Zbyszek


 
  diff --git a/TODO b/TODO
  index ae32388..780084a 100644
  --- a/TODO
  +++ b/TODO
  @@ -164,8 +164,6 @@ Features:
   * as soon as we have kdbus, and sender timestamps, revisit coalescing 
  multiple parallel daemon reloads:
 
  http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
  -* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
  doing per-connection socket activation. use format introduced by xinetd or 
  CGI for this
  -
   * the install state probably shouldn't get confused by generated units, 
  think dbus1/kdbus compat!
 
   * in systemctl list-unit-files: show the install value the presets would 
  suggest for a service in a third column
  diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
  index 3938345..6808179 100644
  --- a/man/systemd.socket.xml
  +++ b/man/systemd.socket.xml
  @@ -357,7 +357,12 @@
   daemons designed for usage with
   
  citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
   to work unmodified with systemd socket
  -activation./para/listitem
  +activation./para
  +
  +paraFor IPv4 and IPv6 connections the 
  varnameREMOTE_ADDR/varname
  +environment variable will contain the remote IP, and 
  varnameREMOTE_PORT/varname
  +will contain the remote port. This is the same as the format used 
  by CGI.
  +For SOCK_RAW the port is the IP protocol./para/listitem
 /varlistentry
 
 varlistentry
  diff --git a/src/core/service.c b/src/core/service.c
  index cc4ea19..bcfce96 100644
  --- a/src/core/service.c
  +++ b/src/core/service.c
  @@ -1095,7 +1095,7 @@ static int service_spawn(
   if (r  0)
   goto fail;
 
  -our_env = new0(char*, 4);
  +our_env = new0(char*, 6);
   if (!our_env) {
   r = -ENOMEM;
   goto fail;
  @@ -1119,6 +1119,45 @@ static int service_spawn(
   goto fail;
   }
 
  +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 (IN_SET(sa.sa.sa_family, AF_INET, AF_INET6)) {
  +_cleanup_free_ char *addr = NULL;
  +char *t;
  +int port;
  +
  +r = sockaddr_pretty(sa.sa, salen, true, false, 
  addr);
  +if (r  0)
  +goto fail;
  +
  +t = strappend(REMOTE_ADDR=, addr);
  +if (!t) {
  +r = -ENOMEM;
  +goto fail;
  +}
  +our_env[n_env++] = t;
  +
  +port = sockaddr_port(sa.sa);
  +if (port  0) {
  +r = port;
  +goto fail;
  +}
  +
  +if (asprintf((our_env + n_env++), 
  REMOTE_PORT=%u, port)  0) {
  +r = -ENOMEM;
  +goto fail;
  +}
  +}
  +}
  +
   final_env = strv_env_merge(2, UNIT(s)-manager-environment, 
  our_env, NULL);
   if (!final_env) {
   r = -ENOMEM;
  diff --git a/src/libsystemd/sd-resolve/test-resolve.c 
  b/src/libsystemd/sd-resolve/test-resolve.c
  index 3187ce9..354a407 100644
  --- a/src/libsystemd/sd-resolve/test-resolve.c
  +++ b/src/libsystemd/sd-resolve/test-resolve.c
  @@ -46,7 +46,7 @@ static int getaddrinfo_handler(sd_resolve_query *q, int 
  ret, const struct addrin
   for (i = ai; i; i = i-ai_next) {
   _cleanup_free_ char *addr = NULL;
 
  -assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, 
  false, addr) == 0);
  +assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, 
  false, true, addr) == 0);
   puts(addr);
   }
 
  diff --git

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

2015-03-10 Thread Lennart Poettering 
On Mon, 09.03.15 23:18, Ronny Chevalier (chevalier.ro...@gmail.com) wrote:

   +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 ?

Yes, I much rpefer this way. (well, except the missing = in the
strappend() invocation...)

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-03-09 Thread Lennart Poettering 
On Sun, 08.03.15 16:24, Shawn Landden (sh...@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 :::a.b.c.d form */
 + sa.sa.sa_family == AF_INET6  
 strchr(a, '.') ?
 +   strempty(startswith(a, :::)) :
 +   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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-03-09 Thread Shawn Landden 
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.

 
  -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
 :::a.b.c.d form */
  +if (translate_ipv6  sa-sa.sa_family == AF_INET6 
 strchr(a, '.')) {
  +ipv4_mapped = true;
  +addr = strempty(startswith(a, :::));

 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

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

2015-03-09 Thread Lennart Poettering 
On Mon, 09.03.15 12:12, Shawn Landden (sh...@churchofgit.com) wrote:

   +
   +if (asprintf(our_env + n_env++,
   + REMOTE_ADDR=%s,
   + /* musl and glibc inet_ntop()
  present v4-mapped addresses in :::a.b.c.d form */
   + sa.sa.sa_family == AF_INET6 
  strchr(a, '.') ?
   +   strempty(startswith(a,
  :::)) :
   +   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.

Hmm, what precisely are you saying? sockaddr_pretty() does use
inet_ntop() for the ipv6 case. Just for the ipv4 case it doesn't,
since formatting that is trivial, and we need to append the port
suffix in it, anyway which makes it easier to just call asprintf()
directly, instead of first using inet_ntop and then asprintf()
always...

But I am not sure I grok what you are trying to say?

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-03-09 Thread Shawn Landden 
---
 TODO |  2 -
 man/systemd.socket.xml   |  6 ++-
 src/core/service.c   | 39 +++-
 src/libsystemd/sd-resolve/test-resolve.c |  2 +-
 src/shared/socket-util.c | 76 +++-
 src/shared/socket-util.h |  4 +-
 src/timesync/timesyncd-server.h  |  2 +-
 7 files changed, 103 insertions(+), 28 deletions(-)

diff --git a/TODO b/TODO
index ae32388..780084a 100644
--- a/TODO
+++ b/TODO
@@ -164,8 +164,6 @@ Features:
 * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple 
parallel daemon reloads:
   http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
-* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
doing per-connection socket activation. use format introduced by xinetd or CGI 
for this
-
 * the install state probably shouldn't get confused by generated units, think 
dbus1/kdbus compat!
 
 * in systemctl list-unit-files: show the install value the presets would 
suggest for a service in a third column
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 3938345..20f1e0c 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -357,7 +357,11 @@
 daemons designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation./para
+paraFor IPv4 and IPv6 connections the varnameREMOTE_ADDR/varname
+environment variable will be set with remote IP, and 
varnameREMOTE_PORT/varname
+environment variable set to the remote port, similar to CGI
+(for SOCK_RAW the port is the IP protocol)./para/listitem
   /varlistentry
 
   varlistentry
diff --git a/src/core/service.c b/src/core/service.c
index cc4ea19..7067a64 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1095,7 +1095,7 @@ static int service_spawn(
 if (r  0)
 goto fail;
 
-our_env = new0(char*, 4);
+our_env = new0(char*, 6);
 if (!our_env) {
 r = -ENOMEM;
 goto fail;
@@ -1119,6 +1119,43 @@ static int service_spawn(
 goto fail;
 }
 
+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;
+int port;
+
+r = sockaddr_pretty(sa.sa, salen, true, false, addr);
+if (r  0)
+goto fail;
+
+if (!(our_env[n_env++] = strappend(REMOTE_ADDR=, 
addr))) {
+r = -ENOMEM;
+goto fail;
+}
+
+port = sockaddr_port(sa.sa);
+if (port  0) {
+r = port;
+goto fail;
+}
+
+if (asprintf(our_env[n_env++], REMOTE_PORT=%u, port) 
 0) {
+r = -ENOMEM;
+goto fail;
+}
+}
+}
+
 final_env = strv_env_merge(2, UNIT(s)-manager-environment, our_env, 
NULL);
 if (!final_env) {
 r = -ENOMEM;
diff --git a/src/libsystemd/sd-resolve/test-resolve.c 
b/src/libsystemd/sd-resolve/test-resolve.c
index 3187ce9..354a407 100644
--- a/src/libsystemd/sd-resolve/test-resolve.c
+++ b/src/libsystemd/sd-resolve/test-resolve.c
@@ -46,7 +46,7 @@ static int getaddrinfo_handler(sd_resolve_query *q, int ret, 
const struct addrin
 for (i = ai; i; i = i-ai_next) {
 _cleanup_free_ char *addr = NULL;
 
-assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
addr) == 0);
+assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
true, addr) == 0);
 puts(addr);
 }
 
diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
index 74d90fa..ca8ff39 100644
--- a/src/shared/socket-util.c
+++ b/src/shared/socket-util.c
@@ -297,7 +297,7 @@ int socket_address_print(const SocketAddress *a, char 
**ret) {
 return 0;
 }
 
-return sockaddr_pretty(a-sockaddr.sa, a-size, false, ret);
+return sockaddr_pretty(a-sockaddr.sa, a-size, false, true, ret);
 }
 
 bool socket_address_can_accept(const SocketAddress *a) {
@@ -466,7 +466,21 @@ bool

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

2015-03-09 Thread Shawn Landden 
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.

 
  -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
 :::a.b.c.d form */
  +if (translate_ipv6  sa-sa.sa_family == AF_INET6 
 strchr(a, '.')) {
  +ipv4_mapped = true;
  +addr = strempty(startswith(a, :::));

 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.

https://tools.ietf.org/html/rfc5952#section-4.3 says lowercase-only
https://tools.ietf.org/html/rfc5952#section-5 specifies formatting of
ipv4-mapped addresses

and it is significantly longer that way, but sure, I can do it that way.


  +} 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

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

2015-03-09 Thread Shawn Landden 
---
 TODO |  2 -
 man/systemd.socket.xml   |  6 ++-
 src/core/service.c   | 35 +-
 src/libsystemd/sd-resolve/test-resolve.c |  2 +-
 src/shared/socket-util.c | 80 ++--
 src/shared/socket-util.h |  4 +-
 src/timesync/timesyncd-server.h  |  2 +-
 7 files changed, 88 insertions(+), 43 deletions(-)

diff --git a/TODO b/TODO
index ae32388..780084a 100644
--- a/TODO
+++ b/TODO
@@ -164,8 +164,6 @@ Features:
 * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple 
parallel daemon reloads:
   http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
-* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
doing per-connection socket activation. use format introduced by xinetd or CGI 
for this
-
 * the install state probably shouldn't get confused by generated units, think 
dbus1/kdbus compat!
 
 * in systemctl list-unit-files: show the install value the presets would 
suggest for a service in a third column
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 3938345..20f1e0c 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -357,7 +357,11 @@
 daemons designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation./para
+paraFor IPv4 and IPv6 connections the varnameREMOTE_ADDR/varname
+environment variable will be set with remote IP, and 
varnameREMOTE_PORT/varname
+environment variable set to the remote port, similar to CGI
+(for SOCK_RAW the port is the IP protocol)./para/listitem
   /varlistentry
 
   varlistentry
diff --git a/src/core/service.c b/src/core/service.c
index cc4ea19..89feec4 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1095,7 +1095,7 @@ static int service_spawn(
 if (r  0)
 goto fail;
 
-our_env = new0(char*, 4);
+our_env = new0(char*, 6);
 if (!our_env) {
 r = -ENOMEM;
 goto fail;
@@ -1119,6 +1119,39 @@ static int service_spawn(
 goto fail;
 }
 
+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);
+
+r = sockaddr_pretty(sa.sa, salen, true, false, addr);
+if (r  0)
+goto fail;
+
+if (!(our_env[n_env++] = strappend(REMOTE_ADDR=, 
addr))) {
+r = -ENOMEM;
+goto fail;
+}
+
+if (asprintf(our_env + n_env++,
+ REMOTE_PORT=%u,
+ port)  0) {
+r = -ENOMEM;
+goto fail;
+}
+}
+}
+
 final_env = strv_env_merge(2, UNIT(s)-manager-environment, our_env, 
NULL);
 if (!final_env) {
 r = -ENOMEM;
diff --git a/src/libsystemd/sd-resolve/test-resolve.c 
b/src/libsystemd/sd-resolve/test-resolve.c
index 3187ce9..354a407 100644
--- a/src/libsystemd/sd-resolve/test-resolve.c
+++ b/src/libsystemd/sd-resolve/test-resolve.c
@@ -46,7 +46,7 @@ static int getaddrinfo_handler(sd_resolve_query *q, int ret, 
const struct addrin
 for (i = ai; i; i = i-ai_next) {
 _cleanup_free_ char *addr = NULL;
 
-assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
addr) == 0);
+assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
true, addr) == 0);
 puts(addr);
 }
 
diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
index 74d90fa..d7d34f8 100644
--- a/src/shared/socket-util.c
+++ b/src/shared/socket-util.c
@@ -297,7 +297,7 @@ int socket_address_print(const SocketAddress *a, char 
**ret) {
 return 0;
 }
 
-return sockaddr_pretty(a-sockaddr.sa, a-size, false, ret);
+return sockaddr_pretty(a-sockaddr.sa, a-size, false, true, ret);
 }
 
 bool socket_address_can_accept(const SocketAddress *a) {
@@ -466,7 +466,17 @@ bool socket_address_matches_fd(const SocketAddress *a, int 
fd) {
 return socket_address_equal(a, b);

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

2015-03-09 Thread Lennart Poettering 
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.

  
 -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 
 :::a.b.c.d form */
 +if (translate_ipv6  sa-sa.sa_family == AF_INET6  
 strchr(a, '.')) {
 +ipv4_mapped = true;
 +addr = strempty(startswith(a, :::));

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

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

2015-03-09 Thread Shawn Landden 
On Mon, Mar 9, 2015 at 9:22 AM, Lennart Poettering lenn...@poettering.net
wrote:

 On Sun, 08.03.15 16:24, Shawn Landden (sh...@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 :::a.b.c.d form */
  + sa.sa.sa_family == AF_INET6 
 strchr(a, '.') ?
  +   strempty(startswith(a,
 :::)) :
  +   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@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

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

2015-03-09 Thread Ronny Chevalier 
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
  :::a.b.c.d form */
  +if (translate_ipv6  sa-sa.sa_family == AF_INET6 
  strchr(a, '.')) {
  +ipv4_mapped = true;
  +addr = strempty(startswith(a, :::));

 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.

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

2015-03-09 Thread Zbigniew Jędrzejewski-Szmek 
On Mon, Mar 09, 2015 at 03:11:37PM -0700, Shawn Landden wrote:
 ---
  TODO |  2 -
  man/systemd.socket.xml   |  6 ++-
  src/core/service.c   | 39 +++-
  src/libsystemd/sd-resolve/test-resolve.c |  2 +-
  src/shared/socket-util.c | 76 
 +++-
  src/shared/socket-util.h |  4 +-
  src/timesync/timesyncd-server.h  |  2 +-
  7 files changed, 103 insertions(+), 28 deletions(-)
 
 diff --git a/TODO b/TODO
 index ae32388..780084a 100644
 --- a/TODO
 +++ b/TODO
 @@ -164,8 +164,6 @@ Features:
  * as soon as we have kdbus, and sender timestamps, revisit coalescing 
 multiple parallel daemon reloads:

 http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
  
 -* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
 doing per-connection socket activation. use format introduced by xinetd or 
 CGI for this
 -
  * the install state probably shouldn't get confused by generated units, 
 think dbus1/kdbus compat!
  
  * in systemctl list-unit-files: show the install value the presets would 
 suggest for a service in a third column
 diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
 index 3938345..20f1e0c 100644
 --- a/man/systemd.socket.xml
 +++ b/man/systemd.socket.xml
 @@ -357,7 +357,11 @@
  daemons designed for usage with
  
 citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
  to work unmodified with systemd socket
 -activation./para/listitem
 +activation./para
Empty line between paragraphs.

 +paraFor IPv4 and IPv6 connections the 
 varnameREMOTE_ADDR/varname
 +environment variable will be set with remote IP, and 
 varnameREMOTE_PORT/varname
will be set with → will contain

 +environment variable set to the remote port, similar to CGI
set to → will contain

 +(for SOCK_RAW the port is the IP protocol)./para/listitem
Can you change , similar to CGI to an explicit sentence like This
is the same format as the one used by xinetd and CGI.. This will help
potential users.

/varlistentry
  
varlistentry
 diff --git a/src/core/service.c b/src/core/service.c
 index cc4ea19..7067a64 100644
 --- a/src/core/service.c
 +++ b/src/core/service.c
 @@ -1095,7 +1095,7 @@ static int service_spawn(
  if (r  0)
  goto fail;
  
 -our_env = new0(char*, 4);
 +our_env = new0(char*, 6);
  if (!our_env) {
  r = -ENOMEM;
  goto fail;
 @@ -1119,6 +1119,43 @@ static int service_spawn(
  goto fail;
  }
  
 +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) {
IN_SET(sa.sa.sa_family, AF_INET, AF_INET6)

 +_cleanup_free_ char *addr = NULL;
 +int port;
 +
 +r = sockaddr_pretty(sa.sa, salen, true, false, 
 addr);
 +if (r  0)
 +goto fail;
 +
 +if (!(our_env[n_env++] = strappend(REMOTE_ADDR=, 
 addr))) {
Using a temporary variable like Ronny suggested seems a good idea.

 +r = -ENOMEM;
 +goto fail;
 +}
 +
 +port = sockaddr_port(sa.sa);
 +if (port  0) {
 +r = port;
 +goto fail;
 +}
 +
 +if (asprintf(our_env[n_env++], REMOTE_PORT=%u, 
 port)  0) {
 +r = -ENOMEM;
 +goto fail;
 +}
 +}
 +}
 +
  final_env = strv_env_merge(2, UNIT(s)-manager-environment, 
 our_env, NULL);
  if (!final_env) {
  r = -ENOMEM;
 diff --git a/src/libsystemd/sd-resolve/test-resolve.c 
 b/src/libsystemd/sd-resolve/test-resolve.c
 index 3187ce9..354a407 100644
 --- a/src/libsystemd/sd-resolve/test-resolve.c
 +++ b/src/libsystemd/sd-resolve/test-resolve.c
 @@ -46,7 +46,7 @@ static int getaddrinfo_handler(sd_resolve_query *q, int 
 ret, const struct addrin
  for (i = ai; i; i = i-ai_next) {
  _cleanup_free_ char *addr = NULL;
  
 -assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
 addr) == 0);
 +assert_se(sockaddr_pretty(i-ai_addr, i-ai_addrlen, false, 
 true, addr) == 0);

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

2015-03-08 Thread Shawn Landden 
---
 TODO   |  2 --
 man/systemd.socket.xml |  6 +-
 src/core/service.c | 47 +++
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/TODO b/TODO
index ae32388..780084a 100644
--- a/TODO
+++ b/TODO
@@ -164,8 +164,6 @@ Features:
 * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple 
parallel daemon reloads:
   http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
-* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
doing per-connection socket activation. use format introduced by xinetd or CGI 
for this
-
 * the install state probably shouldn't get confused by generated units, think 
dbus1/kdbus compat!
 
 * in systemctl list-unit-files: show the install value the presets would 
suggest for a service in a third column
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 3938345..20f1e0c 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -357,7 +357,11 @@
 daemons designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation./para
+paraFor IPv4 and IPv6 connections the varnameREMOTE_ADDR/varname
+environment variable will be set with remote IP, and 
varnameREMOTE_PORT/varname
+environment variable set to the remote port, similar to CGI
+(for SOCK_RAW the port is the IP protocol)./para/listitem
   /varlistentry
 
   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) {
+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;
+}
+
+if (asprintf(our_env + n_env++,
+ REMOTE_ADDR=%s,
+ /* musl and glibc inet_ntop() present 
v4-mapped addresses in :::a.b.c.d form */
+ sa.sa.sa_family == AF_INET6  strchr(a, 
'.') ?
+   strempty(startswith(a, :::)) :
+   a)  0) {
+r = -ENOMEM;
+goto fail;
+}
+
+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;
+}
+}
+}
+
 final_env = strv_env_merge(2, UNIT(s)-manager-environment, our_env, 
NULL);
 if (!final_env) {
 r = -ENOMEM;
-- 
2.2.1.209.g41e5f3a

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel