lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(xce-test-cli-ux): build flag-descriptor parsing/rejection core

Browse source 
Adds flagspec_t descriptor table, getopt_long option-table builder, and
the shared stderr-routed rejection path that every flag-accepting
command will migrate onto in later tasks. TDD'd via test_flagspec.c
(1273/1273 controller suite passing).

Task 1.1 of .kiro/specs/xce-test-cli-ux/tasks.md.
Requirements: 2.1, 2.2, 2.3, 2.4, 3.1

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019jaMf9QEwpugH2W9KAuc7f
This commit is contained in:
Rongsong Shen 2026-09-09 07:16:36 +00:00
parent 7bcafa68e7
commit 0748f10cc6
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
6 changed files with 827 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

180
.kiro/specs/xce-test-cli-ux/tasks.md Normal file
View file

@ -0,0 +1,180 @@
# Implementation Plan
- [ ] 1. Foundation: shared libraries, process-level fixes, and test harness
- [x] 1.1 Build the flag-descriptor table format and getopt_long-based parsing/rejection core
- Define the flag-descriptor structure (short form, long form, concept name, required flag, default description) and a builder that produces a getopt_long-compatible option table from it
- Implement the shared rejection path: an unrecognized or malformed flag prints the offending flag plus the command's usage summary to stderr and returns a nonzero result
- Write the unit test first (red), then implement until it passes (green): a fake command's descriptor table correctly builds its option table, and both an unrecognized flag and a flag missing its required value are rejected with the right message
- Observable completion: running the new unit test suite shows the descriptor-driven rejection producing the same messages for a made-up two-flag test command regardless of which flag is malformed, and confirms the rejection text lands on stderr, not stdout
- _Requirements: 2.1, 2.2, 2.3, 2.4, 3.1_
- _Boundary: Flag Parsing Layer_
- [ ] 1.2 Build the flag typo-suggestion utility and the generated help-flag-block renderer
- Add a small edit-distance comparison so an unrecognized flag that closely matches exactly one flag in the descriptor table is named as the likely intended one in the rejection message
- Add a renderer that produces the flag-listing portion of help text (short form, long form, required/optional, default) directly from the descriptor table
- Observable completion: the unit test suite shows a single close-match flag typo producing a "did you mean" suggestion, a non-close-match producing no suggestion, and the renderer producing a correctly formatted block for a multi-flag test table
- _Requirements: 6.1, 6.2, 6.3, 6.4, 8.2, 10.1, 10.2, 10.3_
- _Boundary: Flag Parsing Layer_
- _Depends: 1.1_
- [ ] 1.3 Correct the SIGINT installation and expose an interruption accessor
- Replace the process's existing signal-based SIGINT installation with an explicit installation that clears the automatic-restart behavior, so a blocking read reliably returns an interrupted result instead of silently retrying
- Expose a small accessor over the existing interrupt flag so the interruptible-call adapter (built next, in 1.4/1.5) can poll it without depending on the flag's storage details
- Observable completion: a small standalone test that sends an interrupt signal to the process during a blocking read confirms the read returns immediately with an interrupted result rather than continuing to block
- _Requirements: 9.9_
- [ ] 1.4 Build the interruptible-call adapter's heap-block-ownership mechanism
- Implement a wrapper that runs a given blocking call on a detached worker thread while the caller polls the interruption accessor from 1.3 on a short interval
- Implement the atomic ownership handoff so that whichever side (the polling caller or the worker) is left holding the call's heap-allocated state is the one that frees it, with no path that frees it from both sides
- Observable completion: the unit test suite shows a normal (uninterrupted) call returning its real result and freeing cleanly, and a forced-timeout call returning immediately with no double-free and no leak (verified under the project's leak/ASAN tooling)
- _Requirements: 1.1, 1.2, 1.3, 1.4_
- _Boundary: Interruptible Call Adapter_
- _Depends: 1.3_
- [ ] 1.5 Add the per-connection lock registry to the interruptible-call adapter
- Add a small registry, keyed by connection identity, that serializes any two calls sharing a connection so an abandoned call's worker thread is never running concurrently with a newly issued call on the same connection
- Make lock acquisition itself interruptible, so a new call waiting on a connection still held by a prior abandoned call can itself be cancelled
- Observable completion: the unit test suite shows two overlapping calls against the same fake connection identity (one abandoned, one issued immediately after) never executing concurrently, while two calls against different connection identities run independently
- _Requirements: 1.1, 1.5_
- _Boundary: Interruptible Call Adapter_
- _Depends: 1.4_
- [ ] 1.6 Add process-level help and version discovery
- Ensure invoking the process with a help or version flag prints the appropriate output and exits without starting the interactive/scripted session
- Extend the process-level help output to describe the in-session help command as the way to list and learn available commands
- Observable completion: invoking the process with the help flag prints usage text mentioning the in-session help command, and invoking with the version flag prints version information, both without entering the REPL
- _Requirements: 4.1, 4.2, 4.3_
- _Boundary: Process Entry_
- [ ] 1.7 Build the black-box CLI test harness
- Add a subprocess/pty-driven test harness capable of launching the built client, sending input lines (including partial input followed by a tab keypress and raw signals), and capturing its stdout/stderr/exit code separately
- Observable completion: a smoke test using the new harness launches the client, sends a trivial command, and confirms the expected output arrives on the correct stream with the expected exit code
- _Boundary: Testing Infrastructure_
- [ ] 2. Core: independent new capabilities and standalone fixes
- [ ] 2.1 (P) Build the destructive-command confirmation gate and its session-scoped toggle
- Implement a confirmation check that prompts the operator on stderr before proceeding, returns a distinct "not run" outcome on decline, and treats an interrupt during the prompt the same as a decline
- Add a bypass flag usable per invocation and a session-scoped toggle (mirroring the existing continue-on-error toggle's style) that both default to requiring confirmation and both reset at process start
- Ensure the confirmation check is skipped automatically when input is not from an interactive session
- Observable completion: the unit test suite shows a simulated non-interactive session skipping the prompt, a simulated decline and a simulated interrupt both producing the "not run" outcome, and the toggle switching prompting on and off
- _Requirements: 9.1, 9.2, 9.3, 9.4, 9.6, 9.7, 9.8, 9.9_
- _Boundary: Confirmation Gate_
- _Depends: 1.1, 1.3_
- [ ] 2.2 (P) Build the JSON list-output helper library
- Implement shared helpers for building a JSON array of row objects and printing it, usable by any listing command's existing per-row data
- Observable completion: the unit test suite shows a multi-row, multi-field test payload serializing to valid JSON with no field truncated regardless of value length
- _Requirements: 5.1, 5.3_
- _Boundary: JSON List Formatter_
- [ ] 2.3 (P) Build command-name tab completion scoped to the active context
- Implement completion that, when the session is interactive, offers only command names valid in the currently active context, completes an unambiguous partial match in full, and lists candidates without altering the input line when more than one match exists
- Observable completion: a black-box test (using the harness from 1.7) drives a partial command name plus a tab keypress in two different contexts and confirms the completion offered differs by context and matches the single-match/multiple-match behavior
- _Requirements: 7.1, 7.2, 7.3, 7.4_
- _Boundary: Tab Completion_
- _Depends: 1.7_
- [ ] 2.4 (P) Build the in-session source command with missing-file and cycle detection
- Implement a command that reads and executes another file's lines in order, reusing the existing rc-file loading primitive, reporting an explicit error (rather than silently doing nothing) when the file does not exist or cannot be read
- Add cycle detection using a canonicalized in-flight path stack so a file that sources itself, directly or transitively, is rejected with an error instead of hanging
- Ensure a failing line inside the sourced file follows the session's current abort-on-failure/continue-on-error setting, and that nested sourcing (one file sourcing another, non-cyclically) composes correctly
- Observable completion: the unit/integration test suite shows a two-file mutual-cycle case rejected with an error, a missing-file case rejected with an error, and a nested non-cyclic case executing every line from both files
- _Requirements: 13.1, 13.2, 13.3, 13.4, 13.5, 13.6_
- _Boundary: Source Command_
- [ ] 2.5 (P) Fix bug-1711's display and text defects
- Correct the misspelled unrecognized-worker-status text and the misspelled default launch-type label (the malformed help-not-found grammar is fixed in 3.5, alongside that same code path's broader rework)
- Left-justify the resource-listing command's text columns to match the worker-listing command's convention, and label/prefix its flags column as hexadecimal
- Format the job-listing command's submit/start/finish time columns using the existing human-readable time formatting instead of raw epoch seconds, split its combined task-count column into two correctly labeled columns, and add a raw-state fallback for its ambiguous run-plus-exit state
- Replace the configuration-listing command's silently empty group/config sections with either their real contents or an explicit not-implemented marker
- Observable completion: running each affected command against a small fixture shows the corrected text/formatting for every defect listed above
- _Requirements: 11.1, 11.3, 11.4, 11.5, 11.6, 11.7, 11.8_
- _Boundary: Bug-1711 Fixes_
- [ ] 2.6 (P) Fix bug-1557's join maxmem default
- Correct the simulated worker-join command's default memory value to the codebase's current bytes-canonical convention
- Observable completion: joining a simulated worker without specifying memory reports the corrected byte-scale default instead of the stale value
- _Requirements: 12.1_
- _Boundary: Bug-1557 Fix_
- [ ] 3. Integration: migrate existing commands onto the new foundations
- [ ] 3.1 Migrate every flag-accepting command's parsing to the shared validation library
- Convert every command currently parsing flags via the older mechanisms (including the one command using a fully hand-rolled long-flag parser) to the shared descriptor-table-driven parsing built in 1.1/1.2, so every one uniformly rejects an unrecognized or malformed flag and gains a long-form equivalent for each concept
- While migrating the queue-control command, initialize its action selection to an explicit unset sentinel and reject the invocation if none of its mutually-exclusive action flags was given, matching its sibling host-control command's existing pattern
- Observable completion: every migrated command rejects a deliberately invalid flag with a usage summary and, where applicable, a suggestion, and the queue-control command rejects an invocation given none of its action flags instead of transmitting an indeterminate value
- _Requirements: 2.1, 2.2, 2.3, 2.4, 6.1, 6.2, 6.3, 6.4, 10.1, 10.2, 10.3_
- _Boundary: Flag Parsing Layer_
- _Depends: 1.1, 1.2_
- [ ] 3.2 Wrap every blocking request with the interruptible-call adapter
- Route every command's blocking request through the interruptible-call adapter built in 1.4/1.5, so an interrupt during any of them abandons the wait immediately, reports the command as failed, and leaves the active connection and context intact
- Ensure the session never exits as a side effect of a single interrupt keypress, and that an interrupt at an empty prompt simply discards the current input line
- Observable completion: interrupting a command that is blocked waiting on a request returns control to the prompt immediately (well before the request's own timeout) and the same connection accepts a subsequent command normally
- _Requirements: 1.1, 1.2, 1.3, 1.4, 1.5_
- _Boundary: Interruptible Call Adapter_
- _Depends: 1.4, 1.5, 3.1_
- [ ] 3.3 Wire the five destructive commands to the confirmation gate
- Gate the job-kill, shutdown, queue-close/inactivate, host-close, and configuration-upgrade actions behind the confirmation gate built in 2.1, using each command's now-correctly-parsed flags to decide whether a given invocation counts as destructive
- Add the bypass flag to each of these commands via the shared flag-descriptor table
- Observable completion: each of the five destructive actions prompts interactively by default, proceeds without prompting when the bypass flag or the session toggle is used, and proceeds without prompting when input is not interactive; separately, confirm that a reversal invocation (queue-active/open, host-open) and the fake-worker job-lifecycle commands never prompt at all, since they are excluded from the destructive set
- _Requirements: 9.1, 9.2, 9.3, 9.4, 9.5, 9.6, 9.7, 9.8, 9.9_
- _Boundary: Confirmation Gate_
- _Depends: 2.1, 3.1_
- [ ] 3.4 Add JSON output mode to the three listing commands
- Bring the worker-listing, job-listing, and resource-listing commands onto the shared flag-descriptor library from 1.1 for the first time (none currently accepts any flag), adding a consistently named output-format flag that uses the JSON list-output library from 2.2 to emit every field present in each command's human-readable table, unabbreviated
- Leave each command's existing human-readable output unchanged when the new flag is not given
- Observable completion: running each of the three listing commands with the new flag against a fixture with a long field value produces valid JSON containing every table field with no truncation, while the default (no flag) invocation still prints the existing table
- _Requirements: 5.1, 5.2_
- _Boundary: JSON List Formatter_
- _Depends: 1.1, 2.2, 2.5_
- [ ] 3.5 Generate and consolidate every command's help content
- Regenerate each command's flag-listing help text from the shared descriptor table (built in 1.2), and revise each command's remaining help text to include a purpose description, at least one example invocation, which contexts it is valid in when shared across contexts, and any related command or scripting side effect
- Where a shared command's behavior differs by active context, describe the current context's specific behavior rather than a generic description
- Ensure a help lookup for a name not found in the active context reports it as not found there without checking other contexts, and correct the existing malformed not-found message text
- Document the join command's corrected default from 2.6, and the JSON output flag added in 3.4, in their respective commands' help text
- Observable completion: looking up help for a representative command in each context shows a description, full flag documentation, an example, and (where applicable) related-command notes; looking up help for one of the three listing commands explicitly shows how to request its JSON output mode; and looking up an unrecognized name shows the corrected not-found message with no other-context hint
- _Requirements: 5.4, 8.1, 8.2, 8.3, 8.4, 8.5, 8.6, 8.7, 11.2, 12.2_
- _Boundary: Help Renderer_
- _Depends: 1.2, 3.1, 2.6, 3.4_
- [ ] 3.6 Audit and correct stdout/stderr stream routing across existing error paths
- Review every command's remaining error paths not already covered by the shared flag-rejection path built in 1.1 — request failures and internal errors — and ensure diagnostic/error text goes to stderr while a command's successful result data goes to stdout only
- Confirm the same routing holds identically whether stdout and stderr are both attached to a terminal or redirected to separate destinations (a script or pipe)
- Observable completion: redirecting only stdout to a file while triggering a request failure or an internal error on a representative command shows the diagnostic text still appearing on the terminal while the redirected file contains no diagnostic text; separately, running a successful command with only stdout redirected shows its result data captured in the redirected file with nothing sent to stderr
- _Requirements: 3.1, 3.2, 3.3, 3.4_
- _Boundary: Command Dispatcher_
- _Depends: 3.1, 3.2, 3.3, 3.4, 3.5_
- [ ] 4. Validation: end-to-end verification
- [ ] 4.1 Verify Ctrl-C cancellation end-to-end
- Confirm interrupting a command blocked on a real request abandons the wait well before its own timeout, reports the command as failed, and leaves the session's context and connection usable
- Confirm that issuing a second command on the same connection immediately after an interrupt does not race the abandoned request, and that a command on a different connection is unaffected
- Observable completion: a live end-to-end run demonstrates both the immediate-cancellation behavior and the no-overlap behavior on the same connection
- _Requirements: 1.1, 1.2, 1.3, 1.4, 1.5_
- _Depends: 3.2, 1.7_
- [ ] 4.2 Verify the confirmation gate end-to-end
- Confirm each of the five destructive commands prompts by default, respects its bypass flag and the session toggle, and auto-skips under non-interactive input
- Confirm that a real interrupt signal delivered while the confirmation prompt is displayed is observed as an interruption and reported as not run, not as failed
- Observable completion: a live end-to-end run against each destructive command demonstrates the prompt, the bypass, the toggle, the non-interactive auto-skip, and the real-signal-during-prompt case
- _Requirements: 9.1, 9.2, 9.3, 9.4, 9.5, 9.6, 9.7, 9.8, 9.9_
- _Depends: 3.3, 1.7_
- [ ] 4.3 Verify source's interaction with the confirmation gate
- Confirm a destructive command inside a sourced file, run within an otherwise-interactive session, still prompts for confirmation unless the session toggle is set, proving the gate's interactive check is based on the top-level session rather than on whether the current line came from a file
- Observable completion: a live end-to-end run sources a file containing a destructive command from an interactive session and observes the prompt appear, then observes it skipped once the toggle is set
- _Requirements: 13.7_
- _Depends: 2.4, 3.3, 1.7_
- [ ] 4.4 Verify process-level help, tab completion, and consolidated help content end-to-end
- Confirm the help/version flags and in-session help behavior from 1.6 and 3.5, and the tab-completion behavior from 2.3, hold together in the full built client, not just in isolation
- Observable completion: a black-box run confirms the help flag's output describes the in-session help command, a representative command's help output includes its generated flag block and hand-authored example, and tab completion in two different contexts offers the expected candidates
- _Requirements: 4.1, 4.2, 4.3, 7.1, 7.2, 7.3, 7.4, 8.1, 8.2, 8.3, 8.4, 8.5, 8.6, 8.7, 11.2_
- _Depends: 1.6, 2.3, 3.5, 1.7_
## Implementation Notes
- `src/tools/xce-build.sh controller asan xce.build --incremental` writes its build dir at `src/xce.build/` (relative to the script's own location), not a repo-root `xce.build/` — meson logs are at `src/xce.build/meson-logs/testlog.txt`. (task 1.1)
- New Unity test files need two wiring points, not just meson.build's source list: also register an `extern void test_<module>(void);` + `TEST(test_<module>);` in `src/controller/testsuite/controller_test.c`, mirroring the existing `test_rcfile` registration, or the suite silently never runs. (task 1.1)

4
src/controller/meson.build
View file

@ -55,7 +55,7 @@ controller = executable('xce_controller',
link_args: wrap_link_args, link_args: wrap_link_args,
install: true) install: true)
xce_test_src = [ 'testsuite/test.c', 'testsuite/waldump.c', 'testsuite/walcompare.c', 'testsuite/snapverify.c', 'testsuite/snapdiff.c', 'testsuite/configcompare.c', 'testsuite/resval_typeresolve.c', 'testsuite/rcfile.c'] xce_test_src = [ 'testsuite/test.c', 'testsuite/waldump.c', 'testsuite/walcompare.c', 'testsuite/snapverify.c', 'testsuite/snapdiff.c', 'testsuite/configcompare.c', 'testsuite/resval_typeresolve.c', 'testsuite/rcfile.c', 'testsuite/flagspec.c']
# BSD-licensed readline replacement (scheduler-core spec, task 12.1 review # BSD-licensed readline replacement (scheduler-core spec, task 12.1 review
# finding): GNU readline is GPL; xce_test's REPL only needs readline()/ # finding): GNU readline is GPL; xce_test's REPL only needs readline()/
@ -168,6 +168,8 @@ controller_unit_test_src = [
'testsuite/configcompare.c', 'testsuite/configcompare.c',
'testsuite/test_rcfile.c', 'testsuite/test_rcfile.c',
'testsuite/rcfile.c', 'testsuite/rcfile.c',
'testsuite/test_flagspec.c',
'testsuite/flagspec.c',
'testsuite/test_persist_snapshot_entities.c', 'testsuite/test_persist_snapshot_entities.c',
'testsuite/test_apply_seam.c', 'testsuite/test_apply_seam.c',
'testsuite/test_apply_committed_entry.c', 'testsuite/test_apply_committed_entry.c',

2
src/controller/testsuite/controller_test.c
View file

@ -71,6 +71,7 @@ extern void test_snapverify(void);
extern void test_snapdiff(void); extern void test_snapdiff(void);
extern void test_configcompare(void); extern void test_configcompare(void);
extern void test_rcfile(void); extern void test_rcfile(void);
extern void test_flagspec(void);
extern void test_drive_async_config(void); extern void test_drive_async_config(void);
extern void test_slowlane(void); extern void test_slowlane(void);
extern void test_commit_admission_pool(void); extern void test_commit_admission_pool(void);
@ -251,6 +252,7 @@ int main(int argc, char* argv[])
TEST(test_snapdiff); TEST(test_snapdiff);
TEST(test_configcompare); TEST(test_configcompare);
TEST(test_rcfile); TEST(test_rcfile);
TEST(test_flagspec);
TEST(test_drive_async_config); TEST(test_drive_async_config);
TEST(test_slowlane); TEST(test_slowlane);
TEST(test_commit_admission_pool); TEST(test_commit_admission_pool);

119
src/controller/testsuite/flagspec.c Normal file
View file

@ -0,0 +1,119 @@
#define _GNU_SOURCE
#include "flagspec.h"
#include <errno.h>
#include <stdio.h>
#include <string.h>
/* True iff `token` (a raw command-line argument, e.g. "-h", "--host", or
* -- defensively -- a dash-free bare name) identifies the flag described
* by `fs`. A "--name" token is matched against fs->long_opt; a "-X" token
* is matched against fs->short_opt (single character only, so "-xy" never
* falsely matches a single-letter short_opt). */
static int flag_token_matches(const char* token, const flagspec_t* fs)
{
if (!token || !fs)
return 0;
if (token[0] == '-' && token[1] == '-')
return fs->long_opt != NULL && strcmp(token + 2, fs->long_opt) == 0;
if (token[0] == '-' && token[1] != '\0')
return fs->short_opt != 0 && token[1] == fs->short_opt &&
token[2] == '\0';
/* No leading dash at all -- not how a real getopt_long() failure
* token looks, but handled so this helper never mis-signals on
* unexpected input from a caller that skips the usual
* argv[optind - 1] convention. */
if (fs->long_opt != NULL && strcmp(token, fs->long_opt) == 0)
return 1;
return fs->short_opt != 0 && token[0] == fs->short_opt &&
token[1] == '\0';
}
int flagspec_build_opts(const flagspec_t* spec, size_t n,
struct option* out_opts, char* out_optstring,
size_t out_optstring_cap)
{
if (!spec || !out_opts || !out_optstring || n == 0 ||
out_optstring_cap == 0)
return -EINVAL;
size_t str_idx = 0;
for (size_t i = 0; i < n; i++) {
const flagspec_t* fs = &spec[i];
if (!fs->long_opt || !*fs->long_opt)
return -EINVAL;
if (fs->has_arg != no_argument && fs->has_arg != required_argument)
return -EINVAL;
out_opts[i].name = fs->long_opt;
out_opts[i].has_arg = fs->has_arg;
out_opts[i].flag = NULL;
out_opts[i].val = fs->short_opt
? (int)fs->short_opt
: (int)(FLAGSPEC_LONG_ONLY_BASE + i);
if (fs->short_opt != 0) {
size_t need = (fs->has_arg == required_argument) ? 2 : 1;
/* Leave room for the final NUL written after the loop. */
if (str_idx + need >= out_optstring_cap)
return -ENOSPC;
out_optstring[str_idx++] = fs->short_opt;
if (fs->has_arg == required_argument)
out_optstring[str_idx++] = ':';
}
}
out_optstring[str_idx] = '\0';
/* Trailing all-zero sentinel getopt_long() requires to terminate the
* table; out_opts must have room for n+1 entries (precondition). */
out_opts[n].name = NULL;
out_opts[n].has_arg = 0;
out_opts[n].flag = NULL;
out_opts[n].val = 0;
return 0;
}
int flagspec_reject(const char* cmd_name, const flagspec_t* spec, size_t n,
const char* bad_flag, const char* usage_text)
{
const char* display = bad_flag ? bad_flag : "?";
const char* who = cmd_name ? cmd_name : "?";
const flagspec_t* match = NULL;
if (spec) {
for (size_t i = 0; i < n; i++) {
if (flag_token_matches(bad_flag, &spec[i])) {
match = &spec[i];
break;
}
}
}
if (match && match->has_arg == required_argument) {
fprintf(stderr, "xce_test: %s: flag '%s' requires a value\n", who,
display);
} else if (match) {
fprintf(stderr, "xce_test: %s: flag '%s' does not take a value\n",
who, display);
} else {
fprintf(stderr, "xce_test: %s: unrecognized flag '%s'\n", who,
display);
}
if (usage_text && *usage_text) {
size_t len = strlen(usage_text);
if (len > 0 && usage_text[len - 1] == '\n')
fprintf(stderr, "usage: %s", usage_text);
else
fprintf(stderr, "usage: %s\n", usage_text);
}
return -EINVAL;
}

130
src/controller/testsuite/flagspec.h Normal file
View file

@ -0,0 +1,130 @@
#pragma once
/*
* flagspec.h -- flag-descriptor table format and getopt_long-based parsing
* core for xce_test's command-line surface (xce-test-cli-ux spec, task 1.1;
* Requirements 2.1-2.4, 3.1; the flagspec_t shape is a stable contract also
* relied on by task 1.2 (typo suggestion, flagspec_render_help) and task
* 3.1 (migrating every existing cmd_* command onto this table)).
*
* bug-1711's root cause was that xce_test's flag parsing had no shared
* helper at all: a plain getopt() loop per command, each with its own
* hand-written switch/default, drifted independently from its
* hand-authored USAGE_* string. This module gives every flag-accepting
* command ONE descriptor table to build its getopt_long() option table
* from and ONE shared rejection path to call on any parse failure, so the
* table is the single source of truth for both.
*
* THIS FILE/TASK (1.1) declares and implements only flagspec_build_opts()
* and flagspec_reject(). flagspec_render_help() (the help-text flag-block
* renderer) and flagspec_reject()'s typo-suggestion behavior are task
* 1.2 and are intentionally NOT declared here yet -- task 1.2 extends
* this header when it adds them.
*/
#include <getopt.h>
#include <stddef.h>
/* One flag's full description: its short/long spellings, the real-world
* concept it represents (for cross-command naming consistency, see
* design.md Requirement 6.2), whether getopt_long() expects a value, and
* documentation state consumed by later tasks (help rendering).
*
* Any two flagspec_t[] tables (i.e. any two commands) that describe the
* "same concept" (e.g. a target host, a job id) MUST use the identical
* long_opt string for it -- enforced by table review, not at runtime. */
typedef struct {
char short_opt; /* 0 if long-only */
const char* long_opt; /* stable, cross-command-consistent name;
* spelled WITHOUT leading dashes, e.g.
* "host", not "--host" */
const char* concept; /* e.g. "host", "jobid", "command-line";
* consumed by help rendering (task 1.2),
* not by flagspec_build_opts()/
* flagspec_reject() */
int has_arg; /* no_argument | required_argument, per
* <getopt.h>; this design uses no
* optional_argument flags anywhere */
int required; /* 1 if the command fails without it;
* enforcement is each command's own
* post-parse-loop check (see design.md's
* bug-1704/cmd_qctrl note), not this
* module's job */
const char* default_desc; /* human text describing the default when
* the flag is omitted, NULL if none;
* consumed by help rendering (task 1.2) */
} flagspec_t;
/* Synthetic getopt_long() `val` assigned to a long-only flag (short_opt
* == 0) at spec index i: FLAGSPEC_LONG_ONLY_BASE + i. Chosen safely above
* the full unsigned-char range (0-255) so it can never collide with a
* real short_opt value, with getopt_long()'s own -1 end-of-options
* sentinel, or with '?' (parse error). Every flagspec_t entry -- long-only
* or not -- therefore returns a value from getopt_long() that a caller's
* switch() can distinguish. */
#define FLAGSPEC_LONG_ONLY_BASE 0x100
/* Builds a getopt_long()-compatible option table and optstring from a
* flagspec_t descriptor table.
*
* `out_opts` must have room for at least n+1 entries -- this function
* appends the trailing all-zero sentinel entry getopt_long() requires to
* terminate the table at index n. `out_optstring` is filled with the
* short-option spec getopt_long()'s `optstring` argument expects: each
* flag with a nonzero short_opt contributes that character, followed by
* ':' when has_arg == required_argument; a long-only flag (short_opt ==
* 0) contributes nothing. The result is NUL-terminated.
*
* Invariant: the built optstring never begins with ':' -- this module
* deliberately does not use getopt_long()'s own leading-':' convention
* for distinguishing "missing argument" from "unrecognized option";
* flagspec_reject() makes that distinction itself by looking the
* offending token up in `spec`. The caller must set `opterr = 0` before
* its getopt_long() loop so libc's own diagnostic never doubles up with
* flagspec_reject()'s.
*
* Returns 0 on success. Returns a negative errno on failure:
* -EINVAL spec/out_opts/out_optstring is NULL, n == 0,
* out_optstring_cap == 0, or a spec entry is malformed
* (long_opt is NULL/empty, or has_arg is neither no_argument
* nor required_argument)
* -ENOSPC out_optstring_cap is too small to hold the built optstring
* (including its terminating NUL)
*
* On any failure, the contents of out_opts/out_optstring are unspecified
* -- the caller must not use them. */
int flagspec_build_opts(const flagspec_t* spec, size_t n,
struct option* out_opts, char* out_optstring,
size_t out_optstring_cap);
/* Shared rejection path for an unrecognized or malformed flag, called
* from a command's getopt_long() loop's `case '?':` arm (or right after
* the loop, for a bad_flag captured some other way).
*
* `bad_flag` is the literal command-line token that failed -- e.g. "-z"
* or "--bogus" -- ordinarily read as `argv[optind - 1]` right after
* getopt_long() returns '?'. flagspec_reject() looks `bad_flag` up
* against `spec` (matching a "-X" token by short_opt, a "--name" token by
* long_opt) to decide which case applies:
* - `bad_flag` matches no entry in `spec` ->
* "unrecognized flag" (Requirement 2.1)
* - `bad_flag` matches an entry with has_arg == required_argument ->
* "flag ... requires a value" (Requirement 2.2)
* - `bad_flag` matches an entry that takes no argument (getopt_long()
* still returned '?', e.g. "--flag=value" given for a no_argument
* long flag) -> "flag ... does not take a value"
* Either way, it prints the offending flag and `usage_text` (Requirement
* 2.4) to stderr only (Requirement 3.1 -- never stdout) and returns a
* negative value; it never partially executes the command.
*
* Typo suggestion ("did you mean --host?", Requirement 10) is task 1.2,
* not implemented by this function yet.
*
* `usage_text` is expected to be the command's one-line usage summary
* (e.g. "jobkill -j <jobid> [-s <signo>]"), not its full help block; it
* may be NULL to omit the usage line entirely (defensive only -- every
* real call site always has one).
*
* Returns -EINVAL (a fixed, nonzero, negative-errno result) in every
* case. */
int flagspec_reject(const char* cmd_name, const flagspec_t* spec, size_t n,
const char* bad_flag, const char* usage_text);

393
src/controller/testsuite/test_flagspec.c Normal file
View file

@ -0,0 +1,393 @@
#define _GNU_SOURCE
#include <errno.h>
#include <getopt.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <unity/unity.h>
#include "flagspec.h"
/* ---- fixture: a made-up two-flag test command -------------------------
* "testcmd -h <host> [-c <cmdline>]" -- one required flag with a short
* and long form, one optional flag with a short and long form. Used by
* every test below so the same descriptor table exercises both
* flagspec_build_opts() and flagspec_reject(), and so a rejection's usage
* summary is provably the SAME string no matter which of the two flags is
* the malformed one (the task's stated observable-completion criterion).
* ------------------------------------------------------------------- */
static const flagspec_t g_testcmd_spec[2] = {
{ 'h', "host", "host", required_argument, 1, NULL },
{ 'c', "command", "command-line", no_argument, 0, "off" },
};
static const char g_testcmd_usage[] = "testcmd -h <host> [-c]";
/* ---- stream-capture helpers --------------------------------------------
* Mirrors test_rcfile.c's dup/dup2/freopen technique for proving what a
* function wrote to stderr, extended to capture stdout AND stderr at the
* same time so a test can prove Requirement 3.1: a rejection message
* lands on stderr only, never stdout. Uses a fresh temp dir per test
* (mkdtemp, like test_rcfile.c/test_configcompare.c's fresh_dir()) so
* capture files never collide with anything else in this shared /tmp. */
static char g_dir[256];
static void fresh_dir(void)
{
strcpy(g_dir, "/tmp/xce_flagspec_test_XXXXXX");
TEST_ASSERT_NOT_NULL(mkdtemp(g_dir));
}
static void cleanup_dir(void)
{
char cmd[512];
snprintf(cmd, sizeof(cmd), "rm -rf %s", g_dir);
system(cmd);
}
typedef struct {
int saved_stdout_fd;
int saved_stderr_fd;
char stdout_path[300];
char stderr_path[300];
} capture_t;
static void capture_begin(capture_t* cap)
{
fflush(stdout);
fflush(stderr);
cap->saved_stdout_fd = dup(fileno(stdout));
cap->saved_stderr_fd = dup(fileno(stderr));
TEST_ASSERT_TRUE(cap->saved_stdout_fd >= 0);
TEST_ASSERT_TRUE(cap->saved_stderr_fd >= 0);
snprintf(cap->stdout_path, sizeof(cap->stdout_path), "%s/out", g_dir);
snprintf(cap->stderr_path, sizeof(cap->stderr_path), "%s/err", g_dir);
TEST_ASSERT_NOT_NULL(freopen(cap->stdout_path, "w", stdout));
TEST_ASSERT_NOT_NULL(freopen(cap->stderr_path, "w", stderr));
}
/* Restores the real stdout/stderr fds and reads both captured files back
* into caller-provided, NUL-terminated buffers. */
static void capture_end(capture_t* cap, char* out_stdout,
size_t out_stdout_cap, char* out_stderr,
size_t out_stderr_cap)
{
fflush(stdout);
fflush(stderr);
TEST_ASSERT_TRUE(dup2(cap->saved_stdout_fd, fileno(stdout)) >= 0);
TEST_ASSERT_TRUE(dup2(cap->saved_stderr_fd, fileno(stderr)) >= 0);
close(cap->saved_stdout_fd);
close(cap->saved_stderr_fd);
clearerr(stdout);
clearerr(stderr);
FILE* f;
size_t n;
f = fopen(cap->stdout_path, "r");
TEST_ASSERT_NOT_NULL(f);
n = fread(out_stdout, 1, out_stdout_cap - 1, f);
out_stdout[n] = '\0';
fclose(f);
f = fopen(cap->stderr_path, "r");
TEST_ASSERT_NOT_NULL(f);
n = fread(out_stderr, 1, out_stderr_cap - 1, f);
out_stderr[n] = '\0';
fclose(f);
}
/* ---- flagspec_build_opts() tests --------------------------------------- */
static void test_build_opts_produces_correct_table_for_two_flag_command(void)
{
struct option opts[3];
char optstring[8];
int rc = flagspec_build_opts(g_testcmd_spec, 2, opts, optstring,
sizeof(optstring));
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_STRING("host", opts[0].name);
TEST_ASSERT_EQUAL_INT(required_argument, opts[0].has_arg);
TEST_ASSERT_NULL(opts[0].flag);
TEST_ASSERT_EQUAL_INT('h', opts[0].val);
TEST_ASSERT_EQUAL_STRING("command", opts[1].name);
TEST_ASSERT_EQUAL_INT(no_argument, opts[1].has_arg);
TEST_ASSERT_NULL(opts[1].flag);
TEST_ASSERT_EQUAL_INT('c', opts[1].val);
/* trailing all-zero sentinel getopt_long() requires */
TEST_ASSERT_NULL(opts[2].name);
TEST_ASSERT_EQUAL_INT(0, opts[2].has_arg);
TEST_ASSERT_NULL(opts[2].flag);
TEST_ASSERT_EQUAL_INT(0, opts[2].val);
TEST_ASSERT_EQUAL_STRING("h:c", optstring);
}
static void test_build_opts_assigns_synthetic_val_to_long_only_flag(void)
{
flagspec_t spec[3];
memcpy(spec, g_testcmd_spec, sizeof(g_testcmd_spec));
spec[2] = (flagspec_t){ 0, "json", "output-format", no_argument, 0,
"table" };
struct option opts[4];
char optstring[8];
int rc = flagspec_build_opts(spec, 3, opts, optstring,
sizeof(optstring));
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_STRING("json", opts[2].name);
TEST_ASSERT_EQUAL_INT(no_argument, opts[2].has_arg);
TEST_ASSERT_EQUAL_INT(FLAGSPEC_LONG_ONLY_BASE + 2, opts[2].val);
/* a long-only flag contributes nothing to the short optstring */
TEST_ASSERT_EQUAL_STRING("h:c", optstring);
/* still terminated correctly at the new, later index */
TEST_ASSERT_NULL(opts[3].name);
TEST_ASSERT_EQUAL_INT(0, opts[3].val);
}
static void test_build_opts_rejects_null_spec(void)
{
struct option opts[3];
char optstring[8];
TEST_ASSERT_EQUAL_INT(-EINVAL,
flagspec_build_opts(NULL, 2, opts, optstring,
sizeof(optstring)));
}
static void test_build_opts_rejects_zero_count(void)
{
struct option opts[3];
char optstring[8];
TEST_ASSERT_EQUAL_INT(
-EINVAL, flagspec_build_opts(g_testcmd_spec, 0, opts, optstring,
sizeof(optstring)));
}
static void test_build_opts_rejects_entry_with_null_long_opt(void)
{
flagspec_t bad_spec[1] = {
{ 'x', NULL, "x", no_argument, 0, NULL },
};
struct option opts[2];
char optstring[8];
TEST_ASSERT_EQUAL_INT(
-EINVAL,
flagspec_build_opts(bad_spec, 1, opts, optstring, sizeof(optstring)));
}
static void test_build_opts_rejects_too_small_optstring_buffer(void)
{
struct option opts[3];
/* g_testcmd_spec builds "h:c" (h requires a value, c does not) --
* exactly 4 bytes including the terminating NUL. A 3-byte buffer is
* one short. */
char optstring[3];
TEST_ASSERT_EQUAL_INT(-ENOSPC,
flagspec_build_opts(g_testcmd_spec, 2, opts,
optstring, sizeof(optstring)));
}
/* ---- flagspec_reject() tests: real getopt_long() pipeline -------------
* Each case builds the real option table via flagspec_build_opts(), then
* drives an actual getopt_long() call over a fabricated argv so the
* bad_flag handed to flagspec_reject() is exactly what a real cmd_*
* caller would see from its own `case '?':` arm -- not a hand-picked
* string. */
static int run_getopt_long_expect_error(int argc, char* argv[],
const struct option* opts,
const char* optstring)
{
optind = 1;
opterr = 0;
int opt = getopt_long(argc, argv, optstring, opts, NULL);
TEST_ASSERT_EQUAL_INT('?', opt);
return optind;
}
static void test_reject_rejects_unrecognized_long_flag(void)
{
struct option opts[3];
char optstring[8];
TEST_ASSERT_EQUAL_INT(0, flagspec_build_opts(g_testcmd_spec, 2, opts,
optstring,
sizeof(optstring)));
char* argv[] = { "testcmd", "--bogus", NULL };
int after = run_getopt_long_expect_error(2, argv, opts, optstring);
const char* bad_flag = argv[after - 1];
TEST_ASSERT_EQUAL_STRING("--bogus", bad_flag);
fresh_dir();
capture_t cap;
capture_begin(&cap);
int rc = flagspec_reject("testcmd", g_testcmd_spec, 2, bad_flag,
g_testcmd_usage);
char out_buf[512], err_buf[512];
capture_end(&cap, out_buf, sizeof(out_buf), err_buf, sizeof(err_buf));
TEST_ASSERT_TRUE(rc < 0);
TEST_ASSERT_EQUAL_STRING("", out_buf);
TEST_ASSERT_NOT_NULL(strstr(err_buf, "unrecognized flag '--bogus'"));
TEST_ASSERT_NOT_NULL(strstr(err_buf, "usage: testcmd -h <host> [-c]"));
cleanup_dir();
}
static void test_reject_rejects_unrecognized_short_flag(void)
{
struct option opts[3];
char optstring[8];
TEST_ASSERT_EQUAL_INT(0, flagspec_build_opts(g_testcmd_spec, 2, opts,
optstring,
sizeof(optstring)));
char* argv[] = { "testcmd", "-z", NULL };
int after = run_getopt_long_expect_error(2, argv, opts, optstring);
const char* bad_flag = argv[after - 1];
TEST_ASSERT_EQUAL_STRING("-z", bad_flag);
fresh_dir();
capture_t cap;
capture_begin(&cap);
int rc = flagspec_reject("testcmd", g_testcmd_spec, 2, bad_flag,
g_testcmd_usage);
char out_buf[512], err_buf[512];
capture_end(&cap, out_buf, sizeof(out_buf), err_buf, sizeof(err_buf));
TEST_ASSERT_TRUE(rc < 0);
TEST_ASSERT_EQUAL_STRING("", out_buf);
TEST_ASSERT_NOT_NULL(strstr(err_buf, "unrecognized flag '-z'"));
TEST_ASSERT_NOT_NULL(strstr(err_buf, "usage: testcmd -h <host> [-c]"));
cleanup_dir();
}
static void test_reject_rejects_missing_value_long_flag(void)
{
struct option opts[3];
char optstring[8];
TEST_ASSERT_EQUAL_INT(0, flagspec_build_opts(g_testcmd_spec, 2, opts,
optstring,
sizeof(optstring)));
/* "--host" is the last argv element -- getopt_long() has nothing to
* consume as its required value. */
char* argv[] = { "testcmd", "--host", NULL };
int after = run_getopt_long_expect_error(2, argv, opts, optstring);
const char* bad_flag = argv[after - 1];
TEST_ASSERT_EQUAL_STRING("--host", bad_flag);
fresh_dir();
capture_t cap;
capture_begin(&cap);
int rc = flagspec_reject("testcmd", g_testcmd_spec, 2, bad_flag,
g_testcmd_usage);
char out_buf[512], err_buf[512];
capture_end(&cap, out_buf, sizeof(out_buf), err_buf, sizeof(err_buf));
TEST_ASSERT_TRUE(rc < 0);
TEST_ASSERT_EQUAL_STRING("", out_buf);
TEST_ASSERT_NOT_NULL(strstr(err_buf, "flag '--host' requires a value"));
TEST_ASSERT_NOT_NULL(strstr(err_buf, "usage: testcmd -h <host> [-c]"));
cleanup_dir();
}
static void test_reject_rejects_missing_value_short_flag(void)
{
struct option opts[3];
char optstring[8];
TEST_ASSERT_EQUAL_INT(0, flagspec_build_opts(g_testcmd_spec, 2, opts,
optstring,
sizeof(optstring)));
char* argv[] = { "testcmd", "-h", NULL };
int after = run_getopt_long_expect_error(2, argv, opts, optstring);
const char* bad_flag = argv[after - 1];
TEST_ASSERT_EQUAL_STRING("-h", bad_flag);
fresh_dir();
capture_t cap;
capture_begin(&cap);
int rc = flagspec_reject("testcmd", g_testcmd_spec, 2, bad_flag,
g_testcmd_usage);
char out_buf[512], err_buf[512];
capture_end(&cap, out_buf, sizeof(out_buf), err_buf, sizeof(err_buf));
TEST_ASSERT_TRUE(rc < 0);
TEST_ASSERT_EQUAL_STRING("", out_buf);
TEST_ASSERT_NOT_NULL(strstr(err_buf, "flag '-h' requires a value"));
TEST_ASSERT_NOT_NULL(strstr(err_buf, "usage: testcmd -h <host> [-c]"));
cleanup_dir();
}
/* Direct-call (non-getopt_long) tests: exercises flagspec_reject()'s own
* argument handling in isolation, including NULL inputs it must survive
* without crashing. */
static void test_reject_handles_null_bad_flag_and_null_usage_gracefully(void)
{
fresh_dir();
capture_t cap;
capture_begin(&cap);
int rc = flagspec_reject("testcmd", g_testcmd_spec, 2, NULL, NULL);
char out_buf[512], err_buf[512];
capture_end(&cap, out_buf, sizeof(out_buf), err_buf, sizeof(err_buf));
TEST_ASSERT_TRUE(rc < 0);
TEST_ASSERT_EQUAL_STRING("", out_buf);
TEST_ASSERT_NOT_NULL(strstr(err_buf, "unrecognized flag"));
cleanup_dir();
}
static void test_reject_no_argument_flag_given_a_value_reports_distinctly(void)
{
/* "-c" never requires a value; a caller that somehow reaches
* flagspec_reject() with the bare "-c" token (matches an entry, but
* that entry takes no_argument) must be told a different thing than
* either the unrecognized-flag or missing-value message. */
fresh_dir();
capture_t cap;
capture_begin(&cap);
int rc = flagspec_reject("testcmd", g_testcmd_spec, 2, "-c",
g_testcmd_usage);
char out_buf[512], err_buf[512];
capture_end(&cap, out_buf, sizeof(out_buf), err_buf, sizeof(err_buf));
TEST_ASSERT_TRUE(rc < 0);
TEST_ASSERT_EQUAL_STRING("", out_buf);
TEST_ASSERT_NOT_NULL(strstr(err_buf, "flag '-c' does not take a value"));
TEST_ASSERT_NOT_NULL(strstr(err_buf, "usage: testcmd -h <host> [-c]"));
cleanup_dir();
}
void test_flagspec(void)
{
UnitySetTestFile(__FILE__);
RUN_TEST(test_build_opts_produces_correct_table_for_two_flag_command);
RUN_TEST(test_build_opts_assigns_synthetic_val_to_long_only_flag);
RUN_TEST(test_build_opts_rejects_null_spec);
RUN_TEST(test_build_opts_rejects_zero_count);
RUN_TEST(test_build_opts_rejects_entry_with_null_long_opt);
RUN_TEST(test_build_opts_rejects_too_small_optstring_buffer);
RUN_TEST(test_reject_rejects_unrecognized_long_flag);
RUN_TEST(test_reject_rejects_unrecognized_short_flag);
RUN_TEST(test_reject_rejects_missing_value_long_flag);
RUN_TEST(test_reject_rejects_missing_value_short_flag);
RUN_TEST(test_reject_handles_null_bad_flag_and_null_usage_gracefully);
RUN_TEST(test_reject_no_argument_flag_given_a_value_reports_distinctly);
}