Implement sigtimedwait for use by suckless init by cahirwpz · Pull Request #1320 · cahirwpz/mimiker

cahirwpz · 2022-07-26T11:24:56Z

Implement sigtimedwait system call.

Finished sigtimedwait, but there's a bug, test crashes in some unexpected place.

…kless-sinit

cahirwpz · 2023-01-01T19:11:52Z

@panantoni01 Please extract sinit to another PR according to new method of importing external software. Please take into account etc/ directory as well as stub of sigtimedwait syscall. Just leave implementation of sys_sigtimedwait and its userspace tests to @korbiniak.

sys/kern/signal.c

franciscozdo

Mostly minor changes or questions, because I am not an expert in signals.

I am also wondering what are the files etc/rc.init and etc/rc.shutdown. I think they are not needed by any of changes in this PR.

sys/kern/syscalls.c

include/sys/syscallargs.h

franciscozdo · 2023-01-18T12:35:30Z

sys/kern/sleepq.c

  }

-  td->td_flags |= (timeout > 0) ? TDF_SLPTIMED : TDF_SLPINTR;
+  td->td_flags |= TDF_SLPINTR;


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.

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/1054133625731424337

Okay, I agree with that, but I also think that @cahirwpz should take a look at that.

I think it has to do with possible deficiencies in sleepq API. ATM we have two wait functions - with and without timeout. IMO there should be wait and wait_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 interruptible wait should be used in "slow" device drivers code (where read or write 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.

To answer the question - the change is ok, since function is documented as follows:

/*! \brief Performs interruptible sleep with timeout. * * Timed sleep is also interruptible. [...] */

sys/kern/signal.c

bin/utest/signal.c

franciscozdo · 2023-01-18T18:38:40Z

bin/utest/signal.c

+/* ======= signal_sigtimedwait_timeout ======= */
+static int sigusr2_handled = 0;
+
+void sigtimedwait_timeout_sigusr2_handler(int signo) {


Why don't you name it sigusr2_handler?

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?

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?

@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 for signo signal,

int signal_counter(int signo) - returns a number of times signal signo was delivered.

If you have a better idea how to name those functions then feel free to do so.

bin/utest/signal.c

sys/kern/signal.c

sys/kern/syscalls.c

pj1031999

What is a purpose of /etc/rc.init /etc/rc.shutdown? How it is related to sigtimedwait or /dev/tty?

bin/utest/signal.c

sys/kern/syscalls.c

sys/kern/sleepq.c

sys/kern/signal.c

include/sys/signal.h

cahirwpz · 2023-05-20T10:08:25Z

@korbiniak I'd like to take over the PR and finish the work. WDYT?

Add /sbin/init based on suckless sinit.

a219223

cahirwpz added the WiP not ready for code review label Jul 26, 2022

First check tests to run then run init.

7286e68

cahirwpz assigned korbiniak Jul 26, 2022

cahirwpz and others added 23 commits October 14, 2022 14:18

Merge branch 'master' into suckless-sinit

e07f7c1

wip

6f5ad93

[WIP] do_sigtimedwait

3d15ce3

Merge branch 'master' into suckless-sinit

1e3744d

[WIP] Sigtimedwait finished, first test

1ce829f

Finished sigtimedwait, but there's a bug, test crashes in some unexpected place.

Add /sbin/init based on suckless sinit.

000d730

First check tests to run then run init.

2d7ca28

wip

b386909

[WIP] do_sigtimedwait

fb3eaa3

[WIP] Sigtimedwait finished, first test

e41929e

Finished sigtimedwait, but there's a bug, test crashes in some unexpected place.

Merge branch 'master' into suckless-sinit

7abe9f9

Fixes after merge.

35a1ecc

Revert changes to the build & test infrastructure.

c486015

Merge branch 'suckless-sinit' of github.com:cahirwpz/mimiker into suc…

bc5288f

…kless-sinit

Sigtimedwait finished

7b0199c

format

239ca0e

Fixed llvm version

da05dd4

format

54b38ef

Fixed launch changes

1994eb2

Fixed race in timeout test

b301a1c

Format.

ac3908b

Lowered the timeout in signal_sigtimedwait_timeout test.

51bbe9f

Lower the timeout even more.

cbf8c25

cahirwpz commented Jan 1, 2023

View reviewed changes

sys/kern/signal.c Outdated Show resolved Hide resolved

Merge branch 'master' into suckless-sinit

29c302b

Use utest_spawn and utest_child_exited

f7977d5

franciscozdo requested changes Jan 18, 2023

View reviewed changes

franciscozdo added review please review this PR and removed WiP not ready for code review labels Jan 18, 2023

franciscozdo reviewed Jan 18, 2023

View reviewed changes

sys/kern/signal.c Show resolved Hide resolved

franciscozdo reviewed Jan 18, 2023

View reviewed changes

bin/utest/signal.c Outdated Show resolved Hide resolved

franciscozdo reviewed Jan 18, 2023

View reviewed changes

bin/utest/signal.c Outdated Show resolved Hide resolved

korbiniak added 2 commits January 19, 2023 14:47

Clean up do_sigtimedwait

97d0591

Refactor tests

f9014c9

cahirwpz commented Jan 20, 2023

View reviewed changes

bin/utest/signal.c Outdated Show resolved Hide resolved

bin/utest/signal.c Outdated Show resolved Hide resolved

sys/kern/signal.c Outdated Show resolved Hide resolved

sys/kern/syscalls.c Outdated Show resolved Hide resolved

Tidy up

a5fb873

pj1031999 reviewed Jan 20, 2023

View reviewed changes

korbiniak reviewed Jan 21, 2023

View reviewed changes

include/sys/signal.h Outdated Show resolved Hide resolved

cahirwpz added 4 commits June 8, 2023 09:56

Merge branch 'master' into suckless-sinit

0c30ce6

Merge branch 'master' into suckless-sinit

86c4f14

Address some long standing remarks (pt. 1)

3c7670d

Address some long lasting remarks (pt. 2)

4b8edb8

cahirwpz changed the title ~~Add kernel support for running /sbin/init correctly~~ Implement sigtimedwait for use by suckless init Jun 15, 2023

cahirwpz added 4 commits June 15, 2023 19:03

Address some long lasting remarks (pt. 3)

65da74e

Merge branch 'master' into suckless-sinit

b4caf4b

Fix tests.

d1df936

Merge branch 'master' into suckless-sinit

59d3c58

cahirwpz added a commit that referenced this pull request Jul 28, 2023

Extract changes from #1320 that could be commited sooner.

a44c826

cahirwpz added a commit that referenced this pull request Jul 28, 2023

Extract changes from #1320 (#1406)

95cc3b2

cahirwpz added 3 commits July 28, 2023 22:57

Merge branch 'master' into suckless-sinit

c3c4abd

Merge branch 'master' into suckless-sinit

cfab58a

Merge branch 'master' into suckless-sinit

78e955b

franciscozdo pushed a commit that referenced this pull request Aug 24, 2023

Extract changes from #1320 (#1406)

388f16c

Conversation

cahirwpz commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cahirwpz commented Jan 1, 2023

Uh oh!

Uh oh!

franciscozdo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

korbiniak Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pj1031999 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cahirwpz commented May 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cahirwpz commented Jul 26, 2022 •

edited

Loading

korbiniak Jan 19, 2023 •

edited

Loading