Error counter log none JSON format output

[Rick Chen]

Error counter log did not output in the json format.

[Rick Chen]

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

[Christian Franke]

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).

[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.

[Rick Chen]

git diff patch file

[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.

[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.

2. 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).

3. 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.

[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"

[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.

[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.

[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.

[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.

[Christian Franke]

Related: tickets #1082 and #1083.

[Rick Chen]

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

[Christian Franke]

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

[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

[Sage Weil]

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

[Christian Franke]

Patch applied in r4815, thanks!

[Christian Franke]

Milestone renamed