Migration of Process Module #98
Conversation
WalkthroughAdds a new module autils.devel.process implementing extensive external-process utilities (CmdError/CmdInputError, can_sudo, capability inspection, pid/parent/children queries, safe_kill, kill_process_tree, pattern-based termination, process ownership, binary_from_shell_cmd), a CmdResult model, FDDrainer for background FD draining, and a SubProcess wrapper plus high-level helpers (run, system, system_output, getoutput, getstatusoutput). Adds tests/utils.py helpers (logger setup and skip decorators), new unit and functional test modules exercising SubProcess and helpers, documentation entry docs/source/utils.rst, and metadata/devel/process.yml. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.14.8)tests/unit/modules/devel/process.py139-139: Unused function argument: (ARG001) 139-139: Unused function argument: (ARG001) 194-194: Function call with (S604) 202-202: Function call with (S604) 208-208: Function call with (S604) 253-253: Function call with (S604) 261-261: Function call with (S604) 267-267: Function call with (S604) 643-643: Function call with (S604) 727-727: Unused lambda argument: (ARG005) 728-728: Unused lambda argument: (ARG005) 777-777: Unused method argument: (ARG002) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
metadata/process/process.yml (1)
11-14: Consider updating supported platform versions.Fedora 36 (EOL May 2023) and Fedora 37 (EOL December 2023) are end-of-life. Consider updating to currently supported versions (e.g., Fedora 40, 41) or clarifying if this list represents minimum supported versions rather than actively tested platforms.
autils/process/process.py (2)
217-221: Consider catching a more specific exception.The broad
except Exceptioncatch is intentional for robustness, but catching specific exceptions likeOSErrorandProcessLookupErrorwould be more precise and avoid masking unexpected errors.try: os.kill(pid, signal) return True - except Exception: # pylint: disable=W0703 + except (OSError, ProcessLookupError): return False
977-979: Consider using a custom exception for zombie processes.
AssertionErroris typically for programming errors. A custom exception likeZombieProcessErrorwould be more semantically appropriate and easier to catch specifically.tests/unit/modules/process/process.py (2)
30-42: Bareexceptin test script is intentional but could use a comment.The bare
exceptat line 36 inREFUSE_TO_DIEis intentional to ignore all signals for testing timeout behavior. Consider adding a brief comment to clarify this is deliberate.for sig in range(64): try: signal.signal(sig, signal.SIG_IGN) - except: + except Exception: # Intentionally broad - ignore all signal setup failures pass
729-731: Unused lambda parameter is required byassertRaisesAPI.The unused
xparameter inlambda x: result.stdout_textis a known pattern when testing that property access raises an exception. The static analysis warning can be safely ignored, but you could use a more explicit approach:- self.assertRaises(TypeError, lambda x: result.stdout_text) - self.assertRaises(TypeError, lambda x: result.stderr_text) + with self.assertRaises(TypeError): + _ = result.stdout_text + with self.assertRaises(TypeError): + _ = result.stderr_text
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
autils/process/process.py(1 hunks)metadata/process/process.yml(1 hunks)tests/functional/modules/process/process.py(1 hunks)tests/unit/modules/process/process.py(1 hunks)tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/unit/modules/process/process.py (2)
autils/file/path.py (1)
CmdNotFoundError(30-51)autils/process/process.py (22)
run(1023-1045)run(1049-1116)get_pid(1002-1007)stdout_text(501-506)start(608-611)start(838-849)wait(916-981)binary_from_shell_cmd(402-430)get_parent_pid(224-244)get_children_pids(254-292)safe_kill(191-221)CmdError(45-59)get_owner_id(1387-1410)has_capability(137-162)CmdResult(439-514)kill_process_by_pattern(352-373)system(1120-1182)stderr_text(509-514)FDDrainer(517-630)flush(613-630)get_command_output_matching(1413-1432)get_capabilities(105-134)
tests/utils.py (1)
tests/functional/modules/process/process.py (1)
setUp(127-137)
tests/functional/modules/process/process.py (1)
autils/process/process.py (7)
SubProcess(633-1045)run(1023-1045)run(1049-1116)system_output(1186-1255)getstatusoutput(1321-1384)stdout_text(501-506)binary_from_shell_cmd(402-430)
🪛 Ruff (0.14.6)
tests/unit/modules/process/process.py
138-138: Unused function argument: default
(ARG001)
138-138: Unused function argument: check_exec
(ARG001)
193-193: Function call with shell=True parameter identified, security issue
(S604)
201-201: Function call with shell=True parameter identified, security issue
(S604)
207-207: Function call with shell=True parameter identified, security issue
(S604)
252-252: Function call with shell=True parameter identified, security issue
(S604)
260-260: Function call with shell=True parameter identified, security issue
(S604)
266-266: Function call with shell=True parameter identified, security issue
(S604)
646-646: Function call with shell=True parameter identified, security issue
(S604)
730-730: Unused lambda argument: x
(ARG005)
731-731: Unused lambda argument: x
(ARG005)
780-780: Unused method argument: record
(ARG002)
autils/process/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
342-345: Avoid specifying long messages outside the exception class
(TRY003)
430-430: Avoid specifying long messages outside the exception class
(TRY003)
506-506: Avoid specifying long messages outside the exception class
(TRY003)
514-514: Avoid specifying long messages outside the exception class
(TRY003)
736-736: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
737-741: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
765-765: subprocess call: check for execution of untrusted input
(S603)
774-774: Use raise without specifying exception name
Remove exception name
(TRY201)
946-952: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
979-979: Avoid specifying long messages outside the exception class
(TRY003)
1099-1099: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-stability-tests
- GitHub Check: modules-tests
🔇 Additional comments (5)
tests/utils.py (1)
1-77: LGTM!The test utilities are well-structured with proper docstrings. The logging setup, temporary directory management, and skip decorators follow standard unittest patterns.
autils/process/process.py (2)
517-630: LGTM!The
FDDrainerclass correctly handles asynchronous reading from file descriptors, buffering, line-by-line logging, and proper cleanup on flush. The handling of partial lines and background process detection is well-implemented.
1048-1116: LGTM!The
runfunction correctly orchestrates subprocess execution with proper timeout handling, encoding support, and error propagation whenignore_status=False.tests/unit/modules/process/process.py (1)
1-956: Comprehensive test coverage!The unit tests provide excellent coverage of the process module including edge cases for sudo handling, signal management, encoding, timeout behavior, and capability checks. The mocking strategy is well-executed.
tests/functional/modules/process/process.py (1)
1-272: LGTM!Excellent functional test coverage! The tests effectively exercise the process module's real-world behavior including subprocess lifecycle management, environment propagation, encoding handling, concurrent execution, and large output capture. The fake script fixtures are well-designed for controlled testing.
| try: | ||
| with open(proc_stat, "rb") as proc_stat_fp: | ||
| this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) | ||
| except IOError: | ||
| continue |
There was a problem hiding this comment.
Same fragile parsing issue as get_parent_pid.
The /proc/*/stat parsing using split(b" ")[-49] is fragile for process names containing spaces. Apply the same fix using rfind(b")") to locate the command name boundary.
🤖 Prompt for AI Agents
In autils/process/process.py around lines 280-284, the code reads
/proc/<pid>/stat and uses split(b" ")[-49], which breaks when the command name
contains spaces; change to find the ')' that ends the comm field (idx =
proc_stat_fp.read().rfind(b")")), then take the bytes after that boundary (tail
= data[idx+2:]), split tail on whitespace and parse the second token as the
parent PID (this_parent_pid = int(tail.split()[1])); update the try block to use
this robust parsing and keep the same IOError handling.
|
Depends on script and wait module |
|
Waiting on script and wait module to be merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
tests/unit/modules/process/process.py (1)
471-515: Mock target mismatch: tests mocktime.timebut code usestime.monotonic.The tests at lines 471-499 and 501-515 mock
autils.process.process.time.time, but the implementation inautils/process/process.pyline 326 usestime.monotonic(). This means the mocks have no effect on the actual code behavior being tested.Apply this diff to both tests:
@unittest.mock.patch("autils.process.process.safe_kill") @unittest.mock.patch("autils.process.process.get_children_pids") - @unittest.mock.patch("autils.process.process.time.time") + @unittest.mock.patch("autils.process.process.time.monotonic") @unittest.mock.patch("autils.process.process.time.sleep") @unittest.mock.patch("autils.process.process.pid_exists") def test_kill_process_tree_timeout_3s( - self, pid_exists, sleep, p_time, get_children_pids, safe_kill + self, pid_exists, sleep, p_monotonic, get_children_pids, safe_kill ):autils/process/process.py (5)
53-59: PotentialAttributeErrorwhenresultisNone.The
__str__method accessesself.result.stdoutandself.result.stderrwithout checking ifresultisNone. IfCmdErroris instantiated withresult=None, callingstr()on it will raiseAttributeError.def __str__(self): + if self.result is None: + return ( + f"Command '{self.command}' failed.\n" + f"additional_info: {self.additional_text}" + ) return ( f"Command '{self.command}' failed.\nstdout: " f"{self.result.stdout!r}\nstderr: " f"{self.result.stderr!r}\nadditional_info: " f"{self.additional_text}" )
242-244: Fragile/proc/statparsing breaks with spaces in process names.The code splits
/proc/{pid}/statby space and uses a fixed index[-49], which fails when the process name (field 2, in parentheses) contains spaces. The PPID is field 4, but appears after the command name.def get_parent_pid(pid): with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: - parent_pid = proc_stat.read().split(b" ")[-49] - return int(parent_pid) + content = proc_stat.read() + # Find the closing paren of the command name, then parse fields after it + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + # PPID is the 2nd field after the closing paren (state is first) + return int(fields[1])
280-284: Same fragile/proc/statparsing issue asget_parent_pid.Apply the same fix here to handle process names with spaces correctly.
try: with open(proc_stat, "rb") as proc_stat_fp: - this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) + content = proc_stat_fp.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + this_parent_pid = int(fields[1]) except IOError: continue
372-373: Shell metacharacters inpatternare not escaped, risking command injection.The
patternparameter is interpolated directly into the shell command without escaping. If the pattern contains shell metacharacters (e.g.,;,|,$), it could lead to command injection or unexpected behavior.+import shlex + def kill_process_by_pattern(pattern): - cmd = f"pkill -f {pattern}" + cmd = f"pkill -f {shlex.quote(pattern)}" result = run(cmd, ignore_status=True)
460-463: Stale docstring referencesavocado.utilsinstead ofautils.The docstring references
avocado.utils.astring.ENCODINGbut this module has been migrated toautils. Based on line 33, the correct reference should beautils.devel.astring.ENCODING.Similar stale references likely exist at lines 696, 1117, and 1261.
:param encoding: the encoding to use for the text version of stdout and stderr, by default - :data:`avocado.utils.astring.ENCODING` + :data:`autils.devel.astring.ENCODING` :type encoding: str
🧹 Nitpick comments (1)
tests/utils.py (1)
22-26: Clarify parameter type fortemp_dir_prefix.The parameter name
klasssuggests it should be a class, but the implementation accessesklass.__class__.__name__, indicating it expects an instance. Consider either:
- Renaming the parameter to
instanceorobjto match usage- Or changing to accept a class directly and use
klass.__name__-def temp_dir_prefix(klass): +def temp_dir_prefix(instance): """ Returns a standard name for the temp dir prefix used by the tests """ - return f"autils_{klass.__class__.__name__}_" + return f"autils_{instance.__class__.__name__}_"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
autils/process/process.py(1 hunks)metadata/process/process.yml(1 hunks)tests/functional/modules/process/process.py(1 hunks)tests/unit/modules/process/process.py(1 hunks)tests/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- metadata/process/process.yml
🧰 Additional context used
🧬 Code graph analysis (4)
tests/utils.py (1)
tests/functional/modules/process/process.py (1)
setUp(146-156)
tests/functional/modules/process/process.py (1)
autils/process/process.py (12)
SubProcess(654-1072)start(627-631)start(855-865)terminate(885-892)wait(932-1001)stop(1003-1022)run(1051-1072)run(1076-1146)system_output(1217-1285)getstatusoutput(1351-1414)stdout_text(506-517)binary_from_shell_cmd(408-436)
autils/process/process.py (1)
autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
tests/unit/modules/process/process.py (2)
autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)autils/process/process.py (23)
run(1051-1072)run(1076-1146)start(627-631)start(855-865)binary_from_shell_cmd(408-436)get_parent_pid(224-244)get_children_pids(254-292)safe_kill(191-221)CmdError(43-59)get_owner_id(1417-1440)pid_exists(165-188)CmdInputError(62-63)can_sudo(66-102)has_capability(137-162)CmdResult(445-531)system(1150-1213)system_output(1217-1285)getoutput(1289-1347)getstatusoutput(1351-1414)FDDrainer(534-651)flush(633-651)get_command_output_matching(1443-1462)get_capabilities(105-134)
🪛 Ruff (0.14.7)
autils/process/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/modules/process/process.py
138-138: Unused function argument: default
(ARG001)
138-138: Unused function argument: check_exec
(ARG001)
193-193: Function call with shell=True parameter identified, security issue
(S604)
201-201: Function call with shell=True parameter identified, security issue
(S604)
207-207: Function call with shell=True parameter identified, security issue
(S604)
252-252: Function call with shell=True parameter identified, security issue
(S604)
260-260: Function call with shell=True parameter identified, security issue
(S604)
266-266: Function call with shell=True parameter identified, security issue
(S604)
646-646: Function call with shell=True parameter identified, security issue
(S604)
730-730: Unused lambda argument: x
(ARG005)
731-731: Unused lambda argument: x
(ARG005)
780-780: Unused method argument: record
(ARG002)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
autils/process/process.py (5)
43-59: GuardCmdError.__str__againstresult=None.
__str__unconditionally accessesself.result.stdout/stderr. IfCmdErroris ever instantiated withresult=None(the default), this will raiseAttributeErrorwhen the error is stringified, masking the real failure. This matches a prior review.A simple defensive implementation:
def __str__(self): - return ( - f"Command '{self.command}' failed.\nstdout: " - f"{self.result.stdout!r}\nstderr: " - f"{self.result.stderr!r}\nadditional_info: " - f"{self.additional_text}" - ) + if self.result is None: + return ( + f"Command '{self.command}' failed.\n" + f"additional_info: {self.additional_text}" + ) + return ( + f"Command '{self.command}' failed.\nstdout: " + f"{self.result.stdout!r}\nstderr: " + f"{self.result.stderr!r}\nadditional_info: " + f"{self.additional_text}" + )
191-221: Narrow the broadexcept Exceptioninsafe_killtoOSError.
safe_kill()currently does:try: os.kill(pid, signal) return True except Exception: return FalseCatching
Exceptionwill also hide unexpected programming errors (e.g., bad argument types). Sinceos.kill()only raisesOSErrorfor OS‑level failures, you can safely restrict the except clause:try: os.kill(pid, signal) return True - except Exception: # pylint: disable=W0703 + except OSError: return FalseThis keeps the external behavior (False on OS errors) while avoiding masking other bugs.
445-488: Update stale docstring references toavocado.utils.astring.ENCODING.The
CmdResultdocstring still referencesavocado.utils.astring.ENCODING, but this module now importsastringfromautils.devel. The same pattern appears in other docstrings (SubProcess,run,system,system_output).For consistency with the actual imports and to avoid confusion, update these references, e.g.:
- :data:`avocado.utils.astring.ENCODING` + :data:`autils.devel.astring.ENCODING`(or whichever public path you intend to document).
224-245: Fragile/proc/<pid>/statparsing inget_parent_pid.Splitting the entire line and using
split(b" ")[-49]assumes a fixed field count and breaks if the comm field (inside parentheses) contains spaces. A more robust approach is to locate the closing)of the comm field and parse fields after it, as suggested earlier:def get_parent_pid(pid): @@ - with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: - parent_pid = proc_stat.read().split(b" ")[-49] - return int(parent_pid) + with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: + data = proc_stat.read() + # Find the closing parenthesis of the comm field, then parse fields after it + close_paren = data.rfind(b")") + tail = data[close_paren + 2 :] # skip ") " + fields = tail.split() + # Fields after comm: state (1), PPID (2), ... + return int(fields[1])This will handle process names with spaces correctly.
254-292: Apply the same robust parsing toget_children_pids.
get_children_pids()repeats the samesplit(b" ")[-49]pattern when derivingthis_parent_pid, so it has the same failure mode when process names contain spaces. Reuse the robust parsing logic here:- for proc_stat in proc_stats: - try: - with open(proc_stat, "rb") as proc_stat_fp: - this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) - except IOError: - continue + for proc_stat in proc_stats: + try: + with open(proc_stat, "rb") as proc_stat_fp: + data = proc_stat_fp.read() + close_paren = data.rfind(b")") + tail = data[close_paren + 2 :] + fields = tail.split() + this_parent_pid = int(fields[1]) + except IOError: + continueThis keeps behavior but avoids misparsing when command names include spaces.
tests/unit/modules/process/process.py (1)
471-516: Patch the correct time function (time.monotonic) in kill_process_tree timeout tests.
kill_process_tree()now usestime.monotonic(), but these tests still patchtime.time, so the mocks don’t affect behavior and the timing assertions become meaningless. This matches the earlier review note.Update both tests to patch
time.monotonicinstead and adjust the argument name:- @unittest.mock.patch("autils.process.process.safe_kill") - @unittest.mock.patch("autils.process.process.get_children_pids") - @unittest.mock.patch("autils.process.process.time.time") - @unittest.mock.patch("autils.process.process.time.sleep") - @unittest.mock.patch("autils.process.process.pid_exists") - def test_kill_process_tree_timeout_3s( - self, pid_exists, sleep, p_time, get_children_pids, safe_kill - ): + @unittest.mock.patch("autils.process.process.safe_kill") + @unittest.mock.patch("autils.process.process.get_children_pids") + @unittest.mock.patch("autils.process.process.time.monotonic") + @unittest.mock.patch("autils.process.process.time.sleep") + @unittest.mock.patch("autils.process.process.pid_exists") + def test_kill_process_tree_timeout_3s( + self, pid_exists, sleep, p_monotonic, get_children_pids, safe_kill + ): @@ - p_time.side_effect = [ + p_monotonic.side_effect = [ @@ - self.assertLess(p_time.call_count, 10) + self.assertLess(p_monotonic.call_count, 10) @@ - @unittest.mock.patch("autils.process.process.safe_kill") - @unittest.mock.patch("autils.process.process.get_children_pids") - @unittest.mock.patch("autils.process.process.time.time") - @unittest.mock.patch("autils.process.process.time.sleep") - @unittest.mock.patch("autils.process.process.pid_exists") - def test_kill_process_tree_dont_timeout_3s( - self, pid_exists, sleep, p_time, get_children_pids, safe_kill - ): + @unittest.mock.patch("autils.process.process.safe_kill") + @unittest.mock.patch("autils.process.process.get_children_pids") + @unittest.mock.patch("autils.process.process.time.monotonic") + @unittest.mock.patch("autils.process.process.time.sleep") + @unittest.mock.patch("autils.process.process.pid_exists") + def test_kill_process_tree_dont_timeout_3s( + self, pid_exists, sleep, p_monotonic, get_children_pids, safe_kill + ): @@ - p_time.side_effect = [500, 502, 502, 502, 502, 502, 502, 502, 502, 503] + p_monotonic.side_effect = [500, 502, 502, 502, 502, 502, 502, 502, 502, 503] @@ - self.assertLess(p_time.call_count, 10) + self.assertLess(p_monotonic.call_count, 10)
🧹 Nitpick comments (4)
tests/unit/modules/process/process.py (1)
726-731: ClarifyassertRaisesusage intest_cmd_result_stdout_stderr_other_type.The current pattern:
self.assertRaises(TypeError, lambda x: result.stdout_text) self.assertRaises(TypeError, lambda x: result.stderr_text)raises
TypeErrorbecause the lambda signature doesn’t match howassertRaisescalls it, not because of the property access itself. Use a zero-arg callable or context manager to truly test the property behavior:- self.assertRaises(TypeError, lambda x: result.stdout_text) - self.assertRaises(TypeError, lambda x: result.stderr_text) + self.assertRaises(TypeError, lambda: result.stdout_text) + self.assertRaises(TypeError, lambda: result.stderr_text) + # or, more idiomatically: + # with self.assertRaises(TypeError): + # _ = result.stdout_text + # with self.assertRaises(TypeError): + # _ = result.stderr_texttests/functional/modules/process/process.py (1)
171-208: Consider shortening sleeps to reduce functional test wall time (optional).
test_process_start/test_process_stop_*usetime.sleep(3)before stopping the vmstat script. On slow CI this is fine, but you might be able to lower these waits (e.g., 1–2 seconds) without hurting reliability, especially since the tests are already gated byskipOnLevelsInferiorThan(2).autils/process/process.py (2)
356-378:kill_process_by_patternbehavior with multi-word patterns (semantics, not security).Because
run()usesshell=Falseby default andSubProcesssplits the string withshlex.split(), a pattern containing spaces:kill_process_by_pattern("foo bar")produces arguments like
["pkill", "-f", "foo", "bar"], which is not the single patternfoo barpkill expects and can change matching behavior or even cause pkill to complain about extra arguments. There is no shell‑injection risk here (noshell=True), but semantics are surprising.Consider either:
- Documenting that
patternmust be a single token (no spaces), or- Quoting and running via a shell:
-import subprocess +import shlex @@ - cmd = f"pkill -f {pattern}" - result = run(cmd, ignore_status=True) + cmd = f"pkill -f {shlex.quote(pattern)}" + result = run(cmd, ignore_status=True, shell=True)and updating the unit tests accordingly.
932-1001: Timeout handling inSubProcess.waitis robust but a bit subtle.The combination of:
- Optional
threading.Timerfor positive timeouts,- Fallback polling window (1s) when
rcis stillNone,nuke_myself()usingkill_process_treewith a final SIGKILL attempt,- Assertion on zombie processes,
gives a solid safety net around stuck processes. The nested
nuke_myselfclosure rebindingtimeoutfor the message is slightly confusing but functionally fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
autils/process/process.py(1 hunks)metadata/process/process.yml(1 hunks)tests/functional/modules/process/process.py(1 hunks)tests/unit/modules/process/process.py(1 hunks)tests/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- metadata/process/process.yml
- tests/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/functional/modules/process/process.py (1)
autils/process/process.py (7)
SubProcess(654-1072)run(1051-1072)run(1076-1146)system_output(1217-1285)getstatusoutput(1351-1414)stdout_text(506-517)binary_from_shell_cmd(408-436)
tests/unit/modules/process/process.py (1)
autils/process/process.py (32)
run(1051-1072)run(1076-1146)get_pid(1024-1031)SubProcess(654-1072)send_signal(903-918)get_user_id(1033-1040)is_sudo_enabled(1042-1049)stdout_text(506-517)start(627-631)start(855-865)wait(932-1001)binary_from_shell_cmd(408-436)get_parent_pid(224-244)get_children_pids(254-292)safe_kill(191-221)CmdError(43-59)get_owner_id(1417-1440)pid_exists(165-188)CmdInputError(62-63)can_sudo(66-102)has_capability(137-162)CmdResult(445-531)kill_process_by_pattern(356-377)system(1150-1213)system_output(1217-1285)getoutput(1289-1347)getstatusoutput(1351-1414)stderr_text(520-531)FDDrainer(534-651)flush(633-651)get_command_output_matching(1443-1462)get_capabilities(105-134)
autils/process/process.py (1)
autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
🪛 Ruff (0.14.7)
tests/unit/modules/process/process.py
138-138: Unused function argument: default
(ARG001)
138-138: Unused function argument: check_exec
(ARG001)
193-193: Function call with shell=True parameter identified, security issue
(S604)
201-201: Function call with shell=True parameter identified, security issue
(S604)
207-207: Function call with shell=True parameter identified, security issue
(S604)
252-252: Function call with shell=True parameter identified, security issue
(S604)
260-260: Function call with shell=True parameter identified, security issue
(S604)
266-266: Function call with shell=True parameter identified, security issue
(S604)
646-646: Function call with shell=True parameter identified, security issue
(S604)
730-730: Unused lambda argument: x
(ARG005)
731-731: Unused lambda argument: x
(ARG005)
780-780: Unused method argument: record
(ARG002)
autils/process/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (13)
tests/unit/modules/process/process.py (1)
755-851: FDDrainer tests look solid and exercise tricky logging/stream cases.The
FDDrainerTestssuite nicely covers pipe draining, logging, closed streams, handlers withoutfileno(), and replacement of undecodable characters. The coverage here should give good confidence in FDDrainer behavior.tests/functional/modules/process/process.py (3)
145-212: Good functional coverage of core subprocess flows.
ProcessTestexercises real scripts forSubProcess.start/stop/run, timeout handling (including interrupt vs non-interrupt), and basic success cases (fake_uptime). The tmpdir-based script creation and DEFAULT_MODE handling look correct.
215-279: Environment, large-output, and encoding scenarios are well covered.The tests around
envpropagation (test_run_and_system_output_with_environment),getstatusoutputintegration, concurrent subprocesses, large stdout (test_process_with_large_output), and UTF‑8 handling (test_cmdresult_encoding) give good end‑to‑end confidence in therun/system_output/getstatusoutputAPIs.
280-286:binary_from_shell_cmd_realworldnicely complements unit tests.These real‑world strings (env assignments +
/usr/bin/python, and a sudo + bash pipeline) are good sanity checks thatbinary_from_shell_cmdbehaves as expected beyond the synthetic unit tests.autils/process/process.py (9)
505-532:CmdResult.stdout_text/stderr_textbehavior matches tests and is reasonable.The decoding logic (bytes→decode with configured encoding, passthrough for
str,TypeErrorfor anything else) aligns with how the tests exercise these properties, and the default encoding fallback toastring.ENCODINGis consistent with the rest of the module.
533-652: FDDrainer implementation looks correct and is well covered by tests.The threaded
FDDrainercorrectly handles:
- Optional select‑based early exit when
ignore_bg_processesis True.- Buffered line logging with partial lines preserved.
- Safe flushing of various logging handlers (including those without
fileno()or with closed streams), as exercised in the unit tests.No functional issues stand out here.
654-747: SubProcess initialization and representation are sound.
SubProcess.__init__correctly:
- Applies sudo via
_prepend_sudoonly when requested.- Initializes
CmdResultwith the resolved command and encoding.- Merges custom
envwith the current environment when provided.- Sets up logger hierarchy.
__repr__/__str__give helpful diagnostics without side effects. This all lines up with the unit and functional tests.
768-822: Subprocess spawning and FD draining look correct (with expected security tradeoff).
_init_subprocess:
- Uses
shlex.splitwhenshell=Falseand passesshell=self.shellintosubprocess.Popen, which is good for avoiding shell injection when possible.- Initializes
FDDrainerinstances for stdout/stderr with appropriate loggers and starts them before returning.- Uses
time.monotonic()forstart_time, which is appropriate for measuring durations.The
subprocess.Popencall will naturally reflect whatever security posture callers choose viashellandcmdstrings, which is expected for a generic process helper.
1075-1147: High-levelrun()helper is consistent and well-tested.
run()correctly:
- Rejects empty commands via
CmdInputError.- Applies the default encoding.
- Wraps
SubProcesswith timeout / ignore_bg_processes / sudo, and returns aCmdResult.- Raises
CmdErrorwhen exit_status is non-zero or interrupted andignore_statusis False.This is in line with the unit and functional tests (including
test_empty_command, unicode output, env propagation, and timeout behavior).
1149-1414: Wrapperssystem,system_output,getoutput, andgetstatusoutputlook correct.These thin wrappers delegate to
run()with appropriate defaults:
system()returns onlyexit_status.system_output()returns bytes, with configurable newline stripping.getstatusoutput()returns(status, text), trimming a final newline like the legacycommandsmodule.getoutput()just returns theoutputcomponent.The unit tests cover success and failure cases plus newline stripping, so behavior here appears consistent.
1417-1441:get_owner_idbehavior matches tests and Linux/procsemantics.Using
os.stat(f"/proc/{int(pid)}/").st_uidand returningNoneonOSErroraligns with the unit tests and is appropriate for Linux‑only logic. The path format matches whatTestProcessRunpatches in tests.
1443-1462:get_command_output_matchingis simple and behaves as documented.The implementation:
return [line for line in run(command).stdout_text.splitlines() if pattern in line]is straightforward and matches the functional tests that check matching, multiple matches, and no matches. Just be aware that a failing command will raise
CmdErrorhere becauseignore_statusdefaults to False; that appears intentional.
380-405: The type comparison is correct as written.
system_output()returnsstr(notbytes), so the substring check"<defunct>" in proc_nameis valid. The suggested fix usingb"<defunct>"would actually cause aTypeErrorin Python 3. No change is needed for the type comparison itself.Likely an incorrect or invalid review comment.
| def test_get_parent_pid(self): | ||
| stat = b"18405 (bash) S 24139 18405 18405 34818 8056 4210688 9792 170102 0 7 11 4 257 84 20 0 1 0 44336493 235409408 4281 18446744073709551615 94723230367744 94723231442728 140723100226000 0 0 0 65536 3670020 1266777851 0 0 0 17 1 0 0 0 0 0 94723233541456 94723233588580 94723248717824 140723100229613 140723100229623 140723100229623 140723100233710 0" | ||
| with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): | ||
| self.assertTrue(process.get_parent_pid(0), 24139) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the test file
find . -type f -name "process.py" -path "*/tests/unit/modules/process/*"Repository: avocado-framework/aautils
Length of output: 194
🏁 Script executed:
# Get the file and examine the specific lines
wc -l tests/unit/modules/process/process.pyRepository: avocado-framework/aautils
Length of output: 194
🏁 Script executed:
# Read the relevant section of the test file
sed -n '370,385p' tests/unit/modules/process/process.pyRepository: avocado-framework/aautils
Length of output: 194
🏁 Script executed:
# Let's also check the get_parent_pid implementation to understand what it returns
find . -type f -name "*.py" -path "*/modules/process/*" ! -path "*/tests/*" | head -5Repository: avocado-framework/aautils
Length of output: 194
Fix assertion in test_get_parent_pid to validate the parent PID value.
assertTrue(process.get_parent_pid(0), 24139) only checks if the return value is truthy; the second argument becomes the failure message, not a comparison value. The test never validates that the returned PID equals the expected value 24139.
Use assertEqual instead:
- with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)):
- self.assertTrue(process.get_parent_pid(0), 24139)
+ with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)):
+ self.assertEqual(process.get_parent_pid(0), 24139)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_get_parent_pid(self): | |
| stat = b"18405 (bash) S 24139 18405 18405 34818 8056 4210688 9792 170102 0 7 11 4 257 84 20 0 1 0 44336493 235409408 4281 18446744073709551615 94723230367744 94723231442728 140723100226000 0 0 0 65536 3670020 1266777851 0 0 0 17 1 0 0 0 0 0 94723233541456 94723233588580 94723248717824 140723100229613 140723100229623 140723100229623 140723100233710 0" | |
| with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): | |
| self.assertTrue(process.get_parent_pid(0), 24139) | |
| def test_get_parent_pid(self): | |
| stat = b"18405 (bash) S 24139 18405 18405 34818 8056 4210688 9792 170102 0 7 11 4 257 84 20 0 1 0 44336493 235409408 4281 18446744073709551615 94723230367744 94723231442728 140723100226000 0 0 0 65536 3670020 1266777851 0 0 0 17 1 0 0 0 0 0 94723233541456 94723233588580 94723248717824 140723100229613 140723100229623 140723100229623 140723100233710 0" | |
| with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): | |
| self.assertEqual(process.get_parent_pid(0), 24139) |
🤖 Prompt for AI Agents
In tests/unit/modules/process/process.py around lines 375 to 379, the test uses
assertTrue(process.get_parent_pid(0), 24139) which only checks truthiness and
treats 24139 as the failure message; replace this with an equality assertion so
the test actually validates the PID value (use assertEqual with the function
result as the first argument and 24139 as the second).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
tests/unit/modules/process/process.py (2)
376-380: Fixtest_get_parent_pidassertion (currently doesn’t compare values).
This is usingassertTrue(actual, message)rather than validating the PPID.- with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): - self.assertTrue(process.get_parent_pid(0), 24139) + with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): + self.assertEqual(process.get_parent_pid(0), 24139)
472-517: Patchtime.monotonic, nottime.time, in kill-process-tree timeout tests.
Current mocks won’t influence the production logic.autils/process/process.py (4)
43-59:CmdError.__str__can raiseAttributeErrorwhenresultisNone.
This should never raise while formatting an exception.class CmdError(Exception): @@ def __str__(self): + if self.result is None: + return ( + f"Command '{self.command}' failed.\n" + f"additional_info: {self.additional_text}" + ) return ( f"Command '{self.command}' failed.\nstdout: " f"{self.result.stdout!r}\nstderr: " f"{self.result.stderr!r}\nadditional_info: " f"{self.additional_text}" )
224-293:/proc/<pid>/statparsing is fragile for process names containing spaces.
Usingsplit(b" ")[-49]can break on valid process names; parse after the closing).def get_parent_pid(pid): @@ - with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: - parent_pid = proc_stat.read().split(b" ")[-49] - return int(parent_pid) + with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: + content = proc_stat.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2 :].split() + # fields[0] = state, fields[1] = ppid + return int(fields[1]) @@ def get_children_pids(parent_pid, recursive=False): @@ - with open(proc_stat, "rb") as proc_stat_fp: - this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) + with open(proc_stat, "rb") as proc_stat_fp: + content = proc_stat_fp.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2 :].split() + this_parent_pid = int(fields[1])
356-378: Shell injection risk inkill_process_by_pattern(); quote the pattern (or avoid shell).
Right now, shell metacharacters inpatterncan change command behavior.def kill_process_by_pattern(pattern): @@ - cmd = f"pkill -f {pattern}" + cmd = f"pkill -f {shlex.quote(pattern)}" result = run(cmd, ignore_status=True)
445-704: Docstrings still referenceavocado.utils.astring.ENCODING/avocado.utils.process.
Since this isautils, update those docstrings to avoid confusing users.Also applies to: 1076-1123
🧹 Nitpick comments (5)
metadata/process/process.yml (1)
1-18: Metadata file looks correct; consider tightening wording + platform staleness.
descriptioncould be “Functions dedicated to finding and running external commands”. Also, Fedora 36/37 are EOL—if this list is meant to be “actively supported”, consider updating.tests/utils.py (3)
7-20: Preferlogger.addHandler()over mutatinglogger.handlersdirectly.
This is more idiomatic and avoids subtle issues with handler bookkeeping.- if not logger.handlers: - logger.handlers.append(logging.NullHandler()) + if not logger.handlers: + logger.addHandler(logging.NullHandler())
37-43: Callsuper().setUp()/super().tearDown()for safer composition.
Especially helpful if tests later mix in additional base classes.def setUp(self): + super().setUp() prefix = temp_dir_prefix(self) self.tmpdir = tempfile.TemporaryDirectory(prefix=prefix) def tearDown(self): self.tmpdir.cleanup() + super().tearDown()
45-60: Guard against invalidAUTILS_CHECK_LEVELvalues.
A non-integer env var will currently raiseValueErrorduring decoration.- return unittest.skipIf( - int(os.environ.get("AUTILS_CHECK_LEVEL", 0)) < level, + try: + check_level = int(os.environ.get("AUTILS_CHECK_LEVEL", "0")) + except ValueError: + check_level = 0 + return unittest.skipIf( + check_level < level, "Skipping test that take a long time to run, are " "resource intensive or time sensitive", )tests/functional/modules/process/process.py (1)
171-208: Replace fixedsleep(3)with a “wait until output/condition” to reduce flakiness.
CI variance can makesleep(3)+wait(timeout=1)intermittently fail. Consider pollingproc.get_stdout()(or a helper likewait_for) for the expected header, then proceed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
autils/process/process.py(1 hunks)metadata/process/process.yml(1 hunks)tests/functional/modules/process/process.py(1 hunks)tests/unit/modules/process/process.py(1 hunks)tests/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/utils.py (1)
tests/functional/modules/process/process.py (1)
setUp(146-156)
tests/unit/modules/process/process.py (2)
autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)autils/process/process.py (28)
run(1051-1072)run(1076-1146)get_pid(1024-1031)SubProcess(654-1072)send_signal(903-918)get_user_id(1033-1040)is_sudo_enabled(1042-1049)stdout_text(506-517)start(627-631)start(855-865)wait(932-1001)get_parent_pid(224-244)get_children_pids(254-292)CmdError(43-59)get_owner_id(1417-1440)pid_exists(165-188)CmdInputError(62-63)has_capability(137-162)CmdResult(445-531)system(1150-1213)system_output(1217-1285)getoutput(1289-1347)getstatusoutput(1351-1414)stderr_text(520-531)FDDrainer(534-651)flush(633-651)get_command_output_matching(1443-1462)get_capabilities(105-134)
autils/process/process.py (1)
autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
🪛 Ruff (0.14.8)
tests/unit/modules/process/process.py
139-139: Unused function argument: default
(ARG001)
139-139: Unused function argument: check_exec
(ARG001)
194-194: Function call with shell=True parameter identified, security issue
(S604)
202-202: Function call with shell=True parameter identified, security issue
(S604)
208-208: Function call with shell=True parameter identified, security issue
(S604)
253-253: Function call with shell=True parameter identified, security issue
(S604)
261-261: Function call with shell=True parameter identified, security issue
(S604)
267-267: Function call with shell=True parameter identified, security issue
(S604)
643-643: Function call with shell=True parameter identified, security issue
(S604)
727-727: Unused lambda argument: x
(ARG005)
728-728: Unused lambda argument: x
(ARG005)
777-777: Unused method argument: record
(ARG002)
autils/process/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
tests/functional/modules/process/process.py (1)
145-169: Script scaffolding looks solid.
Tempdir isolation + explicit chmod makes these tests nicely self-contained.
| def safe_kill(pid, signal): # pylint: disable=W0621 | ||
| """Attempt to send a signal to a process that may or may not exist. | ||
|
|
||
| This function safely sends a signal to a process, handling cases where | ||
| the process might not exist or require elevated privileges. | ||
|
|
||
| :param pid: The process ID to send the signal to. | ||
| :type pid: int | ||
| :param signal: The signal number to send (e.g., signal.SIGTERM). | ||
| :type signal: int | ||
| :return: True if signal was sent successfully, False otherwise. | ||
| :rtype: bool | ||
|
|
||
| Example:: | ||
|
|
||
| >>> safe_kill(1234, signal.SIGTERM) | ||
| True | ||
| """ | ||
| if not get_owner_id(int(pid)): | ||
| kill_cmd = f"kill -{int(signal)} {int(pid)}" | ||
| try: | ||
| run(kill_cmd, sudo=True) | ||
| return True | ||
| except CmdError: | ||
| return False | ||
|
|
||
| try: | ||
| os.kill(pid, signal) | ||
| return True | ||
| except Exception: # pylint: disable=W0703 | ||
| return False | ||
|
|
There was a problem hiding this comment.
safe_kill() conflates “PID missing” with “root-owned”, and catches overly broad exceptions.
if not get_owner_id(...) is true for both 0 (root) and None (no such pid). Also prefer catching OSError from os.kill().
def safe_kill(pid, signal): # pylint: disable=W0621
@@
- if not get_owner_id(int(pid)):
+ owner_id = get_owner_id(int(pid))
+ if owner_id == 0:
kill_cmd = f"kill -{int(signal)} {int(pid)}"
try:
run(kill_cmd, sudo=True)
return True
except CmdError:
return False
@@
try:
os.kill(pid, signal)
return True
- except Exception: # pylint: disable=W0703
+ except OSError:
return False🧰 Tools
🪛 Ruff (0.14.8)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In autils/process/process.py around lines 191 to 222, safe_kill incorrectly
treats owner_id==0 (root) the same as owner_id==None (no such PID) and it
catches a too-broad Exception from os.kill; change the owner check to explicitly
test for None to detect missing PIDs, treat owner_id==0 as a root-owned process
(use the sudo kill branch for root), ensure pid and signal are int before
building/issuing the kill command, and replace the broad except Exception with
except OSError to only catch expected OS-level errors and return False on
failure.
| def test_cmd_result_stdout_stderr_other_type(self): | ||
| result = process.CmdResult() | ||
| result.stdout = None | ||
| result.stderr = None | ||
| self.assertRaises(TypeError, lambda x: result.stdout_text) | ||
| self.assertRaises(TypeError, lambda x: result.stderr_text) | ||
|
|
There was a problem hiding this comment.
assertRaises(TypeError, lambda x: ...) is testing the wrong failure mode.
These lambdas require an argument, so assertRaises will raise TypeError due to the callable signature, not due to stdout_text/stderr_text.
- self.assertRaises(TypeError, lambda x: result.stdout_text)
- self.assertRaises(TypeError, lambda x: result.stderr_text)
+ self.assertRaises(TypeError, lambda: result.stdout_text)
+ self.assertRaises(TypeError, lambda: result.stderr_text)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_cmd_result_stdout_stderr_other_type(self): | |
| result = process.CmdResult() | |
| result.stdout = None | |
| result.stderr = None | |
| self.assertRaises(TypeError, lambda x: result.stdout_text) | |
| self.assertRaises(TypeError, lambda x: result.stderr_text) | |
| def test_cmd_result_stdout_stderr_other_type(self): | |
| result = process.CmdResult() | |
| result.stdout = None | |
| result.stderr = None | |
| self.assertRaises(TypeError, lambda: result.stdout_text) | |
| self.assertRaises(TypeError, lambda: result.stderr_text) |
🧰 Tools
🪛 Ruff (0.14.8)
727-727: Unused lambda argument: x
(ARG005)
728-728: Unused lambda argument: x
(ARG005)
🤖 Prompt for AI Agents
In tests/unit/modules/process/process.py around lines 723 to 729, the test uses
lambdas expecting an argument so assertRaises triggers a TypeError from the
lambda signature rather than from accessing stdout_text/stderr_text; change the
calls to use zero-argument callables that access the properties (e.g., lambda:
result.stdout_text and lambda: result.stderr_text) so assertRaises(TypeError,
...) checks the property access failure mode correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
tests/unit/modules/process/process.py (3)
376-380: Fix assertion -assertTruedoesn't compare values.Line 379 uses
assertTrue(process.get_parent_pid(0), 24139)which only checks truthiness. The second argument24139becomes the failure message, not a comparison value. The test never validates the actual PID.Apply this diff:
- with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): - self.assertTrue(process.get_parent_pid(0), 24139) + with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): + self.assertEqual(process.get_parent_pid(0), 24139)
472-500: Mock targetstime.timebut code usestime.monotonic.Line 474 mocks
autils.process.process.time.timebutkill_process_treeusestime.monotonic()(line 326 in process.py). The mock has no effect on the actual code.Apply this diff:
@unittest.mock.patch("autils.process.process.safe_kill") @unittest.mock.patch("autils.process.process.get_children_pids") - @unittest.mock.patch("autils.process.process.time.time") + @unittest.mock.patch("autils.process.process.time.monotonic") @unittest.mock.patch("autils.process.process.time.sleep") @unittest.mock.patch("autils.process.process.pid_exists") def test_kill_process_tree_timeout_3s( - self, pid_exists, sleep, p_time, get_children_pids, safe_kill + self, pid_exists, sleep, p_monotonic, get_children_pids, safe_kill ):Also apply to
test_kill_process_tree_dont_timeout_3sat lines 502-516.
723-729: Lambda signature prevents testing the intended failure mode.Lines 727-728 use
lambda x:butassertRaisescalls with no arguments, causingTypeErrorfrom the lambda signature rather than from the property access being tested.Apply this diff:
- self.assertRaises(TypeError, lambda x: result.stdout_text) - self.assertRaises(TypeError, lambda x: result.stderr_text) + self.assertRaises(TypeError, lambda: result.stdout_text) + self.assertRaises(TypeError, lambda: result.stderr_text)autils/process/process.py (8)
53-59:AttributeErrorrisk whenresultis None.Lines 56-57 access
self.result.stdoutandself.result.stderrwithout checking ifself.resultis None, which will raiseAttributeError.Apply this diff:
def __str__(self): + if self.result is None: + return ( + f"Command '{self.command}' failed.\n" + f"additional_info: {self.additional_text}" + ) return ( f"Command '{self.command}' failed.\nstdout: " f"{self.result.stdout!r}\nstderr: " f"{self.result.stderr!r}\nadditional_info: " f"{self.additional_text}" )
66-103: Bytes/str mismatch causescan_sudo()to fail.Line 98 compares
system_output(...).strip()(returns bytes) to string"0", which always fails. Must compare tob"0"or decode the bytes first.Apply this diff:
- if system_output("id -u", ignore_status=True, sudo=True).strip() == "0": + if system_output("id -u", ignore_status=True, sudo=True).strip() == b"0": return True
191-222:safe_kill()conflates root (0) with missing PID (None) and catches overly broad exceptions.Line 209:
if not get_owner_id(...)is true for both0(root) andNone(no such PID), causing incorrect logic.
Line 220: Catching bareExceptionmasks unexpected errors; should catchOSError.Apply this diff:
def safe_kill(pid, signal): - if not get_owner_id(int(pid)): + owner_id = get_owner_id(int(pid)) + if owner_id == 0: kill_cmd = f"kill -{int(signal)} {int(pid)}" try: run(kill_cmd, sudo=True) return True except CmdError: return False + if owner_id is None: + return False try: os.kill(pid, signal) return True - except Exception: + except OSError: return False
242-244: Fragile/proc/statparsing breaks with spaces in process names.Line 243 uses
split(b" ")[-49]which breaks when the process name (field 2, in parentheses) contains spaces. Parse after the closing parenthesis instead.Apply this diff:
def get_parent_pid(pid): with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: - parent_pid = proc_stat.read().split(b" ")[-49] - return int(parent_pid) + content = proc_stat.read() + # Find closing paren of command name, parse fields after + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + # PPID is field 4 (2nd after state) + return int(fields[1])
280-284: Same fragile parsing issue asget_parent_pid.Line 282 uses
split(b" ")[-49]which breaks with spaces in process names. Apply the same robust parsing usingrfind(b")").Apply this diff:
try: with open(proc_stat, "rb") as proc_stat_fp: - this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) + content = proc_stat_fp.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + this_parent_pid = int(fields[1]) except IOError: continue
356-378: Shell injection risk - pattern not escaped.Line 372 interpolates
patterndirectly into a shell command without escaping, allowing shell metacharacter interpretation and potential command injection.Apply this diff:
+import shlex + def kill_process_by_pattern(pattern): - cmd = f"pkill -f {pattern}" + cmd = f"pkill -f {shlex.quote(pattern)}" result = run(cmd, ignore_status=True)
380-406: Bytes/str mismatch in defunct check.Line 402 checks
"<defunct>" in proc_namebutsystem_output()returns bytes. Compare tob"<defunct>"or decode first.Apply this diff:
proc_name = system_output(cmd, ignore_status=True, verbose=False) - if "<defunct>" in proc_name: + if b"<defunct>" in proc_name: defunct = True break
460-463: Stale docstring references toavocado.utils.Line 462 (and similar at lines 696, 1117, 1191, 1261) references
avocado.utils.astring.ENCODINGbut the module is nowautils. Update all docstring references.Update docstrings to reference
autils.astring.ENCODINGinstead ofavocado.utils.astring.ENCODING.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
autils/process/process.py(1 hunks)metadata/process/process.yml(1 hunks)tests/functional/modules/process/process.py(1 hunks)tests/unit/modules/process/process.py(1 hunks)tests/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- metadata/process/process.yml
🧰 Additional context used
🧬 Code graph analysis (1)
autils/process/process.py (2)
autils/devel/wait.py (1)
wait_for(15-104)autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
🪛 Ruff (0.14.8)
autils/process/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/modules/process/process.py
139-139: Unused function argument: default
(ARG001)
139-139: Unused function argument: check_exec
(ARG001)
194-194: Function call with shell=True parameter identified, security issue
(S604)
202-202: Function call with shell=True parameter identified, security issue
(S604)
208-208: Function call with shell=True parameter identified, security issue
(S604)
253-253: Function call with shell=True parameter identified, security issue
(S604)
261-261: Function call with shell=True parameter identified, security issue
(S604)
267-267: Function call with shell=True parameter identified, security issue
(S604)
643-643: Function call with shell=True parameter identified, security issue
(S604)
727-727: Unused lambda argument: x
(ARG005)
728-728: Unused lambda argument: x
(ARG005)
777-777: Unused method argument: record
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-stability-tests
- GitHub Check: modules-tests
🔇 Additional comments (5)
tests/utils.py (2)
7-20: LGTM! Logger setup prevents handler warnings.The function correctly adds NullHandler to prevent "No handlers could be found" warnings during test execution.
29-43: LGTM! TestCaseTmpDir provides clean temp directory management.The implementation correctly manages temp directory lifecycle with proper cleanup.
tests/functional/modules/process/process.py (3)
145-169: LGTM! Test setup creates executable scripts correctly.The setUp and
_create_scripthelper properly create executable test scripts with appropriate permissions.
238-253: LGTM! Concurrent subprocess test validates proper isolation.The test correctly validates that multiple subprocess instances run independently and capture their respective outputs.
254-287: LGTM! Tests cover large output, encoding, and command parsing.The tests validate critical scenarios including complete large output capture, proper UTF-8 encoding handling, and accurate binary extraction from shell commands.
richtja
left a comment
richtja
left a comment
There was a problem hiding this comment.
Hi @harvey0100, can you please fix the CI issues? It looks like that is related to some wrong imports.
Another thing is that IMO the process utility can under the devel module instead of its own.
Also, there are some differences between this and avocado version:
diff autils/process/process.py ../avocado/avocado/utils/process.py
33,35c33,34
< from autils.devel import astring
< from autils.devel.wait import wait_for
< from autils.file import path
---
> from avocado.utils import astring, path
> from avocado.utils.wait import wait_for
98c97
< if system_output("id -u", ignore_status=True, sudo=True).strip() == "0":
---
> if getoutput("id -u", sudo=True).strip() == "0":
And last part, I am missing the documentation here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
autils/devel/process.py (7)
66-103: Bytes/str comparison issue incan_sudo().
system_output()returnsbytes, so comparing.strip() == "0"will always be False. This was flagged in a previous review.Apply this fix:
- if system_output("id -u", ignore_status=True, sudo=True).strip() == "0": + if system_output("id -u", ignore_status=True, sudo=True).strip() == b"0": return True
191-222:safe_kill()has logic issue and catches overly broad exception.
if not get_owner_id(int(pid))is true for both0(root) andNone(no such PID), conflating two cases.- Catching bare
Exceptionmasks unexpected errors; preferOSError.This was flagged in a previous review.
Apply this fix:
def safe_kill(pid, signal): # pylint: disable=W0621 - if not get_owner_id(int(pid)): + owner_id = get_owner_id(int(pid)) + if owner_id == 0: kill_cmd = f"kill -{int(signal)} {int(pid)}" try: run(kill_cmd, sudo=True) return True except CmdError: return False try: os.kill(pid, signal) return True - except Exception: # pylint: disable=W0703 + except OSError: return False
242-244: Fragile/proc/statparsing when process names contain spaces.Using
split(b" ")[-49]breaks if the command name (field 2, enclosed in parentheses) contains spaces. This was flagged in a previous review.Apply this fix:
def get_parent_pid(pid): with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: - parent_pid = proc_stat.read().split(b" ")[-49] - return int(parent_pid) + content = proc_stat.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + return int(fields[1]) # PPID is field 4, index 1 after state
277-292: Same fragile/proc/statparsing issue inget_children_pids().The parsing at line 282 uses the same fragile
split(b" ")[-49]pattern. This was flagged in a previous review.Apply this fix:
try: with open(proc_stat, "rb") as proc_stat_fp: - this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) + content = proc_stat_fp.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + this_parent_pid = int(fields[1]) except IOError: continue
356-378: Shell metacharacters in pattern are not escaped inkill_process_by_pattern().The
patternparameter is interpolated directly into the command string without escaping, which could cause unexpected behavior or command injection. This was flagged in a previous review.Apply this fix:
def kill_process_by_pattern(pattern): - cmd = f"pkill -f {pattern}" + cmd = f"pkill -f {shlex.quote(pattern)}" result = run(cmd, ignore_status=True)
380-406:process_in_ptree_is_defunct()will TypeError due to bytes/str mismatch.
system_output()returnsbytes, so checkingif "<defunct>" in proc_namewill raise a TypeError. This was flagged in a previous review.Apply this fix:
proc_name = system_output(cmd, ignore_status=True, verbose=False) - if "<defunct>" in proc_name: + if b"<defunct>" in proc_name: defunct = True
460-464: Stale docstring references toavocado.utils.The docstring references
avocado.utils.astring.ENCODINGbut this module is inautils. This was flagged in a previous review.Update to reference
autils.devel.astring.ENCODINGat lines 462, 696, 1117, and 1191.tests/unit/modules/devel/process.py (2)
376-380: Fix assertion intest_get_parent_pidto validate the parent PID value.
assertTrue(process.get_parent_pid(0), 24139)only checks if the return value is truthy; the second argument becomes the failure message, not a comparison value. This was flagged in a previous review.Apply this fix:
with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): - self.assertTrue(process.get_parent_pid(0), 24139) + self.assertEqual(process.get_parent_pid(0), 24139)
686-730: Fix lambda signature intest_cmd_result_stdout_stderr_other_type.The lambdas
lambda x: result.stdout_textrequire an argument, soassertRaiseswill raiseTypeErrordue to the callable signature, not due to property access. This was flagged in a previous review.Apply this fix:
result.stdout = None result.stderr = None - self.assertRaises(TypeError, lambda x: result.stdout_text) - self.assertRaises(TypeError, lambda x: result.stderr_text) + self.assertRaises(TypeError, lambda: result.stdout_text) + self.assertRaises(TypeError, lambda: result.stderr_text)
🧹 Nitpick comments (3)
autils/devel/process.py (2)
903-919: Suppress overly broad exception insend_signal().Line 915 uses
contextlib.suppress(Exception)which masks all exceptions. Consider catching specific exceptions (CmdError,OSError).for pid in pids: kill_cmd = f"kill -{int(sig)} {int(pid)}" - with contextlib.suppress(Exception): + with contextlib.suppress(CmdError, OSError): run(kill_cmd, sudo=True)
1351-1415: Potential IndexError ingetstatusoutput()for empty output.Line 1412 accesses
text[-1:]which is safe, but the pattern could be simplified.The current implementation is safe but could be slightly cleaner:
text = cmd_result.stdout_text sts = cmd_result.exit_status - if text[-1:] == "\n": - text = text[:-1] + text = text.rstrip("\n") return (sts, text)tests/unit/modules/devel/process.py (1)
31-44: Bare except clause inREFUSE_TO_DIEtest script.The script uses a bare
except:clause at line 37 which catches all exceptions includingKeyboardInterruptandSystemExit. While this is intentional for the test scenario (to make the process hard to kill), consider documenting this explicitly.for sig in range(64): try: signal.signal(sig, signal.SIG_IGN) - except: + except Exception: # Intentionally broad to ignore all signal errors pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
autils/devel/process.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/devel/process.yml(1 hunks)tests/functional/modules/devel/process.py(1 hunks)tests/unit/modules/devel/process.py(1 hunks)tests/utils.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/utils.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/functional/modules/devel/process.py (1)
autils/devel/process.py (13)
SubProcess(654-1072)start(627-631)start(855-865)terminate(885-892)wait(932-1001)get_stdout(867-874)stop(1003-1022)run(1051-1072)run(1076-1146)system_output(1217-1285)getstatusoutput(1351-1414)stdout_text(506-517)binary_from_shell_cmd(408-436)
autils/devel/process.py (2)
autils/devel/wait.py (1)
wait_for(15-104)autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
🪛 Ruff (0.14.8)
tests/unit/modules/devel/process.py
139-139: Unused function argument: default
(ARG001)
139-139: Unused function argument: check_exec
(ARG001)
194-194: Function call with shell=True parameter identified, security issue
(S604)
202-202: Function call with shell=True parameter identified, security issue
(S604)
208-208: Function call with shell=True parameter identified, security issue
(S604)
253-253: Function call with shell=True parameter identified, security issue
(S604)
261-261: Function call with shell=True parameter identified, security issue
(S604)
267-267: Function call with shell=True parameter identified, security issue
(S604)
643-643: Function call with shell=True parameter identified, security issue
(S604)
727-727: Unused lambda argument: x
(ARG005)
728-728: Unused lambda argument: x
(ARG005)
777-777: Unused method argument: record
(ARG002)
autils/devel/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (26)
autils/devel/process.py (12)
1-41: LGTM - Module setup and imports.The imports are appropriate for the subprocess management functionality. The license header and module docstring are properly defined.
43-64: LGTM - Exception classes are well-defined.
CmdErrorandCmdInputErrorprovide clear exception types for command execution failures and invalid input.
105-135: LGTM - Capability utilities.
get_capabilities()andhas_capability()correctly handle both the modern and legacygetpcapsoutput formats.
165-189: LGTM -pid_exists()implementation.Correctly uses
os.kill(pid, 0)to check process existence and properly handlesESRCHto indicate non-existence.
534-652: LGTM -FDDrainerclass implementation.The file descriptor draining logic is well-implemented with proper thread handling, logging support, and cleanup via
flush().
654-767: LGTM -SubProcessclass initialization and helper methods.The class properly handles sudo prepending, environment variable merging, and subprocess initialization with appropriate error handling.
768-828: LGTM -_init_subprocessand drainer setup.The subprocess initialization with signal handler and FDDrainer setup for stdout/stderr is correctly implemented.
932-1002: LGTM -wait()method with timeout handling.The wait implementation properly handles timeout scenarios with SIGTERM escalation to SIGKILL, and correctly detects zombie processes.
1076-1147: LGTM -run()function.The high-level run function properly validates input, creates the subprocess, and handles error propagation based on
ignore_status.
1150-1214: LGTM -system()function.A clean wrapper that returns exit status from the underlying
run()call.
1217-1286: LGTM -system_output()function.Correctly handles trailing newline stripping and returns raw bytes as documented.
1417-1463: LGTM -get_owner_id()andget_command_output_matching().Both utility functions are correctly implemented with proper error handling and clear documentation.
metadata/devel/process.yml (1)
1-19: LGTM - Metadata file is well-structured.The metadata correctly describes the process module with appropriate categorization, maintainer information, supported platforms, and test file references.
tests/functional/modules/devel/process.py (6)
1-20: LGTM - Imports and permission constants.The imports and
DEFAULT_MODEpermission constant are appropriately defined for the test utilities.
21-143: LGTM - Test script templates.The mock scripts (
FAKE_VMSTAT_CONTENTS,FAKE_UPTIME_CONTENTS, etc.) are well-designed for testing various process scenarios including signal handling, environment variables, and unicode output.
145-170: LGTM - Test class setup and helper methods.The
ProcessTestclass properly sets up temporary script files and provides a clean_create_scripthelper for test script generation.
171-214: LGTM - Process start/stop tests.The tests for
process.start(),stop()with and without interruption are correctly structured with appropriate assertions and skip decorators for slower tests.
215-253: LGTM - Environment and concurrent subprocess tests.Tests properly verify environment variable propagation and concurrent subprocess execution with per-instance output validation.
254-287: LGTM - Large output, encoding, and command parsing tests.The tests verify complete capture of large outputs, proper UTF-8 encoding handling, and real-world command pattern parsing through
binary_from_shell_cmd.tests/unit/modules/devel/process.py (7)
1-30: LGTM - Test module setup.Imports, logger setup, and the
probe_binaryhelper function are appropriate for the test utilities.
46-137: LGTM -TestSubProcessclass.The tests for
send_signal(),get_user_id(), andis_sudo_enabled()are well-structured with proper mocking.
139-150: Unused function arguments inmock_fail_find_cmd.Static analysis flagged
defaultandcheck_execas unused. This is expected since the mock is intentionally ignoring these arguments to simulate a failure. No action needed.
152-343: LGTM -TestProcessRunclass.Comprehensive tests for subprocess command construction with/without sudo, shell mode, and timeout handling. The mocking approach is appropriate for unit testing.
463-543: LGTM -kill_process_treetests.The tests cover various timeout scenarios (no wait, timeout with failure, success, infinity) and child process handling with appropriate mocking.
544-684: LGTM - Miscellaneous process utility tests.Tests for
can_sudo,has_capability,pid_exists,kill_process_by_pattern,system,system_output,getoutput, andgetstatusoutputare comprehensive and properly structured.
731-950: LGTM -CmdErrorTests,FDDrainerTests,GetCommandOutputPattern, andGetCapabilitiestest classes.These test classes are well-structured with comprehensive coverage of edge cases including encoding issues, handler flushing, and capability parsing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
autils/devel/process.py (7)
98-100:can_sudo()bytes/str comparison bug remains unfixed.
system_output()returnsbytes, so comparing.strip() == "0"always fails. This was flagged in a previous review.- if system_output("id -u", ignore_status=True, sudo=True).strip() == "0": + if system_output("id -u", ignore_status=True, sudo=True).strip() == b"0": return True
209-221:safe_kill()logic and exception handling issues remain unfixed.Two issues flagged in previous review:
if not get_owner_id(...)conflates owner_id==0 (root) with owner_id==None (no such PID)- Line 220 catches bare
Exceptioninstead ofOSErrordef safe_kill(pid, signal): # pylint: disable=W0621 - if not get_owner_id(int(pid)): + owner_id = get_owner_id(int(pid)) + if owner_id == 0: kill_cmd = f"kill -{int(signal)} {int(pid)}" try: run(kill_cmd, sudo=True) return True except CmdError: return False try: os.kill(pid, signal) return True - except Exception: # pylint: disable=W0703 + except OSError: return False
242-244: Fragile/proc/statparsing remains unfixed.This was flagged in a previous review. The current approach using
split(b" ")[-49]breaks if the process name contains spaces.
280-284: Same fragile/proc/statparsing issue inget_children_pids.Previously flagged - uses the same fragile
split(b" ")[-49]approach.
372-373: Shell metacharacters in pattern are not escaped.Previously flagged - the
patternis interpolated directly without escaping.def kill_process_by_pattern(pattern): - cmd = f"pkill -f {pattern}" + cmd = f"pkill -f {shlex.quote(pattern)}" result = run(cmd, ignore_status=True)
401-402:process_in_ptree_is_defunct()bytes/str mismatch remains unfixed.
system_output()returnsbytes, so checking"<defunct>" in proc_namewill fail.proc_name = system_output(cmd, ignore_status=True, verbose=False) - if "<defunct>" in proc_name: + if b"<defunct>" in proc_name: defunct = True
460-463: Stale docstring references toavocado.utilsremain.The docstring references
avocado.utils.astring.ENCODINGbut this module is inautils. Similar stale references exist at lines 696, 1117, and 1121.tests/utils.py (1)
56-61: Missing error handling for non-numericAUTILS_CHECK_LEVEL.If
AUTILS_CHECK_LEVELis set to a non-numeric value,int()raisesValueErrorduring decorator application, crashing test collection. This was flagged in a previous review.+def _get_check_level(): + try: + return int(os.environ.get("AUTILS_CHECK_LEVEL", 0)) + except (ValueError, TypeError): + return 0 + def skipOnLevelsInferiorThan(level): ... return unittest.skipIf( - int(os.environ.get("AUTILS_CHECK_LEVEL", 0)) < level, + _get_check_level() < level, "Skipping test that take a long time to run, are " "resource intensive or time sensitive", )tests/unit/modules/devel/process.py (2)
372-375: Fix assertion intest_get_parent_pidto validate the parent PID value.
assertTrue(process.get_parent_pid(0), 24139)only checks if the return value is truthy; the second argument becomes the failure message. This was flagged in a previous review.with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): - self.assertTrue(process.get_parent_pid(0), 24139) + self.assertEqual(process.get_parent_pid(0), 24139)
719-724: Lambda signature causes wrong TypeError source.
assertRaises(TypeError, lambda x: result.stdout_text)raisesTypeErrordue to missing argument, not from the property access. This was flagged in a previous review.- self.assertRaises(TypeError, lambda x: result.stdout_text) - self.assertRaises(TypeError, lambda x: result.stderr_text) + self.assertRaises(TypeError, lambda: result.stdout_text) + self.assertRaises(TypeError, lambda: result.stderr_text)
🧹 Nitpick comments (1)
autils/devel/process.py (1)
915-916: Overly broad exception suppression insend_signal.
contextlib.suppress(Exception)masks all errors when sending signals via sudo, including unexpected exceptions that might indicate bugs. Consider suppressing only expected exceptions.- with contextlib.suppress(Exception): + with contextlib.suppress(process.CmdError, OSError): run(kill_cmd, sudo=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
autils/devel/process.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/devel/process.yml(1 hunks)tests/functional/modules/devel/process.py(1 hunks)tests/unit/modules/devel/process.py(1 hunks)tests/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/source/utils.rst
- metadata/devel/process.yml
🧰 Additional context used
🧬 Code graph analysis (2)
tests/functional/modules/devel/process.py (2)
tests/utils.py (3)
TestCaseTmpDir(29-43)skipOnLevelsInferiorThan(46-61)setUp(37-40)autils/devel/process.py (13)
SubProcess(654-1072)start(627-631)start(855-865)terminate(885-892)wait(932-1001)get_stdout(867-874)stop(1003-1022)run(1051-1072)run(1076-1146)system_output(1217-1285)getstatusoutput(1351-1414)stdout_text(506-517)binary_from_shell_cmd(408-436)
autils/devel/process.py (2)
autils/devel/wait.py (1)
wait_for(15-104)autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
🪛 Ruff (0.14.8)
tests/unit/modules/devel/process.py
135-135: Unused function argument: default
(ARG001)
135-135: Unused function argument: check_exec
(ARG001)
190-190: Function call with shell=True parameter identified, security issue
(S604)
198-198: Function call with shell=True parameter identified, security issue
(S604)
204-204: Function call with shell=True parameter identified, security issue
(S604)
249-249: Function call with shell=True parameter identified, security issue
(S604)
257-257: Function call with shell=True parameter identified, security issue
(S604)
263-263: Function call with shell=True parameter identified, security issue
(S604)
639-639: Function call with shell=True parameter identified, security issue
(S604)
723-723: Unused lambda argument: x
(ARG005)
724-724: Unused lambda argument: x
(ARG005)
773-773: Unused method argument: record
(ARG002)
autils/devel/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: modules-tests
- GitHub Check: api-stability-tests
🔇 Additional comments (12)
autils/devel/process.py (1)
1-36: LGTM - Module structure and imports are well organized.The module imports are properly organized, and the dependencies on
autils.develandautils.fileare appropriate for this process execution toolkit.tests/utils.py (2)
7-19: LGTM - Logger setup prevents handler warnings.The
setup_autils_loggersfunction correctly addsNullHandlerto prevent "No handlers could be found" warnings during test execution.
64-78: LGTM - Path existence skip decorator is well implemented.Clean implementation of a conditional skip decorator based on filesystem path availability.
tests/functional/modules/devel/process.py (5)
1-19: LGTM - Imports and permission constants are correct.The
DEFAULT_MODEusing stat constants correctly represents 0775 permissions for executable scripts.
21-117: Well-designed fake vmstat for timeout testing.The
FakeVMStatclass with configurableinterrupt_delayenables testing both graceful termination and timeout scenarios effectively.
145-169: LGTM - Test setup and helper methods are well structured.The
setUpmethod properly creates test scripts with correct permissions, and_create_scriptis a clean helper for creating additional scripts with interpreter substitution.
238-252: LGTM - Concurrent subprocess test is well designed.The test properly verifies that multiple SubProcess instances can run concurrently and produce correct per-process output.
254-270: LGTM - Large output test validates complete capture.Good verification that the full 1000 lines are captured with correct formatting on first and last lines.
tests/unit/modules/devel/process.py (4)
1-14: LGTM - Test imports and logger setup are correct.Proper use of
setup_autils_loggers()at module level ensures loggers are configured before tests run.
42-91: LGTM - SubProcess signal sending tests are thorough.Good coverage of both normal signal sending and exception handling paths when sudo is enabled.
748-813: LGTM - FDDrainer tests provide excellent coverage.Thorough testing of pipe draining, logging integration, handler edge cases, and character encoding replacement.
867-945: LGTM - Capability parsing tests cover both formats.Good coverage of current and legacy getpcaps output formats, plus failure cases.
| time.sleep(3) | ||
| proc.stop(4) | ||
| result = proc.result | ||
| self.assertFalse(result.interrupted, "Process was interrupted to early") |
There was a problem hiding this comment.
Typo in assertion message.
Minor typo: "to early" should be "too early".
- self.assertFalse(result.interrupted, "Process was interrupted to early")
+ self.assertFalse(result.interrupted, "Process was interrupted too early")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.assertFalse(result.interrupted, "Process was interrupted to early") | |
| self.assertFalse(result.interrupted, "Process was interrupted too early") |
🤖 Prompt for AI Agents
In tests/functional/modules/devel/process.py around line 207, the assertion
message contains a typo: change "to early" to "too early" in the assertFalse
call's message so the error message reads "Process was interrupted too early".
Migration of Process Module Improved Unit/Functional / RST Docstrings Reference: avocado-framework#52 Signed-off-by: Harvey Lynden <hlynden@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
autils/devel/process.py (7)
460-463: Stale docstring references toavocado.utils.The docstring references
avocado.utils.astring.ENCODINGbut this module is inautils. Similar stale references exist at lines 696, 1117, 1191, and 1261. Update to referenceautils.devel.astring.ENCODINGinstead.:param encoding: the encoding to use for the text version of stdout and stderr, by default - :data:`avocado.utils.astring.ENCODING` + :data:`autils.devel.astring.ENCODING` :type encoding: str
96-100:can_sudo()has a bytes/str mismatch causing incorrect results.
system_output()returnsbytes, so.strip() == "0"is alwaysFalse. This causescan_sudo()to incorrectly returnFalseeven when sudo is available.try: if cmd: # Am I able to run the cmd or plain sudo id? return not system(cmd, ignore_status=True, sudo=True) - if system_output("id -u", ignore_status=True, sudo=True).strip() == "0": + if system_output("id -u", ignore_status=True, sudo=True).strip() == b"0": return True return False
209-221:safe_kill()conflates "PID missing" with "root-owned" and catches overly broad exception.
if not get_owner_id(...)isTruefor both0(root) andNone(no such PID). This causes the function to attempt sudo kill even for non-existent processes. Additionally, catching bareExceptionmasks unexpected errors.def safe_kill(pid, signal): # pylint: disable=W0621 - if not get_owner_id(int(pid)): + owner_id = get_owner_id(int(pid)) + if owner_id == 0: kill_cmd = f"kill -{int(signal)} {int(pid)}" try: run(kill_cmd, sudo=True) return True except CmdError: return False try: os.kill(pid, signal) return True - except Exception: # pylint: disable=W0703 + except OSError: return False
242-244: Fragile/proc/statparsing when process names contain spaces.The current approach splits by space and uses index
-49, which breaks if the process name (field 2, enclosed in parentheses) contains spaces. Parse after the closing parenthesis instead.def get_parent_pid(pid): with open(f"/proc/{int(pid)}/stat", "rb") as proc_stat: - parent_pid = proc_stat.read().split(b" ")[-49] - return int(parent_pid) + content = proc_stat.read() + # Find the closing paren of the command name, then parse fields after it + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + # PPID is the 2nd field after the closing paren (state is first) + return int(fields[1])
280-284: Same fragile parsing issue asget_parent_pid.The
/proc/*/statparsing usingsplit(b" ")[-49]is fragile for process names containing spaces. Apply the same fix usingrfind(b")")to locate the command name boundary.try: with open(proc_stat, "rb") as proc_stat_fp: - this_parent_pid = int(proc_stat_fp.read().split(b" ")[-49]) + content = proc_stat_fp.read() + close_paren = content.rfind(b")") + fields = content[close_paren + 2:].split() + this_parent_pid = int(fields[1]) except IOError: continue
372-373: Shell metacharacters in pattern are not escaped.The
patternparameter is interpolated directly into the command string without escaping. Patterns containing shell metacharacters could cause unexpected behavior.def kill_process_by_pattern(pattern): - cmd = f"pkill -f {pattern}" + cmd = f"pkill -f {shlex.quote(pattern)}" result = run(cmd, ignore_status=True)
400-404:process_in_ptree_is_defunct()will raise TypeError due to bytes/str mismatch.
system_output()returnsbytes, so checkingif "<defunct>" in proc_namewill fail. Useb"<defunct>"or decode the output.for pid in pids: cmd = f"ps --no-headers -o cmd {int(pid)}" proc_name = system_output(cmd, ignore_status=True, verbose=False) - if "<defunct>" in proc_name: + if b"<defunct>" in proc_name: defunct = True breaktests/unit/modules/devel/process.py (2)
376-380: Fix assertion intest_get_parent_pidto validate the parent PID value.
assertTrue(process.get_parent_pid(0), 24139)only checks if the return value is truthy; the second argument becomes the failure message, not a comparison value. The test never validates that the returned PID equals the expected value24139.def test_get_parent_pid(self): stat = b"18405 (bash) S 24139 18405 18405 34818 8056 4210688 9792 170102 0 7 11 4 257 84 20 0 1 0 44336493 235409408 4281 18446744073709551615 94723230367744 94723231442728 140723100226000 0 0 0 65536 3670020 1266777851 0 0 0 17 1 0 0 0 0 0 94723233541456 94723233588580 94723248717824 140723100229613 140723100229623 140723100229623 140723100233710 0" with unittest.mock.patch("builtins.open", return_value=io.BytesIO(stat)): - self.assertTrue(process.get_parent_pid(0), 24139) + self.assertEqual(process.get_parent_pid(0), 24139)
723-729:assertRaises(TypeError, lambda x: ...)is testing the wrong failure mode.These lambdas require an argument, so
assertRaiseswill raiseTypeErrordue to the callable signature mismatch, not due tostdout_text/stderr_textproperty access onNone.def test_cmd_result_stdout_stderr_other_type(self): result = process.CmdResult() result.stdout = None result.stderr = None - self.assertRaises(TypeError, lambda x: result.stdout_text) - self.assertRaises(TypeError, lambda x: result.stderr_text) + self.assertRaises(TypeError, lambda: result.stdout_text) + self.assertRaises(TypeError, lambda: result.stderr_text)
🧹 Nitpick comments (2)
autils/devel/process.py (2)
791-793: Use bareraiseto preserve the original traceback.When re-raising an exception, use
raiseinstead ofraise detailsto preserve the complete traceback.except OSError as details: details.strerror += f" ({self.cmd})" - raise details + raise
913-916: Consider catching specific exceptions instead of suppressing all.
contextlib.suppress(Exception)is overly broad. Consider catchingCmdErrorandOSErrorspecifically to avoid hiding unexpected failures.for pid in pids: kill_cmd = f"kill -{int(sig)} {int(pid)}" - with contextlib.suppress(Exception): + with contextlib.suppress(CmdError, OSError): run(kill_cmd, sudo=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
autils/devel/process.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/devel/process.yml(1 hunks)tests/functional/modules/devel/process.py(1 hunks)tests/unit/modules/devel/process.py(1 hunks)tests/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- metadata/devel/process.yml
- tests/functional/modules/devel/process.py
- tests/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
autils/devel/process.py (2)
autils/devel/wait.py (1)
wait_for(15-104)autils/file/path.py (2)
find_command(90-133)CmdNotFoundError(30-51)
🪛 Ruff (0.14.8)
tests/unit/modules/devel/process.py
139-139: Unused function argument: default
(ARG001)
139-139: Unused function argument: check_exec
(ARG001)
194-194: Function call with shell=True parameter identified, security issue
(S604)
202-202: Function call with shell=True parameter identified, security issue
(S604)
208-208: Function call with shell=True parameter identified, security issue
(S604)
253-253: Function call with shell=True parameter identified, security issue
(S604)
261-261: Function call with shell=True parameter identified, security issue
(S604)
267-267: Function call with shell=True parameter identified, security issue
(S604)
643-643: Function call with shell=True parameter identified, security issue
(S604)
727-727: Unused lambda argument: x
(ARG005)
728-728: Unused lambda argument: x
(ARG005)
777-777: Unused method argument: record
(ARG002)
autils/devel/process.py
100-100: Consider moving this statement to an else block
(TRY300)
213-213: Consider moving this statement to an else block
(TRY300)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
346-349: Avoid specifying long messages outside the exception class
(TRY003)
436-436: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Avoid specifying long messages outside the exception class
(TRY003)
531-531: Avoid specifying long messages outside the exception class
(TRY003)
755-755: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-760: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
784-784: subprocess call: check for execution of untrusted input
(S603)
793-793: Use raise without specifying exception name
Remove exception name
(TRY201)
966-972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
999-999: Avoid specifying long messages outside the exception class
(TRY003)
1129-1129: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-stability-tests
- GitHub Check: modules-tests
🔇 Additional comments (5)
docs/source/utils.rst (1)
42-45: LGTM!The documentation entry follows the established pattern for other modules in this file and properly exposes the new
autils.devel.processmodule.tests/unit/modules/devel/process.py (2)
31-43: Bareexceptin test fixture is acceptable for this use case.The
REFUSE_TO_DIEscript intentionally uses a bareexceptto ignore all errors when setting up signal handlers. This creates an unkillable process for testing timeout handling, which is the intended behavior.
1-18: Test module setup looks good.The imports, logger setup, and helper function
probe_binaryprovide a clean foundation for the test suite.autils/devel/process.py (2)
445-503: Well-structuredCmdResultclass.The
CmdResultclass provides a clean interface for command execution results with proper type handling for stdout/stderr text conversion.
1076-1146: Well-designedrun()function with proper error handling.The
run()function provides a clean interface with comprehensive parameter documentation and proper error handling for empty commands and failed executions.
| def is_sudo_enabled(self): | ||
| """Return whether the subprocess is running with sudo enabled. | ||
| :return: True if running as root (UID 0), False otherwise. | ||
| :rtype: bool | ||
| """ | ||
| self._init_subprocess() | ||
| return not self.get_user_id() |
There was a problem hiding this comment.
is_sudo_enabled() conflates "missing PID" with "root-owned".
return not self.get_user_id() returns True for both user_id == 0 (root) and user_id == None (PID doesn't exist). Should explicitly check for 0.
def is_sudo_enabled(self):
"""Return whether the subprocess is running with sudo enabled.
:return: True if running as root (UID 0), False otherwise.
:rtype: bool
"""
self._init_subprocess()
- return not self.get_user_id()
+ return self.get_user_id() == 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def is_sudo_enabled(self): | |
| """Return whether the subprocess is running with sudo enabled. | |
| :return: True if running as root (UID 0), False otherwise. | |
| :rtype: bool | |
| """ | |
| self._init_subprocess() | |
| return not self.get_user_id() | |
| def is_sudo_enabled(self): | |
| """Return whether the subprocess is running with sudo enabled. | |
| :return: True if running as root (UID 0), False otherwise. | |
| :rtype: bool | |
| """ | |
| self._init_subprocess() | |
| return self.get_user_id() == 0 |
🤖 Prompt for AI Agents
In autils/devel/process.py around lines 1042 to 1049, is_sudo_enabled()
currently returns not self.get_user_id(), which treats a missing PID (None) the
same as root (0); change the logic to explicitly check for UID 0 by returning
self.get_user_id() == 0 so only root yields True and None yields False. Ensure
_init_subprocess() is still called before the check and no other side effects
are introduced.
The test was mocking 'system' but can_sudo() without a cmd argument calls getoutput() internally.
|
Now includes the latest CI fixes from avocado side. |
2 participants
Migration of Process Module
Improved Unit/Functional / RST Docstrings
Reference: #52
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.