Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does smartctl always output JSON, even when there's an error? And what about stderr in case the command failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still would have checked the return code, and whenever the return code differs from 0 and 4, replace result[disk] with something which contains the return code, raw stdout and stderr (but just for the affected disks). That's why I'd also check with XO so that we know how they would handle this case. Do they display the JSON, uninterpreted? Do they look for specific pieces of information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we are already returning stdout that contains the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they just display the raw json (and we see the error code in it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this, the man page and a test I made gives me all I wanted to know: it wraps the call to "regular" smartctl and apparently always puts the result inside JSON, even when there are non-recoverable errors (unless there's an error generating the JSON itself, I guess).
See this example where I gave it a non recognised argument:
So LGTM.