Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-28 Thread NRK
On Sat, Jan 28, 2023 at 12:53:48PM +0100, Hiltjo Posthuma wrote: > waitpid() and sigaction() can also fail with EINTR, which may mean our zombie > handling fails, so block signals while setting things up to be careful. IMO this line should be removed from the commit message because: * It's not

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-28 Thread Hiltjo Posthuma
On Sat, Jan 28, 2023 at 03:04:50PM +0600, NRK wrote: > Hi Hiltjo, > > On Sat, Jan 28, 2023 at 12:11:44AM +0100, Hiltjo Posthuma wrote: > > We do not need to waitpid() on child processes. It is handled already > > by using sigaction(). > > Here's a test case. I see `cat` turning into a zombie on

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-28 Thread NRK
Hi Hiltjo, On Sat, Jan 28, 2023 at 12:11:44AM +0100, Hiltjo Posthuma wrote: > We do not need to waitpid() on child processes. It is handled already > by using sigaction(). Here's a test case. I see `cat` turning into a zombie on my system without the `waitpid`: [/tmp]~> cat test.c

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-27 Thread Hiltjo Posthuma
On Thu, Jan 26, 2023 at 07:44:08PM +0600, NRK wrote: > On Wed, Jan 25, 2023 at 09:17:53PM +0100, Hiltjo Posthuma wrote: > > Using the new patch it does not handle zombie/defunct processes anymore on > > this > > machine. To reproduce it, in .xinitrc, on a dusty machine: > > > > sleep 10 & >

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-26 Thread NRK
On Wed, Jan 25, 2023 at 09:17:53PM +0100, Hiltjo Posthuma wrote: > Using the new patch it does not handle zombie/defunct processes anymore on > this > machine. To reproduce it, in .xinitrc, on a dusty machine: > > sleep 10 & > exec dwm For ease of testing I wrote this dummy program.

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-25 Thread Hiltjo Posthuma
On Wed, Jan 25, 2023 at 04:24:36PM +0600, NRK wrote: > On Wed, Jan 25, 2023 at 09:37:58AM +0100, Hiltjo Posthuma wrote: > > I don't think there can be any zombies since it is run in setup() before any > > processes can spawn (via keybindings etc) or am I overlooking something > > obvious? > > It

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-25 Thread Hiltjo Posthuma
On Wed, Jan 25, 2023 at 08:39:51AM +, Chris Down wrote: > Hiltjo Posthuma writes: > > I don't think there can be any zombies since it is run in setup() before any > > processes can spawn (via keybindings etc) or am I overlooking something > > obvious? > > You can have zombies from what's

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-25 Thread Chris Down
Hiltjo Posthuma writes: I don't think there can be any zombies since it is run in setup() before any processes can spawn (via keybindings etc) or am I overlooking something obvious? You can have zombies from what's spawned in .xinitrc or similar, because usually the final step is `exec dwm'.

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-25 Thread NRK
On Wed, Jan 25, 2023 at 09:37:58AM +0100, Hiltjo Posthuma wrote: > I don't think there can be any zombies since it is run in setup() before any > processes can spawn (via keybindings etc) or am I overlooking something > obvious? It was recently discussed here:

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-25 Thread Hiltjo Posthuma
On Wed, Jan 25, 2023 at 12:06:25PM +0600, NRK wrote: > On Tue, Jan 24, 2023 at 09:01:08PM +0100, Hiltjo Posthuma wrote: > > Although of course checking errno on a success condition is a bit wonky in > > this test case. > > It was just an illustration that the malloc succeeded :) > A more

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-24 Thread NRK
On Tue, Jan 24, 2023 at 09:01:08PM +0100, Hiltjo Posthuma wrote: > Although of course checking errno on a success condition is a bit wonky in > this test case. It was just an illustration that the malloc succeeded :) A more real-world check would be something like this (in fact, I'm quite sure

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-24 Thread Chris Down
Hiltjo Posthuma writes: - sigchld(0); + if (signal(SIGCHLD, SIG_IGN) == SIG_ERR) + die("can't ignore SIGCHLD:"); I didn't test it yet, so maybe I'm misremembering/misreading the patch, but I don't think this will work: SIG_IGN will ignore new children, but it won't

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-24 Thread Hiltjo Posthuma
On Tue, Jan 24, 2023 at 10:25:11AM +0600, NRK wrote: > On Mon, Jan 23, 2023 at 08:05:19PM +0100, Hiltjo Posthuma wrote: > > Do you perhaps also have some simple way to reproduce where it causes > > issues in > > some real world use-case? > > > > Ideally some command or script to reproduce the

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-23 Thread NRK
On Mon, Jan 23, 2023 at 08:05:19PM +0100, Hiltjo Posthuma wrote: > Do you perhaps also have some simple way to reproduce where it causes issues > in > some real world use-case? > > Ideally some command or script to reproduce the issue. To trigger the issue, you need to have 3 conditions met:

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-23 Thread Hiltjo Posthuma
On Mon, Jan 23, 2023 at 07:34:04AM +, Chris Down wrote: > Thanks NRK for bringing this one up again :-) > > NRK writes: > > If there's any specific issue with Chris' patch which is blocking the > > merge then I'd be happy to address them. > > Likewise -- happy to act on any feedback. > >

[hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-22 Thread Chris Down
Thanks NRK for bringing this one up again :-) NRK writes: If there's any specific issue with Chris' patch which is blocking the merge then I'd be happy to address them. Likewise -- happy to act on any feedback. I've been running this for the last half a year or so and haven't noticed any

[hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2023-01-21 Thread NRK
On Wed, Jul 27, 2022 at 07:36:28AM +0100, Chris Down wrote: > -void > -sigchld(int unused) > -{ > - if (signal(SIGCHLD, sigchld) == SIG_ERR) > - die("can't install SIGCHLD handler:"); > - while (0 < waitpid(-1, NULL, WNOHANG)); > -} One other problem with this type of

[hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2022-09-20 Thread Chris Down
Hey Hiltjo, Chris Down writes: signal() semantics are pretty unclearly specified. For example, depending on OS kernel and libc, the handler may be returned to SIG_DFL (hence the inner call to readd the signal handler). Moving to sigaction() means the behaviour is consistently defined. Using

Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2022-08-09 Thread Chris Down
NRK writes: ping! In case this got lost in the noise :) Thanks for the poke :-) From my side, I've been running with this patch for a couple of weeks and everything works fine, so I think it's good to go.

[hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2022-07-27 Thread Chris Down
Chris Down writes: sigprocmask() only returns an error if you pass an invalid first argument, but we're using SIG_SETMASK as part of the userspace ABI contract, so that doesn't seem very relevant. (One could make the same argument about sigaction(), of course -- that's mostly just an EINTR

[hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

2022-07-27 Thread Chris Down
NRK writes: If I understand the manpage correctly, then this patch looks mostly okay. Only issue I can think of is that both `sigfillset` and `sigprocmask` manpage states that they may return -1 on errors. So perhaps they should be error checked just in case: if (sigfillset() < 0 ||