Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Meta: enable both IPv4 and IPv6 if we're unable to query network interfaces. #2908

Merged
merged 1 commit into from Mar 5, 2017

Conversation

@mkrautz
Copy link
Member

commented Mar 5, 2017

On systems using systemd, Murmur is often started early, even before
Murmur can query the network interfaces.

This commit adds a fallback, where Murmur will expect such a system to
support both IPv4 and IPv6.

Fixes #1629
Fixes #1904

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Mar 5, 2017

@Kissaki

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

Murmur is often started early, even before Murmur can query the network interfaces

Shouldn't the systemd service description depend on that then to make it work?

Murmur will expect such a system to support both IPv4 and IPv6.

What happens if such an expectation turns out to not be met? Will we log a warning? Will nothing happen?

@Kissaki

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

Shouldn't the systemd service description depend on that then to make it work?

This comment on #1904 mentions:

the After=network.target stanza won't insure that network is actually working, only that network target has started.

Not sure if that is all we got.

I am using my own systemd configuration of type forking rather than simple (as the arch configuration as linked in #1904) which works fine even without a pre-exec sleep. This may be a runtime issue, but I am certain I restarted my server twice with it working as intended (and a followup Ice authenticator service working fine as well).

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

@Kissaki

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

No, I was not aware of that one.

Mine as a gist

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

Murmur is often started early, even before Murmur can query the network interfaces

Shouldn't the systemd service description depend on that then to make it work?

People have tried to depend on network, and even then, Murmurd is attempted to be launched before Murmur can query the network interfaces. (As you've noted... I began writing this reply before your second comment. :-))

Murmur will expect such a system to support both IPv4 and IPv6.

What happens if such an expectation turns out to not be met? Will we log a warning? Will nothing happen?

I don't think we can detect it. But for Linux systems that have systemd, I believe it is a safe assumption.
I believe the network interface detection stuff is there for legacy systems, such as pre-Vista Windows, etc.

}

if (nif == 0) {
qWarning("Meta: Unable to query network interfaces. Assuming support for both IPv4 and IPv6.");

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 5, 2017

Member

I don't like the "query" here. If we were able to check individual interfaces for their "up" and "running" and "loopback" states, we surely were able to query them!?

QNetworkInterface::allInterfaces() can return an empty list on a "failure".

I think we should make it two checks; if the list is empty, we were not able to "query" them - because of some kind of failure. If the list is non-empty but we do not find a interface matching our filter (up, running, no loopback) we warn about that. That would provide more specific and elaborative messages.

Looking the current changes against the change description - I described two "error" cases we handle as one here. So our change is not only for the described "could not query"/"interfaces not ready yet" (whatever that means), but we now also do this on allInterfaces()s failure - which we ignore before (at least here).

I guess configuration determines the interfaces gettable through allInterfaces - not sure at what point (after/on the systemd network interfaces target?), then they get enabled/upped, once the finished "upping" they are "running". If we will always get the interfaces, but they are just not running or enabled yet, we can separate the failure case.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

I don't like the "query" here. If we were able to check individual interfaces for their "up" and "running" and "loopback" states, we surely were able to query them!?

No we couldn't query it for the information we wanted.

QNetworkInterface::allInterfaces() can return an empty list on a "failure".

I think we should make it two checks; if the list is empty, we were not able to "query" them - because of some kind of failure. If the list is non-empty but we do not find a interface matching our filter (up, running, no loopback) we warn about that. That would provide more specific and elaborative messages.

Looking the current changes against the change description - I described two "error" cases we handle as one here. So our change is not only for the described "could not query"/"interfaces not ready yet" (whatever that means), but we now also do this on allInterfaces()s failure - which we ignore before (at least here).

I guess configuration determines the interfaces gettable through allInterfaces - not sure at what point (after/on the systemd network interfaces target?), then they get enabled/upped, once the finished "upping" they are "running". If we will always get the interfaces, but they are just not running or enabled yet, we can separate the failure case.

I agree with the sentiment. I'll see if I can get it to make sense in the error messages.

Meta: enable both IPv4 and IPv6 if we're unable to query network inte…
…rfaces.

On systems using systemd, Murmur is often started early, even before
Murmur can query the network interfaces.

This commit adds a fallback, where Murmur will expect such a system to
support both IPv4 and IPv6.

Fixes #1629
Fixes #1904

@mkrautz mkrautz force-pushed the mkrautz:meta-ipv4-ipv6 branch from 009c887 to a9b9dfe Mar 5, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

@Kissaki How about this? (Updated the commit.)

@Kissaki
Kissaki approved these changes Mar 5, 2017

@mkrautz mkrautz merged commit cb952e0 into mumble-voip:master Mar 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Kissaki

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

Actually, my autostarting setup does not work. So don't mind my memory from above…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.