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

miniupnpd: add service #11548

Merged
merged 1 commit into from
Dec 10, 2015
Merged

miniupnpd: add service #11548

merged 1 commit into from
Dec 10, 2015

Conversation

jgillich
Copy link
Member

@jgillich jgillich commented Dec 8, 2015

Not sure if there are any more common config options to add.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers

{
options = {
services.miniupnpd = {
enable = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use mkEnableOption.

@jgillich
Copy link
Member Author

jgillich commented Dec 8, 2015

@jagajaga Thanks, based all of this on another service so I had no idea mkEnableOption even exists. Is there documentation for them or where can I find them in the source tree?

Anyway, all fixed, also fixed the descriptions that made no sense.

{
options = {
services.miniupnpd = {
enable = mkEnableOption "Enable the MiniUPnP daemon.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an argument you need to write just the name of option :)
In this case you need to write just the name of service.
You can search through nixpkgs and find some examples.

@jgillich
Copy link
Member Author

jgillich commented Dec 8, 2015

Oh I did look at examples, but seem to have missed that. ;)

@jgillich
Copy link
Member Author

jgillich commented Dec 9, 2015

Btw why is there no mkDisableOption as a shorthand for default = true?

@jgillich
Copy link
Member Author

jgillich commented Dec 9, 2015

Please don't merge yet, the -d option is really verbose, I need to change it to use the PID file.

@zimbatm
Copy link
Member

zimbatm commented Dec 9, 2015

Is it possible to run miniupnpd in it's own user instead of root ?

@jgillich
Copy link
Member Author

@zimbatm You tell me haha. This thing has bascially zero documentation and I'm not exactly an expert in upnp.

@zimbatm
Copy link
Member

zimbatm commented Dec 10, 2015

@jgillich Looking at Gentoo again, it seems like it's touching iptables so probably needs root access. Their init script also runs a /etc/miniupnpd/iptables_removeall.sh script on stop which might apply for us as well ? See https://gitweb.gentoo.org/repo/gentoo.git/tree/net-misc/miniupnpd/files/miniupnpd-init.d

@zimbatm
Copy link
Member

zimbatm commented Dec 10, 2015

@jgillich
Copy link
Member Author

They also call a script to create the rules before starting the service, I'm currently doing that via firewall.extraCommands. That could probably be integrated into the service, I'll also try running it as non-root.

@zimbatm
Copy link
Member

zimbatm commented Dec 10, 2015

Yeah, I wonder how it interacts with the NixOS built-in firewall, are the rules not going to overlap ?

@jgillich
Copy link
Member Author

By "they" I meant Arch and Gentoo. :) miniupnpd only uses the chains set via upnp_forward_chain and upnp_nat_chain in the config and doesn't touch the firewall.

@zimbatm
Copy link
Member

zimbatm commented Dec 10, 2015

👍

jagajaga added a commit that referenced this pull request Dec 10, 2015
@jagajaga jagajaga merged commit bc8d08a into NixOS:master Dec 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants