Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -a DELAY_TIME, SIGTERM delayment feature #253

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

qirlib
Copy link

@qirlib qirlib commented Jul 1, 2021

A new option, with a functionality of:

-a DELAY_TIME

When SIGTERM conditions are met, send a notification to the user and
wait at least DELAY_TIME seconds before issuing SIGTERM, unless situation
improves. SIGTERM is cancelled and a separate notification is sent
when SIGTERM conditions have not been met, continuously, for at least
DELAY_TIME seconds. Countdown updates only when SIGTERM conditions
are satisfied. This provides a chance for a user to gracefully close
an important application and/or consciously select and close
the least needed application.

-n must be explicitly enabled.

There is also a small refactoring of notify().

return;
}

const size_t stringc_len = strlen("string:");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would alarm SCA because of the \0.

@qirlib
Copy link
Author

qirlib commented Jul 2, 2021

Yeah, the Codacy issue. Am I supposed to do something about that? Its reason for "Not up to standards. This pull request quality could be better." is kind of off. @CanNuhlar

And, yes, just in case, so one does not have to verify it oneself: that code line is mov eax, 7 on gcc/clang even with -O0 -g3.

@qirlib
Copy link
Author

qirlib commented Jul 2, 2021

Am I supposed to do something about that?

It's just that Codacy requires you to link your GitHub account with a permission to Act on your behalf, and I really don't want to do that just to see if I can, as a patch author, tick this issue off as a false positive. And I did not find any special in-code ignore-issue directives from my cursory Internet search.

@rfjakob
Copy link
Owner

rfjakob commented Jul 2, 2021

Codacity says

Does not handle strings that are not \0-terminated; if given one it may perform an over-read (it could cause a crash if unprotected) (CWE-126).

which is ridiculous for a string literal. Ignore.

// given that SIGTERM window can be passed rather quickly,
// make one small attempt to terminate process gracefully;
// kill_wait() will quickly escalate to SIGKILL anyway
kill_process(args, SIGTERM, victim);
Copy link
Author

@qirlib qirlib Jul 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I glitched out and have had misread kill_wait(): this does not really make much sense. This line should either be just kill_process(args, sig, victim); or I'll possibly add a single adaptive sleep to not dip too deep into SIGKILL territory. I'm a bit busy now, so it might take a while on my end.

@CanNuhlar
Copy link

Yeah, totally agreed on \0. Just wanted to point out guys :)

@rfjakob
Copy link
Owner

rfjakob commented Jul 13, 2021

Some general questions:

  1. Do you use this on your PC, and what delay time do you set?
  2. Why a delay time instead of a warning at a specific free memory percentage (before SIGTERM)?

Copy link
Owner

@rfjakob rfjakob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qirlib
Copy link
Author

qirlib commented Aug 28, 2021

Sorry for leaving this hanging.

  1. Yes. This code was written mainly to prevent firefox/clion from closing down in my face when I happen to open a new tab or a file. And for this, it works perfectly (well, almost, see below). The delay time is 20 seconds. By experimentation, 10 is too low to meaningfully close anything, 30 is too long for "everything is fine now" feedback.
  2. The main reason, I guess, is that I had not been not aware about or had have not put much thought towards replace-id. This pull request, in fact, originally started as work to add a third tuple parameter, for warnings. However, halfway through I figured that with interface I had — net.nuetzlich.SystemNotifications.Notify — SIGTERM countdown would be better: there is no temptation to just ignore warning for a minute and no need for complicated message rate limit system.

The thing is, when I read

instead of a warning at a specific free memory percentage

, something clicked in my head; I have realized a persistent, updatable warning-notification would be even better. There are also issues like GNOME 3 does not display notifications over fullscreen windows unless urgency=critical. And I figured I'd want things like org.freedesktop.Notifications.CloseNotification. dbus-send is not enough. Consequently, I've been working on a "proper" embedded (optionally compiled, obviously) notification system, and a v2 of this.

While I'm at that, and since it kind-of fits here, are you okay with libdbus? It's effectively GPLv2+. sd-bus is a very nice, but is also a hard dependency on systemd; gdbus is LGPLv2.1+, but it is fat:

$ size --format=GNU libdbus-1.so
      text       data        bss      total filename
    186653     129311        528     316492 libdbus-1.so
$ size --format=GNU --totals libgio-2.0.so libglib-2.0.so libgobject-2.0.so
      text       data        bss      total filename
   1123006     821568       7680    1952254 libgio-2.0.so
    539914     659943       3112    1202969 libglib-2.0.so
    220294     160293       2792     383379 libgobject-2.0.so
   1883214    1641804      13584    3538602 (TOTALS)

Not to mention ldd libgio-2.0.so. Through I'd guess I would expect glib (but not gio) to be already in-memory in almost any graphical environment.

@rfjakob
Copy link
Owner

rfjakob commented Aug 28, 2021

(1) Ok, I see, thanks!

(2) Unfortunately, there seems to be no sane way to send GUI notifications directly from a service.

You could, however, extend the https://github.com/rfjakob/systembus-notify API to do what you need (replace-id and CloseNotification - though I wonder if replace-id would be enough?)

@rfjakob
Copy link
Owner

rfjakob commented Aug 28, 2021

PS: before dbus-send and systembus-notify, we had this, but I don't want to go there again ;)

https://github.com/rfjakob/earlyoom/blob/ab946ee1c3b98236f487c50434181468abdaef67/contrib/notify_all_users.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants