Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement sigtimedwait for use by suckless init #1320
base: master
Are you sure you want to change the base?
Implement sigtimedwait for use by suckless init #1320
Changes from 34 commits
a219223
7286e68
e07f7c1
6f5ad93
3d15ce3
1e3744d
1ce829f
000d730
2d7ca28
b386909
fb3eaa3
e41929e
7abe9f9
35a1ecc
c486015
bc5288f
7b0199c
239ca0e
da05dd4
54b38ef
1994eb2
b301a1c
ac3908b
51bbe9f
cbf8c25
29c302b
e597ef4
eff8b00
76ca8f1
c576c73
db00050
4088dd6
cdfc493
f7977d5
97d0591
f9014c9
a5fb873
0c30ce6
86c4f14
3c7670d
4b8edb8
65da74e
b4caf4b
d1df936
59d3c58
c3c4abd
cfab58a
78e955b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you name it
sigusr2_handler
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way I know that some particular test will be the only user of this function.
Suppose someone in the future would like to use the sigusr2 for testing, then they'd need to create a new function, how would they name it then? They could reuse this function, but I advise against that. If one would like to change the behavior of the handler for some particular test they'd need to create a new function and give it some other name anyway.
EDIT: Maybe it's an overkill, I don't know, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested it because I see that in other signal tests we name functions that looks like that always in the same way. These functions are used in multiple tests and I think there is nothing wrong in that (they are used identically so why we would like to duplicate code?).
If you would like to add some functionality to such function then it is a reason to make function with unique name (and leave the old one unchanged because we expect some specific behavior from it).
@cahirwpz @pwit81 I think that we can think about making such function a standard. They are used pretty often and maybe we don't have to invent new functions for just handling one signal for every written test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franciscozdo @korbiniak It's definitely a good idea to factor it out.
I'd do it by extending our current
utest.h
interface by adding:void signal_count(int signo)
- registers a generic handler forsigno
signal,int signal_counter(int signo)
- returns a number of times signalsigno
was delivered.If you have a better idea how to name those functions then feel free to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with sleepq, but was there a reason why we chose
TDF_SLPINTR
only when we hadn't got a timeout? You have changed the semantics here and I cant tell if it is correct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did change the semantics as I think it was a bug that
TDF_SLPINTR
was not set. See discussion here: https://discord.com/channels/951841212267130950/1054133625731424337There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I agree with that, but I also think that @cahirwpz should take a look at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it has to do with possible deficiencies in
sleepq
API. ATM we have twowait
functions - with and without timeout. IMO there should bewait
andwait_intr
with optional timeout. That should let us express all use cases for putting a thread to sleep.Please note that non-interruptible
wait
should probably be only used by the code that does not interface user-space directly - i.e. the code that does not understand the concept of a signal or need not be burdened with complexity of extra error handling (ETIMEDOUT
). Examples would be waking up a thread from interrupt or "fast" device drivers code. On contrary the interruptiblewait
should be used in "slow" device drivers code (whereread
orwrite
may be interrupted) and all syscalls that can tolerate being interrupted.Perhaps what I wrote above should be redacted and put somewhere into
sleepq.h
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer the question - the change is ok, since function is documented as follows: