Changes for managarm cancelable syscalls #884
Conversation
| @@ -60,31 +60,42 @@ int sys_waitpid(pid_t pid, int *status, int flags, struct rusage *ru, pid_t *ret | |||
| return ENOSYS; | |||
| } | |||
|
|
|||
| SignalGuard sguard; | |||
| //SignalGuard sguard; | |||
| HEL_CHECK(recv_resp.error()); | ||
|
|
||
| managarm::posix::SvrResponse<MemoryAllocator> resp(getSysdepsAllocator()); | ||
| resp.ParseFromArray(recv_resp.data(), recv_resp.length()); | ||
| if(resp.error() == managarm::posix::Errors::ILLEGAL_ARGUMENTS) { | ||
| return EINVAL; | ||
| } | ||
| if (resp.error() == managarm::posix::Errors::INTERRUPTED) { | ||
| mlibc::infoLogger() << "returning EINT" << frg::endlog; |
There was a problem hiding this comment.
Should we put this behind some debug flag?
|
Yeah there are quite some debug logs left currently 😅 |
There was a problem hiding this comment.
LGTM
I do have a question, since this is not instantly obvious to me: would this code work without the changes in managarm, or are we looking at circular dependencies here? To my knowledge, merging in the managarm side first would result in some weird behavior.
| @@ -1,3 +1,4 @@ | |||
| #include "mlibc/debug.hpp" | |||
There was a problem hiding this comment.
Is this LSP? In any case, this should be replaced with <>
Geertiebear
force-pushed
the
cancelable_syscalls
branch
from
1c76936 to
ac7c83e
Compare
Geertiebear
force-pushed
the
cancelable_syscalls
branch
from
ac7c83e to
506d9e4
Compare
Geertiebear
force-pushed
the
cancelable_syscalls
branch
from
506d9e4 to
922dd20
Compare
| @@ -1,3 +1,4 @@ | |||
| #include "mlibc/debug.hpp" | |||
| auto element = globalQueue.dequeueSingle(false); | ||
| if (!element) { | ||
| element = globalQueue.dequeueSingle(); | ||
| __ensure(element); | ||
| } |
There was a problem hiding this comment.
This is the only call where we pass false to dequeueSingle() (AFAICT) and we immediately call into dequeueSingle() again. So what is the difference compared to just doing dequeueSingle(true) here?
There was a problem hiding this comment.
Note we use protocol errors to signify cancellation (e.g., in read() you check for managarm::fs::Errors::INTERRUPTED), so maybe the ignoreCancel = false case is not necessary (and we can always re-try if helFutexWait is interrupted)?
| auto result = parseSimple(element); | ||
| if (!element) { | ||
| return EINTR; | ||
| } | ||
| auto result = parseSimple(*element); |
There was a problem hiding this comment.
We have this change for sleep but AFAICT sleep() is still not cancelable since we don't integrate it with the cancellation event. We could drop support for sleep() in the initial PR.
| @@ -1628,12 +1629,14 @@ int sys_mknodat(int dirfd, const char *path, int mode, int dev) { | |||
| } | |||
|
|
|||
| int sys_read(int fd, void *data, size_t max_size, ssize_t *bytes_read) { | |||
| SignalGuard sguard; | |||
There was a problem hiding this comment.
Why do we need to do this?
This prevents a signal to be raised (= posix-subsystem won't jump to the signal handler), but it should not prevent POSIX from interrupting the thread. I think we still want to keep this.
Since the changes are incomplete and a little unstable, marking it as draft right now.