plugins/amdgpu: Cleanup env variable handling#3014
Merged
avagin merged 1 commit intoMay 19, 2026
Conversation
It is a bit tidier to make the helpers return the value instead of passing in the pointer, so lets make them take the default and return the overall result, allowing the call sites to consolidate into a single line. While at it we make the helpers static since they are not used outside the file and demote the parsing errors to warning log messages since the plugin does continue operating. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## criu-dev #3014 +/- ##
============================================
- Coverage 57.22% 57.22% -0.01%
============================================
Files 154 154
Lines 40440 40441 +1
Branches 8863 8863
============================================
Hits 23142 23142
- Misses 17034 17035 +1
Partials 264 264 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
LGTM |
There was a problem hiding this comment.
Pull request overview
This PR refactors AMDGPU plugin environment-variable parsing helpers to return defaults directly, makes the helpers static, and adjusts log severity for invalid values so the plugin continues with defaults.
Changes:
- Refactor
getenv_bool()/getenv_size_t()to return parsed values (or defaults) instead of mutating via out-params. - Demote some invalid-env-value logs from errors to warnings and always log the effective parameter value.
- Simplify
amdgpu_plugin_init()by inlining defaults into the env parsing calls.
| } | ||
| pr_info("param: %s:%s\n", var, *value ? "Y" : "N"); | ||
|
|
||
| pr_info("param: %s:%u\n", var, val); |
| pr_info("param: %s:0x%lx\n", var, *value); | ||
|
|
||
| out: | ||
| pr_info("param: %s:0x%lx\n", var, val); |
Comment on lines
336
to
+340
| if (value_str) { | ||
| size = (size_t)strtoul(value_str, &endp, 0); | ||
| val = (size_t)strtoul(value_str, &endp, 0); | ||
| if (errno || value_str == endp) { | ||
| pr_err("Ignoring invalid value for %s=%s, expecting a positive integer\n", var, value_str); | ||
| return; | ||
| goto out; |
Comment on lines
338
to
+340
| if (errno || value_str == endp) { | ||
| pr_err("Ignoring invalid value for %s=%s, expecting a positive integer\n", var, value_str); | ||
| return; | ||
| goto out; |
| default: | ||
| pr_err("Ignoring invalid size suffix for %s=%s, expecting 'K'/k', 'M', or 'G'\n", var, value_str); | ||
| return; | ||
| pr_warn("Ignoring invalid size suffix for %s=%s, expecting 'K'/k', 'M', or 'G'\n", var, value_str); |
Member
|
@tursulin I have merged this pr by mistake. please review copilot comments. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is not saving a lot of lines, just two, but perhaps the pattern of:
Is somewhat cleaner than:
I also make the helpers static and demote parsing errors to warnings since the execution does continue.
I am locally toying with the idea of adding a new env variable to ignore gpu id hence I spotted this cleanup opportunity.
cc @fdavid-amd