[libc-commits] [PATCH] D148069: [LIBC] Implement remainder of posix 'sched.h' minus `SCHED_SPORADIC`

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/include/llvm-libc-types/struct_sched_param.h:21-35
+  // This is for the SCHED_SPORADIC extension. Not yet implemented by linux (or
+  // anywhere AFAICT).
+#if 0
+  // Low scheduling priority for sporadic server.
+  int sched_ss_low_priority;
+
+  // Maximum pending replenishments for sporadic server.
----------------
if we don't expect to need `SCHED_SPORADIC` then we can leave this part out of the struct for the moment.


================
Comment at: libc/test/src/sched/get_priority_test.cpp:35
+  // SCHED_BATCH, SCHED_ISO, SCHED_IDLE, SCHED_DEADLINE
+  TestPolicySuccess(SCHED_OTHER);
+  TestPolicySuccess(SCHED_FIFO);
----------------
given that there are relatively few of these it's probably better to just repeat the code of the test. It makes the file longer, but it also makes debugging easier. The `ASSERT` macros only report the line they're on and the failing value, so if one of these is failing it'll be hard to tell from the error which it is. If you want to you can use the {braces} to scope each of the tests, that way redefining the variables each time won't be an issue.


================
Comment at: libc/test/src/sched/get_priority_test.cpp:49
+  // Arbitrary values for which there is no policy.
+  TestPolicyFailure(-1);
+  TestPolicyFailure(30);
----------------
if you want, you could split the failures into a separate test.


================
Comment at: libc/test/src/sched/param_and_scheduler_test.cpp:27-28
+    //       1) Missing permissions -> EPERM. Maybe doable by finding
+    //       another
+    //          pid that exists and changing its policy, but that seems
+    //          risky. Maybe something with fork/clone would work.
----------------
nit: formatting?


================
Comment at: libc/test/src/sched/param_and_scheduler_test.cpp:138-140
+  TestPolicy(SCHED_OTHER, /* req_admin */ false);
+  TestPolicy(SCHED_FIFO, /* req_admin */ true);
+  TestPolicy(SCHED_RR, /* req_admin */ true);
----------------
each of these should probably be in their own test.


================
Comment at: libc/test/src/sched/rr_get_interval_test.cpp:37
+  // We can only set SCHED_RR with CAP_SYS_ADMIN
+  if (getuid() == 0)
+    SetSched(SCHED_RR);
----------------
where is `getuid` included from?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148069/new/

https://reviews.llvm.org/D148069