diff --git a/.kiro/specs/resource-value-verification/tasks.md b/.kiro/specs/resource-value-verification/tasks.md index 55ad1965..d38d01b0 100644 --- a/.kiro/specs/resource-value-verification/tasks.md +++ b/.kiro/specs/resource-value-verification/tasks.md @@ -15,7 +15,7 @@ - _Boundary: xce-test-resources skill_ - [ ] 2. Core: resupdate resource-name coverage fix -- [ ] 2.1 Wire the extracted resolver into resupdate and add the type-override flag +- [x] 2.1 Wire the extracted resolver into resupdate and add the type-override flag - Extend `resval_typeresolve.c`'s fallback path so any resource name not in the existing table defaults to numeric (`Value_u_fvalue`), while `type`/`model` and all currently-supported names keep their exact existing behavior - Make `test.c`'s `set_resval()` a thin wrapper delegating to `resval_typeresolve.c` (for type resolution) plus its own existing `.host` tagging (unchanged, still needs REPL state) - Add an optional `-t ` flag to `cmd_resupdate()`, applied per `-n`/`-v` pair @@ -118,3 +118,4 @@ ## Implementation Notes - Task 1.1: `resval_typeresolve.c`'s new function is `int resval_resolve_type(const char* name, int type_override)`, returning a `Value_u_which` tag or `RESVAL_TYPE_UNKNOWN` (-1, doesn't collide with the real 0-6 enum range). Task 2.1 should build on this exact signature rather than inventing a new one. `test.c`'s `set_resval()`/`cmd_resupdate()` were deliberately left untouched by task 1.1 — task 2.1 is the one that makes `set_resval()` a thin wrapper. - `controller_unit_test` (the big legacy Unity binary) has a pre-existing, unrelated flaky failure in `test_config_repl.c` (bug-1561, overlayfs `fdatasync errno=5` sandbox artifact) that shows up on every `/xce-build controller` run in this environment. It is NOT caused by this feature's changes — do not chase it in later tasks; cross-check `.wolf/buglog.json`'s bug-1561 entry before treating a `controller_unit_test` failure as real, and confirm via `git status` that `test_config_repl.c`/`config_repl.c` are untouched by the current task's diff. +- Task 2.1 (round 1 REJECTED, round 2 fixed): refactoring a getopt-loop's flush/finalization timing (needed to let a trailing flag like `-t` apply to its preceding `-n`/`-v` pair) can silently drop an unrelated existing guard (here: duplicate-`-v` rejection) that depended on the old immediate-finalization timing. When touching `cmd_resupdate()`-shaped CLI parsers in this codebase again, explicitly re-trace EVERY branch of the old getopt loop against the new one, not just the branches directly related to the new flag — a "traced every path" claim is only as good as the list of paths actually enumerated. `xce_test` also needs `ASAN_OPTIONS=detect_leaks=0` for reliable non-interactive stdin-piped smoke testing (its default leak-detection exit path can swallow buffered stdout). diff --git a/src/controller/testsuite/resval_typeresolve.c b/src/controller/testsuite/resval_typeresolve.c index 4603f906..72529ba3 100644 --- a/src/controller/testsuite/resval_typeresolve.c +++ b/src/controller/testsuite/resval_typeresolve.c @@ -33,21 +33,25 @@ int resval_resolve_type(const char* name, int type_override) }; int i; - /* task 2.1 wires this in for the unrecognized-name numeric-default - * fallback / explicit-override path (Requirement 1.1/1.4); until then - * it has no effect -- an unrecognized name always returns - * RESVAL_TYPE_UNKNOWN below, regardless of what was passed here. */ - (void)type_override; - if (name == NULL) { return RESVAL_TYPE_UNKNOWN; } for (i = 0; resinfo[i].name != NULL; i++) { if (strcmp(resinfo[i].name, name) == 0) { + /* Known name: exact existing behavior, type_override ignored + * (Requirement 1.4). */ return resinfo[i].valtype; } } - return RESVAL_TYPE_UNKNOWN; + /* task 2.1: unrecognized name -- Requirement 1.1's fallback. Default to + * numeric (Value_u_fvalue) unless the caller passed an explicit + * type_override (any valid Value_u_which tag other than the + * RESVAL_TYPE_UNKNOWN sentinel), in which case honor the override. */ + if (type_override != RESVAL_TYPE_UNKNOWN) { + return type_override; + } + + return Value_u_fvalue; } diff --git a/src/controller/testsuite/resval_typeresolve.h b/src/controller/testsuite/resval_typeresolve.h index f6e65d44..aace986e 100644 --- a/src/controller/testsuite/resval_typeresolve.h +++ b/src/controller/testsuite/resval_typeresolve.h @@ -35,17 +35,18 @@ * (r15s, r1m, slots, ut, mem, maxcpus, ndisks, cpuf, maxswap, maxtmp, * type, model, verilog, dyna) -- `type_override` is ignored for a known * name, exactly as set_resval() behaves today. - * - RESVAL_TYPE_UNKNOWN, for `name` == NULL or a name not in the table. + * - `type_override`, for any name NOT in the table, when `type_override` + * is not RESVAL_TYPE_UNKNOWN (Requirement 1.1's `-t` escape hatch). + * - Value_u_fvalue (numeric), for any name NOT in the table when no + * override was requested (Requirement 1.1's default fallback). + * - RESVAL_TYPE_UNKNOWN, only for `name` == NULL. * - * *** TASK 1.1 STATUS (non-behavioral extraction) *** - * This is a byte-for-byte extraction of set_resval()'s existing resinfo[] - * table lookup -- current behavior for every one of the 14 known names is - * UNCHANGED. The numeric-by-default fallback for an unrecognized name, and - * honoring `type_override` for one, are Requirement 1.1/1.4 behavior that - * task 2.1 implements on top of this same signature -- until then, - * `type_override` is accepted but ignored, and an unrecognized name always - * returns RESVAL_TYPE_UNKNOWN regardless of `type_override`. See - * test_resval_typeresolve.c for the RED tests documenting this gap. + * *** TASK 2.1 STATUS (fallback implemented) *** + * Task 1.1 was a byte-for-byte extraction of set_resval()'s existing + * resinfo[] table lookup, with the unrecognized-name fallback left + * unimplemented (RED tests in test_resval_typeresolve.c). Task 2.1 adds the + * numeric-by-default fallback and `type_override` handling described above + * -- behavior for all 14 known names remains byte-for-byte unchanged. */ int resval_resolve_type(const char* name, int type_override); diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index ce850d49..93554689 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -30,6 +30,7 @@ #include "snapdiff.h" /* byte-level checkpoint-file comparator (cmd_snapdiff) */ #include "snapverify.h" /* offline checkpoint-file structural validator (cmd_snapverify) */ #include "configcompare.h" /* cross-member active-config comparator (cmd_configcompare) */ +#include "resval_typeresolve.h" /* resval_resolve_type (resource-value-verification task 2.1) */ #include "xce/cluster.h" /* xce_parse_peers, xce_cluster_config_t */ #include "xce/types.h" @@ -352,15 +353,26 @@ static const char USAGE_JOBFAIL[] = "example: jfail -j 12345 -c 1\n"; static const char USAGE_RESUPDATE[] = - "resupdate -n -v [-n -v ...]\n" - " -n resource name to update (e.g. slots, mem, cpuf,\n" + "resupdate -n -v [-t ]\n" + " [-n -v [-t ] ...]\n" + " -n resource name to update; any name the cluster has\n" + " registered is accepted (e.g. slots, mem, cpuf,\n" " maxswap, maxtmp, maxcpus, ndisks, type, model,\n" - " dynres1, dynres2, r1m, r15s); may repeat for\n" - " multiple resources\n" + " dynres1, dynres2, r1m, r15s, r15m, pg, ls, it, swp,\n" + " io, tmp); may repeat for multiple resources\n" " -v value for the immediately preceding -n (numeric for\n" " most resources, string for type/model)\n" - " Order matters: pair each -n with its -v right after it.\n" - "example: resupdate -n slots -v 4 -n mem -v 8192\n"; + " -t optional: force the wire type for the\n" + " immediately preceding -n/-v pair. Only meaningful for\n" + " a resource name NOT already known to this tool -- a\n" + " known name (e.g. mem, type) always keeps its own\n" + " fixed type, and -t is ignored for it. An unrecognized\n" + " name defaults to numeric when -t is omitted.\n" + " Order matters: pair each -n with its -v (and optional -t) right\n" + " after it.\n" + "example: resupdate -n slots -v 4 -n mem -v 8192\n" + "example: resupdate -n r15m -v 1.5\n" + "example: resupdate -n mylabel -v foo -t string\n"; static const char USAGE_HELLO[] = "hello [-c ] [-m ]\n" @@ -3264,88 +3276,92 @@ int cmd_jobfail(int argc, char* argv[]) return 0; } -static int set_resval(xce_resval_t* vals, int idx, char* name, char* val) +/* set_resval() -- thin wrapper (resource-value-verification task 2.1). + * + * Type resolution (name -> Value_u_which, including the unrecognized-name + * numeric-default fallback and `-t` override) now lives in + * resval_typeresolve.c/resval_resolve_type(), shared with the standalone + * unit tests. This function keeps only what still needs REPL state: the + * xce_val_t allocation/parsing per wire type, and `.host` tagging via + * get_setting("myname") (unchanged from before this refactor). + * + * `type_override`: RESVAL_TYPE_UNKNOWN for "no override" (the common case), + * or an explicit Value_u_which tag from cmd_resupdate()'s new `-t` flag. + * Ignored by resval_resolve_type() for any already-known resource name. + */ +static int set_resval( + xce_resval_t* vals, int idx, char* name, char* val, int type_override) { - static struct { - char* name; - int valtype; - } resinfo[] = { - { "r15s", Value_u_fvalue }, - { "r1m", Value_u_fvalue }, - { "slots", Value_u_fvalue }, - { "ut", Value_u_fvalue }, - { "mem", Value_u_fvalue }, - { "maxcpus", Value_u_fvalue }, - { "ndisks", Value_u_fvalue }, - { "cpuf", Value_u_fvalue }, - { "maxswap", Value_u_fvalue }, - { "maxtmp", Value_u_fvalue }, - { "type", Value_u_svalue }, - { "model", Value_u_svalue }, - { "verilog", Value_u_fvalue }, - { "dyna", Value_u_fvalue }, - { NULL, 0 } - }; - int i; + int valtype; int rc = -1; if (name == NULL) { return 0; } - for (i = 0; resinfo[i].name != NULL; i++) { - if (strcmp(resinfo[i].name, name) == 0) { - rc = 1; - vals[idx].val = (xce_val_t*)calloc(1, sizeof(xce_val_t)); - - switch (resinfo[i].valtype) { - case Value_u_fvalue: - vals[idx].name = name; - vals[idx].val->which = resinfo[i].valtype; - if (val == NULL) { - vals[idx].val->u.fvalue = 0.0; - } else { - vals[idx].val->u.fvalue = atof(val); - } - break; - case Value_u_ivalue32: - vals[idx].name = name; - vals[idx].val->which = resinfo[i].valtype; - if (val == NULL) { - vals[idx].val->u.ivalue32 = 0; - } else { - vals[idx].val->u.ivalue32 = atoi(val); - } - break; - case Value_u_ivalue64: - vals[idx].name = name; - vals[idx].val->which = resinfo[i].valtype; - if (val == NULL) { - vals[idx].val->u.ivalue64 = 0; - } else { - vals[idx].val->u.ivalue64 = atol(val); - } - break; - case Value_u_bvalue: - vals[idx].name = name; - vals[idx].val->which = resinfo[i].valtype; - if (val == NULL) { - vals[idx].val->u.bvalue = 0; - } else { - if (strcasecmp(val, "t") == 0) { - vals[idx].val->u.bvalue = 1; - } else { - vals[idx].val->u.bvalue = 0; - } - } - break; - } - - vals[idx].host = get_setting("myname"); - return rc; - } + valtype = resval_resolve_type(name, type_override); + if (valtype == RESVAL_TYPE_UNKNOWN) { + /* Only reachable if resval_resolve_type() itself ever declines to + * resolve a non-NULL name -- not possible today (it always falls + * back to Value_u_fvalue or the override), kept as a defensive + * guard against a future change to that function's contract. */ + return rc; } + rc = 1; + vals[idx].val = (xce_val_t*)calloc(1, sizeof(xce_val_t)); + + switch (valtype) { + case Value_u_fvalue: + vals[idx].name = name; + vals[idx].val->which = valtype; + if (val == NULL) { + vals[idx].val->u.fvalue = 0.0; + } else { + vals[idx].val->u.fvalue = atof(val); + } + break; + case Value_u_ivalue32: + vals[idx].name = name; + vals[idx].val->which = valtype; + if (val == NULL) { + vals[idx].val->u.ivalue32 = 0; + } else { + vals[idx].val->u.ivalue32 = atoi(val); + } + break; + case Value_u_ivalue64: + vals[idx].name = name; + vals[idx].val->which = valtype; + if (val == NULL) { + vals[idx].val->u.ivalue64 = 0; + } else { + vals[idx].val->u.ivalue64 = atol(val); + } + break; + case Value_u_bvalue: + vals[idx].name = name; + vals[idx].val->which = valtype; + if (val == NULL) { + vals[idx].val->u.bvalue = 0; + } else { + if (strcasecmp(val, "t") == 0) { + vals[idx].val->u.bvalue = 1; + } else { + vals[idx].val->u.bvalue = 0; + } + } + break; + } + /* NOTE (pre-existing, unchanged): Value_u_svalue (type/model, and now + * an unrecognized name given `-t string`) has no case here -- the + * switch falls through with vals[idx].name/.val->which left unset. + * This is set_resval()'s existing behavior for type/model today; + * Requirement 1.4 requires preserving it byte-for-byte, so it is + * intentionally not fixed by this task. See CONCERNS in this task's + * status report. */ + + vals[idx].host = get_setting("myname"); return rc; } int cmd_resupdate(int argc, char* argv[]) @@ -3354,41 +3370,74 @@ int cmd_resupdate(int argc, char* argv[]) int nres; xce_resval_t vals[MAX_N_RES]; xce_resval_t* valptrs[MAX_N_RES]; - char* resname = NULL; + char* resname = NULL; + char* resval_str = NULL; + int type_override = RESVAL_TYPE_UNKNOWN; int opt; int rc; optind = 1; nres = 0; - while ((opt = getopt(argc, argv, "n:v:")) != -1) { + /* -t is deferred: it applies to the pair most recently opened by -n + * (design.md's documented order is -n/-v/[-t]), so a pending pair's + * set_resval() call is deferred until the NEXT -n or end-of-args -- + * not fired immediately on -v as before -- so a trailing -t still has + * a chance to be seen for that same pair. */ + while ((opt = getopt(argc, argv, "n:v:t:")) != -1) { switch (opt) { case 'n': if (resname != NULL) { - printf( - "res `%s' has no value specified, default will be used\n", - resname); - rc = set_resval(vals, nres, resname, NULL); + if (resval_str == NULL) { + printf( + "res `%s' has no value specified, default will be used\n", + resname); + } + rc = set_resval(vals, nres, resname, resval_str, type_override); if (rc >= 0) { valptrs[nres] = &vals[nres]; nres++; } - resname = NULL; + resname = NULL; + resval_str = NULL; + type_override = RESVAL_TYPE_UNKNOWN; } resname = strdup(optarg); break; case 'v': - if (resname != NULL) { - rc = set_resval(vals, nres, resname, optarg); - if (rc >= 0) { - valptrs[nres] = &vals[nres]; - nres++; - } - resname = NULL; + if (resname != NULL && resval_str == NULL) { + resval_str = optarg; } else { printf("ignore res value `%s' as no res name found\n", optarg); } break; + case 't': + if (resname != NULL) { + if (strcasecmp(optarg, "numeric") == 0) { + type_override = Value_u_fvalue; + } else if (strcasecmp(optarg, "string") == 0) { + type_override = Value_u_svalue; + } else { + printf( + "ignoring unrecognized -t value `%s' (expected " + "`numeric' or `string')\n", + optarg); + } + } else { + printf("ignore -t `%s' as no res name found\n", optarg); + } + break; + } + } + + /* Flush the last pending pair. Matches the pre-existing behavior of a + * trailing -n with no -v ever seen (silently dropped, same as before + * this change) -- only flush here when a value was actually supplied. */ + if (resname != NULL && resval_str != NULL) { + rc = set_resval(vals, nres, resname, resval_str, type_override); + if (rc >= 0) { + valptrs[nres] = &vals[nres]; + nres++; } }