On Fri, 21 Jan 2022, Leah Leshchinsky wrote:
The implementation of the error handling block in
tuna.get_policy_and_rtprio() caused the exceptions raised by
tuna_sched.Policy to be consumed by the finally block.
When an invalid policy is passed via the '--priority' flag,
this consumption of the exception causes tuna to fail silently.
Additionally, in tuna-cmd.py, if a thread list is passed along with the
'--priority' flag, tuna.get_policy_and_rtprio() is called twice, first
directly, and then again by tuna.threads_set_priority(). The
expectation is that tuna.threads_set_priorty will handle exceptions
raised by tuna.get_policy_and_rtprio(). This results in a failure to
handle exceptions that are raised by the initial direct call to
tuna.get_policy_and_rtprio(). If no exceptions are raised, a duplicate
call to this function is still unnecessary.
Remove the finally block in tuna.get_policy_and_rtprio() and move the
error handling so that exceptions are handled directly within the
function, rather than in various locations outside the function. Let
tuna.threads_set_priority() return the tuple (policy, rtprio) so that it
may be called in tuna-cmd.py without a seperate prior call to
tuna.get_policy_and_rtprio().
Small fix to differentiate between unsupported policy and invalid policy
or priority.
Signed-off-by: Leah Leshchinsky <lleshchi(a)redhat.com>
Thanks for pointing out the issue with the return in the finally block
consuming the exceptions. I do have some comments on the implementation.
diff --git a/tuna-cmd.py b/tuna-cmd.py
index cf117ec046ff..88ff626766a0 100755
--- a/tuna-cmd.py
+++ b/tuna-cmd.py
@@ -601,13 +601,13 @@ def main():
tuna.include_cpus(cpu_list, get_nr_cpus())
elif o in ("-p", "--priority"):
Did you intend to leave this comment in place?
# Save policy and rtprio for future Actions (e.g.
--run).
- (policy, rtprio) = tuna.get_policy_and_rtprio(a)
if not thread_list:
# For backward compatibility
p_waiting_action = True
Should ou put a try except block here like in the else section? (see
comments in get_policy_and_rtprio)
+ (policy, rtprio) = tuna.get_policy_and_rtprio(a)
else:
try:
- tuna.threads_set_priority(thread_list, a, affect_children)
+ (policy, rtprio) = tuna.threads_set_priority(thread_list, a,
affect_children)
except OSError as err:
print("tuna: %s" % err)
sys.exit(2)
diff --git a/tuna/tuna.py b/tuna/tuna.py
index 8fb42121e2e4..7b0cc1f3fe30 100755
--- a/tuna/tuna.py
+++ b/tuna/tuna.py
@@ -507,21 +507,20 @@ def get_policy_and_rtprio(parm):
rtprio = 0
policy = None
- try:
- cp = tuna_sched.Policy(parms[0])
- except:
- if parms[0].isdigit():
- rtprio = int(parms[0])
- else:
- raise ValueError
Taking this out of the try except block is fine
+ if parms[0].isdigit():
+ rtprio = int(parms[0])
else:
- policy = cp.get_policy()
- if len(parms) > 1:
- rtprio = int(parms[1])
- elif cp.is_rt():
- rtprio = 1
- finally:
- return(policy, rtprio)
Putting a lot of code in the try block is not ideal,
it would be better to identify one or two lines that could cause
exceptions.
You might want to send a separate patch that does nothing but remove the
finally block with no other changes.
+ try:
+ cp = tuna_sched.Policy(parms[0])
+ policy = cp.get_policy()
+ if len(parms) > 1:
+ rtprio = int(parms[1])
+ elif cp.is_rt():
+ rtprio = 1
I suppose this is a matter of taste, but I would prefer that you raised
the err and handled it from the call point. This would be more consistent
to the way tuna is written today.
+ except ValueError as err:
+ print(err)
+ sys.exit(2)
+ return (policy, rtprio)
def thread_filtered(tid, cpus_filtered, show_kthreads, show_uthreads):
if cpus_filtered:
@@ -558,11 +557,8 @@ def thread_set_priority(tid, policy, rtprio):
os.sched_setscheduler(tid, policy, param)
def threads_set_priority(tids, parm, affect_children=False):
- try:
- (policy, rtprio) = get_policy_and_rtprio(parm)
- except ValueError:
- print("tuna: " + _("\"%s\" is unsupported priority
value!") % parms[0])
- return
+
+ (policy, rtprio) = get_policy_and_rtprio(parm)
for tid in tids:
try:
@@ -580,6 +576,7 @@ def threads_set_priority(tids, parm, affect_children=False):
if err.args[0] == errno.ESRCH:
continue
raise err
+ return (policy, rtprio)
class sched_tunings:
def __init__(self, name, pid, policy, rtprio, affinity, percpu):
diff --git a/tuna/tuna_sched.py b/tuna/tuna_sched.py
index de9846bb5fae..8c13b914d13a 100644
--- a/tuna/tuna_sched.py
+++ b/tuna/tuna_sched.py
@@ -51,12 +51,12 @@ class Policy:
can use fifo, FIFO, sched_fifo, SCHED_FIFO, etc
"""
self.policy = None
- policy = policy.upper()
- if policy[:6] != "SCHED_":
- policy = "SCHED_" + policy
- if policy not in list(Policy.SCHED_POLICIES):
- raise OSError
- self.policy = policy
I wouldn't rename policy to policy_str, it's
less readable.
If you had a policy as a number in the same method, it would be okay.
+ policy_str = policy.upper()
+ if policy_str[:6] != "SCHED_":
+ policy_str = "SCHED_" + policy_str
+ if policy_str not in list(Policy.SCHED_POLICIES):
This is kind of nitpicky, but there is a difference between being
unsupported and an invalid priority.
+ raise ValueError("tuna: " +
_("\"%s\" is an unsupported priority value!") % policy)
+ self.policy = policy_str
@classmethod
def num_init(cls, policy):
--
2.27.0