-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Make initLibStore
a viable alternative
#7732
Changes from all commits
6e0b710
a692c43
9693076
a58be39
e706ffa
52d6ce6
1107ea3
781d3dc
2196fd1
2445afd
1c0b680
ddebeb9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -47,6 +47,9 @@ extern char * * environ __attribute__((weak)); | |||||||||
|
||||||||||
namespace nix { | ||||||||||
|
||||||||||
void initLibUtil() { | ||||||||||
} | ||||||||||
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is odd for now, but allows libutil to initialize a dependency if need be in the future. |
||||||||||
|
||||||||||
std::optional<std::string> getEnv(const std::string & key) | ||||||||||
{ | ||||||||||
char * value = getenv(key.c_str()); | ||||||||||
|
@@ -1744,14 +1747,40 @@ void triggerInterrupt() | |||||||||
} | ||||||||||
|
||||||||||
static sigset_t savedSignalMask; | ||||||||||
static bool savedSignalMaskIsSet = false; | ||||||||||
|
||||||||||
void startSignalHandlerThread() | ||||||||||
void setChildSignalMask(sigset_t * sigs) | ||||||||||
{ | ||||||||||
updateWindowSize(); | ||||||||||
assert(sigs); // C style function, but think of sigs as a reference | ||||||||||
|
||||||||||
#if _POSIX_C_SOURCE >= 1 || _XOPEN_SOURCE || _POSIX_SOURCE | ||||||||||
sigemptyset(&savedSignalMask); | ||||||||||
// There's no "assign" or "copy" function, so we rely on (math) idempotence | ||||||||||
// of the or operator: a or a = a. | ||||||||||
sigorset(&savedSignalMask, sigs, sigs); | ||||||||||
#else | ||||||||||
// Without sigorset, our best bet is to assume that sigset_t is a type that | ||||||||||
// can be assigned directly, such as is the case for a sigset_t defined as | ||||||||||
// an integer type. | ||||||||||
savedSignalMask = *sigs; | ||||||||||
#endif | ||||||||||
|
||||||||||
savedSignalMaskIsSet = true; | ||||||||||
} | ||||||||||
|
||||||||||
void saveSignalMask() { | ||||||||||
if (sigprocmask(SIG_BLOCK, nullptr, &savedSignalMask)) | ||||||||||
throw SysError("querying signal mask"); | ||||||||||
|
||||||||||
savedSignalMaskIsSet = true; | ||||||||||
} | ||||||||||
|
||||||||||
void startSignalHandlerThread() | ||||||||||
{ | ||||||||||
updateWindowSize(); | ||||||||||
|
||||||||||
saveSignalMask(); | ||||||||||
|
||||||||||
sigset_t set; | ||||||||||
sigemptyset(&set); | ||||||||||
sigaddset(&set, SIGINT); | ||||||||||
|
@@ -1767,6 +1796,20 @@ void startSignalHandlerThread() | |||||||||
|
||||||||||
static void restoreSignals() | ||||||||||
{ | ||||||||||
// If startSignalHandlerThread wasn't called, that means we're not running | ||||||||||
// in a proper libmain process, but a process that presumably manages its | ||||||||||
// own signal handlers. Such a process should call either | ||||||||||
// - initNix(), to be a proper libmain process | ||||||||||
// - startSignalHandlerThread(), to resemble libmain regarding signal | ||||||||||
// handling only | ||||||||||
// - saveSignalMask(), for processes that define their own signal handling | ||||||||||
// thread | ||||||||||
// TODO: Warn about this? Have a default signal mask? The latter depends on | ||||||||||
// whether we should generally inherit signal masks from the caller. | ||||||||||
// I don't know what the larger unix ecosystem expects from us here. | ||||||||||
if (!savedSignalMaskIsSet) | ||||||||||
return; | ||||||||||
Comment on lines
+1810
to
+1811
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @edolstra, would it make sense to define a default signal mask for child processes, or is a unix process like If you think it's sensible to unblock all signals in child processes, I would do this to make the library a bit more robust / work better out of the box.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you don't know or aren't confident whether it's acceptable unixing to unblock all signals in child processes, that's ok too; we'd just stick to the current, conservative approach. |
||||||||||
|
||||||||||
if (sigprocmask(SIG_SETMASK, &savedSignalMask, nullptr)) | ||||||||||
throw SysError("restoring signals"); | ||||||||||
} | ||||||||||
|
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.
Isn't this strictly worse than having it in
initNix()
? I don't think users oflibstore
would expect their environment to be messed with in this way.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 behavior exists for a reason, and for a libstore reason, so it's not strictly worse.
I've opened #7731 so this can be improved later. Until then I think we should stick to the current behavior, so that store users who should move from
initNix
toinitLibStore
don't regress because of this.