nvme: make --opcode argument mandatory#2786
Conversation
| if (err) | ||
| return err; | ||
|
|
||
| if (!argconfig_parse_seen(opts, "opcode")) { |
There was a problem hiding this comment.
As far I can tell, this was not enforced before, it would be a behavior change. @jk-ozlabs
Also the documentation doesn't say it's mandatory.
But I see why you added it. The help says: "opcode (required)".
There was a problem hiding this comment.
I'm fine with the requirement; a default opcode of 0 maps to "Read NVMe-MI data structure", and it would be a bit odd to be relying on that as an implicit command to issue.
I would agree with adding to the docs though.
There was a problem hiding this comment.
Updated the documentations to add the description. (And also rebased the PR branch.) Thank you.
| return err; | ||
| } | ||
|
|
||
| if (!argconfig_parse_seen(opts, "opcode")) { |
There was a problem hiding this comment.
I think we should also update the man pages. They don't mention this option is required, only the help text from above does so.
|
Ah didn't realize I asked for this in #2103 :) For the passthrue case it makes sense. Let's see if for nvme-mi this is also necessary. |
|
The nvme-mi added by the PR: #1887 I created as the opcode option as required as below so looks make sense. Thank you. static int nvme_mi(int argc, char **argv, __u8 admin_opcode, const char *desc)
{
const char *opcode = "opcode (required)";Note: This opcode option is same with the passthrue case as required. |
The shorthand -o change for --output from --opcode before. Then the default opcode value zero used incorrectly if the -o used. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
The opcode behavior changed as required argument. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
ikegami-t
force-pushed
the
nvme-check-opcode
branch
from
922937b to
db27cde
Compare
|
Thanks! |
4 participants
The shorthand -o change for --output from --opcode before. Then the default opcode value zero used incorrectly if the -o used.