feat(resource-value-verification): wire resolver into resupdate, add -t flag (task 2.1)
set_resval() now delegates type resolution to resval_typeresolve.c; unknown resource names default to numeric, overridable via a new -t <numeric|string> flag. cmd_resupdate()'s getopt loop defers per-pair finalization so a trailing -t still applies to its pair, while preserving the exact original duplicate-value rejection behavior (round-2 fix after review caught a last-write-wins regression in that refactor). Live-confirmed: resupdate -n r15m -v 1.5 (previously silently dropped) now succeeds and is visible via resval. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
a336884fc4
commit
058335d2a6
SSH key fingerprint:
SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
4 changed files with 166 additions and 111 deletions
|
|
@ -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 <numeric|string>` 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).
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <resname> -v <value> [-n <resname> -v <value> ...]\n"
|
||||
" -n <resname> resource name to update (e.g. slots, mem, cpuf,\n"
|
||||
"resupdate -n <resname> -v <value> [-t <numeric|string>]\n"
|
||||
" [-n <resname> -v <value> [-t <numeric|string>] ...]\n"
|
||||
" -n <resname> 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> 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 <numeric|string> 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 <count>] [-m <message>]\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++;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue