nvme: add dump-command-metadata command - #3603
Conversation
f84edd3 to
edb77d4
Compare
| void argconfig_set_parse_hook(argconfig_parse_hook_fn hook) | ||
| { | ||
| argconfig_parse_hook = hook; | ||
| } |
There was a problem hiding this comment.
The change is a bit big and there are a few things which could go in first before the main feature. That would make it a bit simpler to review. The hook here could go independent (the commit message should explain what it is for).
| * running getopt or touching any state. Used by dump-command-metadata to | ||
| * capture each command's options array. NULL (the default) means normal | ||
| * parsing. | ||
| */ |
There was a problem hiding this comment.
The LLMs tend to overdocument stuff in the source code. This information is good for the commit message, I don't think we should have it here. It's kind of obvious from the name of the function. I a big a fan of self explaining code. Comments are the last resort IMO.
There was a problem hiding this comment.
Removed comment in other PR that has been merged.
| @@ -0,0 +1,129 @@ | |||
| { | |||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | |||
| "$id": "https://github.com/linux-nvme/nvme-cli/command-metadata-schema.json", | |||
There was a problem hiding this comment.
Could we place this file somewhere else? I still want to cleanup the root directory eventually, it's crowded with code, configuration files, compliance files, project documentation, etc.
For the config-schema.json.in is currently in libnvme/doc/ what about having it in Documentation?
There was a problem hiding this comment.
Moved to new plugin directory.
| * json-c support: nvme-builtin.h does not register it and nvme.c does not | ||
| * define its handler, so dump_command_metadata() is never referenced. | ||
| */ | ||
| #ifdef CONFIG_JSONC |
There was a problem hiding this comment.
I would suggest to make this command a build option instead coupling it to CONFIG_JSONC being around. This way we can disable in default builds and only enable when updating the tab completion files.
| /* | ||
| * Suppress stdout/stderr while invoking command fns: a few commands | ||
| * print before they reach the parser (e.g. gen-hostnqn), and some emit | ||
| * parse-error diagnostics in reaction to the capture sentinel. |
There was a problem hiding this comment.
I was looking at gen-hostnqn (and another one) recently, I a noticed it doesn't use the arg parser. I think we should add this to those commands. Every command should invoke the arg parser otherwise the globals argument options are not supported.
There was a problem hiding this comment.
There are a few other commands that don't use the arg parser. I think this is the definitive list:
- gen-hostnqn
- show-hostnqn
- zns list
- micron plugin-version
- micron cloud-SSD-plugin-version
- seagate plugin-version
- seagate cloud-SSD-plugin-version
Do we want all of these to call the argument parser. I am pretty sure at the very least the --output-format will work on all of them. If so, this should probably be in it's own PR.
There was a problem hiding this comment.
I modified this comment to make it more generic. Once we change these commands, the comment would have been stale.
There was a problem hiding this comment.
Yes, I think we should fix those commands up to use the nvme arg parser. I agree this should go into a separate PR.
| if json_c_dep.found() | ||
| sources += [ | ||
| 'nvme-print-json.c', | ||
| 'command-metadata.c', |
There was a problem hiding this comment.
Let's add a meson build option for this feature.
|
Looks good overall. I'd like to have some of the stuff as preparation patches. And making an dedicated build option, this is not useful for normal users. |
I think this could be useful for normal users. It would allow consuming scripts to determine if a newer command exists, or change how it calls a command based on changes to its options. I've seen too much code in other projects that checks for version numbers and make decisions based on that. This provides usees a machine readable structured representation of the API to use to make decisions within their scripts. |
|
Fair enough. I haven't thought about the runtime introspection use case. I suppose this would be useful when wrapping a GUI on top? What about moving the command into a plugin. I'd like to avoid cluttering the top name space and gives the user the opportunity decided if it's enabled or not. Furthermore, it would bundle the code in one directory. |
edb77d4 to
340815c
Compare
|
Rebased onto current master. The prep changes this depended on are now merged upstream, so the remaining diff is just the command, schema, and test. Also reworded the subject to the nvme: prefix. |
jimmunn
changed the title
340815c to
3ee848e
Compare
|
Added the missing copyright lines in command-metadata.c and command-metadata.h. |
3ee848e to
0564252
Compare
I like the idea of a plugin. Now comes the hardest part of programming, naming. We bounced around some ideas here and we came up with "utils", "cli" and "cli-utils" (plus a few others that didn't make the cut). So, it would look like one of these: I don't know what other commands, if any, may end up in this plugin. The concern with "utils" is that it could become a junk drawer of miscellaneous commands, but that may be OK. I am leaning towards "utils" but am fine with the others. Do you have a preference? Do you want the creation of the new plugin in its own PR, or I could just create a new commit in this PR that just adds the plugin then another commit to move the dump-command-metadata command and related files to the new plugin. |
|
Naming is difficult. I didn't propose any names in my original comment because I could only think of bad ones, and I didn't want to influence your suggestions. My initial idea was Both It's fine if you create the plugin within this PR or as a separate PR. It's both fine. What is simpler for you works for me. |
ccb53c0 to
2a4b0f5
Compare
|
I was about to merge the PR but then I noticed that the Windows build fails: From the build logs, it looks like there is json-c support enabled. So not clear what's going on here. BTW, I think you can squash all three patches together. I would have done that anyway :) |
| (void)write(fd, &"0123456789"[sig / 100], 1); | ||
| if (sig >= 10) | ||
| (void)write(fd, &"0123456789"[(sig / 10) % 10], 1); | ||
| (void)write(fd, &"0123456789"[sig % 10], 1); |
There was a problem hiding this comment.
uff... please use something like
static void write_uint(int fd, unsigned int n)
{
char buf[3];
int i = sizeof(buf);
do {
buf[--i] = '0' + (n % 10);
n /= 10;
} while (n && i);
write(fd, buf + i, sizeof(buf) - i);
}
write_str(fd, "' crashed during option capture (signal ");
write_uint(fd, sig);
write_str(fd, ")\n");There was a problem hiding this comment.
I added your write_uint function. I also added a corresponding write_hex function to follow the same pattern.
There was a problem hiding this comment.
I added write_raw to avoid a build failure.
|
I'm going to merge the first part of PR, then you can rebase and update the last patch. |
ed79109 to
3542a1c
Compare
There was a problem hiding this comment.
Pull request overview
Adds infrastructure to expose nvme-cli’s full command/option surface as machine-readable JSON, intended as an input for generating shell completion scripts and for drift checks/validation in CI.
Changes:
- Improve resilience/diagnostics around metadata capture by installing crash/exception handlers and preserving a usable stderr for fatal messages.
- Enhance the Python schema test’s failure reporting by returning stderr alongside parsed JSON and including it in assertion messages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| unit-py/test_command_metadata_schema.py | Improves test diagnostics by propagating stderr from dump-command-metadata into assertion output. |
| plugins/utils/command-metadata.c | Adds crash/exception handling and stderr preservation during option-capture to avoid silent failures and corrupted JSON output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void write_str(int fd, const char *s) | ||
| { | ||
| if (s) | ||
| write_raw(fd, s, strlen(s)); | ||
| } | ||
|
|
||
| static void write_uint(int fd, unsigned int n) | ||
| { | ||
| char buf[3]; | ||
| int i = sizeof(buf); | ||
|
|
||
| do { | ||
| buf[--i] = '0' + (n % 10); | ||
| n /= 10; | ||
| } while (n && i); | ||
|
|
||
| write_raw(fd, buf + i, sizeof(buf) - i); | ||
| } |
There was a problem hiding this comment.
I didn't know that strlen is not safe to use in the signal handler...
There was a problem hiding this comment.
I addressed the Copilot reported issues in the latest push:
- write_str now computes the length inline instead of calling strlen, which isn't async-signal-safe.
- write_uint's buffer is widened to 10 bytes so any 32-bit unsigned int formats without dropping leading digits.
write_hex already sized its buffer correctly, so it's unchanged.
|
I've tried this on Linux and it doesn't produce any output: For this test I added a |
The lack of output here is by design and expected. If a command crashes while capturing its options, then we didn't capture them — so any output would be silently incomplete. Rather than emit a partial/misleading dump, we emit nothing. Also, SIGTERM isn't a good signal for testing this. SIGTERM can't be raised by the running code itself — it's delivered from outside the process (a kill, a timeout, a service manager). So we don't handle it, and we don't print that a command crashed, because one didn't: the process was terminated externally. To exercise the crash path, inject raise(SIGSEGV) or abort() (SIGABRT) instead — those are the fault signals the capture handler catches, and you'll see the dump-command-metadata: fatal: '' crashed during option capture diagnostic. |
101578f to
78587da
Compare
|
ah okay, that explains it. btw, copilot was unhappy about the haven't really spend time on trying to make sense of it. if you say it is bogus, I'll merge this version. |
Print a diagnostic naming the offending command if it crashes during the capture pass. Surface that diagnostic in the schema test's failure output so a capture crash is diagnosable from CI logs. Signed-off-by: Jim Munn <jlmunn@micron.com>
78587da to
03c0597
Compare
Yeah, it's bogus. Both signal() and raise() are explicitly listed as async-signal-safe in signal-safety(7). The diagnostic already uses write(2), and the reset-to-SIG_DFL-then-raise() is intentional — it preserves the core dump and the correct exit status. Copilot's other comment on that branch was legit, though: install_crash_handlers() didn't guard against signal() returning SIG_ERR, which could then get passed back to signal() on restore. I've pushed a fix that normalizes SIG_ERR to SIG_DFL at install time (via a small set_crash_handler() helper), so restoration is always valid. That's the only delta since you last looked. |
|
Alright, thanks for looking into it. I try to stay away from signals, this is such a stupid and horrible API. |
|
Thanks a lot! |
Add
nvme dump-command-metadata, which walks the live plugin/command tree and emits every command and its options as JSON describing the CLI surface. The command needs no device and is gated on CONFIG_JSONC.This is the first step toward generating shell completion scripts: the JSON output is intended to be consumed by a generator that produces completions for bash, zsh, and PowerShell.
To capture options, dump-command-metadata invokes each command's fn, which calls argconfig_parse(). A hook there copies the command's options array and returns a sentinel so the command unwinds before opening a device, rather than actually running it.
Includes command-metadata-schema.json and a Python unit test (unit-py/) that validates the output against the schema and cross-checks it against --help and
nvme help.