fix: Signal Handlers on Non-Windows Platforms in sslclient

[Nidhoegger]

Only set signals to Ignore on non-Linux of no signal-handler is in place. Also set the SA_RESTART flag to avoid operations returning EINTR.

Code change checklist

• I have ensured that all methods and functions are fully documented using doxygen style comments.
• My code follows the coding style guide.
• I tested that my change works before raising the PR.
• I have ensured that I did not break any existing API calls.
• I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

[CLAassistant]

All committers have signed the CLA.

[netlify]

✅ Deploy Preview for dpp-dev ready!

Name	Link
🔨 Latest commit	1ea7c12
🔍 Latest deploy log	https://app.netlify.com/sites/dpp-dev/deploys/662d3e8f400af4000898751d
😎 Deploy Preview	https://deploy-preview-1123--dpp-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[Jaskowicz1]

Thanks for your contribution! This PR doesn't seem like it's carried anything from master so it's okay to stay up but next time please make your changes on a branch based from dev, rather than using the master branch!

[Jaskowicz1]

Also, next time, please read the description of the PR and choose the template that accurately reflects your PR. I've added the template in for you this time, please fill it out (and correct your PR title to follow our conventions please!)

[braindigitalis]

changes requested

[Nidhoegger]

Since not seeing this on GitHub directly, I will reply here and hope it works. IIRC SIGHUP is generated when the controlling TTY is lost without prior detaching (earlier it was called daemonization iirc). Today, frameworks like runit or launchd are used for daemonization of services, so if SIGHUP is generated, the controlling terminal is closed (most likely) and thus the process will die anyway. So IMHO it does not change anything if SIGHUP is ignored or not. Am Sa., 6. Apr. 2024 um 18:12 Uhr schrieb Craig Edwards (Brain) < ***@***.***>:

…

***@***.**** commented on this pull request. ------------------------------ In src/dpp/sslclient.cpp <#1123 (comment)>: > @@ -237,11 +249,11 @@ ssl_client::ssl_client(const std::string &_hostname, const std::string &_port, b keepalive(reuse) { #ifndef WIN32 - signal(SIGALRM, SIG_IGN); - signal(SIGHUP, SIG_IGN); + set_signal_handler(SIGALRM); + set_signal_handler(SIGHUP); I think SIGHUP should be ignored regardless? if it is captured we don't know if this will interrupt the library and break it? the signals will execute inside the libraries internal threads. — Reply to this email directly

[braindigitalis]

There are many bots that don't use launchd etc for launching and will directly fork. these still need to be supported ⁣Kind regards Craig Edwards Brainbox.cc Creating fun stuff since 1993​

…

On 6 Apr 2024, 22:27, at 22:27, Nidhoegger ***@***.***> wrote: Since not seeing this on GitHub directly, I will reply here and hope it works. IIRC SIGHUP is generated when the controlling TTY is lost without prior detaching (earlier it was called daemonization iirc). Today, frameworks like runit or launchd are used for daemonization of services, so if SIGHUP is generated, the controlling terminal is closed (most likely) and thus the process will die anyway. So IMHO it does not change anything if SIGHUP is ignored or not. Am Sa., 6. Apr. 2024 um 18:12 Uhr schrieb Craig Edwards (Brain) < ***@***.***>: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/dpp/sslclient.cpp > <#1123 (comment)>: > > > @@ -237,11 +249,11 @@ ssl_client::ssl_client(const std::string &_hostname, const std::string &_port, b > keepalive(reuse) > { > #ifndef WIN32 > - signal(SIGALRM, SIG_IGN); > - signal(SIGHUP, SIG_IGN); > + set_signal_handler(SIGALRM); > + set_signal_handler(SIGHUP); > > I think SIGHUP should be ignored regardless? if it is captured we don't > know if this will interrupt the library and break it? the signals will > execute inside the libraries internal threads. > > — > Reply to this email directly, view it on GitHub > <#1123 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/A4EIMOO6QUJWGC3O7BNE44DY4ANG5AVCNFSM6AAAAABF2OPLL6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBUGU4DAOBWGQ> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> > -- Reply to this email directly or view it on GitHub: #1123 (comment) You are receiving this because you commented. Message ID: ***@***.***>

[Nidhoegger]

As said: If they have a controlling TTY and its closed, they will terminate, no matter if they ignore SIGHUP or not. If they fork and use setsid and detach from a terminal, then they wont get SIGHUP on closure. Ignoring SIGHUP will only make them not get the signal and terminate anyways.

[Nidhoegger]

Anyway, changed it. There is no harm done in ignoring it IMHO

[Nidhoegger]

To set everything to a known state. Not everything is checked and other fields might be set.Message ID: ***@***.***>

…

[braindigitalis]

To set everything to a known state. Not everything is checked and other fields might be set.Message ID: @.***>
…

surely the nicer c++ way is just: sa = {};

[Nidhoegger]

Feel free to change it. I dont see a reason to.

[Jaskowicz1]

Is this latest change tested?