Skip to content
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

nvme-print-json: Add remaining feature fields print functions #2088

Merged
merged 40 commits into from
Nov 16, 2023

Conversation

ikegami-t
Copy link
Contributor

No description provided.

@igaw
Copy link
Collaborator

igaw commented Oct 12, 2023

Could you split the change into two bits? One which updates the formatting/codying style and one which just adds the new code? It makes the review a bit simpler. Thanks!

@ikegami-t ikegami-t force-pushed the json-feature-fields branch 2 times, most recently from ff3809c to 8465478 Compare October 12, 2023 14:05
@ikegami-t
Copy link
Contributor Author

Thanks for your comment. Split the patch and added additional a patch to fix the get feature command mixed stdout and json outputs issue.
Note: Still the get feature command outputs d() and d_raw() to stdout and error to stderr so it will be fixed separately.

@ikegami-t ikegami-t force-pushed the json-feature-fields branch 2 times, most recently from 9203476 to 719acc1 Compare October 13, 2023 18:52
@ikegami-t ikegami-t force-pushed the json-feature-fields branch from 7c64b64 to ff87e30 Compare October 14, 2023 01:19
@ikegami-t
Copy link
Contributor Author

Removed the patches below to the PRs #2090 and #2091 since not related to the json output changes.

  1. 5cc7bac nvme: Change short option -o and -v duplicated to upper case
  2. 10e9c65 doc: Change short option -o and -v duplicated to upper case
  3. c2510ad completions: Change short option -o and -v duplicated to upper case
  4. e98bc40 nvme: Change phy-rx-eom-log command to use NVME_ARGS instead

nvme-print-json.c Outdated Show resolved Hide resolved

#define array_add_obj(o, k) json_array_add_value_object(o, k);
#define obj_add_str(o, k, v) json_object_add_value_string(o, k, v)
#define root_add_array(k, v) json_object_add_value_array(root, k, v)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if wouldn't make sense to just use r instead of root as variable name in the functions and the use obj_add_str(r, ..., ...) instead introducing a wrapper macro just for this. What do you think?

As this series is pretty big and I don't want to make you suffer too much by updating every patch again, you can just add a cleanup patch add the end of the series, that is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixed by the commit 6575bf3.

@igaw
Copy link
Collaborator

igaw commented Nov 2, 2023

The feature output is getting closer to be valid JSON:

./nvme get-feature /dev/nvme0 -o json

{
  "Feature":"0x01",
  "Name":"Arbitration",
  "Current":"0x03030302"
}
{
  "Feature":"0x02",
  "Name":"Power Management",
  "Current":"0x00000003"
}
{
  "Error":"get-feature:0x03 (LBA Range Type): "
}
{
  "Error":"Invalid Namespace or Format: The namespace or the format of that namespace is invalid",
  "Type":"nvme",
  "Value":16395
}
{
  "Feature":"0x04",
  "Name":"Temperature Threshold",
  "Current":"0x00000162"
}
{
  "Error":"get-feature:0x05 (Error Recovery): "
}
{
  "Error":"Invalid Namespace or Format: The namespace or the format of that namespace is invalid",
  "Type":"nvme",
  "Value":16395
}
{
  "Feature":"0x06",
  "Name":"Volatile Write Cache",
  "Current":"0x00000001"
}

We need to figure out how handle the error case above and all features should be reported in an array.

There problem is the show_feature API is not really good for the JSON case. Maybe we need to split into show_feature_init, show_feature and show_feature_finish calls. The show_feature_init callback would allow the JSON code to allocate the array and then the pointer is handed into the show_feature function for every single invocation. show_feature_finish allows the JSON code then to print the result in one go.

@ikegami-t ikegami-t force-pushed the json-feature-fields branch from 6575bf3 to 212fb60 Compare November 2, 2023 18:29
@ikegami-t
Copy link
Contributor Author

Rebased with the master and fixed some conflicts.

@ikegami-t
Copy link
Contributor Author

Noted about the show_feature API problem. Yes I thought also to use the global variable root json object in nvme-print-json to resolved the issue. Anyway later will do consider the suggestion also. Thank you.

Note: Still error and data reported separately so will be fixed later.

Signed-off-by: Tokunori Ikegami <[email protected]>
nvme-print.h Show resolved Hide resolved
nvme-print.h Show resolved Hide resolved
@igaw igaw merged commit a6b4ada into linux-nvme:master Nov 16, 2023
15 checks passed
@igaw
Copy link
Collaborator

igaw commented Nov 16, 2023

Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants