Re: Proposal for Signal Detection Refactoring

On 2018-09-24 11:45:10 +0200, Chris Travers wrote:
> I did some more reading.
>
> On Mon, Sep 24, 2018 at 10:15 AM Chris Travers <chris.travers@adjust.com>
> wrote:
>
> > First, thanks for taking the time to write this.  Its very helpful.
> > Additional thoughts inline.
> >
> > On Mon, Sep 24, 2018 at 2:12 AM Michael Paquier <michael@paquier.xyz>
> > wrote:
> >
> >>
> >> There could be value in refactoring things so as all the *Pending flags
> >> of miscadmin.h get stored into one single volatile sig_atomic_t which
> >> uses bit-wise markers, as that's at least 4 bytes because that's stored
> >> as an int for most platforms and can be performed as an atomic operation
> >> safely across signals (If my memory is right;) ).  And this leaves a lot
> >> of room for future flags.
> >>
> >
> > Yeah I will look into this.
> >
>
>
> Ok so having looked into this a bit more....
>
> It looks like to be strictly conforming, you can't just use a series of
> flags because neither C 89 or 99 guarantee that sig_atomic_t is read/write
> round-trip safe in signal handlers.  In other words, if you read, are
> pre-empted by another signal, and then write, you may clobber what the
> other signal handler did to the variable.  So you need atomics, which are
> C11....
>
> What I would suggest instead at least for an initial approach is:
>
> 1.  A struct of volatile bools statically stored
> 2.  macros for accessing/setting/clearing flags
> 3.  Consistent use of these macros throughout the codebase.
>
> To make your solution work it looks like we'd need C11 atomics which would
> be nice and maybe at some point we decide to allow newer feature, or we
> could wrap this itself in checks for C11 features and provide atomic flags
> in the future.  It seems that the above solution would strictly comply to
> C89 and pose no concurrency issues.

It's certainly *NOT* ok to use atomics in signal handlers
indiscriminately, on some hardware / configure option combinations
they're backed by spinlocks (or even semaphores) and thus *NOT*
interrupt save.

This doesn't seem to solve an actual problem, why are we discussing
changing this? What'd be measurably improved, worth the cost of making
backpatching more painful?