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

Signal support #822

Merged
merged 7 commits into from
Oct 4, 2020
Merged

Signal support #822

merged 7 commits into from
Oct 4, 2020

Conversation

Gelox
Copy link

@Gelox Gelox commented Aug 29, 2020

Add a status field to the custom block that takes a value between 0 and 30. These correspond to SIGRTMIN+0 to SIGRTMIN+30.
This causes the block to be updated by sending the corresponding SIGRT signal to i3status-rs. This can be done via
pkill -SIGRTMIN+1 i3status-rs in order to send a signal to blocks listening for signal 1.

This feature allows custom blocks to not need to be updated on an interval, but instead can be updated via scripts sending a signal when they need to update.

A possible flaw is that I was not able to find a good way of pulling in the signal range dynamically from the users computer, instead the range is hard-coded to listen to signals between 34 and 64 (where this is the range on my computer).
The relevant constants can be found in src/signals.rs

grewka added 3 commits August 29, 2020 12:27
Allows custom blocks to update if a specified SIGRT signal is recieved,
also adds support for potential future SIGRT support for other blocks.

Spawns a new thread that listens for specified signals, this uses the
signal_hook crate.

Expands the blocks trait to allow for a signal event as well as to be
able to ask it for which potential signal a block is listening to.
The get_signal() function might be unneeded and could be removed if we
are willing to listen to any RTSIG signal rather than only the ones
specified.

Currently i3status-rs will crash if it recieves a RTSIG signal that it
has not specifically asked to listen to, this was done in order to make
debugging this easier.
Restructure and move functions that deal with signals into their own
file.
Remove the functionality that listened only to the relevant signals
rather than every SIGRT event. The reasoning behind this is that it
required a significant amount of general code for example an extra trait
function in the blocks trait without bringing any important
functionality.
The original reason for selective signal listening was in
order to make debugging easier when a non-listened to signal caused a
crash. Now we avoid crashing instead.

Right now we are listening to every SIGRT event within the allowed span
(SIG 34 - 64). Of course blocks will only update if their corresponding
signal is sent still.
@ammgws
Copy link
Collaborator

ammgws commented Aug 31, 2020

A possible flaw is that I was not able to find a good way of pulling in the signal range dynamically from the users computer, instead the range is hard-coded to listen to signals between 34 and 64 (where this is the range on my computer).

I guess we just have to wait for this: nix-rust/nix#1143

Can you update blocks.md as well?

@ammgws
Copy link
Collaborator

ammgws commented Sep 10, 2020

I haven't had time to test it yet, how has it been going for you so far?

@Gelox
Copy link
Author

Gelox commented Sep 10, 2020

I have used this feature on my own computer, it works fine. I updated the relevant blocks.md parts.
However the signal value constants I don't have an idea for how to make progress on. I think I started an issue in libc asking what they thought about exposing SIGRTMIN and SIGRTMAX but I haven't got a response nor do I think one will be given for a long time. I think the issue is that these specific constants change depending on your system and in libc they expose constants by hard-coding them. One would have to MAYBE create a function, but that entails its own troubles.

So far I don't know how to get the relevant constants in a dynamic way (perhaps we could add C-bindings).

The options that I see available are:

  • Provide this feature behind a feature flag and keep the constants as they are
  • Recreate the update mechanism without signals in some other way
  • Do not merge this feature
  • Keep the constants as they are, it will work on any system that has allocated 2 RT signals for system purposes (which is all the systems that I am familiar with, debian, arch, ..)
  • Explore adding C-bindings
  • Figure out another solution

I'll let the community (or you) decide with what direction they want to go, I do not mind helping with the decided way assuming it's not too much work.

@Gelox
Copy link
Author

Gelox commented Sep 14, 2020

I'll add on to that list of possible solutions.
If there would be a way to get the SIGRTMIN and SIGRTMAX values from a shell then we could run a shell command from rust to get it.
However as far as I can tell there is no such possibility. Getting this value seems to be exceedingly hard if you are not in C.
kill -L and kill -l seem to be out of date for me, so that's not a possibility to finding these values for all systems.

@ammgws
Copy link
Collaborator

ammgws commented Oct 2, 2020

On mobile now so can't review the code but what happens in the case a user sets an invalid signal value? (In the case that we don't check the bounds ourselves and someone sets something like 99)

@Gelox
Copy link
Author

Gelox commented Oct 2, 2020

//In ConfigBlock::new()...
if let Some(signal) = block_config.signal {
         // If the signal is not in the valid range we return an error
         if signal < 0 || signal > SIGMAX - SIGMIN {
            return Err(Error::BlockError(
                    custom.id,
                    format!("The provided signal was out of bounds. An allowed signal needs to be between {} and {}", 0, SIGMAX - SIGMIN)
                    ));
        } else {
            custom.signal = Some(signal + SIGMIN)
        }
    };
}

It returns an error from the ConfigBlock::new() method.

@ammgws
Copy link
Collaborator

ammgws commented Oct 4, 2020

Sorry I meant something like this: For example say we hardcoded the range as 34 to 64, what would happen in the two situations below.

  1. SIGRTMIN starts at 35 on someone's system, and they choose signal 34 for one of their blocks.

  2. Someone only has 20 realtime signals on their system and they choose signal 64 for one of their blocks.

https://man7.org/linux/man-pages/man7/signal.7.html

   Starting with version 2.2, Linux supports real-time signals as
   originally defined in the POSIX.1b real-time extensions (and now
   included in POSIX.1-2001).  The range of supported real-time signals
   is defined by the macros SIGRTMIN and SIGRTMAX.  POSIX.1-2001
   requires that an implementation support at least _POSIX_RTSIG_MAX (8)
   real-time signals.

   The Linux kernel supports a range of 33 different real-time signals,
   numbered 32 to 64.  However, the glibc POSIX threads implementation
   internally uses two (for NPTL) or three (for LinuxThreads) real-time
   signals (see pthreads(7)), and adjusts the value of SIGRTMIN suitably
   (to 34 or 35).  Because the range of available real-time signals
   varies according to the glibc threading implementation (and this
   variation can occur at run time according to the available kernel and
   glibc), and indeed the range of real-time signals varies across UNIX
   systems, programs should never refer to real-time signals using hard-
   coded numbers, but instead should always refer to real-time signals
   using the notation SIGRTMIN+n, and include suitable (run-time) checks
   that SIGRTMIN+n does not exceed SIGRTMAX.

Or realistically, if SIGRTMIN and SIGRTMAX only vary on non-Linux kernels then we could hardcode the range without any any worries?

@Gelox
Copy link
Author

Gelox commented Oct 4, 2020

SIGRTMIN starts at 35 on someone's system, and they choose signal 34 for one of their blocks.

Well this would cause incorrect behavior but not necessarily break anything. If their SIGRTMIN is 35 and they choose to listen to signal 0 (which corresponds to 34) then the result would be that for them to update i3status-rs they'd have to send SIGRTMIN-1 which would be an occupied signal.
However realistically they wouldn't send SIGRTMIN-1 signals, rather they'd try to send SIGRTMIN+0 and see that it does not update (as they'd be sending signal 35 rather than 34).
However I guess you would also be listening to potential 34 signals from the OS, which could potentially create problems?

Someone only has 20 realtime signals on their system and they choose signal 64 for one of their blocks.

As far as I know SIGRTMAX does not vary, someone having 20 realtime signals would have to be a result of SIGRTMIN being 44 which would be the same problem as the previous one.

Or realistically, if SIGRTMIN and SIGRTMAX only vary on non-Linux kernels then we could hardcode the range without any any worries?

Well, if we are only targeting Linux kernels and SIGRTMIN and SIGRTMAX do not vary on Linux kernels then no issue would appear. I have not seen a guarantee that they do not vary however. In fact in the man-page you referenced it seems to indicate that it can sometimes be 35 on Linux kernels.

It seems that libc is open to adding SIGRTMIN support to their library, I'm thinking I might try to add it myself when I get around to it. This means that eventually we should be able to add this feature correctly regardless of merging it right now, but if we do merge it right now we can always push a fix later.
:)

@ammgws
Copy link
Collaborator

ammgws commented Oct 4, 2020

However I guess you would also be listening to potential 34 signals from the OS, which could potentially create problems?

Yeah that was what I was trying to hint at.

it can sometimes be 35 on Linux kernels.

I guess we could just make our range start a bit higher like 40 - 64 to be safe, at least until libc is enhanced.

@Gelox
Copy link
Author

Gelox commented Oct 4, 2020

So, I just checked how difficult it is to bring the SIGRTMIN into rust through C bindings, super simple. I didn't realize this before so I learned my lesson.
Either way this should solve all of our problems, it does require we do 1 temporary C binding but this can be removed when libc updates their API and this propagates to the nix crate.

I'll try to update this right now. How does that sound?

@ammgws
Copy link
Collaborator

ammgws commented Oct 4, 2020

Sounds reasonable to me. Hopefully nix/libc get updated by the next time we want to do a release.

Do you think we could also add support for a signal that schedules an update for the whole bar (every block) at once? Maybe could use SIGUSR1 or SIGUSR2 for that.

In addition to the above another use case we could hopefully cover at some time is reloading the config (see #302) but this one probably isn't trivial enough to cover with this PR.

Remove the hardcoded values for SIGRTMIN and SIGRTMAX and replace them
with C bindings. This fixes the bug that could potentially occur if the
kernel does not have SIGRTMIN = 34 and SIGRTMAX = 64.

The C bindings should be replaced with libc or nix crate bindings when
these become avaliable.
@Gelox
Copy link
Author

Gelox commented Oct 4, 2020

I added the C-bindings, there shouldn't be an issue now.
As for adding support for an entire bar update that isn't added in this PR, I think that's best left to another PR.

I could also look a bit at the reloading thing. But again better left to another PR. :)

SIGUSR1 now refreshes each block, SIGUSR2 is listened for but nothing is
currently done when the signal is recieved, this feature should be saved
for refreshing the .toml config file.
@ammgws
Copy link
Collaborator

ammgws commented Oct 4, 2020

LGTM!

I suppose in the future the implementation could be made block agnostic so it can apply to any block, but it's not a cause for delaying this PR.

Thanks for working on this!

@ammgws ammgws merged commit ab752f9 into greshake:master Oct 4, 2020
@ammgws ammgws linked an issue Oct 4, 2020 that may be closed by this pull request
@Gelox Gelox deleted the signal_support branch October 4, 2020 11:15
@Gelox
Copy link
Author

Gelox commented Oct 4, 2020

Fantastic! Currently no documentation exists that SIGUSR1 updates all blocks, I don't know where this fits. Not sure if it is necessary but just giving a heads up.

Thanks for letting me work on this! 💯

@ammgws
Copy link
Collaborator

ammgws commented Oct 4, 2020

Good catch. Not really sure where to put it either. I think the README needs a rewrite.

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

Successfully merging this pull request may close these issues.

Signal need for update to custom (or all) blocks
2 participants