log: add force option to report data area 4 - #3968
Conversation
|
Note: I am thinking to add the feature FID: 16h (host behavior support) to set ETDAS cheking if supported the feature itself later. |
There was a problem hiding this comment.
🟡 Changes recommended
The new --force path inverts ETDAS error handling, causing ETDAS failures to be ignored in the default path while becoming fatal when --force is set.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a --force option to the log plugin’s telemetry log retrieval so users can request Telemetry Data Area 4 even when DA4S/ETDAS-related prerequisites are not indicated as supported/required by the device.
Changes:
- Introduces a new
--forceCLI flag for telemetry log retrieval. - Allows Data Area 4 reporting to proceed when
--forceis set, even ifLPA:DA4Sis not set. - Adjusts ETDAS-setting error handling for the Data Area 4 path (currently introduces an inverted condition that needs correction).
File summaries
| File | Description |
|---|---|
| plugins/log/log-plugin.c | Adds --force option and alters Data Area 4 support gating / ETDAS handling in telemetry log retrieval. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| err = libnvme_set_etdas(hdl, &host_behavior_changed); | ||
| if (err) { | ||
| if (err && cfg.force) { | ||
| nvme_show_error("%s: Failed to set ETDAS bit", __func__); | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Right and sorry for the error then just fixed the code so please review again. Thank you.
There was a problem hiding this comment.
I woulnd't catch it, that's pure AI :)
There was a problem hiding this comment.
Yes really helpful;-)
Since the LPA: DA4S and ETDAS settings are not mandatory. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
1131816 to
acf90a8
Compare
|
Just added to check if the feature FID: 16h (host behavior support) to set ETDAS supported. |
b81e39c to
bf6ce65
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new libnvme_status_is_invalid_field() helper can mis-handle non-NVMe status types (e.g., NVMe-MI encoded return values) and may incorrectly suppress real errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| static inline bool | ||
| libnvme_status_is_invalid_field(int status) | ||
| { | ||
| if (nvme_status_code_type(status) == NVME_SCT_GENERIC && | ||
| nvme_status_code(status) == NVME_SC_INVALID_FIELD) | ||
| return true; | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Just fixed as suggested. Thank you.
(Added)
Note: To make sure just asked to google AI mode also to review then confirmed as okay.
bf6ce65 to
1368297
Compare
Invalid field returned if host behavior support feature not supported. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
1368297 to
4bba43c
Compare
Since the LPA: DA4S and ETDAS settings are not mandatory.