[PATCH] Support IPv6-less systems with runtime check.

[Lennart Poettering]

On Wed, 08.09.10 23:02, Gustavo Sverzut Barbieri (barbieri at profusion.mobi) wrote:

> This patch introduces socket_ipv6_is_supported() call that checks for
> IPv6 availability. Code then check for it before using specific calls.
> 
> In order to be less intrusive, this patch avoids IPv6 entries being
> parsed at all, this way we don't get such entries in the system and
> all other code paths are automatically ignored. However an extra check
> is done at socket_address_listen() to make sure of that.
> 
> As the number of Netlink messages is not know upfront anymore,
> loopback-setup.c was refactored to dynamically calculate the sequence
> number and count.

Hmm, I like the general approach of this patch. However, I am a bit
concerned that this thing is not dynamic: i.e. if the user loads the
ipv6 module after boot, systemd cannot be informed about that... I think
socket_ipv6_is_supported() should not cache the test result. That should
fix this issue. Given that /sys is virtual always going to access()
should not be expensive.

Some comments:

> ---
>  src/loopback-setup.c |   46 ++++++++++++--------------
>  src/socket-util.c    |   90 +++++++++++++++++++++++++++++++++++++++++++------
>  src/socket-util.h    |    2 +
>  src/socket.c         |   13 +++++--
>  4 files changed, 111 insertions(+), 40 deletions(-)
> 
> diff --git a/src/loopback-setup.c b/src/loopback-setup.c
> index c852ed7..a579060 100644
> --- a/src/loopback-setup.c
> +++ b/src/loopback-setup.c

The patch to loopback-setup.c looks fine.

> diff --git a/src/socket-util.c b/src/socket-util.c
> index 151757c..ecf965d 100644
> --- a/src/socket-util.c
> +++ b/src/socket-util.c
> @@ -35,6 +35,32 @@
>  #include "socket-util.h"
>  #include "missing.h"
>  #include "label.h"
> +#include <sys/ioctl.h>
> +
> +static int ipv4_inetaddr(const char *iface, struct in_addr *ret)
> +{
> +        int fd;
> +        struct ifreq ifr;
> +
> +        fd = socket(AF_INET, SOCK_DGRAM, 0);
> +        if (fd < 0)
> +                return -errno;
> +
> +        ifr.ifr_addr.sa_family = AF_INET;
> +        strncpy(ifr.ifr_name, iface, IF_NAMESIZE-1);
> +
> +        if (ioctl(fd, SIOCGIFADDR, &ifr) < 0) {
> +                int r = -errno;
> +                close(fd);
> +                return r;
> +        }
> +
> +        close(fd);
> +
> +        ret->s_addr = ((struct sockaddr_in *)&ifr.ifr_addr)->sin_addr.s_addr;
> +
> +        return 0;
> +}

The behaviour of this ipv4_inetaddr() function is very different from
sockaddr.in6.sin6_scope_id. I'd prefer if we'd simply not claim we
support per-interface listening on Ipv4, because the kernel just doesn't
support it. i.e. return -EAFNOSUPPORT, don't try to emulate
sockaddr.in6.sin6_scope_id.

Or to put it the other way round: people should just use IPv6 if they
want all those awesome IPv6'ish features like scope IDs.

The differences in behaviour of your suggested ipv4_inetaddr() and
in6_scope_id are quite substantial: the latter we can use even if if no
IP address is configured on an interface, the former hoewver requires we
execute that code only after the iface is fully set up. It's easy to
synchronize the former properly with the deps systemd supports, but the
latter not since systemd does not actively listen for network
configuration changes. In systemd right now we just cover whether a
network iface exists, not whether it is up and has an IP
address. Furthermore the difference in behaviour if the same address is
configured to more than one interface is big, too.

>  int socket_address_parse(SocketAddress *a, const char *s) {

The rest of the socket_address_parse() look good.

>  
> @@ -316,6 +366,9 @@ int socket_address_listen(
>          if ((r = socket_address_verify(a)) < 0)
>                  return r;
>  
> +        if (socket_address_family(a) == AF_INET6 && !socket_ipv6_is_supported())
> +                return -EINVAL;

-EAFNOSUPPORT would be nice here.


>          if (s->ip_ttl >= 0) {
> -                int r, x;
> +                int r;
>  
>                  r = setsockopt(fd, IPPROTO_IP, IP_TTL, &s->ip_ttl, sizeof(s->ip_ttl));
> -                x = setsockopt(fd, IPPROTO_IPV6, IPV6_UNICAST_HOPS, &s->ip_ttl, sizeof(s->ip_ttl));
> +                if (r < 0)
> +                        log_warning("IP_TTL failed: %m");
> +
> +                if (socket_ipv6_is_supported()) {
> +                        r = setsockopt(fd, IPPROTO_IPV6, IPV6_UNICAST_HOPS, &s->ip_ttl, sizeof(s->ip_ttl));
> +                        if (r < 0)
> +                        log_warning("IPV6_UNICAST_HOPS failed: %m");
> +                }
>  
> -                if (r < 0 && x < 0)
> -                        log_warning("IP_TTL/IPV6_UNICAST_HOPS failed: %m");

This turns around the logic. The original code tried to apply both the
IPv4 and the IPv6 TTL and only if both failed printed an error. Your
patch makes an error show up when either fails, which means an error is
printed in all cases.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.