-
-
Notifications
You must be signed in to change notification settings - Fork 299
fix: Use /proc/<id>/status to get uid rather than loginuid. #691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Using loginuid is only possible if the kernel is built with CONFIG_AUDIT enabled. The Uid under which the process is running is available in status. This is different than the uid of the user that initiated the process. Upstream-Status: Inappropriate [OE-specific] Signed-off-by: Mark Butowski <[email protected]>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces CONFIG_AUDIT-dependent loginuid lookup by parsing the Uid field in /proc//status (with fallback for missing matches) and introduces a multiline cat utility. Class diagram for updated ProcessService and new cat_multiline utilityclassDiagram
class ProcessService {
- usernames: dict
- _root_path: str
+ __init__()
+ get_process_info(pid, gpu_mem_usage, process_name, uptime)
}
class cat {
+ cat(path)
}
class cat_multiline {
+ cat_multiline(path, max_length=16384)
}
ProcessService --> cat : uses
ProcessService --> cat_multiline : uses
Flow diagram for process UID retrieval changeflowchart TD
A[Start get_process_info] --> B[Read /proc/<pid>/status with cat_multiline]
B --> C[Extract Uid field using regex]
C --> D{Uid found?}
D -- Yes --> E[Convert Uid to int]
D -- No --> F[Set Uid to -1]
E --> G[Lookup username]
F --> G[Lookup username]
G --> H[Continue process info gathering]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `jtop/core/common.py:119` </location>
<code_context>
return f.readline().rstrip('\x00')
+def cat_multiline(path, max_length=16384):
+ with open(path, 'r') as f:
+ return f.read(max_length)
+
+
</code_context>
<issue_to_address>
Consider handling file read exceptions in cat_multiline.
Currently, if the file is missing or unreadable, an exception will be raised. Consider catching IOError/OSError and returning an empty string or a custom error, as done in 'cat'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def cat_multiline(path, max_length=16384):
with open(path, 'r') as f:
return f.read(max_length)
=======
def cat_multiline(path, max_length=16384):
try:
with open(path, 'r') as f:
return f.read(max_length)
except OSError:
return ""
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `jtop/core/processes.py:98` </location>
<code_context>
- uid = int(cat(os.path.join('/proc', pid, 'loginuid')))
+ proc_status = cat_multiline(os.path.join('/proc', pid, 'status'))
+ uid_matches = PROCESS_UID_REG.findall(proc_status)
+ uid = int(uid_matches[0]) if uid_matches else int(-1)
if uid not in self.usernames:
self.usernames[uid] = pwd.getpwuid(uid).pw_name
</code_context>
<issue_to_address>
Fallback to -1 for missing UID may mask parsing errors.
Consider adding a log message when no UID is found to make parsing issues more visible.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
proc_status = cat_multiline(os.path.join('/proc', pid, 'status'))
uid_matches = PROCESS_UID_REG.findall(proc_status)
uid = int(uid_matches[0]) if uid_matches else int(-1)
if uid not in self.usernames:
self.usernames[uid] = pwd.getpwuid(uid).pw_name
=======
import logging
proc_status = cat_multiline(os.path.join('/proc', pid, 'status'))
uid_matches = PROCESS_UID_REG.findall(proc_status)
if not uid_matches:
logging.warning(f"No UID found in /proc/{pid}/status. Falling back to -1.")
uid = int(uid_matches[0]) if uid_matches else int(-1)
if uid not in self.usernames:
self.usernames[uid] = pwd.getpwuid(uid).pw_name
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
uid = int(cat(os.path.join('/proc', pid, 'loginuid'))) | ||
proc_status = cat_multiline(os.path.join('/proc', pid, 'status')) | ||
uid_matches = PROCESS_UID_REG.findall(proc_status) | ||
uid = int(uid_matches[0]) if uid_matches else int(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): We've found these issues:
- Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast
) - Inline variable that is immediately returned (
inline-immediately-returned-variable
)
* Enable I2C_CHARDEV for reading the CVM EEPROM * Enable I2C_MUX_GPIO and I2C_MUX_PCA954x for camera support * Enable AUDIT for jtop support Signed-off-by: Matt Madison <[email protected]>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Using loginuid is only possible if the kernel is built with CONFIG_AUDIT enabled. The uid under which the process is running is available in status. This is different than the loginuid of the user that first logged in and somehow initiated the process.
At least this is my understanding of /proc//loginuid vs /proc//status.
I think it probably makes more sense to display the uid of the process and avoid the dependency on CONFIG_AUDIT.
Comments welcome.
Summary by Sourcery
Replace retrieval of process user ID via /proc//loginuid with parsing the Uid field from /proc//status to avoid CONFIG_AUDIT dependency and handle unknown UIDs.
Bug Fixes:
Enhancements: