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.

Sign up for GitHub

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

Jump to bottom

initial NixOS module for LIRC #32045

Merged
merged 1 commit into from Sep 17, 2018
Merged

initial NixOS module for LIRC #32045

merged 1 commit into from Sep 17, 2018

Conversation

ck3d
Copy link
Contributor

@ck3d ck3d commented Nov 25, 2017

Motivation for this change

LIRC is not available in NixOS

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

orivej
orivej suggested changes Nov 29, 2017
View reviewed changes
nixos/modules/services/hardware/lirc.nix Outdated
system.activationScripts.lirc = ''
umask 027
mkdir -p /var/run/lirc
chown -R lirc:lirc /var/run/lirc
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for -R.

Copy link
Member

Choose a reason for hiding this comment

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

There are a few things here:

  1. the mkdir/chown lines can be combined using install instead, but
  2. it's superfluous as the /run/lirc directory will be automatically created by the ListenStream bit on your socket configuration ref man systemd.socket.

nixos/modules/services/hardware/lirc.nix Outdated
description = "LIRC";
wantedBy = [ "sockets.target" ];
socketConfig = {
ListenStream = "/var/run/lirc/lircd";
Copy link
Contributor

Choose a reason for hiding this comment

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

Use /run/lirc/lircd.

nixos/modules/services/hardware/lirc.nix Outdated

extraArguments = mkOption {
type = types.str;
default = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a list, like this:

nixpkgs/nixos/modules/services/misc/zookeeper.nix

Lines 94 to 99 in 898aedc

extraCmdLineOptions = mkOption {
description = "Extra command line options for the Zookeeper launcher.";
default = [ "-Dcom.sun.management.jmxremote" "-Dcom.sun.management.jmxremote.local.only=true" ];
type = types.listOf types.str;
example = [ "-Djava.net.preferIPv4Stack=true" "-Dcom.sun.management.jmxremote" "-Dcom.sun.management.jmxremote.local.only=true" ];
};

nixos/modules/services/hardware/lirc.nix Outdated
unitConfig.Documentation = [ "man:lircd(8)" ];

serviceConfig = {
ExecStart = "${pkgs.lirc}/bin/lircd --nodaemon ${cfg.extraArguments} ${configFile}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Use escapeShellArgs:

nixpkgs/nixos/modules/services/misc/zookeeper.nix

Line 122 in 898aedc

${escapeShellArgs cfg.extraCmdLineOptions} \

@ck3d
Copy link
Contributor Author

ck3d commented Nov 29, 2017

Thanks! I introduces all your good suggestions.
I'll invite you to review also the lirc package update in PR #32042

peterhoeg
peterhoeg reviewed Dec 7, 2017
View reviewed changes
nixos/modules/services/hardware/lirc.nix Outdated
description = "LIRC default options (lirc_options.conf)";
};

config = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Where is config used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. I updated the description to clarify the option. The content is written into a file, which is passed to lircd as configuration.

peterhoeg
peterhoeg reviewed Dec 8, 2017
View reviewed changes
nixos/modules/services/hardware/lirc.nix Outdated
mkdir -p /run/lirc
chown lirc:lirc /run/lirc
fi
'';
Copy link
Member

Choose a reason for hiding this comment

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

There are a few things here:

  1. the mkdir/chown lines can be combined using install instead, but
  2. it's superfluous as the /run/lirc directory will be automatically created by the ListenStream bit on your socket configuration ref man systemd.socket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The folder is created to grant lircd write access. It creates a pid file and removes for any reason the socket.
I tried to follow your hints:

  1. I replace it with "install -o vdr -g vdr -m 0750 -d /run/lirc", but for any reason, the folder is owned by root?!
  2. If systemd creates the folder, the owner is root. So I used ExecStartPost to change the owner, which is to late for any reason?!

Thanks for your review, but I do not find an alternative solution.

Copy link
Member

Choose a reason for hiding this comment

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

Since lirc.socket creates the directory, my guess is that it overrides what you are doing in the .service.

You probably need to set the following in the .socket definition instead:

DirectoryMode = "750";
SocketUser = "lirc";
SocketGroup = "lirc";

Then you can get rid of the script.

Copy link
Member

Choose a reason for hiding this comment

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

man systemd.socket is your friend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestions, but

  • SocketUser is already set,
  • SocketGroup is not needed, because Systemd takes the primary group of user given at SocketUser
  • DirectoryMode is by default 755, which is good enough.

Lircd needs to be owner (or at least "create rights") on the folder /run/lirc.

@ck3d
Copy link
Contributor Author

ck3d commented Dec 9, 2017

I removed socket activation at all, because I could not setup a working environment.

@ck3d
Copy link
Contributor Author

ck3d commented Dec 9, 2017

After playing around with the new lirc-0.10.0 and making sure that no service depends on lircd.service, I could get it working.

peterhoeg
peterhoeg reviewed Apr 3, 2018
View reviewed changes
nixos/modules/services/hardware/lirc.nix Outdated
unitConfig.Documentation = [ "man:lircd(8)" ];

serviceConfig = {
ExecStartPre = "${pkgs.coreutils}/bin/install -o lirc -g lirc -m 750 -d /run/lirc";
Copy link
Member

Choose a reason for hiding this comment

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

This should be RuntimeDirectory = "lirc"; instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, it fits perfectly. I updated PR.

@peterhoeg
Copy link
Member

I have been converting a number of services to use DynamicUser = true; instead of statically allocated users and I think this would be a good candidate. Unfortunately I don't have the required hardware to test this out but it would be awesome if we could work together on this here. What is required:

  1. get rid of the static allocations in ids.nix
  2. don't create any users
  3. set serviceConfig.DynamicUser = true;

Does this still work on your side? It should be perfectly fine with socket activation ref http://0pointer.net/blog/dynamic-users-with-systemd.html

@ck3d
Copy link
Contributor Author

ck3d commented Apr 5, 2018

Interesting concept, but I could not fulfill following security requirements:

  1. Only LIRC daemon should have access to LIRC devices. - I defined udev rules setting OWNER to lirc for my LIRC devices.
  2. Access to LIRC socket should be limited to a special group.
    Did I missed something?

@peterhoeg
Copy link
Member

Who would be a member of the group that can write to the socket? Currently logged in user or something else?

@ck3d
Copy link
Contributor Author

ck3d commented Apr 15, 2018

Yes, a logged in user could be a use-case.
In my case, programs like kodi and vdr which are started as systemd services.

@brainrake
Copy link
Contributor

Can we revive this? Conflicts are trivial to resolve. We can merge without DynamicUser as far as I can tell.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 14, 2018

I would appreciate. PR has been updated and is ready for merge.

@brainrake
Copy link
Contributor

@peterhoeg What do you think, can we merge as is?

@peterhoeg peterhoeg merged commit f456d7f into NixOS:master Sep 17, 2018
@lopsided98
Copy link
Contributor

Thanks for this - I was just about to write my own version of it when I saw this PR. Someone should update https://nixos.wiki/wiki/LIRC

Did you test this thoroughly? For me the RuntimeDirectory option seems to cause the socket to be deleted when the service starts. It allows one connection but then stops working because the socket no longer exists. Changing the RuntimeDirectory fixes it. We can probably just drop that option altogether.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 18, 2018 via email

@lopsided98
Copy link
Contributor

RuntimeDirectoryPreserve does not fix the problem, because /run/lirc is owned by root when it is created by the lircd.socket unit, and the ownership does not get changed to lirc:lirc when lircd.service starts, so lircd is unable to write the PID file there. If I put the PID file somewhere else, it works correctly (but then there is no reason to use RuntimeDirectory in the first place). It seems like systemd is not designed to store the socket in the RuntimeDirectory.

Also, the original problem with RuntimeDirectory was not that the directory is deleted when lircd.service stops, but that it appears to be deleted and recreated before the service starts. When a connection is received on the socket, the lircd.service is started, /run/lirc is recreated and systemd passes the file descriptor of the socket to lircd. This allows it to respond to one connection, but the next fails because the socket was deleted.

Some alternatives I can think of are to store the socket somewhere else, prevent lircd from creating a PID file (I don't think there is an option for this), or manually set the permissions on the directory in an ExecStartPre script.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 18, 2018 via email

@lopsided98
Copy link
Contributor

lopsided98 commented Sep 18, 2018 •
edited

This reproduces the problem on my system:

# systemctl stop lircd.socket lircd.service
# rm -r /run/lirc
# systemctl start lircd.socket
# ls -la /run/lirc
total 0
drwxr-xr-x  2 root root  60 Sep 18 18:47 .
drwxr-xr-x 20 root root 540 Sep 18 18:47 ..
srw-rw----  1 lirc lirc   0 Sep 18 18:47 lircd
# irsend send_once ...
Error running command: Protocol error
# ls -la /run/lirc
total 0
drwxr-xr-x  2 root root  60 Sep 18 18:47 .
drwxr-xr-x 20 root root 540 Sep 18 18:47 ..
srw-rw----  1 lirc lirc   0 Sep 18 18:47 lircd
# irsend send_once ...
do_connect: could not connect to socket
connect: Connection refused
Cannot open socket /var/run/lirc/lircd: Connection refused

If I manually start lircd.service (without using socket activation), it creates /run/lirc with the right permissions.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 19, 2018 via email

@lopsided98
Copy link
Contributor

This is the log after the first connection. Systemd tries to restart the service after it fails so this is printed multiple times. The second connection fails with Connection refused because systemd refuses to start the service after it has failed too many times.

Sep 19 16:13:58 Roomba systemd[1]: Started LIRC daemon service.
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Info: lircd:  Opening log, level: Info
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Version: lircd 0.10.1
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: System info: Linux Roomba 4.14.50 #1-NixOS SMP PREEMPT Thu Jan 1 00:00:01 UTC 1970 aarch64 GNU/Linux
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Info: Initial device: /dev/lirc0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Info: Initial device: /dev/lirc0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: driver: default
Sep 19 16:13:58 Roomba lircd[16525]: lircd-0.10.1[16525]: Info: lircd:  Opening log, level: Info
Sep 19 16:13:58 Roomba lircd[16525]: can't open or create /var/run/lirc/lircd.pid: Permission denied
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: output: /var/run/lirc/lircd
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: nodaemon: 1
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: plugindir: /nix/store/4yl5yqmvnbm8yxz9mxna31z6mf3ssx8f-lirc-0.10.1/lib/lirc/plugins
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: logfile: syslog
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: immediate-init: 0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: permission: 666
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: driver-options:
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: pidfile: /var/run/lirc/lircd.pid
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: listen: 0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: connect: (null)
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: userelease: 0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: effective_user: (null)
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: release_suffix: _EVUP
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: allow_simulate: 0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: repeat_max: 600
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: configfile: /nix/store/x8gl0zajkf1q8y569qfiirz7g1hfhnbd-lircd.conf
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Options: dynamic_codes: (null)
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Current driver: default
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Driver API version: 3
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Driver  version: 0.10.0
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Notice: Driver  info: See file:///nix/store/4yl5yqmvnbm8yxz9mxna31z6mf3ssx8f-lirc-0.10.1/share/doc/lirc/plugindocs/default.html
Sep 19 16:13:58 Roomba lircd-0.10.1[16525]: Info: lircd:  Opening log, level: Info
Sep 19 16:13:58 Roomba systemd[1]: lircd.service: Main process exited, code=exited, status=1/FAILURE
Sep 19 16:13:58 Roomba systemd[1]: lircd.service: Failed with result 'exit-code'.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 19, 2018 via email

@peterhoeg
Copy link
Member

And I suspect that RuntimeDirectory and RuntimeDirectoryPreserve is both set.

That's not the problem:

  • RuntimeDirectory creates the directory
  • RuntimeDirectoryPreserve ensures that it doesn't get removed when the service is stopped

Are we sure that lircd actually supports socket activation? If not, the socket gets created by the socket unit and lircd then dies because it tries to create a socket that already exists.

I don't have hardware to test this out on so I'm inclined to revert this until it's tested properly by someone with the correct hardwrae.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 21, 2018 •
edited

This PR implements the LIRC recommended way. The lirc sources itself provides service and socket files and the histroy tells us, that systemd was integrated with 0.9.1 (2014).
I configured the system as described in http://www.lirc.org/html/configuration-guide.html . It works fine, except restarting/stopping the service. On my side RuntimeDirectoryPreserve would fix that issue.
Bens issue seems to be an other.

@lopsided98
Copy link
Contributor

It works for me as long as I configure the PID file to be stored somewhere outside of /run/lirc I wonder if I am running into a systemd bug, possibly one that only affects aarch64. I don't have a lot of time to look into my issue right now, so I think we should just add RuntimeDirectoryPreserve, which will hopefully fix it for most people. If I get a chance, I will debug exactly what is going on with my system.

@ck3d
Copy link
Contributor Author

ck3d commented Sep 21, 2018

OK, I opened PR #47154 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterhoeg peterhoeg peterhoeg left review comments

@orivej orivej orivej requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ck3d @peterhoeg @brainrake @lopsided98 @orivej @GrahamcOfBorg