Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#1079 closed patch (fixed)

Error counter log none JSON format output

Reported by: Rick Chen Owned by: Christian Franke
Priority: minor Milestone: Release 7.0
Component: smartctl Version: 6.6
Keywords: json scsi Cc:

Description

Error counter log did not output in the json format.

Attachments (9)

scsiprint.cpp (86.9 KB) - added by Rick Chen 5 years ago.
patch_json_error_counter_log_1.00.diff (5.6 KB) - added by Rick Chen 5 years ago.
git diff patch file
patch_ChangeLog_v1.01.diff (686 bytes) - added by Rick Chen 5 years ago.
patch_scsiprint_v1.01.diff (72.6 KB) - added by Rick Chen 5 years ago.
patch_scsiprint_v1.01.2.diff (6.7 KB) - added by Rick Chen 5 years ago.
patch_scsiprint_v1.01.3.diff (6.7 KB) - added by Rick Chen 5 years ago.
patch_ChangeLog_v1.02.diff (405 bytes) - added by Rick Chen 5 years ago.
patch_scsiprint_v1.02.diff (2.8 KB) - added by Rick Chen 5 years ago.
error_counter_log.json (4.9 KB) - added by Rick Chen 5 years ago.

Download all attachments as: .zip

Change History (28)

Changed 5 years ago by Rick Chen

Attachment: scsiprint.cpp added

comment:1 Changed 5 years ago by Rick Chen

Milestone: Release 6.7
Type: defectpatch

Sorry, I did not know how to submit patch file for this issue. I attach the scsiprint.cpp for reference.

comment:2 Changed 5 years ago by Christian Franke

Keywords: json scsi added; JSON removed
Milestone: Release 6.7undecided
Owner: Rick Chen deleted

Thanks for this patch.

For future submissions, please:

  • don't set a milestone,
  • don't assign an owner,
  • provide a patch file (e.g. output of svn diff, git diff or diff -ru smartmontools-VERSION.orig smartmontools-VERSION).

comment:3 Changed 5 years ago by Christian Franke

Please change the patch as follows:

  • Rename pout to jout for all plaintext output which now also appears in JSON output (try --json=[iu] option to see the effect).
  • Prefix scsi_ to any newly added JSON output which is SCSI specific.
  • Use Egyptian braces as in the rest of the module.
  • Provide the new version as a patch file.

Patch needs then to be reviewed by the maintainer of smartmontools SCSI code.

Changed 5 years ago by Rick Chen

git diff patch file

comment:4 Changed 5 years ago by Rick Chen

HI Christian:

  1. The patch use jglb output, it should better than jout. It clear and easy to read.
  2. Does I need add prefix "scsi_" to newly key name?
  3. I add new patch for your requirement. patch_json_error_counter_log_1.00.diff​

Thanks.

comment:5 in reply to:  4 Changed 5 years ago by Christian Franke

  1. The patch use jglb output, it should better than jout. It clear and easy to read.

Please see comments above pout() and jout() in smartctl.cpp and commits mentioned in ticket #766 to see why pout shall be renamed to jout.

  1. Does I need add prefix "scsi_" to newly key name?

At least error_counter_log should be named scsi_error_counter_log unless there are similar data structures in ATA and/or NVMe (there aren't). We could change the other names later (for example if and only if temperature.drive_trip (SCSI) means the same as temperature.op_limit_max (ATA), the same name should be used).

  1. I add new patch for your requirement. patch_json_error_counter_log_1.00.diff​

Looks good so far, except it still breaks indentation style for {...}.

Please also include a proposed ChangeLog entry to show whether we could publish your real name and (optional) mail address.

Changed 5 years ago by Rick Chen

Attachment: patch_ChangeLog_v1.01.diff added

Changed 5 years ago by Rick Chen

Attachment: patch_scsiprint_v1.01.diff added

comment:6 Changed 5 years ago by Rick Chen

HI Christian:
Thank you for your advice.
I refresh patch files for you reference. "patch_ChangeLog_v1.01.diff", "patch_scsiprint_v1.01.diff"

comment:7 Changed 5 years ago by Christian Franke

Patch rejected because it apparently renames all pout to jout. Please rename only those pout where corresponding JSON output is added by the patch. Please reread the related comments in smartctl.cpp. Read the -j, --json[=giosu] section on smartctl man page. Test the effect of --json=iu with and without your changes.

Changed 5 years ago by Rick Chen

comment:8 in reply to:  7 Changed 5 years ago by Rick Chen

Replying to Christian Franke:

Patch rejected because it apparently renames all pout to jout. Please rename only those pout where corresponding JSON output is added by the patch. Please reread the related comments in smartctl.cpp. Read the -j, --json[=giosu] section on smartctl man page. Test the effect of --json=iu with and without your changes.

Sorry, I understand your advise. I has corrected the patch and attached the file "patch_scsiprint_v1.01.2.diff". Please help me to review it.
Thanks.

Last edited 5 years ago by Rick Chen (previous) (diff)

Changed 5 years ago by Rick Chen

comment:9 Changed 5 years ago by Christian Franke

Error counter log looks good so far.

JSON names and structure of Seagate factory lpage and background scan lpage do not reflect the origin of the values.

For example:

scsi_accumulated_power_on_time_minutes : 62,
scsi_number_sacns_performed : 3, // BTW: Typo
scsi_number_medium_scans_performed : 2,

Better reflect original protocol specific data structures as is and provide copies of common protocol independent values:

scsi_background_scan_log : {
  status: {
    accumulated_power_on_time_minutes : 62,
    number_scans_performed : 3,
    number_medium_scans_performed : 2
  }
},
power_on_time : { hours : 1, minutes : 2 },

This eases later extensions, for example the scsi_background_scan_log.results[] information.

power_on_time is already implemented, see r4762.

For now, please reduce the patch to original topic (Error counter log) and provide the off-topic parts as separate patches. If possible, provide sample --json=o outputs from a real world device which supports these log pages.

Changed 5 years ago by Rick Chen

Attachment: patch_ChangeLog_v1.02.diff added

Changed 5 years ago by Rick Chen

Attachment: patch_scsiprint_v1.02.diff added

Changed 5 years ago by Rick Chen

Attachment: error_counter_log.json added

comment:10 Changed 5 years ago by Rick Chen

HI Sir:
I refresh the patch file. "patch_ChangeLog.v1.02.diff", "patch_scsiprint_v1.02.diff", and real device json output file "error_counter_log.json".
If these still have problems, please give me advise, thanks.

comment:11 Changed 5 years ago by Christian Franke

Related: tickets #1082 and #1083.

comment:12 Changed 5 years ago by Rick Chen

Hi Sir:
Does this patch has any problem or advise? Does it have chance to merged?

comment:13 Changed 5 years ago by Christian Franke

Patch needs to be reviewed by the owner of the SCSI code. This may take some time.

comment:14 Changed 5 years ago by brianjeng

Hi Christian is there any way to find contact information for the SCSI code owner? It'd be helpful if we can get some more insight on what he expects to approve a merge

comment:15 Changed 5 years ago by Rick Chen

Last edited 5 years ago by Rick Chen (previous) (diff)

comment:16 Changed 5 years ago by Sage Weil

This looks good from our end. Any progress on getting a review from the maintainer of the SCSI code? Thanks!

comment:17 Changed 5 years ago by Christian Franke

Milestone: undecidedRelease 6.7
Owner: set to Christian Franke
Status: assignedaccepted

comment:18 Changed 5 years ago by Christian Franke

Resolution: fixed
Status: acceptedclosed

Patch applied in r4815, thanks!

comment:19 Changed 5 years ago by Christian Franke

Milestone: Release 6.7Release 7.0

Milestone renamed

Note: See TracTickets for help on using tickets.