Abort murmur if no ipv6 and no ipv4 are found.

[fedpop]

This is my fix for issue #1629 (and #1904)

It will exit murmur if it can not find any address to bind. Then init can manage the restart. The second commit alters murmur.service so that it won't fail due to ratelimit.

It works on master and 1.2.10.

Comments from mkrautz on IRC

<fedpop> If I was to create a patch for #1629 what would be the desired behavior? if hasipv4 and     hasipv6 are both false at the end of the block should murmur call qfatal? or should murmur bind to ::? or should murmur enter a loop and keep trying? the relevant blocks are here https://github.com/mumble-voip/mumble/blob/1.2.10/src/murmur/Meta.cpp#L210-L256
<mkrautz> fedpop: if that's it, ISTM that it should qFatal, and .service should have After=network-online.target
<mkrautz> fedpop: sgtm if the restart-daemon-on-exit is not something that could confuse users because it'd appear in syslog or wherever...

[mkrautz]

It seems wrong to rely on the qFatal branch to be executed, and the daemon to be restarted.

Is it not better to just use After=network-online.target?

[mkrautz]

systemd docs recommend against it though:
http://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/

[fedpop]

after=network-online.target is not reliable in my experience.

Any daemon I have ever worked with has to solve this in one of two ways. Either exit if network is not up, or enter a loop and try until network is up. I prefer the first way as it lets the sysadmin have more control.

I think that qFatal needs to be added somewhere regardless though. Murmur shouldn't continue if it has nothing to bind to.

[fedpop]

I think we need to rework the logic. Here: https://github.com/mumble-voip/mumble/blob/master/src/murmur/Meta.cpp#L225-L281

According to murmur.ini

# If this is left blank (default), Murmur will bind to all available addresses.

If that's the case, we should check the system for ipv6/dual stack support and bind to :: , if ipv4 only bind to 0.0.0.0

The way the logic currently detects network support is a little bit silly and causing the root of our issues.

I'll give it a shot and make a new PR.

[mkrautz]

I think it is a bit tricky to get right. Mainly because we have to support Qt 4 and 5, as well as Windows, Linux and OS X.

Officially, we're allowing Qt 4 for 1.3.0, but technically, it's only Linux and the other FOSS OSes.

So in that regard, testing should be easier.

[Now, the above was before I looked at the code...]

I wonder why we don't just simplify the code to do:

#if QT_VERSION >= 0x050000
    if (SslServer::hasDualStackSupport()) {
        qlBind << QHostAddress(QHostAddress::Any);
    } else {
        qlBind << QHostAddress(QHostAddress::AnyIPv6);
        qlBind << QHostAddress(QHostAddress::AnyIPv4);
    }
#else // QT_VERSION < 0x050000
    // For Qt 4 AnyIPv6 resulted in a dual stack socket on dual stack
    // capable systems while Any resulted in an IPv4 only socket. For
    // Qt 5 this has been reworked so that AnyIPv6/v4 are now exclusive
    // IPv6/4 sockets while Any is the dual stack socket.
    qlBind << QHostAddress(QHostAddress::AnyIPv6);
    if (!SslServer::hasDualStackSupport()) {
        qlBind << QHostAddress(QHostAddress::Any);
    }
#endif

And avoid the interface querying...

[hacst]

@mkrautz Won't this make us try to create sockets which attempts to bind the same IPv4 stuff twice for the legacy fallback? Is that ok. Also what happens if you tell it to bind to an IPv6 socket on an IPv4 only system in the non-legacy part?

Definitely would love to be able to simplify it like that though. Didn't think ahead this far after my first round of refactoring.

[mkrautz]

ISTM that

Qt 4 QHostAddress::AnyIPv6 == Qt 5 QHostAddress::Any when hasDualStackSupport() == true

but when !hasDualStackSupport(), we need to listen for both ::AnyIPv6 and ::Any.

I don't see how that makes us listen twice?

[hacst]

You are correct. Didn't think the !hasDualStackSupport() part through properly. The IPv6 on IPv4 only non-dual-stack systems still stands though.

[mkrautz]

Maybe we don't need it on Qt 5? Now that there is an explicit Any that covers both IPv4 and IPv6, it would seem to me that it should work in all cases. That is, the code becomes:

#if QT_VERSION >= 0x050000
    qlBind << QHostAddress(QHostAddress::Any);
#else // QT_VERSION < 0x050000
    // For Qt 4 AnyIPv6 resulted in a dual stack socket on dual stack
    // capable systems while Any resulted in an IPv4 only socket. For
    // Qt 5 this has been reworked so that AnyIPv6/v4 are now exclusive
    // IPv6/4 sockets while Any is the dual stack socket.
    qlBind << QHostAddress(QHostAddress::AnyIPv6);
    if (!SslServer::hasDualStackSupport()) {
        qlBind << QHostAddress(QHostAddress::Any);
    }
#endif

[hacst]

Are you sure Qt 5 handles the non-dual-stack code path transparently? I somehow doubt that as it would have to manage two sockets under the hood which have to act like one to the user. Possible but surely not easy to do with the given interface of the socket classes.

[mkrautz]

OK, so scrap that.

It seems to me we have the same problem for Qt 4 and Qt 5.

If the system is not dual stack, what happens on Qt 5 when...

• We listen for QHostAddress::AnyIPv6 on an IPv4-only system?
• Alternatively, we listen for QHostAddress::AnyIPv4 on an an IPv6-only system?

The same applies to Qt 4. What happens when we...

• Listen for QHostAddress::AnyIPv6 on an IPv4-only system?
• Listen for QHostAddress::Any on an IPv6-only system?

Do you agree with that?

[mkrautz]

If the above cases are the only problems left, we could transform the code into the following (pseudo code):

    if (dualstack) {
#if Qt 5
        qlBind << QHostAddress(QHostAddress::Any);
#else // Qt 4
        qlBind << QHostAddress(QHostAddress::AnyIPv6);
#endif
    } else {
         hasipv4, hasipv4 = queryInterfaces();
#if Qt 5
         if (hasipv4) qlBind << QHostAddress(QHostAddress::AnyIPv4);
         if (hasipv6) qlBind << QHostAddress(QHostAddress::AnyIPv6);
#else // Qt 4
         if (hasipv4) qlBind << QHostAddress(QHostAddress::Any);
         if (hasipv6) qlBind << QHostAddress(QHostAddress::AnyIPv6);
#endif
    }

[mkrautz]

That would make dualstack our primary method, for sane, modern systems.

And non-dualstack would involve querying the interfaces, and qFatal()'ing if none are available at daemon start.

[hacst]

LGTM. Just throw in a comment or two in the fallback path ;)

[mkrautz]

I am not implementing this at the moment.

@fedpop Does the above sound good to you too?