Opened 5 months ago

Closed 5 months ago

#23539 closed defect (fixed)

We've defined "don't use kist" as a negative interval, so don't check for -1

Reported by: pastly Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-sched, easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: pastly Sponsor:


Of course == -1 doesn't make sense when KISTSchedRunInterval is a unsigned as it currently is (#23538)

@@ -288,9 +288,9 @@ select_scheduler(void)
       if (!scheduler_can_use_kist()) {
-        if (get_options()->KISTSchedRunInterval == -1) {
+        if (get_options()->KISTSchedRunInterval < 0) {
           log_info(LD_SCHED, "Scheduler type KIST can not be used. It is "
-                             "disabled because KISTSchedRunInterval=-1");
+                             "disabled because KISTSchedRunInterval is < 0");
         } else {
           log_notice(LD_SCHED, "Scheduler type KIST has been disabled by "
                                "the consensus.");

Child Tickets

Change History (11)

comment:1 Changed 5 months ago by pastly

Parent ID: #23538

comment:2 Changed 5 months ago by pastly

Owner: set to dgoulet
Status: newassigned

comment:3 Changed 5 months ago by dgoulet

Status: assignedneeds_information

We'll wait on this decision before we do anything else about this #23538 but I think we might be leaning towards no negative value at all.

comment:4 Changed 5 months ago by pastly

Since we are going to keep Interval unsigned, we should probably be clamping the Interval here? Hmmm not necessarily. It is clamped from above in config.c. So all that we might want to do is BUG. Up to you.

comment:5 Changed 5 months ago by dgoulet

Parent ID: #23538

comment:6 Changed 5 months ago by dgoulet

Status: needs_informationneeds_review

See branch: bug23539_032_01

Making KISTSchedRunInterval a uint32_t has a lot of ramification so this needs careful review. Basically, now if the consensus sets that value to 0, it disables KIST. If the user sets it to 0 (default value), it will ask the consensus for its opinion.

comment:7 Changed 5 months ago by pastly

Reviewer: pastly

comment:8 Changed 5 months ago by pastly

Looks good. I did

  • Change KISTSchedRunInterval from int32_t to uint32_t
  • Lint the changes file
  • Minor comment/log message changes

See bug23539_032_01 on my gitweb.

comment:9 Changed 5 months ago by nickm

problem here -- INTERVAL types in the configuration _must_ be int in the options. I bet this has caused somebody a problem somewhere.

comment:10 Changed 5 months ago by pastly

Updated my branch making it an int; initialized sched_run_interval with KIST_SCHED_RUN_INTERVAL_DEFAULT instead of just 10.

comment:11 Changed 5 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged! let's keep an eye on jenkins for 32-bit errors

Note: See TracTickets for help on using tickets.