Allow reloading config without restarting the daemon #52

ghost · 2019-10-03T17:16:11Z

Closes #13. There are two supported ways to do it:

Run the command kanshictl reload
Send the process a SIGHUP

The varlink protocol is used, so that should make it easy to add more commands to kanshictl.

ghost · 2019-10-13T22:47:50Z

This is broken in CI because varlink is not in the AUR. It can be added as a subproject if we want to fix that in the meantime.

emersion · 2019-10-16T11:09:16Z

Added libvarlink to the AUR.

ghost · 2019-10-26T23:35:29Z

Looks good on CI, ready for review

emersion

Here's a first round of comments. Thanks for your work and sorry for the long delay! I'm excited for this new feature :)

If it's fine by you, I'd prefer to merge the varlink support first and then the signal stuff in another review round.

include/ipc.h

include/kanshi.h

main.c

ctl.c

kanshictl.1.scd

meson.build

zsugabubus · 2020-01-15T16:33:42Z

Change my mind, but is it really necessary to add such a bloat? Config reload (that you almost never need) and profile switching could be (easily) done in other ways too.

The varlink protocol is used, so that should make it easy to add more commands to kanshictl.

What other commands you want to add (don't misunderstand!) to a sh*tty program which only task is to load preset configurations?

ghost · 2020-01-17T17:55:18Z

@emersion Sorry for the delay, no time to work on Wayland stuff recently, but I will be getting to this soon.

@zsugabubus The other use case for IPC is documented in #10. The original implementation used a simple custom IPC, but that was not flexible and was going to be even more code to integrate it well. My personal use case for this is in cyclopsian/wdisplays#4.

zsugabubus · 2020-01-18T00:14:03Z

Why not just kill and restart?

ghost · 2020-01-18T01:01:04Z

Without an IPC mechanism there is no way to find the right process. Doing killall kanshi is not safe.

ghost · 2020-01-18T01:57:46Z

It should also be noted that in general, procfs + Unix signals are not appropriate for IPC. See these articles for some more info on the problem and why either a workaround or a kernel-level solution is needed: https://lwn.net/Articles/773459/ https://lwn.net/Articles/794707/

zsugabubus · 2020-01-18T11:18:52Z

Doing killall kanshi is not safe.

I don't get it. What do you mean by unsafe? Even if you have parallel Wayland session with the same user, you could use:

PID files,
add a simple signal handler,
wrap it inside a (systemd) service.

They would just about 10-15 LOC instead of adding a noname library(*) as dependency plus more hundred lines. For a task that you do only once... okay... twice in your lifetime.

*: Dunno if anybody have tried to build it at all but meson fails with an error.

It should also be noted that in general, procfs + Unix signals are not appropriate for IPC. See these articles for some more info on the problem [...]

Do you think it is really relevant in our case?

ghost · 2020-01-18T16:01:43Z

The original implementation did that and there were issues so I scrapped it. A daemon should never ever manage its own PID file. If the program is killed then the PID file will stick around and could potentially point to the wrong process. We could only use systemd if it was running in user mode, which not everybody does. Also systemd is optional in wlroots/sway so we want to retain that. Even with systemd there is still no reasonable way to use unix signals for IPC and have it work in all cases, read those articles for more information about this.

I encourage you to read more about varlink, it supports a very simple process-to-process IPC. Using pid files and signals is not a realistic alternative for IPC. The only realistic alternative is dbus which I would like to avoid if we can because that would be unnecessary; we don't need another daemon to handle multicast. If you're having a problem with building varlink, please confirm that it is a bug in the package and not a missing meson dependency. Then file an issue with varlink if appropriate.

zsugabubus · 2020-01-18T18:22:25Z

I would like to stay constructive.

Of course, everything is true what you write. I just saw a simple task, but the solution is an external library (installing, updating, what if abandoned), JSON and more thousands(!) LOC.

When you want to change your background you restart swaybg; when you want to update your sway config you restart it. For me it would be totally okay if kanshi would work the same (although it's not only for sway).

In addition, I have never met a program that had a special ability for rereading the configuration. (Maybe not without reason?)

If the program is killed then the PID file will stick around and could potentially point to the wrong process

Of course. Add a lockfile. ~~With this, it's way more race-condition free than varlink is now.~~

I encourage you to read more about varlink, it supports a very simple process-to-process IPC.

It's surely a nice thing for a process that indent to handle more than one, parameterless command. (Switching doesn't need to be an IPC command. See my PR how I imagined it could work.)

The only realistic alternative is d[...]

Don't continue. 😄

Even with systemd there is still no reasonable way to use unix signals for IPC and have it work in all cases

In my service-like (systemd...) version you don't need signals anymore. You just restart the right instance of the service to reload the config.

It would be good to hear what @emersion thinks about it. But I guess he is okay with it, otherwise he wouldn't add it to AUR...

I think I stop trying convincing you. 😭

ghost · 2020-01-18T19:39:52Z

Sway is a bad example. It also includes JSON and many thousand of lines of code to implement the i3 IPC protocol over a socket. When you type swaymsg reload that does not kill the program, it sends an IPC message to re-read the config. Your service manager can track PIDs of its child processes itself which is why it's safe to use it to send signals, although this is not totally race-free unless it uses pidfd or something similar.

A lock file is adequate while the process is running, but still has race conditions in this use case. For example, if the program is killed after the other program tries to read the lock file. File locking is also unfortunately not an adequate primitive for IPC.

ghost · 2020-01-18T22:13:32Z

BTW, systemd is now using varlink for the homed API: https://systemd.io/USER_GROUP_API/

emersion · 2020-01-21T14:04:30Z

I agree with @cyclopsian. Adding a simple IPC mechanism doesn't add a lot of complexity.

ghost · 2020-02-05T03:16:39Z

This is updated, can squash/reorganize commits when everything is ready.

emersion · 2020-02-18T14:52:32Z

README.md

@@ -15,6 +15,7 @@ Dependencies:

 * wayland-client
 * scdoc (optional, for man pages)
+* libvarlink


Would be nice to explain that it's optional.

emersion · 2020-02-18T14:54:10Z

main.c

+int kanshi_main_loop(struct kanshi_state *state);
+
+#ifdef KANSHI_HAS_VARLINK
+int kanshi_init_ipc(struct kanshi_state *state);


Can we put these decls in a header file ipc.h?

emersion · 2020-02-18T14:57:43Z

include/ipc.h

+#include <stdio.h>
+#include <stdlib.h>
+
+static int get_ipc_address(char *address, size_t size) {


We don't put code in headers

emersion · 2020-02-18T15:05:18Z

event-loop.c

+
+static void signal_handler(int signum) {
+	if (write(signal_pipefds[1], &signum, sizeof(signum)) == -1) {
+		_exit(signum | 0x80);


Maybe print an error message to stderr too?

emersion · 2020-02-18T15:08:55Z

ipc.c

+	struct kanshi_state *state = userdata;
+	kanshi_reload_config(state);
+	// ensure that changes are applied on the server before sending the response
+	struct wl_callback *callback = wl_display_sync(state->display);


Ah, I see. This only ensures that the server has received the configuration request, the server is free to wait an arbitrary amount of time before applying the configuration.

We can add a TODO about using the wlr-output-management event instead.

emersion · 2020-02-18T15:09:30Z

ipc.c

+	char address[PATH_MAX];
+	get_ipc_address(address, sizeof(address));
+	if (varlink_service_new(&service,
+			"emersion", "kanshi", "1.1", "https://wayland.emersion.fr/kanshi/",


Can we get this version number from a -D flag set by Meson?

emersion

On the whole this looks pretty good to me! After these comments are addressed and the commits are re-organized I'll merge this.

Thanks for your work!

somini · 2020-03-25T18:29:58Z

What's the status of this?

This blocks https://github.com/cyclopsian/wdisplays/issues/4, from what I can tell.

ghost · 2020-03-25T19:57:00Z

Planning to finish this soon, due to current events I've been away from the main multi-monitor setup I have but I think I can find another way to test it.

ghost · 2020-05-17T17:26:43Z

Now that kanshi is in debian unstable, I'm trying to get libvarlink into debian before I finish this up https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=960434

maricn · 2021-03-10T10:53:28Z

Just trying to get a sense if I should look into checking this branch out locally or if there's any chance @cyclopsian would be picking up the PR any time soon? And thanks for the work so far!

maricn · 2021-04-13T14:59:22Z

Just trying to get a sense if I should look into checking this branch out locally or if there's any chance @cyclopsian would be picking up the PR any time soon? And thanks for the work so far!

OK, so this user has left GitHub, and is now replaced with the ghost account. 👻

I'm able to checkout this branch at varlink, but I don't code in C and it will take me another year to push this through. As the OP has left the GitHub, this is basically an open call for anyone to jump in, merge this with the mainline branch and address the rather minor PR comments, so - any takers?

emersion · 2021-04-13T15:02:05Z

Yes, anyone's welcome to jump it and adopt this work, keeping the attribution if applicable. I'd prefer to have two separate pull requests though (one for signal handling, another one for IPC).

ericonr · 2021-06-30T20:21:26Z

Assuming no one's started work on this yet, I will try my hand on finishing up the PR.

emersion · 2021-07-24T06:35:26Z

Superseded by #107

ammen99 mentioned this pull request Oct 15, 2019

Add different idle options based on profile #54

Closed

emersion suggested changes Nov 29, 2019

View reviewed changes

Jason Francis added 4 commits February 6, 2020 22:59

Add config reloading with varlink

675da58

Add kanshictl utility

6d7bc49

Add signal handlers

763724e

Reorganize and fix style

86a263f

emersion reviewed Feb 18, 2020

View reviewed changes

emersion mentioned this pull request Oct 14, 2020

Apply or select a given profile #88

Open

ericonr mentioned this pull request Jul 1, 2021

Implement reload config command #107

Merged

emersion closed this Jul 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow reloading config without restarting the daemon #52

Allow reloading config without restarting the daemon #52

ghost commented Oct 3, 2019

ghost commented Oct 13, 2019

emersion commented Oct 16, 2019

ghost commented Oct 26, 2019

emersion left a comment

zsugabubus commented Jan 15, 2020

ghost commented Jan 17, 2020

zsugabubus commented Jan 18, 2020

ghost commented Jan 18, 2020 •

edited by ghost

Loading

ghost commented Jan 18, 2020 •

edited by ghost

Loading

zsugabubus commented Jan 18, 2020 •

edited

Loading

ghost commented Jan 18, 2020 •

edited by ghost

Loading

zsugabubus commented Jan 18, 2020 •

edited

Loading

ghost commented Jan 18, 2020

ghost commented Jan 18, 2020

emersion commented Jan 21, 2020

ghost commented Feb 5, 2020

emersion Feb 18, 2020

emersion Feb 18, 2020

emersion Feb 18, 2020

emersion Feb 18, 2020

emersion Feb 18, 2020

emersion Feb 18, 2020

emersion left a comment

somini commented Mar 25, 2020

ghost commented Mar 25, 2020

ghost commented May 17, 2020

maricn commented Mar 10, 2021

maricn commented Apr 13, 2021

emersion commented Apr 13, 2021

ericonr commented Jun 30, 2021

emersion commented Jul 24, 2021

Allow reloading config without restarting the daemon #52

Allow reloading config without restarting the daemon #52

Conversation

ghost commented Oct 3, 2019

ghost commented Oct 13, 2019

emersion commented Oct 16, 2019

ghost commented Oct 26, 2019

emersion left a comment

Choose a reason for hiding this comment

zsugabubus commented Jan 15, 2020

ghost commented Jan 17, 2020

zsugabubus commented Jan 18, 2020

ghost commented Jan 18, 2020 • edited by ghost Loading

ghost commented Jan 18, 2020 • edited by ghost Loading

zsugabubus commented Jan 18, 2020 • edited Loading

ghost commented Jan 18, 2020 • edited by ghost Loading

zsugabubus commented Jan 18, 2020 • edited Loading

I would like to stay constructive.

ghost commented Jan 18, 2020

ghost commented Jan 18, 2020

emersion commented Jan 21, 2020

ghost commented Feb 5, 2020

emersion Feb 18, 2020

Choose a reason for hiding this comment

emersion Feb 18, 2020

Choose a reason for hiding this comment

emersion Feb 18, 2020

Choose a reason for hiding this comment

emersion Feb 18, 2020

Choose a reason for hiding this comment

emersion Feb 18, 2020

Choose a reason for hiding this comment

emersion Feb 18, 2020

Choose a reason for hiding this comment

emersion left a comment

Choose a reason for hiding this comment

somini commented Mar 25, 2020

ghost commented Mar 25, 2020

ghost commented May 17, 2020

maricn commented Mar 10, 2021

maricn commented Apr 13, 2021

emersion commented Apr 13, 2021

ericonr commented Jun 30, 2021

emersion commented Jul 24, 2021

ghost commented Jan 18, 2020 •

edited by ghost

Loading

ghost commented Jan 18, 2020 •

edited by ghost

Loading

zsugabubus commented Jan 18, 2020 •

edited

Loading

ghost commented Jan 18, 2020 •

edited by ghost

Loading

zsugabubus commented Jan 18, 2020 •

edited

Loading