Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors subprocess calls to support timeouts, centralizes ELF parsing constants, enhances logging, and fixes a typo in distribution parsing.
- Introduce
check_output_with_timeoutwith signal-based timeout and replace allcheck_outputcalls - Add constants (
ELF_MAGIC_BYTES,PROC_TIMEOUT,MAX_NOTE_SIZE,BYTE_ALIGNMENT), note-size validation, and logging improvements - Correct
description_version_idkey typo and update tests (including Unicode normalization)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| uchecker.py | Added timeout wrapper, constants, ELF validation, improved logging and error handling |
| tests/test_kernelcare.py | Updated mocks to use check_output_with_timeout and added Unicode normalize test |
Comments suppressed due to low confidence (2)
tests/test_kernelcare.py:22
- Add dedicated unit tests for
check_output_with_timeoutto cover normal execution, subprocess errors, and timeout-triggered paths to ensure cleanup of alarms and handlers.
def tests_get_patched_data(mock_system, tmpdir):
uchecker.py:78
- [nitpick] Expand this docstring to clarify behavior on platforms without
SIGALRM(no timeout enforcement) and describe return values on errors or timeouts.
"""Enhanced check_output with timeout support for Python 2/3."""
uchecker.py
Outdated
| signal.signal(signal.SIGALRM, old_handler) | ||
| raise | ||
|
|
||
| except (subprocess.SubprocessError, OSError) as e: |
There was a problem hiding this comment.
Referencing subprocess.SubprocessError may break on Python versions where SubprocessError isn’t defined (e.g., Python 2). Consider using getattr(subprocess, 'SubprocessError', OSError) or catching OSError and subprocess.CalledProcessError instead.
| except (subprocess.SubprocessError, OSError) as e: | |
| except (getattr(subprocess, 'SubprocessError', OSError), OSError) as e: |
There was a problem hiding this comment.
Pull Request Overview
Adds timeout support to subprocess calls, replaces magic values with named constants for ELF parsing, and fixes a typo in distribution parsing.
- Introduces
check_output_with_timeoutwith a default timeout and enhanced exception handling - Defines
ELF_MAGIC_BYTES,PROC_TIMEOUT,MAX_NOTE_SIZE, andBYTE_ALIGNMENT, replacing inline values inget_build_id - Corrects the
description_version_idtypo in_linux_distributionand updates tests for the new function and Unicode normalization
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| uchecker.py | Added constants, implemented check_output_with_timeout, replaced magic values, improved logging formatting, fixed typo in distribution parsing |
| tests/test_kernelcare.py | Updated tests to patch check_output_with_timeout and added test_normalize_unicode |
Comments suppressed due to low confidence (2)
tests/test_kernelcare.py:117
- There are no tests covering
check_output_with_timeout, including timeout and error paths. Add unit tests for normal execution, timeouts, non-zero exit codes, and subprocess errors.
def test_normalize_unicode():
uchecker.py:49
- [nitpick] Consider renaming
PROC_TIMEOUTtoDEFAULT_PROC_TIMEOUT_SECSor similar for clearer intent that this value is in seconds.
PROC_TIMEOUT = 30
No description provided.