[libc-commits] [PATCH] D156700: [libc] Fix sched_getscheduler return value

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/test/src/sched/param_and_scheduler_test.cpp:40
 
-  int policy = SCHED_OTHER;
-  bool can_set = true;
-
-  int init_policy = __llvm_libc::sched_getscheduler(0);
-  ASSERT_GE(init_policy, 0);
-  ASSERT_EQ(libc_errno, 0);
-
-  int max_priority = __llvm_libc::sched_get_priority_max(policy);
-  ASSERT_GE(max_priority, 0);
-  ASSERT_EQ(libc_errno, 0);
-  int min_priority = __llvm_libc::sched_get_priority_min(policy);
-  ASSERT_GE(min_priority, 0);
-  ASSERT_EQ(libc_errno, 0);
-
-  struct sched_param param = {min_priority};
-
-  // Negative pid
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(-1, policy, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(-1), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  // Invalid Policy
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy | 128, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  // Out of bounds priority
-  param.sched_priority = min_priority - 1;
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  param.sched_priority = max_priority + 1;
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param), -1);
-  // A bit hard to test as depending if we are root or not we can run into
-  // different issues.
-  ASSERT_TRUE(libc_errno == EINVAL || libc_errno == EPERM);
-  libc_errno = 0;
-
-  // Some sched policies require permissions, so skip
-  param.sched_priority = min_priority;
-  // Success / missing permissions.
-  ASSERT_EQ(__llvm_libc::sched_setscheduler(0, policy, &param),
-            can_set ? 0 : -1);
-  ASSERT_TRUE(can_set ? (libc_errno == 0)
-                      : (libc_errno == EINVAL || libc_errno == EPERM));
-  libc_errno = 0;
-
-  ASSERT_EQ(__llvm_libc::sched_getscheduler(0), can_set ? policy : init_policy);
-  ASSERT_EQ(libc_errno, 0);
-
-  // Out of bounds priority
-  param.sched_priority = -1;
-  ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
-
-  param.sched_priority = max_priority + 1;
-  ASSERT_EQ(__llvm_libc::sched_setparam(0, &param), -1);
-  ASSERT_EQ(libc_errno, EINVAL);
-  libc_errno = 0;
+  auto testSched = [&](int policy, bool can_set) -> void {
+    int init_policy = __llvm_libc::sched_getscheduler(0);
----------------
sivachandra wrote:
> I think the duplicated code is present to help with debugging.
the reason these tests were separated to begin with is the `ASSERT` macro only reports the line its on. This makes it very hard to debug test failures where the error doesn't obviously map to a specific input to your test function. In this case I'd recommend making `testSched` a proper function, then making each of the calls its own `TEST` to solve that problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156700