Skip to content

Commit 92ad058

Browse files
Ericson2314edolstra
authored andcommitted
Use statusToString for child process exit diagnostics
Use `statusOk`/`statusToString` consistently when checking child process wait statuses, so that failures produce human-readable messages (e.g. "exited with code 2") rather than raw integer comparisons or nothing at all. Also give distinct exit codes to each failure path in the `fchmodatTryNoFollow` test for easier debugging. Note that `status == 0` and `statusOk(status)` do not do the same thing, because the latter does not check all the bits. So by changing the code from the former to the latter, we are technically changing behavior. However, it is not really proper to check the other bits, rather than use the macros which (essentially) parse a discriminated union. The other bits are probably guaranteed to be 0 in practice, but in theory, they are reserved for future use, and we should not guessing that that future use is / what the bits mean in that case.
1 parent adb65ab commit 92ad058

3 files changed

Lines changed: 20 additions & 14 deletions

File tree

src/libstore/unix/build/linux-derivation-builder.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -473,10 +473,10 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
473473

474474
sendPid.writeSide.close();
475475

476-
if (helper.wait() != 0) {
476+
if (auto status = helper.wait(); !statusOk(status)) {
477477
processSandboxSetupMessages();
478478
// Only reached if the child process didn't send an exception.
479-
throw Error("unable to start build process");
479+
throw Error("unable to start build process: %s", statusToString(status));
480480
}
481481

482482
userNamespaceSync.readSide = -1;
@@ -857,8 +857,8 @@ struct ChrootLinuxDerivationBuilder : ChrootDerivationBuilder, LinuxDerivationBu
857857
}));
858858

859859
int status = child.wait();
860-
if (status != 0)
861-
throw Error("could not add path '%s' to sandbox", store.printStorePath(path));
860+
if (!statusOk(status))
861+
throw Error("could not add path '%s' to sandbox: %s", store.printStorePath(path), statusToString(status));
862862
}
863863

864864
ActiveBuild getActiveBuild() override

src/libutil-tests/unix/file-system-at.cc

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -96,22 +96,22 @@ TEST(fchmodatTryNoFollow, fallbackWithoutProc)
9696
Pid pid = startProcess(
9797
[&] {
9898
if (unshare(CLONE_NEWNS) == -1)
99-
_exit(1);
99+
_exit(2);
100100

101101
if (mount(0, "/", 0, MS_PRIVATE | MS_REC, 0) == -1)
102-
_exit(1);
102+
_exit(2);
103103

104104
if (mount("tmpfs", "/proc", "tmpfs", 0, 0) == -1)
105-
_exit(1);
105+
_exit(2);
106106

107107
auto dirFd = openDirectory(tmpDir);
108108
if (!dirFd)
109-
exit(1);
109+
_exit(3);
110110

111111
try {
112112
fchmodatTryNoFollow(dirFd.get(), CanonPath("file"), 0600);
113113
} catch (SysError & e) {
114-
_exit(1);
114+
_exit(4);
115115
}
116116

117117
try {
@@ -121,12 +121,15 @@ TEST(fchmodatTryNoFollow, fallbackWithoutProc)
121121
_exit(0); /* Success. */
122122
}
123123

124-
_exit(1); /* Didn't throw the expected exception. */
124+
_exit(5); /* Didn't throw the expected exception. */
125125
},
126126
{.cloneFlags = CLONE_NEWUSER});
127127

128128
int status = pid.wait();
129-
ASSERT_TRUE(statusOk(status));
129+
EXPECT_TRUE(WIFEXITED(status));
130+
if (WEXITSTATUS(status) == 2)
131+
GTEST_SKIP() << "Could not mount, system may be misconfigured";
132+
EXPECT_EQ(WEXITSTATUS(status), 0);
130133

131134
struct ::stat st;
132135
ASSERT_EQ(stat((tmpDir / "file").c_str(), &st), 0);

src/libutil/linux/linux-namespaces.cc

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,10 @@ bool userNamespacesSupported()
3939
Pid pid = startProcess([&]() { _exit(0); }, {.cloneFlags = CLONE_NEWUSER});
4040

4141
auto r = pid.wait();
42-
assert(!r);
42+
/* The assert is OK because if we cannot do CLONE_NEWUSER we will
43+
throw above, and if the process does run, it must exit this way
44+
(or something else is really wrong). */
45+
assert(statusOk(r));
4346
} catch (SysError & e) {
4447
debug("user namespaces do not work on this system: %s", e.msg());
4548
return false;
@@ -72,8 +75,8 @@ bool mountAndPidNamespacesSupported()
7275
},
7376
{.cloneFlags = CLONE_NEWNS | CLONE_NEWPID | (userNamespacesSupported() ? CLONE_NEWUSER : 0)});
7477

75-
if (pid.wait()) {
76-
debug("PID namespaces do not work on this system: cannot remount /proc");
78+
if (auto status = pid.wait(); !statusOk(status)) {
79+
debug("PID namespaces do not work on this system: cannot remount /proc: %s", statusToString(status));
7780
return false;
7881
}
7982

0 commit comments

Comments
 (0)