linux-nvme / nvme-cli

NVMe management command line interface.
https://nvmexpress.org
GNU General Public License v2.0
1.41k stars 637 forks source link

plugins/ocp: Fixed the OCP Telemetry Internal Log (LID:7h, 8h) fix #2374

Closed VigneshwaranSaravana closed 1 week ago

VigneshwaranSaravana commented 3 weeks ago

This PR will take care the below failures for "internal-log"

  1. Handle the Offset and NUMD value calculation properly as NVMe spec.
  2. Changed the Statistic Info and Event FIFO info printing logic
  3. Resolved the segmentation fault issue in buffer accessing time.

Signed-off-by: Vigneshwaran Saravanan/Vigneshwaran Saravanan s.vignesh@samsung.com

Reviewed-by: Karthik Balan karthik.b82@samsung.com, Reviewed-by: Arunpandian J arun.j@samsung.com, Reviewed-by: Jayakanthan Rajender jaya.ganthan@samsung.com

igaw commented 2 weeks ago

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: line length of 108 exceeds 100 columns
#78: FILE: plugins/ocp/ocp-nvme.c:926:
+ __le64 data_len, void *data, __u8 nLSP, __u8 nRAE,
Error: WARNING: line length of 108 exceeds 100 columns

WARNING: line length of 114 exceeds 100 columns
#97: FILE: plugins/ocp/ocp-nvme.c:946:
+static void print_telemetry_data_area_1(struct telemetry_data_area_1 *da1, int tele_type, __le64 *eve_fifo_ofs_sz)
Error: WARNING: line length of 114 exceeds 100 columns

[...]
VigneshwaranSaravana commented 2 weeks ago

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: line length of 108 exceeds 100 columns
#78: FILE: plugins/ocp/ocp-nvme.c:926:
+ __le64 data_len, void *data, __u8 nLSP, __u8 nRAE,
Error: WARNING: line length of 108 exceeds 100 columns

WARNING: line length of 114 exceeds 100 columns
#97: FILE: plugins/ocp/ocp-nvme.c:946:
+static void print_telemetry_data_area_1(struct telemetry_data_area_1 *da1, int tele_type, __le64 *eve_fifo_ofs_sz)
Error: WARNING: line length of 114 exceeds 100 columns

[...]
VigneshwaranSaravana commented 2 weeks ago

@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 commented 2 weeks ago

The commit message should look something like

plugins/ocp: Fixed the OCP Telemetry Internal Log (LID:7h, 8h)

[describe the why/what you are changing]

Signed-off-by: Vigneshwaran Saravanan <s.vignesh@samsung.net>
Reviewed-by: Karthik Balan <karthik.b82@samsung.com>
Reviewed-by: Arunpandian J <arun.j@samsung.com>
Reviewed-by: Jayakanthan Rajendr <jaya.ganthan@samsung.com>

The subject title is also a bit meh. Fixed doesn't say anything. If you have a hard time to find a proper subject title because the change does several things, it is a clear indication the patch does too much. In this case it's better to split it up into smaller patches.

I stress this fact here, because reviewing/maintaining a code base is very important and good commits are a key element for this. Please check also what the Linux kernel coding/patching guidelines says on this. There are a lot of excellent good tips: https://docs.kernel.org/process/submitting-patches.html

VigneshwaranSaravana commented 2 weeks ago

@igaw - Incorporated the review comments. Please check.

igaw commented 2 weeks ago

Did you push your changes? I don't see any changes in this PR.

jeff-lien-wdc commented 1 week ago

@VigneshwaranSaravana I have been working on similar changes. I could finish them up and send you a link if you'd like to take a look. Maybe we could merge them together into one new PR.

jeff-lien-wdc commented 1 week ago

@VigneshwaranSaravana I merged your changes with mine and created PR #2393. Could you review it? Maybe it could be merged in place of this PR.

igaw commented 1 week ago

I've merged https://github.com/linux-nvme/nvme-cli/pull/2393 thus closing this one.

VigneshwaranSaravana commented 1 week ago

This PR is superset of https://github.com/linux-nvme/nvme-cli/pull/2393 due to that it's closed.