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

plugins/ocp : Fixed the OCP Telemetry String Log Page (LID:C9h) fix #2373

Conversation

VigneshwaranSaravana
Copy link
Contributor

@VigneshwaranSaravana VigneshwaranSaravana commented Jun 21, 2024

This PR will take care the below failures for "telemetry-string-log"
Resolved the json printing issue
Changed the Statistic Identifier, Event String, Vendor Unique, ASCII table offset and size calculation

Signed-off-by: Vigneshwaran Saravanan/Vigneshwaran Saravanan [email protected]

Reviewed-by: Karthik Balan [email protected],
Reviewed-by: Arunpandian J [email protected],
Reviewed-by: Jayakanthan Rajendran [email protected]

@igaw
Copy link
Collaborator

igaw commented Jun 25, 2024

Please address the issues reported by checkpatch

WARNING: Missing commit description - Add an appropriate one
Error: WARNING: Missing commit description - Add an appropriate one


WARNING: Unexpected content after email: 'Karthik Balan <[email protected]>, Arunpandian J', should be: 'Karthik Balan <[email protected]> (, Arunpandian J)'
#8:
Reviewed-by: Karthik Balan <[email protected]>, Arunpandian J
Error: WARNING: Unexpected content after email: 'Karthik Balan <[email protected]>, Arunpandian J', should be: 'Karthik Balan <[email protected]> (, Arunpandian J)'


ERROR: space required before the open brace '{'
#61: FILE: plugins/ocp/ocp-nvme.c:2900:
+ if (log_data->sitsz != 0){
Error: ERROR: space required before the open brace '{'

[...]

@VigneshwaranSaravana
Copy link
Contributor Author

@igaw - I have resolved all the coding guidelines issue. Even I have updated the commit description but still patch run failed due to WARNING: Missing commit description - Add an appropriate one. Could you please check

@igaw
Copy link
Collaborator

igaw commented Jun 27, 2024

I have the same feedback as in the other PR (#2374 (comment))

@VigneshwaranSaravana
Copy link
Contributor Author

@igaw - Incorporated the review comment. Please check

@igaw
Copy link
Collaborator

igaw commented Jul 2, 2024

I don't see any changes here. Still the version from last week.

@VigneshwaranSaravana VigneshwaranSaravana force-pushed the plugin/ocp_telemetry_str_log_fix branch 3 times, most recently from c3c8756 to c7e03cb Compare July 4, 2024 07:20
@@ -3271,7 +3327,7 @@ static int get_c9_log_page(struct nvme_dev *dev, char *format)
int ret = 0;
__u8 *header_data;
struct telemetry_str_log_format *log_data;
nvme_print_flags_t fmt;
enum nvme_print_flags fmt;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we keep this type as nvme_print_flags_t here. We just changed the printing flags type to this. We define the flags as enum type but then we want to be able to use several bits in variable. Thus this shouldn't be an enum anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igaw
Fixed. Please check.

I have fixed all the coding guideline issue. Patch failed due to below one. Even I have update commit description. Please check

[1/1] Check commit - c143e40

WARNING: Missing commit description - Add an appropriate one
Error: WARNING: Missing commit description - Add an appropriate one

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your commit message needs to be edited. This has nothing to do what you edit in github's PR. checkpatch complains about the git commit itself.

git commit --edit --amend should open your text editor and you should fix this

image

@VigneshwaranSaravana VigneshwaranSaravana force-pushed the plugin/ocp_telemetry_str_log_fix branch from c7e03cb to c143e40 Compare July 4, 2024 08:37
@igaw
Copy link
Collaborator

igaw commented Jul 5, 2024

@arthurshau as we are almost there with this PR, could you have a look?

@arthurshau
Copy link
Contributor

Logic LGTM

Take care the below failures for "telemetry-string-log".

Resolved the json printing issue.

Changed the Statistic Identifier, Event String, Vendor Unique, ASCII
table offset and size calculation.

Reviewed-by: Karthik Balan <[email protected]>
Reviewed-by: Arunpandian J <[email protected]>
Reviewed-by: Jayakanthan Rajendran <[email protected]>
Signed-off-by: Vigneshwaran Saravanan <[email protected]>
@igaw igaw force-pushed the plugin/ocp_telemetry_str_log_fix branch from c143e40 to eb533c2 Compare July 9, 2024 10:09
@igaw
Copy link
Collaborator

igaw commented Jul 9, 2024

Updated the commit message myself.

@igaw igaw merged commit 02c6260 into linux-nvme:master Jul 9, 2024
15 of 16 checks passed
@igaw
Copy link
Collaborator

igaw commented Jul 9, 2024

Thanks.

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.

3 participants