On 2022/12/29 16:11:57 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Thu, Dec 29, 2022 at 12:55:03PM +0100, Omar Polo wrote:
> > > +	switch (sig) {
> > > +	case SIGHUP:
> > > +		break;
> > > +	case SIGUSR1:
> > > +		break;
> > 
> > just for curiosity, what's the point of catching SIGUSR1 too?  the
> > listener process and gotwebd do that too (but gotwebd due to proc.c I
> > guess), but otherwise they don't do anything with it.
> 
> No idea. This was just copied along from other daemons.
> The HUP and USR signals are often used to trigger actions such as
> reloading config files. I suspect that is why they are present in
> all these signal handlers.

I see.  diff below drops it for gotd but I wouldn't mind to keep it
since it's there even in other daemons that don't do anything with it
(e.g. see relayd.)
 
> > > +	if (client->auth->iev.ibuf.fd != fd)
> > > +		fatalx("%s: unexpected fd %d", __func__, fd);
> > 
> > it's a bit redundant to check the fd, since find_client_by_proc_fd
> > should return a client with that fd.  what i'm missing?
> 
> Indeed. This is just a guard against accidents. I am not very happy
> with find_client_by_proc_fd() because it is looping over the entire
> client hash table. But I don't want to store a pointer inside struct
> gotd_imsgev either, because this makes use-after-free bugs more likely
> to happen. I am not sure yet if there is a better way.
> 
> Once we have all separate processes in place, we can do another sweep
> of all the sanity checks and see which ones still make sense. There is
> also the verify_imsg_src() function which is currently just being copied
> along, and could eventually be dropped or refined.

ACK! :)

> > > +	if ((n = imsg_get(ibuf, &imsg)) == -1)
> > > +		fatal("%s: imsg_get error", __func__);
> > > +	if (n == 0)	/* No more messages. */
> > > +		return;
> > 
> > any reason not to do the usual loop here around imsg_get?  it works
> > like this, but if we ever grow the set of messages it could lead to
> > issues, and looping would reduce the diff with the other dispatch
> > functions.
> > 
> > (well, not that the auth process should do more, so it's fine like it
> > is already)
> 
> The reason is that we expect the auth process to send us only one message
> before we will dispose of it. I don't expect this to change, provided we
> will not extend our authentication feature-set in the future.

Sure.


diff /home/op/w/gotd
commit - 5e25db14db9eb20ee11b68048b45b3e0f54d50eb
path + /home/op/w/gotd
blob - 57b17a1dd26385f232f8f989e4fa041babfeafe3
file + gotd/auth.c
--- gotd/auth.c
+++ gotd/auth.c
@@ -59,8 +59,6 @@ auth_sighdlr(int sig, short event, void *arg)
 	switch (sig) {
 	case SIGHUP:
 		break;
-	case SIGUSR1:
-		break;
 	case SIGTERM:
 	case SIGINT:
 		auth_shutdown();
@@ -276,7 +274,7 @@ auth_main(const char *title, struct gotd_repolist *rep
 {
 	struct gotd_repo *repo = NULL;
 	struct gotd_imsgev iev;
-	struct event evsigint, evsigterm, evsighup, evsigusr1;
+	struct event evsigint, evsigterm, evsighup;
 
 	gotd_auth.title = title;
 	gotd_auth.pid = getpid();
@@ -292,13 +290,11 @@ auth_main(const char *title, struct gotd_repolist *rep
 	signal_set(&evsigint, SIGINT, auth_sighdlr, NULL);
 	signal_set(&evsigterm, SIGTERM, auth_sighdlr, NULL);
 	signal_set(&evsighup, SIGHUP, auth_sighdlr, NULL);
-	signal_set(&evsigusr1, SIGUSR1, auth_sighdlr, NULL);
 	signal(SIGPIPE, SIG_IGN);
 
 	signal_add(&evsigint, NULL);
 	signal_add(&evsigterm, NULL);
 	signal_add(&evsighup, NULL);
-	signal_add(&evsigusr1, NULL);
 
 	imsg_init(&iev.ibuf, GOTD_FILENO_MSG_PIPE);
 	iev.handler = auth_dispatch;
blob - 2ae681053c360d76a04ee62d816fbcb536e57c4b
file + gotd/gotd.c
--- gotd/gotd.c
+++ gotd/gotd.c
@@ -1349,9 +1349,6 @@ gotd_sighdlr(int sig, short event, void *arg)
 	case SIGHUP:
 		log_info("%s: ignoring SIGHUP", __func__);
 		break;
-	case SIGUSR1:
-		log_info("%s: ignoring SIGUSR1", __func__);
-		break;
 	case SIGTERM:
 	case SIGINT:
 		gotd_shutdown();
@@ -2396,7 +2393,7 @@ main(int argc, char **argv)
 	struct group *gr = NULL;
 	char *repo_path = NULL;
 	enum gotd_procid proc_id = PROC_GOTD;
-	struct event evsigint, evsigterm, evsighup, evsigusr1;
+	struct event evsigint, evsigterm, evsighup;
 	int *pack_fds = NULL, *temp_fds = NULL;
 
 	log_init(1, LOG_DAEMON); /* Log to stderr until daemonized. */
@@ -2610,13 +2607,11 @@ main(int argc, char **argv)
 	signal_set(&evsigint, SIGINT, gotd_sighdlr, NULL);
 	signal_set(&evsigterm, SIGTERM, gotd_sighdlr, NULL);
 	signal_set(&evsighup, SIGHUP, gotd_sighdlr, NULL);
-	signal_set(&evsigusr1, SIGUSR1, gotd_sighdlr, NULL);
 	signal(SIGPIPE, SIG_IGN);
 
 	signal_add(&evsigint, NULL);
 	signal_add(&evsigterm, NULL);
 	signal_add(&evsighup, NULL);
-	signal_add(&evsigusr1, NULL);
 
 	gotd_imsg_event_add(&gotd.listen_proc.iev);
 
blob - 96427e857a4b693de2dec2665df312e682a43f73
file + gotd/listen.c
--- gotd/listen.c
+++ gotd/listen.c
@@ -77,8 +77,6 @@ listen_sighdlr(int sig, short event, void *arg)
 	switch (sig) {
 	case SIGHUP:
 		break;
-	case SIGUSR1:
-		break;
 	case SIGTERM:
 	case SIGINT:
 		listen_shutdown();
@@ -340,7 +338,7 @@ listen_main(const char *title, int gotd_socket)
 listen_main(const char *title, int gotd_socket)
 {
 	struct gotd_imsgev iev;
-	struct event evsigint, evsigterm, evsighup, evsigusr1;
+	struct event evsigint, evsigterm, evsighup;
 
 	gotd_listen.title = title;
 	gotd_listen.pid = getpid();
@@ -349,13 +347,11 @@ listen_main(const char *title, int gotd_socket)
 	signal_set(&evsigint, SIGINT, listen_sighdlr, NULL);
 	signal_set(&evsigterm, SIGTERM, listen_sighdlr, NULL);
 	signal_set(&evsighup, SIGHUP, listen_sighdlr, NULL);
-	signal_set(&evsigusr1, SIGUSR1, listen_sighdlr, NULL);
 	signal(SIGPIPE, SIG_IGN);
 
 	signal_add(&evsigint, NULL);
 	signal_add(&evsigterm, NULL);
 	signal_add(&evsighup, NULL);
-	signal_add(&evsigusr1, NULL);
 
 	imsg_init(&iev.ibuf, GOTD_FILENO_MSG_PIPE);
 	iev.handler = listen_dispatch;