Opened 6 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 6 years ago.
patch_json_error_counter_log_1.00.diff (5.6 KB ) - added by Rick Chen 6 years ago.
git diff patch file
patch_ChangeLog_v1.01.diff (686 bytes ) - added by Rick Chen 6 years ago.
patch_scsiprint_v1.01.diff (72.6 KB ) - added by Rick Chen 6 years ago.
patch_scsiprint_v1.01.2.diff (6.7 KB ) - added by Rick Chen 6 years ago.
patch_scsiprint_v1.01.3.diff (6.7 KB ) - added by Rick Chen 6 years ago.
patch_ChangeLog_v1.02.diff (405 bytes ) - added by Rick Chen 6 years ago.
patch_scsiprint_v1.02.diff (2.8 KB ) - added by Rick Chen 6 years ago.
error_counter_log.json (4.9 KB ) - added by Rick Chen 6 years ago.

Download all attachments as: .zip

Change History (28)

by Rick Chen, 6 years ago

Attachment: scsiprint.cpp added

comment:1 by Rick Chen, 6 years ago

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 by Christian Franke, 6 years ago

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

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 by Christian Franke, 6 years ago

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.

by Rick Chen, 6 years ago

git diff patch file

comment:4 by Rick Chen, 6 years ago

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.

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

  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.

by Rick Chen, 6 years ago

Attachment: patch_ChangeLog_v1.01.diff added

by Rick Chen, 6 years ago

Attachment: patch_scsiprint_v1.01.diff added

comment:6 by Rick Chen, 6 years ago

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 by Christian Franke, 6 years ago

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.

by Rick Chen, 6 years ago

in reply to:  7 comment:8 by Rick Chen, 6 years ago

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 6 years ago by Rick Chen (previous) (diff)

by Rick Chen, 6 years ago

comment:9 by Christian Franke, 6 years ago

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.

by Rick Chen, 6 years ago

Attachment: patch_ChangeLog_v1.02.diff added

by Rick Chen, 6 years ago

Attachment: patch_scsiprint_v1.02.diff added

by Rick Chen, 6 years ago

Attachment: error_counter_log.json added

comment:10 by Rick Chen, 6 years ago

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 by Christian Franke, 6 years ago

Related: tickets #1082 and #1083.

comment:12 by Rick Chen, 6 years ago

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

comment:13 by Christian Franke, 6 years ago

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

comment:14 by brianjeng, 5 years ago

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 by Rick Chen, 5 years ago

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

comment:16 by Sage Weil, 5 years ago

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

comment:17 by Christian Franke, 5 years ago

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

comment:18 by Christian Franke, 5 years ago

Resolution: fixed
Status: acceptedclosed

Patch applied in r4815, thanks!

comment:19 by Christian Franke, 5 years ago

Milestone: Release 6.7Release 7.0

Milestone renamed

Note: See TracTickets for help on using tickets.