D16218: [KDevelop/Core]: safe signal-handler implementation (WIP)

[Aaron Puchert]

aaronpuchert accepted this revision as: aaronpuchert.
aaronpuchert added a comment.


  The signal handling and processing gets a +1 from me, but someone else should review the overall code structure.
  
  In D16218#343364 <https://phabricator.kde.org/D16218#343364>, @kfunk wrote:
  
  > Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).
  
  
  Arguably that still stands, but it's outside my area of expertise to comment on it.

INLINE COMMENTS

> core.cpp:68-69
> +    // Get the signal number that was written to the pipe in the actual
> +    // signal handler. A bit awkward but we have to flush the read side
> +    // of the pipe anyway.
> +    // Failure to flush can lead to repetitive signals from QSocketNotifier

I don't find this awkward. You're reacting to a pipe write, so that seems like the right way to get the information that you want to react to.

> core.cpp:103
> +        write(signalPipeWrite, &sig, sizeof(sig));
> +    }
> +}

Maybe you can add an else branch where you re-raise the signal with the default handler.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk, aaronpuchert
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181020/bcfc7aa4/attachment-0001.html>