Update systemd service to restart at new input device

[yarcod]

An initial attempt at restarting the systemd service whenever a new device is added to /dev/input. In addition, I relocated the binary to $HOME/.local/bin in order to avoid needing sudo access.

[yarcod]

I'm new to Github PRs, but I seem unable to push additional changes to my opened PR. Either way, I have an additional commit coming with updated documentation (README.md) to reflect these changes, should they be accepted.

[Adda0]

Hey, thank you for the PR. I will get to it, hopefully rather sooner than later.

You should be able to push with just git push when checked out to your branch (assuming you have set upstream branch for your local branch). Alternatively, git push <your remote name> <your remote branch name>.

If that does not help, what is the error you get?

[yarcod]

I was using VSCode to authenticate against Github, which worked with the https clone I had done since it can handle OAuth and 2FA. However, it did not allow me to push again but instead wanted me to create another Fork. I changed the origin remote to use ssh instead and could then successfully push from command line.

TLDR; additional fixes pushed :)

[Adda0]

I am glad you have figured it out. And the introduced changes and the whole PR look interesting. I will make sure to check it out as soon as I get some spare time. Thank you for your contribution.

[Adda0]

Well, I did some testing with partial success. It works for me whenever an already plugged-in device with touchcursor running with it is disconnected. Then, the service is started anew. However, I think we need to restart the service here, not just start. Then, it would work even if the previously used device is still connected but a new device higher in the config file is connected.

Currently, the following happens for me. I use my external keyboard, touchcursor is running. I disconnect my external keyboard, touchcursor stops (exits), service is started anew (it was stopped already, so it can start now) and my notebook keyboard is found by touchcursor. It works for the notebook keyboard now. Then I connect the external keyboard again. However, the notebook keyboard is still present and touchcursor is still running. Therefore, the touchcursor.path tries to start the service (but it is still running, so nothing happens). Now, my external keyboard is left alone and touchcursor runs still on the notebook keyboard.

I believe a forced restart is necessary, as explained in this topic, for example.

Furthermore, I have problems with permissions when the binary is in ~/.local/bin. This will need resolving, too.

What are your thoughts about this?

[yarcod]

Thanks for giving it a spin! :)

From what you describe it certainly sounds like a restart will be necessary, as opposed to just trying to start the service anew. I also think that further adapting the systemd service is a good approach, unless you see some other benefit in adjusting the application itself with, e.g., a --update flag of some sorts?

Lastly, what kind of permission issues are you getting in ~/.local/bin? As long as you are starting the service as the same user which home path you utilise you should be the owner of that directory. Or do you mean that there would need to be added execution permissions to the binary when placed in ~/.local/bin?

[Adda0]

I also think that further adapting the systemd service is a good approach, unless you see some other benefit in adjusting the application itself with, e.g., a --update flag of some sorts?

I would rather modify the service. I do not think that a flag to update the application would be beneficial in this case. If we are creating another service to automatically restart the current service, it should do so. This should not be a responsibility of the application itself, in my opinion. Therefore, I would like to see this handled on systemd service-level entirely.

Lastly, what kind of permission issues are you getting in ~/.local/bin? As long as you are starting the service as the same user which home path you utilise you should be the owner of that directory. Or do you mean that there would need to be added execution permissions to the binary when placed in ~/.local/bin?

From my limited testing, I seem to be having insufficient permissions to bind to some device in /dev/input/ when the binary is in ~/.local/bin. I need to explore this problem further as I am not entirely sure if I understood the underlying issue correctly, but this was my first impression.

Never mind the issues, I am beginning to become a fan of this idea and the options it provides. I now consider automatic restarts of the service to be a necessary feature, at least for my C++ port of touchcursor-linux. But, if it all works well, I think this should definitely make it to main here as well.

[Adda0]

@donniebreve Does the service run normally (binds to the required keyboard) for you when the application is in the ~/.local/bin? If so, then the problem with permissions is just on my end (as it logically should, I think) and it does not concern the PR.

[donniebreve]

I switched to @yarcod's branch and couldn't install:

# Stopping the service
systemctl --user stop touchcursor.path
Failed to stop touchcursor.path: Unit touchcursor.path not loaded.
make: [Makefile:49: install] Error 5 (ignored)

# Copying application to //home/.../.local/bin
cp ./out/touchcursor //home/.../.local/bin
chmod u+s //home/.../.local/bin/touchcursor
chmod: cannot access '//home/.../.local/bin/touchcursor': Not a directory
make: *** [Makefile:54: install] Error 1

After fixing some things in the make file:

diff --git a/Makefile b/Makefile
index 5b89d24..e453993 100644
--- a/Makefile
+++ b/Makefile
@@ -5,7 +5,7 @@ TARGET = touchcursor
 # LIBS = -lm
 CC = gcc
 CFLAGS = -g -Wall
-INSTALLPATH?=/$(HOME)/.local/bin
+INSTALLPATH ?= $(HOME)/.local/bin
 # Configuration variables
 SERVICEPATH = $(HOME)/.config/systemd/user
 SERVICEFILE = touchcursor.service
@@ -50,6 +50,7 @@ install:
        @echo ""

        @echo "# Copying application to $(INSTALLPATH)"
+       mkdir -p $(INSTALLPATH)
        cp $(OUTPATH)/$(TARGET) $(INSTALLPATH)
        chmod u+s $(INSTALLPATH)/$(TARGET)
        @echo ""

and running the application seems to sit at

systemd[1080]: Started Touchcursor Path Monitor.

so I don't think the makefile starts the main service.

[yarcod]

I've not had much time to take this further lately. However, I did manage to get a watcher working in the end, and I tried to clean up my mess a little in the Makefile. Also, after having done a clean uninstall and install, I also realised that there would permission issues with the ~/.local/bin location -- since the application wants to watch /dev/input it requires escalated privileges anyway -- so I moved it back to /usr/bin.

Please do test this thoroughly though, because I ran into some issues when starting the application this way. I'm very uncertain what caused it, but I never experienced something similar before. Essentially the whole button capturing and "translation" was making keys getting stuck, some keys ignored and a whole lot of other messes. It was like that sometimes and sometimes it worked as intended. I am not sure if that is related to some other troubles I've had with the current installation. When the error appeared it mentioned something with a buffer being too small -- I couldn't copy or take a PS of it since it captured all keys on the keyboard 😅

Edit: Also, I lack the Github know-how to be able to handle the review you added @donniebreve, as well as the conversations. I tried to address them as best I could, but I can't seem to find an obvious way to mark them as handled.

Edit2: In the name of science I re-ran the service and seem able to reproduce the error every time now. It throws me an add_uevent_var: buffer size is too small error, which might be related to this kernel bug. Since the service worked before the watcher there is probably a work-around

[Adda0]

Hey, no worries. This resolves my permission issues, then. I will test the added watcher in the following days, too.

The bug you mention is weird, indeed. I will keep an eye on it. Right now, I have no idea what might be causing this behaviour. We will have to figure something out.

You handled the conversations (resolved them) well. There is nothing more you can do than mark them as resolved exactly as you have done. And the review is just an output of diff of what needs to be done. You addressed both appropriately.

Thank you for your work. Have a nice day.

[yarcod]

I made the solution look better, and probably be the more systemd way of doing things. The previous commit would also eventually break completely for me, since the restarter service, for some reason, could not stop restarting the touchcursor.service once started.

I've gone back to a simpler path watcher, which in turn will restart the service whenever a new device is detected. I also added an additional path watch for changes of the configuration, so when you update the config file the service will automatically read and use it. I also experimented with having a make target add to configure one device, in order to simplify the initial setup of the application. Would that be of interest?

I'm still seeing the previously mentioned error from time to time, but I can't tell when or why it happens.

[donniebreve]

This looks great @yarcod. I'll pull down the latest changes and try them out soon.

Edit: Also, I lack the Github know-how to be able to handle the review you added @donniebreve, as well as the conversations. I tried to address them as best I could, but I can't seem to find an obvious way to mark them as handled.

The diff I gave is just a concise way to present what I changed. You handled it appropriately by making the changes on your branch. I would advise you to usually allow the person who started a comment/conversation to resolve it, but I think it was fine here.

Edit2: In the name of science I re-ran the service and seem able to reproduce the error every time now. It throws me an add_uevent_var: buffer size is too small error, which might be related to this kernel bug. Since the service worked before the watcher there is probably a work-around

Definitely something we need to look into here (maybe it's safe to ignore?). I don't think I've seen this before. It could have been shipped in the kernel recently. It also seems to be related to certain devices.

I also experimented with having a make target add to configure one device, in order to simplify the initial setup of the application. Would that be of interest?

Absolutely. Just start a new pull request for those changes.

[donniebreve]

I've finally made some time to sit with this and investigate further. I've found that the current setup (touchcursor.service and touchcursor.path) will only start the touchcursor service, but won't restart the service.

To get restart behavior, we have to create three files: touchcursor.service, touchcursor-restarter.service, touchcursor-restarter.path. Unfortunately, after spending some time with this, I've discovered something in systemd that will continuously trigger the restarter.path and restarter.service ad infinitum.

When a service unit triggered by a path unit terminates (regardless whether it exited successfully or failed), monitored paths are checked immediately again, and the service accordingly restarted instantly. https://github.com/systemd/systemd/issues/16669#issuecomment-670246495

I believe this means the file in PathChanged is immediately checked again, but for some reason systemd still considers the file changed and starts the service again, even when the file hasn't been modified.

If unit limiters are introduced (using StartLimit or TriggerLimit) the service goes into a failed state when the limit is hit and then has to be restarted manually, defeating the purpose of this whole endeavor.

I don't see a way out of this issue currently.

I'll keep experimenting but I'm starting to think systemd can't do what we want, and maybe I'll have to start investigating monitoring in the binary.

Here's a preview of the changes I've made so far:

diff --git a/Makefile b/Makefile
index 80d5772..ad9e152 100644
--- a/Makefile
+++ b/Makefile
@@ -2,18 +2,19 @@ SRCPATH = ./src
 OBJPATH = ./obj
 OUTPATH = ./out
 TARGET = touchcursor
+
 # LIBS = -lm
 CC = gcc
 CFLAGS = -g -Wall
-# INSTALLPATH ?= $(HOME)/.local/bin
-INSTALLPATH ?= /usr/bin
-# OLDINSTALLPATH ?= /usr/bin/touchcursor
+
 # Configuration variables
+INSTALLPATH ?= /usr/bin
 SERVICE-PATH = $(HOME)/.config/systemd/user
-SERVICE-FILE = touchcursor.service
-SERVICE-PATH-WATCHER := touchcursor.path
-CONFIGPATH = $(HOME)/.config/touchcursor
-CONFIGFILE = touchcursor.conf
+SERVICE = touchcursor.service
+SERVICE-RESTARTER := touchcursor-restarter.service
+SERVICE-PATH-MONITOR := touchcursor-restarter.path
+CONFIG-PATH = $(HOME)/.config/touchcursor
+CONFIG = touchcursor.conf

 .PHONY: default all clean

@@ -45,45 +46,56 @@ clean:
    -rm -f $(OUTPATH)/$(TARGET)

 install:
-   @echo "# Stopping the service"
-   -systemctl --user stop $(SERVICE-PATH-WATCHER)
+   @echo "# Stopping the services"
+   -systemctl --user disable --now $(SERVICE-PATH-MONITOR)
+   -systemctl --user disable --now $(SERVICE-RESTARTER)
+   -systemctl --user disable --now $(SERVICE)
+   systemctl --user daemon-reload
    @echo ""
-   
+
    @echo "# Copying application to $(INSTALLPATH)"
    @echo "# This action requires sudo."
    sudo cp $(OUTPATH)/$(TARGET) $(INSTALLPATH)
    sudo chmod u+s $(INSTALLPATH)/$(TARGET)
    @echo ""

-   @echo "# Copying default configuration file to $(CONFIGPATH)/$(CONFIGFILE)"
-   mkdir -p $(CONFIGPATH)
-   cp -n $(CONFIGFILE) $(CONFIGPATH)
-   @echo ""
-   
    @echo "# Copying service files to $(SERVICE-PATH)"
    mkdir -p $(SERVICE-PATH)
-   cp -f $(SERVICE-FILE) $(SERVICE-PATH)
-   cp -f $(SERVICE-PATH-WATCHER) $(SERVICE-PATH)
+   cp -f $(SERVICE-PATH-MONITOR) $(SERVICE-PATH)
+   cp -f $(SERVICE-RESTARTER) $(SERVICE-PATH)
+   cp -f $(SERVICE) $(SERVICE-PATH)
    @echo ""
-   
+
+   @echo "# Copying default configuration file to $(CONFIG-PATH)/$(CONFIG)"
+   mkdir -p $(CONFIG-PATH)
+   -cp -n $(CONFIG) $(CONFIG-PATH)
+   @echo ""
+
    @echo "# Enabling and starting the service"
    systemctl --user daemon-reload
-   systemctl --user enable --now $(SERVICE-PATH-WATCHER)
+   systemctl --user enable --now $(SERVICE)
+   systemctl --user enable $(SERVICE-RESTARTER)
+   systemctl --user enable --now $(SERVICE-PATH-MONITOR)

 uninstall:
-   @echo "# Stopping and disabling the service"
-   -systemctl --user disable --now $(SERVICE-PATH-WATCHER)
-   -systemctl --user daemon-reload
+   @echo "# The configuration file will not be removed:"
+   @echo "# $(CONFIG-PATH)/$(CONFIG)"
+   @echo ""
+   @echo "# To uninstall everything (including the configuration file)"
+   @echo "# run 'make uninstall-full'"
    @echo ""

-   @echo "# Removing configuration file $(CONFIGPATH)/$(CONFIGFILE)"
-   -rm $(CONFIGPATH)/$(CONFIGFILE)
-   -rm -r $(CONFIGPATH)
+   @echo "# Stopping the services"
+   -systemctl --user disable --now $(SERVICE-PATH-MONITOR)
+   -systemctl --user disable --now $(SERVICE-RESTARTER)
+   -systemctl --user disable --now $(SERVICE)
+   systemctl --user daemon-reload
    @echo ""

    @echo "# Removing service files from $(SERVICE-PATH)"
-   -rm $(SERVICE-PATH)/$(SERVICE-FILE)
-   -rm $(SERVICE-PATH)/$(SERVICE-PATH-WATCHER)
+   -rm $(SERVICE-PATH)/$(SERVICE-PATH-MONITOR)
+   -rm $(SERVICE-PATH)/$(SERVICE-RESTARTER)
+   -rm $(SERVICE-PATH)/$(SERVICE)
    -rm -d $(SERVICE-PATH)
    @echo ""

@@ -91,3 +103,31 @@ uninstall:
    @echo "# This action requires sudo."
    -sudo rm $(INSTALLPATH)/$(TARGET)
    @echo ""
+
+uninstall-full:
+   @echo "# Stopping the services"
+   -systemctl --user disable --now $(SERVICE-PATH-MONITOR)
+   -systemctl --user disable --now $(SERVICE-RESTARTER)
+   -systemctl --user disable --now $(SERVICE)
+   systemctl --user daemon-reload
+   @echo ""
+
+   @echo "# Removing service files from $(SERVICE-PATH)"
+   -rm $(SERVICE-PATH)/$(SERVICE-PATH-MONITOR)
+   -rm $(SERVICE-PATH)/$(SERVICE-RESTARTER)
+   -rm $(SERVICE-PATH)/$(SERVICE)
+   -rm -d $(SERVICE-PATH)
+   @echo ""
+
+   @echo "# Removing application from $(INSTALLPATH)"
+   @echo "# This action requires sudo."
+   -sudo rm $(INSTALLPATH)/$(TARGET)
+   @echo ""
+
+   @echo "# Removing configuration file $(CONFIG-PATH)/$(CONFIG)"
+   -rm $(CONFIG-PATH)/$(CONFIG)
+   -rm -d $(CONFIG-PATH)
+   @echo ""

diff --git a/touchcursor-restarter.path b/touchcursor-restarter.path
new file mode 100644
index 0000000..9b0ce49
--- /dev/null
+++ b/touchcursor-restarter.path
@@ -0,0 +1,10 @@
+[Unit]
+Description=touchcursor-linux restarter monitor
+
+[Path]
+PathChanged=/dev/input
+PathModified=%h/.config/touchcursor/touchcursor.conf
+Unit=touchcursor-restarter.service
+
+[Install]
+WantedBy=default.target

diff --git a/touchcursor-restarter.service b/touchcursor-restarter.service
new file mode 100644
index 0000000..40bfc15
--- /dev/null
+++ b/touchcursor-restarter.service
@@ -0,0 +1,11 @@
+[Unit]
+Description=touchcursor-linux restarter
+
+[Service]
+Type=oneshot
+ExecStart=/usr/bin/systemctl --user restart touchcursor.service
+
+[Install]
+WantedBy=default.target

diff --git a/touchcursor.path b/touchcursor.path
deleted file mode 100644
index a4dea0a..0000000
--- a/touchcursor.path
+++ /dev/null
@@ -1,17 +0,0 @@
-[Unit]
-Description=Touchcursor Path Monitor
-StartLimitBurst=0
-PropagatesStopTo=touchcursor.service
-PropagatesReloadTo=touchcursor.service
-# Every time a hit to the given path is triggered, the service is reloaded
-# "[...]monitored paths are checked immediately again, and the service accordingly restarted instantly"
-# https://www.freedesktop.org/software/systemd/man/systemd.path.html#Description
-
-[Path]
-Unit=touchcursor.service
-PathChanged=/dev/input
-PathModified=%h/.config/touchcursor/touchcursor.conf
-
-[Install]
-Also=touchcursor.service
-WantedBy=default.target

--- a/touchcursor.service
+++ b/touchcursor.service
@@ -1,7 +1,11 @@
 [Unit]
-Description=Touch Cursor Service
+Description=touchcursor-linux
-PartOf=touchcursor.path

 [Service]
+Type=exec
 ExecStart=/usr/bin/touchcursor
+
+[Install]
+WantedBy=default.target

[yarcod]

@donniebreve I really thought I had checked that the service was indeed restarted and not just started. Also, based on a comment (that I had accidentally left in the servce.path file), it really sounds like the monitoring/path service is indeed restarted at every change in the monitored path.

Every time a hit to the given path is triggered, the service is reloaded "[...]monitored paths are checked immediately again, and the service accordingly restarted instantly" https://www.freedesktop.org/software/systemd/man/systemd.path.html#Description

Perhaps this could work then if the Unit=touchcursor.service part was taken out from touchcursor.path, as I have interpreted that as the trigger. If the monitoring service can instead rely on the PropagatesReloadTo=touchcursor.service then the restart could perhaps propagate that part only?

I'll see when I can have a look at this myself, but otherwise you're of course welcome to adapt my latest attempt and give it a spin.

[donniebreve]

I put things back to how you had them just to be sure, but I've confirmed that the simple service/path combo does not restart the service.

Also, based on a comment (that I had accidentally left in the servce.path file), it really sounds like the monitoring/path service is indeed restarted at every change in the monitored path.

Every time a hit to the given path is triggered, the service is reloaded "[...]monitored paths are checked immediately again, and the service accordingly restarted instantly" https://www.freedesktop.org/software/systemd/man/systemd.path.html#Description

The full line from the documentation provides the clue:

When a service unit triggered by a path unit terminates (regardless whether it exited successfully or failed), monitored paths are checked immediately again, and the service accordingly restarted instantly.

This means that the path unit is only triggered again if the service unit exits. Unfortunately this also causes an endless loop with a Type=oneshot service unit.

Perhaps this could work then if the Unit=touchcursor.service part was taken out from touchcursor.path, as I have interpreted that as the trigger. If the monitoring service can instead rely on the PropagatesReloadTo=touchcursor.service then the restart could perhaps propagate that part only?

Unit= The unit to activate when any of the configured paths changes. The argument is a unit name, whose suffix is not ".path". If not specified, this value defaults to a service that has the same name as the path unit, except for the suffix. (See above.) It is recommended that the unit name that is activated and the unit name of the path unit are named identical, except for the suffix.

Omitting the Unit=touchcursor.service line shouldn't make a difference, since that would be the default value without the line.

Some useful commands to see what is happening:

systemctl --user status 'touchcursor*' -all

journalctl -f

You can test by opening a terminal with journalctl -f and then edit your configuration file or plug/unplug a keyboard. Once the touchcursor.service is active (running), the path unit does not restart the service.

I think this means we need to implement monitoring in the binary.

[yarcod]

I took another look at it again, and I'll be damned if this isn't possible to solve with systemd! Not that I prefer it any which way, but it just feels like that this is exactly what systemd should be good at!

Anyway, rant aside, I think I managed to hack a way around it. It boils down to issuing a service reload before starting the service, and a reload triggers the "registered" binary to be killed, after which it is started again. It is not pretty, but from what I can tell it will actually restart the application the way we need it to. The one caveat is that systemctl --user reload touchcursor.service will kill the application instead of doing an actual reload of config files. It will remain dead until started either manually or triggered by the .path service for any reason.

See if this solves the issues we previously had, and if the solution is acceptable for that matter :)

[Adda0]

I took another look at it again, and I'll be damned if this isn't possible to solve with systemd! Not that I prefer it any which way, but it just feels like that this is exactly what systemd should be good at!

I completely agree with you. However, as it seems, we are still hitting a wall here.

I tried these changes, but I am still stuck at the same issue. The service does not get restarted when a new keyboard is plugged in while the previous one (the notebook keyboard in my case) remains connected with the service running. If the service is running on the external keyboard which is later unplugged, TouchCursor is correctly restarted and connects to another keyboard (as it was the case originally).

[yarcod]

I tried these changes, but I am still stuck at the same issue. The service does not get restarted when a new keyboard is plugged in while the previous one (the notebook keyboard in my case) remains connected with the service running. If the service is running on the external keyboard which is later unplugged, TouchCursor is correctly restarted and connects to another keyboard (as it was the case originally).

Hmm. I really wish I had a second keyboard to test with. I am not sure that the service is actually killed in this instance, but instead that only the application (/usr/bin/touchcursor) is killed and started again. But I guess you are still seeing the same symptoms as when the service needed to be actually restarted?

[Adda0]

But I guess you are still seeing the same symptoms as when the service needed to be actually restarted?

Yeah. Exactly.