Attachments (9)
Change History (28)
by , 6 years ago
Attachment: | scsiprint.cpp added |
---|
comment:1 by , 6 years ago
Milestone: | → Release 6.7 |
---|---|
Type: | defect → patch |
comment:2 by , 6 years ago
Keywords: | json scsi added; JSON removed |
---|---|
Milestone: | Release 6.7 → undecided |
Owner: | 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
ordiff -ru smartmontools-VERSION.orig smartmontools-VERSION
).
comment:3 by , 6 years ago
Please change the patch as follows:
- Rename
pout
tojout
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.
follow-up: 5 comment:4 by , 6 years ago
HI Christian:
- The patch use jglb output, it should better than jout. It clear and easy to read.
- Does I need add prefix "scsi_" to newly key name?
- I add new patch for your requirement. patch_json_error_counter_log_1.00.diff
Thanks.
comment:5 by , 6 years ago
- 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
.
- 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).
- 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 , 6 years ago
Attachment: | patch_ChangeLog_v1.01.diff added |
---|
by , 6 years ago
Attachment: | patch_scsiprint_v1.01.diff added |
---|
comment:6 by , 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"
follow-up: 8 comment:7 by , 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 , 6 years ago
Attachment: | patch_scsiprint_v1.01.2.diff added |
---|
comment:8 by , 6 years ago
Replying to Christian Franke:
Patch rejected because it apparently renames all
pout
tojout
. Please rename only thosepout
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.
by , 6 years ago
Attachment: | patch_scsiprint_v1.01.3.diff added |
---|
comment:9 by , 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 , 6 years ago
Attachment: | patch_ChangeLog_v1.02.diff added |
---|
by , 6 years ago
Attachment: | patch_scsiprint_v1.02.diff added |
---|
by , 6 years ago
Attachment: | error_counter_log.json added |
---|
comment:10 by , 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:12 by , 6 years ago
Hi Sir:
Does this patch has any problem or advise? Does it have chance to merged?
comment:13 by , 6 years ago
Patch needs to be reviewed by the owner of the SCSI code. This may take some time.
comment:14 by , 6 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 , 6 years ago
Hi Christian is there any way to find contact information for the SCSI code owner?
comment:16 by , 6 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 , 6 years ago
Milestone: | undecided → Release 6.7 |
---|---|
Owner: | set to |
Status: | assigned → accepted |
comment:18 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
Patch applied in r4815, thanks!
Sorry, I did not know how to submit patch file for this issue. I attach the scsiprint.cpp for reference.