From da1a663f058da835fc3de2a9616c29821e9ad3d6 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Mon, 28 Sep 2026 04:17:04 +0000 Subject: [PATCH] fix(harness): xce_test argument validation, RPC error text and leaks (13 bugs) Stack overflows on user input: - bug-1702 jobrun -m: host segment strncpy'd into a 256-byte buffer unbounded (ASAN stack-buffer-overflow); now cmdutil_split_host_tasks() refuses an over-long host. - bug-1703 jobsub -n: min strncpy'd into a 64-byte buffer; now cmdutil_parse_ntasks() parses in place, 1 <= min <= max. - resupdate's 16-entry vals[] had no bound either; a 17th -n overflowed. Wrong or silent results: - bug-1574 resupdate type/model: set_resval() had no string case, left the entry's name uninitialized, and crashed the encoder. Moved to resval_typeresolve.c as resval_fill(), which fills every type. - bug-1705 jobsub -x was overwritten (and leaked) by the default exdata; cmdutil_default_exdata() only fills an unset one. jobsub's execuser/exechome/execcwd copies leaked on every call. - bug-1707 a trailing -n without -v was dropped silently; resupdate also returned 0 after its RPC failed and "succeeded" with nothing to send. - bug-1708 jobkill -s SIGKILL became signal 0 and was sent. - bug-1709 jobinfo exited 0 with no -j, a malformed -j, or no hits. - bug-1710 hello -c 0 sent nothing and reported success. - bug-1712 a duplicate -v was reported as "no res name found". - bug-1777 workerlist showed join-time maxmem/maxswap/maxtmp, not the effective values (hosts.yaml overrides) the reply already carries in resvals_to_sched and the scheduler uses. - bug-1793 join's maxswap/maxtmp defaults were 1 KB / 2 KB in bytes; now 1 GiB / 2 GiB, and -m/-s/-t no longer overflow atoi() above 2 GB. Error reporting and leaks: - bug-1796 RPC failures printed perror() of a stale errno ("Connection refused" / "Invalid argument" / "Inappropriate ioctl for device" for the same failure). cmdutil_rpc_error() prints the RPC's own code (E_XCE_* included) at all 19 sites. - bug-1797 qctrl leaked the queue name on the RPC-error path; repeated -q/-h leaked the earlier copy. New src/harness/cmdutil.c (+ cmdutil_unit_test, 14 cases); 9 new test_test_cli.c cases, 8 RED on the old xce_test; new resval_fill, worker_effective_attr and join-default tests (join ones RED on the old join_worker_args.c). Harness suite 6/6 (asan), harness_unit_test 141/0. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/harness/cmdutil.c | 146 ++++++ src/harness/include/cmdutil.h | 41 ++ src/harness/include/joblist_display.h | 11 + src/harness/include/resval_typeresolve.h | 13 + src/harness/joblist_display.c | 32 ++ src/harness/join_worker_args.c | 41 +- src/harness/meson.build | 17 + src/harness/resval_typeresolve.c | 62 +++ src/harness/test.c | 472 ++++++++---------- src/harness/testsuite/test_cmdutil.c | 214 ++++++++ src/harness/testsuite/test_joblist_display.c | 45 ++ src/harness/testsuite/test_join_worker_args.c | 62 ++- .../testsuite/test_resval_typeresolve.c | 52 ++ src/harness/testsuite/test_test_cli.c | 185 +++++++ 14 files changed, 1107 insertions(+), 286 deletions(-) create mode 100644 src/harness/cmdutil.c create mode 100644 src/harness/include/cmdutil.h create mode 100644 src/harness/testsuite/test_cmdutil.c diff --git a/src/harness/cmdutil.c b/src/harness/cmdutil.c new file mode 100644 index 00000000..e359f820 --- /dev/null +++ b/src/harness/cmdutil.c @@ -0,0 +1,146 @@ +/* + * cmdutil.c -- xce_test command-argument and RPC-error helpers (see + * include/cmdutil.h). + */ +#include +#include +#include +#include +#include + +#include + +#include "cmdutil.h" +#include "utils.h" /* xce_strerror_r */ +#include "xce/errnox.h" + +/* strtol over [s, s + len): the whole span must be digits (optional + * leading '-'), then range-checked. */ +static int parse_span(const char* s, size_t len, long lo, long hi, long* out) +{ + char buf[32]; + char* end; + long v; + + if ((s == NULL) || (len == 0)) { + return -EINVAL; + } + for (size_t i = 0; i < len; i++) { + if (!((s[i] >= '0' && s[i] <= '9') || (i == 0 && s[i] == '-'))) { + return -EINVAL; + } + } + if (len >= sizeof(buf)) { + return -ERANGE; /* more digits than any long holds */ + } + memcpy(buf, s, len); + buf[len] = '\0'; + + errno = 0; + v = strtol(buf, &end, 10); + if ((end == buf) || (*end != '\0')) { + return -EINVAL; + } + if ((errno == ERANGE) || (v < lo) || (v > hi)) { + return -ERANGE; + } + *out = v; + return 0; +} + +int cmdutil_parse_long(const char* s, long lo, long hi, long* out) +{ + return parse_span(s, (s == NULL) ? 0 : strlen(s), lo, hi, out); +} + +int cmdutil_split_host_tasks(const char* arg, char* host, size_t hostsz, + int* ntasks) +{ + const char* colon = strchr(arg, ':'); + size_t hlen = (colon != NULL) ? (size_t)(colon - arg) : strlen(arg); + long n = 1; + + if (hlen == 0) { + return -EINVAL; + } + if (hlen >= hostsz) { + return -ENAMETOOLONG; + } + if ((colon != NULL) && (cmdutil_parse_long(colon + 1, 1, INT_MAX, &n) < 0)) { + return -EINVAL; + } + memcpy(host, arg, hlen); + host[hlen] = '\0'; + *ntasks = (int)n; + return 0; +} + +int cmdutil_parse_ntasks(const char* arg, uint32_t* min, uint32_t* max) +{ + const char* comma = strchr(arg, ','); + size_t mlen = (comma != NULL) ? (size_t)(comma - arg) : strlen(arg); + long mn, mx; + int rc; + + rc = parse_span(arg, mlen, 1, (long)UINT32_MAX, &mn); + if (rc < 0) { + return rc; + } + mx = mn; + if (comma != NULL) { + rc = cmdutil_parse_long(comma + 1, 1, (long)UINT32_MAX, &mx); + if (rc < 0) { + return rc; + } + if (mx < mn) { + return -EINVAL; + } + } + *min = (uint32_t)mn; + *max = (uint32_t)mx; + return 0; +} + +int cmdutil_default_exdata(char** exdata, const char* path) +{ + cJSON* root; + cJSON* envs; + + if (*exdata != NULL) { + return 0; + } + + root = cJSON_CreateObject(); + envs = cJSON_CreateObject(); + if ((root == NULL) || (envs == NULL) + || (cJSON_AddStringToObject(envs, "PATH", path ? path : "") == NULL)) { + cJSON_Delete(root); + cJSON_Delete(envs); + return -ENOMEM; + } + cJSON_AddItemToObject(root, "sub_env_vars", envs); + + *exdata = cJSON_Print(root); + cJSON_Delete(root); + return (*exdata != NULL) ? 0 : -ENOMEM; +} + +const char* cmdutil_errstr(int rc, char* buf, size_t bufsize) +{ + int code = (rc < 0) ? -rc : rc; + + if (code >= XCE_ERRNO_BASE) { + xce_strerror_r(code, buf, bufsize); + } else { + snprintf(buf, bufsize, "%s", strerror(code)); + } + return buf; +} + +void cmdutil_rpc_error(const char* what, int rc) +{ + char buf[128]; + + fprintf(stderr, "%s: %s\n", what, cmdutil_errstr(rc, buf, sizeof(buf))); + errno = (rc < 0) ? -rc : rc; +} diff --git a/src/harness/include/cmdutil.h b/src/harness/include/cmdutil.h new file mode 100644 index 00000000..68e0ee81 --- /dev/null +++ b/src/harness/include/cmdutil.h @@ -0,0 +1,41 @@ +/* + * cmdutil.h -- xce_test command-argument and RPC-error helpers, extracted + * from test.c so they can be unit-tested without a live controller + * (testsuite/test_cmdutil.c). + */ +#ifndef CMDUTIL_H +#define CMDUTIL_H + +#include /* size_t */ +#include /* uint32_t */ + +/* Parse all of `s` as a base-10 integer in [lo, hi] into *out. + * Returns 0, -EINVAL (empty, NULL or not entirely numeric) or -ERANGE + * (outside [lo, hi]); *out is left unchanged on error. */ +int cmdutil_parse_long(const char* s, long lo, long hi, long* out); + +/* Split jobrun's "[:]" into `host` (hostsz bytes including the + * NUL) and *ntasks (1 when omitted). Returns 0, -ENAMETOOLONG (host segment + * does not fit), or -EINVAL (empty host, or ntasks not an integer >= 1). */ +int cmdutil_split_host_tasks(const char* arg, char* host, size_t hostsz, + int* ntasks); + +/* Parse jobsub's "[,]" (max defaults to min). Returns 0, -EINVAL + * (not numeric, or max < min) or -ERANGE (min < 1, or beyond UINT32_MAX). */ +int cmdutil_parse_ntasks(const char* arg, uint32_t* min, uint32_t* max); + +/* jobsub's default exdata: when *exdata is NULL, set it to a newly + * allocated {"sub_env_vars":{"PATH":}} JSON string; an exdata the + * caller already set (-x) is left alone. Returns 0 or -ENOMEM. */ +int cmdutil_default_exdata(char** exdata, const char* path); + +/* Text for an RPC's negative return code `rc` (libc errno or E_XCE_*), in + * `buf`; never reads the global errno. Returns buf. */ +const char* cmdutil_errstr(int rc, char* buf, size_t bufsize); + +/* Print ": " to stderr and set errno to -rc: the + * replacement for perror() on an RPC-failure path, where errno does not + * hold the RPC's result. */ +void cmdutil_rpc_error(const char* what, int rc); + +#endif /* CMDUTIL_H */ diff --git a/src/harness/include/joblist_display.h b/src/harness/include/joblist_display.h index 20205285..7453935a 100644 --- a/src/harness/include/joblist_display.h +++ b/src/harness/include/joblist_display.h @@ -23,6 +23,9 @@ #include /* uint32_t */ #include /* time_t */ +#include "controller.capnp.h" /* Value_u_which */ +#include "xce/types.h" /* xce_worker_t */ + /* Render a worker's status bitmask (xce_worker_t.status, XCE_WORKER_* bits * in xce/types.h) as a short display string for cmd_workerlist's Status * column. @@ -78,4 +81,12 @@ char* job_state_str(int state); * `buf`. */ const char* format_job_time(time_t t, char* buf, size_t bufsize); +/* The value workerlist shows for a worker's byte-valued host attribute + * (`resname` = "maxmem", "maxswap" or "maxtmp"): the effective numeric + * value the controller reports in w->resvals_to_sched (it reflects a + * hosts.yaml override, and is what the scheduler uses), else + * `attr_value`, the join-time attrs field (bug-1777). */ +double worker_effective_attr(const xce_worker_t* w, const char* resname, + double attr_value); + #endif /* JOBLIST_DISPLAY_H */ diff --git a/src/harness/include/resval_typeresolve.h b/src/harness/include/resval_typeresolve.h index aace986e..535d222d 100644 --- a/src/harness/include/resval_typeresolve.h +++ b/src/harness/include/resval_typeresolve.h @@ -16,6 +16,7 @@ */ #include "controller.capnp.h" /* enum Value_u_which */ +#include "xce/types.h" /* xce_resval_t */ /* Sentinel: not a valid Value_u_which value. Used both as: * - the RETURN value when `name` is not present in the known-resource @@ -50,4 +51,16 @@ */ int resval_resolve_type(const char* name, int type_override); +/* Fill `rv` for one resupdate entry: rv->name = name and rv->host = host + * (both borrowed, not copied), and a newly allocated rv->val of the type + * resval_resolve_type() gives, holding `val` (NULL = the type's zero value; + * a string value is copied). Formerly test.c's set_resval(), which had no + * string case: a type/model entry kept an uninitialized name (bug-1574). + * Returns 1, 0 when `name` is NULL (nothing filled), or -ENOMEM. */ +int resval_fill(xce_resval_t* rv, char* name, const char* val, + int type_override, char* host); + +/* Free what resval_fill() allocated (rv->val and a string value in it). */ +void resval_fill_release(xce_resval_t* rv); + #endif /* RESVAL_TYPERESOLVE_H */ diff --git a/src/harness/joblist_display.c b/src/harness/joblist_display.c index c799115e..ec3fa8f5 100644 --- a/src/harness/joblist_display.c +++ b/src/harness/joblist_display.c @@ -7,6 +7,7 @@ #include "joblist_display.h" #include /* snprintf */ +#include /* strcmp */ #include "xce/ctrlstatus_fmt.h" /* ctrlstatus_format_time_ms */ #include "xce/types.h" /* XCE_WORKER_*, JOB_STAT_* bit definitions */ @@ -97,3 +98,34 @@ const char* format_job_time(time_t t, char* buf, size_t bufsize) return buf; } + +double worker_effective_attr(const xce_worker_t* w, const char* resname, + double attr_value) +{ + if (w == NULL) { + return attr_value; + } + for (int i = 0; i < w->nresvals_to_sched; i++) { + const xce_name_val_t* nv = w->resvals_to_sched[i]; + + if ((nv == NULL) || (nv->name == NULL) || (nv->val == NULL) + || (strcmp(nv->name, resname) != 0)) { + continue; + } + switch (nv->val->which) { + case Value_u_fvalue: + return nv->val->u.fvalue; + case Value_u_ivalue32: + return (double)nv->val->u.ivalue32; + case Value_u_uvalue32: + return (double)nv->val->u.uvalue32; + case Value_u_ivalue64: + return (double)nv->val->u.ivalue64; + case Value_u_uvalue64: + return (double)nv->val->u.uvalue64; + default: + return attr_value; /* not numeric */ + } + } + return attr_value; +} diff --git a/src/harness/join_worker_args.c b/src/harness/join_worker_args.c index 10b7c52a..939ecd46 100644 --- a/src/harness/join_worker_args.c +++ b/src/harness/join_worker_args.c @@ -27,8 +27,10 @@ const flagspec_t join_flags[] = { { 'c', "maxcpus", "maxcpus", required_argument, 0, "4" }, { 'm', "maxmem", "maxmem", required_argument, 0, "64 * 1024 * 1024 (64 MB in bytes)" }, - { 's', "maxswap", "maxswap", required_argument, 0, "1024" }, - { 't', "maxtmp", "maxtmp", required_argument, 0, "2048" }, + { 's', "maxswap", "maxswap", required_argument, 0, + "1024 * 1024 * 1024 (1 GB in bytes)" }, + { 't', "maxtmp", "maxtmp", required_argument, 0, + "2048 * 1024 * 1024 (2 GB in bytes)" }, { 'd', "ndisks", "ndisks", required_argument, 0, "1" }, }; /* JOIN_NFLAGS is an internal alias of the header's JOIN_FLAGS_COUNT (a @@ -39,6 +41,22 @@ const flagspec_t join_flags[] = { * future edit changes the entry count here without updating the header. */ #define JOIN_NFLAGS JOIN_FLAGS_COUNT +/* A non-negative byte count, as a double (hostattrs_t's type). */ +static int parse_bytes(const char* flag, const char* s, double* out) +{ + char* end; + double v; + + errno = 0; + v = strtod(s, &end); + if ((end == s) || (*end != '\0') || (errno == ERANGE) || (v < 0)) { + fprintf(stderr, "join: %s expects a byte count, got '%s'\n", flag, s); + return -EINVAL; + } + *out = v; + return 0; +} + int join_worker_parse_attrs(int argc, char* argv[], hostattrs_t* attrs) { int opt; @@ -67,8 +85,10 @@ int join_worker_parse_attrs(int argc, char* argv[], hostattrs_t* attrs) attrs->cpufactor = 1.0; attrs->maxcpus = 4; attrs->maxmem = 64 * 1024 * 1024; - attrs->maxswap = 1024; - attrs->maxtmp = 2048; + /* bug-1793: same stale-unit bug as maxmem above -- these read 1024 and + * 2048 (MB once), i.e. 1 KB of swap and 2 KB of tmp in bytes. */ + attrs->maxswap = 1024.0 * 1024 * 1024; + attrs->maxtmp = 2048.0 * 1024 * 1024; attrs->ndisks = 1; attrs->hostmodel = "Intel64"; @@ -86,14 +106,21 @@ int join_worker_parse_attrs(int argc, char* argv[], hostattrs_t* attrs) case 'c': attrs->maxcpus = atoi(optarg); break; + /* byte values: atoi() overflowed above 2 GB (bug-1793) */ case 'm': - attrs->maxmem = atoi(optarg); + if (parse_bytes("maxmem", optarg, &attrs->maxmem) < 0) { + return -EINVAL; + } break; case 's': - attrs->maxswap = atoi(optarg); + if (parse_bytes("maxswap", optarg, &attrs->maxswap) < 0) { + return -EINVAL; + } break; case 't': - attrs->maxtmp = atoi(optarg); + if (parse_bytes("maxtmp", optarg, &attrs->maxtmp) < 0) { + return -EINVAL; + } break; case 'd': attrs->ndisks = atoi(optarg); diff --git a/src/harness/meson.build b/src/harness/meson.build index fe2ea923..ce748f49 100644 --- a/src/harness/meson.build +++ b/src/harness/meson.build @@ -26,6 +26,7 @@ xce_test_src = [ 'confirm.c', 'listfmt_json.c', 'listrow_json.c', + 'cmdutil.c', ] xce_test_deps = [ @@ -236,3 +237,19 @@ test('listrow json test', listrow_json_unit_test, suite: 'harness', timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) + +# cmdutil.c: xce_test's argument-parsing and RPC-error helpers (bug-1702, +# 1703, 1705, 1708, 1710, 1796), extracted from test.c so the bounds, +# range and exdata rules can be tested without a live controller -- same +# standalone-binary rationale as the tests above. +cmdutil_unit_test = executable('cmdutil_unit_test', + ['testsuite/test_cmdutil.c', 'cmdutil.c'], + include_directories: [incdir], + dependencies: harness_unit_test_deps, + c_args: ['-DXCE_TESTING'], + install: false) + +test('cmdutil test', cmdutil_unit_test, + suite: 'harness', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) diff --git a/src/harness/resval_typeresolve.c b/src/harness/resval_typeresolve.c index 72529ba3..ec5fd12c 100644 --- a/src/harness/resval_typeresolve.c +++ b/src/harness/resval_typeresolve.c @@ -2,7 +2,10 @@ * * See resval_typeresolve.h for the full rationale and TASK 1.1 STATUS note. */ +#include +#include #include +#include #include "resval_typeresolve.h" @@ -55,3 +58,62 @@ int resval_resolve_type(const char* name, int type_override) return Value_u_fvalue; } + +int resval_fill(xce_resval_t* rv, char* name, const char* val, + int type_override, char* host) +{ + int which; + xce_val_t* v; + + memset(rv, 0, sizeof(*rv)); + if (name == NULL) { + return 0; + } + + which = resval_resolve_type(name, type_override); + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + return -ENOMEM; + } + v->which = which; + + switch (which) { + case Value_u_svalue: + v->u.svalue = strdup((val != NULL) ? val : ""); + if (v->u.svalue == NULL) { + free(v); + return -ENOMEM; + } + break; + case Value_u_ivalue32: + v->u.ivalue32 = (val != NULL) ? atoi(val) : 0; + break; + case Value_u_ivalue64: + v->u.ivalue64 = (val != NULL) ? atol(val) : 0; + break; + case Value_u_bvalue: + v->u.bvalue = (val != NULL) && (strcasecmp(val, "t") == 0); + break; + case Value_u_fvalue: + default: + v->which = Value_u_fvalue; + v->u.fvalue = (val != NULL) ? atof(val) : 0.0; + break; + } + + rv->name = name; + rv->host = host; + rv->val = v; + return 1; +} + +void resval_fill_release(xce_resval_t* rv) +{ + if (rv->val != NULL) { + if (rv->val->which == Value_u_svalue) { + free(rv->val->u.svalue); + } + free(rv->val); + rv->val = NULL; + } +} diff --git a/src/harness/test.c b/src/harness/test.c index 0c20860d..e41eddda 100644 --- a/src/harness/test.c +++ b/src/harness/test.c @@ -63,6 +63,7 @@ #include "snapverify.h" /* offline checkpoint-file structural validator (cmd_snapverify) */ #include "configcompare.h" /* cross-member active-config comparator (cmd_configcompare) */ #include "confirm.h" /* destructive-command confirmation gate (xce-test-cli-ux task 2.1) */ +#include "cmdutil.h" /* argument parsing + RPC-error reporting helpers */ #include "joblist_display.h" /* worker_status_str/job_state_str/format_job_time * (xce-test-cli-ux task 2.5 remediation) */ #include "join_worker_args.h" /* join_worker_parse_attrs (xce-test-cli-ux @@ -156,14 +157,6 @@ typedef struct { int ntasks; } host_task_t; -typedef struct { - char* PATH; -} sub_envs_t; - -typedef struct { - sub_envs_t sub_env_vars; -} exdata_t; - #define MAX_RUN_JOBS (20) #define MAX_TASK_HOSTS (5) #define MAX_RUN_TASKS (50) @@ -425,7 +418,8 @@ static const char USAGE_JOIN_WORKER_HEAD[] = " [-t ] [-d ]\n" " Joins the cluster as a worker (hosttype LINUX64, hostmodel Intel64).\n"; -static const char USAGE_JOIN_WORKER_TAIL[] = "example: join -c 8 -m 16384\n"; +static const char USAGE_JOIN_WORKER_TAIL[] + = "example: join -c 8 -m 17179869184 (16 GB in bytes)\n"; static const char USAGE_JOBSTART_HEAD[] = "jstart -j [-p ]\n" @@ -2055,9 +2049,19 @@ int cmd_hello(int argc, char* argv[]) while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) != -1) { switch (opt) { - case 'c': - count = atoi(optarg); + case 'c': { + long n; + + /* bug-1710: 0, negative or non-numeric sent nothing and still + * reported success */ + if (cmdutil_parse_long(optarg, 1, INT_MAX, &n) < 0) { + fprintf(stderr, "hello: -c expects a count >= 1, got '%s'\n", + optarg); + return -EINVAL; + } + count = (int)n; break; + } case 'm': msg = optarg; break; @@ -2274,8 +2278,7 @@ int cmd_reslist(int argc, char* argv[]) free(call); if (rc < 0) { - SET_ERROR(-rc); - perror("xce_reslist"); + cmdutil_rpc_error("xce_reslist", rc); return rc; } @@ -2417,8 +2420,7 @@ int cmd_workerlist(int argc, char* argv[]) free(call); if (rc < 0) { - SET_ERROR(-rc); - perror("xce_workerlist"); + cmdutil_rpc_error("xce_workerlist", rc); return rc; } @@ -2438,16 +2440,18 @@ int cmd_workerlist(int argc, char* argv[]) for (i = 0; i < rc; i++) { cJSON* row = listfmt_json_new_row(array); - workerlist_json_row(row, workers[i]->name, - worker_status_str(workers[i]->status), - workers[i]->attrs->hosttype, - workers[i]->attrs->hostmodel, - workers[i]->attrs->cpufactor, - workers[i]->attrs->maxcpus, - workers[i]->attrs->maxmem, - workers[i]->attrs->maxswap, - workers[i]->attrs->maxtmp, - workers[i]->attrs->ndisks); + /* bug-1777: effective values, as the scheduler sees them */ + workerlist_json_row( + row, workers[i]->name, worker_status_str(workers[i]->status), + workers[i]->attrs->hosttype, workers[i]->attrs->hostmodel, + workers[i]->attrs->cpufactor, workers[i]->attrs->maxcpus, + worker_effective_attr(workers[i], "maxmem", + workers[i]->attrs->maxmem), + worker_effective_attr(workers[i], "maxswap", + workers[i]->attrs->maxswap), + worker_effective_attr(workers[i], "maxtmp", + workers[i]->attrs->maxtmp), + workers[i]->attrs->ndisks); } listfmt_json_finish(array); @@ -2468,12 +2472,17 @@ int cmd_workerlist(int argc, char* argv[]) char maxswap_buf[64]; char maxtmp_buf[64]; - format_bytes_human(workers[i]->attrs->maxmem, maxmem_buf, - sizeof(maxmem_buf)); - format_bytes_human(workers[i]->attrs->maxswap, maxswap_buf, - sizeof(maxswap_buf)); - format_bytes_human(workers[i]->attrs->maxtmp, maxtmp_buf, - sizeof(maxtmp_buf)); + /* bug-1777: the effective value (a hosts.yaml override included), + * which is what the scheduler uses, not the join-time attrs */ + format_bytes_human(worker_effective_attr(workers[i], "maxmem", + workers[i]->attrs->maxmem), + maxmem_buf, sizeof(maxmem_buf)); + format_bytes_human(worker_effective_attr(workers[i], "maxswap", + workers[i]->attrs->maxswap), + maxswap_buf, sizeof(maxswap_buf)); + format_bytes_human(worker_effective_attr(workers[i], "maxtmp", + workers[i]->attrs->maxtmp), + maxtmp_buf, sizeof(maxtmp_buf)); printf("%-20s%-8s%-25s%-25s%12f%12d%12s%12s%12s%12d\n", workers[i]->name, worker_status_str(workers[i]->status), @@ -2568,7 +2577,7 @@ static int joblist_query(const char* find_jobid, int* contains, int print_table, * and EXPECTS failure until a primary serves) suppress the per-attempt * error print so a normal failover wait does not spam the console. */ if (!quiet) { - perror("xce_joblist"); + cmdutil_rpc_error("xce_joblist", rc); } return rc; } @@ -3289,6 +3298,9 @@ static int jobinfo_thunk(void* ctxp) int cmd_jobinfo(int argc, char* argv[]) { int opt; + int ngiven = 0; /* -j values given */ + int nfound = 0; /* lookups that succeeded */ + int lastrc = -EINVAL; struct option long_opts[JOBINFO_NFLAGS + 1]; char optstring[16]; @@ -3311,9 +3323,16 @@ int cmd_jobinfo(int argc, char* argv[]) switch (opt) { case 'j': + ngiven++; parse_jobid(&jobid, optarg); + if (jobid == 0) { + /* bug-1709: a malformed jobid used to be skipped silently */ + fprintf(stderr, "jobinfo: invalid jobid '%s'\n", optarg); + lastrc = -EINVAL; + continue; + } - if (jobid > 0) { + { jobinfo_call_t* call = (jobinfo_call_t*)calloc(1, sizeof(*call)); @@ -3341,12 +3360,14 @@ int cmd_jobinfo(int argc, char* argv[]) free(call); if (rc < 0) { - perror("xcecli_jobinfo"); + cmdutil_rpc_error("xcecli_jobinfo", rc); + lastrc = rc; continue; } print_jobinfo(job); free_Job_ptr(&job); + nfound++; } break; @@ -3356,7 +3377,14 @@ int cmd_jobinfo(int argc, char* argv[]) "jobinfo -j [-j ...]"); } } - return 0; + + /* bug-1709: fail when there was nothing to look up or every lookup + * failed, so a script sees it in the exit code */ + if (ngiven == 0) { + fprintf(stderr, "jobinfo: no jobid given (use -j )\n"); + return -EINVAL; + } + return (nfound > 0) ? 0 : lastrc; } static int fill_job_alloc(xce_job_alloc_t* jalloc, int nhosts, @@ -3457,19 +3485,28 @@ int cmd_jobrun(int argc, char* argv[]) return -1; } - if (strchr(optarg, (int)':') != NULL) { - char* p; - char buf[256]; + { + char buf[256]; + int prc = cmdutil_split_host_tasks(optarg, buf, sizeof(buf), + &count); - p = strchr(optarg, (int)':'); - count = atoi(p + 1); - memset(buf, 0x0, sizeof(buf)); - - strncpy(buf, optarg, (int)(p - optarg)); + /* bug-1702: the host segment used to be strncpy'd into buf + * with no length check */ + if (prc == -ENAMETOOLONG) { + fprintf(stderr, + "jobrun: host name in -m '%.32s...' is longer " + "than %zu characters\n", + optarg, sizeof(buf) - 1); + return -EINVAL; + } + if (prc < 0) { + fprintf(stderr, + "jobrun: -m expects [:] with " + "ntasks >= 1, got '%s'\n", + optarg); + return -EINVAL; + } host = strdup(buf); - } else { - host = strdup(optarg); - count = 1; } hosts[nhosts].host = host; @@ -3523,8 +3560,7 @@ int cmd_jobrun(int argc, char* argv[]) } if (rc < 0) { - SET_ERROR(-rc); - perror("xce_jobdispatch"); + cmdutil_rpc_error("xce_jobdispatch", rc); return rc; } @@ -3536,29 +3572,6 @@ int cmd_jobrun(int argc, char* argv[]) return 0; } -static int build_job_envs(xce_job_submit_t* submit) -{ - exdata_t exdata = { .sub_env_vars = { .PATH = getenv("PATH") } }; - - s2j_create_json_obj(json_exdata); - s2j_create_json_obj(json_envs); - s2j_json_set_basic_element(json_envs, &(exdata.sub_env_vars), var_string, - PATH); - cJSON_AddItemToObject(json_exdata, "sub_env_vars", json_envs); - - submit->submit->start_info->exdata = cJSON_Print(json_exdata); - cJSON_Delete(json_exdata); - - return 0; -} - -static int build_job_exdata(xce_job_submit_t* submit) -{ - build_job_envs(submit); - - return 0; -} - static const flagspec_t jobsub_flags[] = { { 'u', "user", "user", required_argument, 1, NULL }, { 'q', "queue", "queue", required_argument, 1, NULL }, @@ -3639,27 +3652,19 @@ int cmd_jobsub(int argc, char* argv[]) submit.start_info->cmdline = strdup(optarg); break; case 'x': + FREEUP(submit.start_info->exdata); submit.start_info->exdata = strdup(optarg); break; case 'n': - if (strchr(optarg, (int)',') != NULL) { - char buf[64]; - char* p = strchr(optarg, (int)','); - - max = atoi(p + 1); - memset(buf, 0x0, sizeof(buf)); - strncpy(buf, optarg, (int)(p - optarg)); - min = atoi(buf); - - if (max == 0) { - fprintf(stderr, - "Incorrect setting of max tasks count, set to " - "same as min task value\n"); - max = min; - } - } else { - min = atoi(optarg); - max = min; + /* bug-1703: the min part used to be strncpy'd into a 64-byte + * buffer with no length check */ + if (cmdutil_parse_ntasks(optarg, &min, &max) < 0) { + fprintf(stderr, + "jobsub: -n expects [,] with 1 <= min <= " + "max, got '%.40s'\n", + optarg); + rc = -EINVAL; + goto cleanup; } break; case 'a': @@ -3677,12 +3682,6 @@ int cmd_jobsub(int argc, char* argv[]) } } - if (min == 0) { - fprintf(stderr, - "Incorrect setting of min task count, set to 1 as default\n"); - min = 1; - } - if (NULLSTR(submit.user)) { fprintf(stderr, "you need to specify job user with -u\n"); rc = -EINVAL; @@ -3734,7 +3733,12 @@ int cmd_jobsub(int argc, char* argv[]) submit.start_info->execcwd = strdup("/tmp"); } - build_job_exdata(&jobsubmit); + /* bug-1705: only when -x did not give one -- this used to overwrite + * (and leak) -x's exdata unconditionally */ + if (cmdutil_default_exdata(&submit.start_info->exdata, getenv("PATH")) < 0) { + rc = -ENOMEM; + goto cleanup; + } jobsubmit.array_size = arsize; @@ -3781,6 +3785,9 @@ cleanup: FREEUP(submit.resreq); FREEUP(submit.start_info->cmdline); FREEUP(submit.start_info->exdata); + FREEUP(submit.start_info->execuser); + FREEUP(submit.start_info->exechome); + FREEUP(submit.start_info->execcwd); FREEUP(submit.start_info); /* Report success only on a clean ack that echoes back the EXACT JobId we @@ -4115,9 +4122,20 @@ int cmd_jobkill(int argc, char* argv[]) while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) != -1) { switch (opt) { - case 's': - signo = atoi(optarg); + case 's': { + long n; + + /* bug-1708: a non-numeric value became signal 0 and the kill + * was still sent and reported */ + if (cmdutil_parse_long(optarg, 1, NSIG - 1, &n) < 0) { + fprintf(stderr, + "jobkill: -s expects a signal number 1-%d, got '%s'\n", + NSIG - 1, optarg); + return -EINVAL; + } + signo = (int)n; break; + } case 'j': parse_jobid(&jobid, optarg); break; @@ -4185,7 +4203,7 @@ int cmd_jobkill(int argc, char* argv[]) } if (rc < 0) { - perror("xcecli_jobctrl"); + cmdutil_rpc_error("xcecli_jobctrl", rc); return rc; } @@ -4302,7 +4320,7 @@ int cmd_jobckpt(int argc, char* argv[]) } if (rc < 0) { - perror("xcecli_jobctrl"); + cmdutil_rpc_error("xcecli_jobctrl", rc); return rc; } @@ -4385,6 +4403,7 @@ int cmd_qctrl(int argc, char* argv[]) action = QUEUE_CLOSE; break; case 'q': + FREEUP(queue); queue = strdup(optarg); break; case 'y': @@ -4437,6 +4456,7 @@ int cmd_qctrl(int argc, char* argv[]) qctrl_call_t* call = (qctrl_call_t*)calloc(1, sizeof(*call)); if (call == NULL) { + free(queue); return -ENOMEM; } @@ -4457,7 +4477,8 @@ int cmd_qctrl(int argc, char* argv[]) } if (rc < 0) { - perror("xcecli_queuectrl"); + cmdutil_rpc_error("xcecli_queuectrl", rc); + free(queue); /* bug-1797: leaked on this path */ return rc; } @@ -4533,6 +4554,7 @@ int cmd_hostctrl(int argc, char* argv[]) action = HOST_CLOSE; break; case 'h': + FREEUP(hostname); hostname = strdup(optarg); break; case 'y': @@ -4599,7 +4621,7 @@ int cmd_hostctrl(int argc, char* argv[]) } if (rc < 0) { - perror("xcecli_hostctrl"); + cmdutil_rpc_error("xcecli_hostctrl", rc); free(hostname); return rc; } @@ -4955,8 +4977,7 @@ int cmd_runtasks(int argc, char* argv[]) } if (rc < 0) { - errno = -rc; - perror("xcecli_runtask"); + cmdutil_rpc_error("xcecli_runtask", rc); } else { printf("run task ok\n"); } @@ -5108,7 +5129,7 @@ int cmd_setpend(int argc, char* argv[]) } if (rc < 0) { - perror("xcecli_set_pend_reason"); + cmdutil_rpc_error("xcecli_set_pend_reason", rc); erase_job_preason(&jreasons); return rc; } @@ -5174,8 +5195,7 @@ int cmd_join_worker(int argc, char* argv[]) } if (rc < 0) { - SET_ERROR(-rc); - perror("xce_join"); + cmdutil_rpc_error("xce_join", rc); return rc; } @@ -5291,8 +5311,7 @@ int cmd_jobstart(int argc, char* argv[]) } if (rc < 0) { - SET_ERROR(-rc); - perror("xcecli_jobstart"); + cmdutil_rpc_error("xcecli_jobstart", rc); return rc; } @@ -5394,8 +5413,7 @@ int cmd_jobfinish(int argc, char* argv[]) } if (rc < 0) { - SET_ERROR(-rc); - perror("xcecli_jobfinish"); + cmdutil_rpc_error("xcecli_jobfinish", rc); return rc; } @@ -5504,8 +5522,7 @@ int cmd_jobfail(int argc, char* argv[]) } if (rc < 0) { - SET_ERROR(-rc); - perror("xcecli_jobfail"); + cmdutil_rpc_error("xcecli_jobfail", rc); return rc; } @@ -5514,95 +5531,6 @@ int cmd_jobfail(int argc, char* argv[]) return 0; } -/* 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) -{ - int valtype; - int rc = -1; - - if (name == NULL) { - return 0; - } - - 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; -} - /* * resource-value-verification task 2.2: the controller's own update RPC * reply can never distinguish per-name accept/reject -- update_resval_list() @@ -5668,9 +5596,6 @@ static int resupdate_report_one(char* name, char* host, xce_val_t* sent) int ever_found = 0; if (name == NULL) { - /* set_resval()'s Value_u_svalue fallback path leaves .name unset - * (pre-existing gap predating this task -- see set_resval()'s own - * comment above); nothing usable to query or report here. */ return 0; } @@ -5743,6 +5668,37 @@ static int resupdate_report_one(char* name, char* host, xce_val_t* sent) return 0; } +/* Append one -n/-v pair to cmd_resupdate's vals[] (16 entries), taking + * ownership of `name`. A missing value gets the type's default with a + * warning. Returns 0, or -E2BIG when vals[] is full (the old code had no + * bound) or -ENOMEM; on error `name` is freed. */ +static int resupdate_add(xce_resval_t* vals, int* nres, char* name, + const char* val, int type_override) +{ + int rc; + + if (*nres >= 16) { + fprintf(stderr, "resupdate: at most 16 resources per call\n"); + free(name); + return -E2BIG; + } + if (val == NULL) { + fprintf(stderr, + "res `%s' has no value specified, default will be used\n", + name); + } + /* bug-1574: resval_fill() sets name/value for string resources + * (type, model) too; set_resval() left them uninitialized */ + rc = resval_fill(&vals[*nres], name, val, type_override, + get_setting("myname")); + if (rc < 0) { + free(name); + return rc; + } + (*nres)++; + return 0; +} + static const flagspec_t resupdate_flags[] = { { 'n', "resname", "resname", required_argument, 1, NULL }, { 'v', "value", "value", required_argument, 1, NULL }, @@ -5776,8 +5732,6 @@ int cmd_resupdate(int argc, char* argv[]) #define MAX_N_RES (16) int nres; xce_resval_t vals[MAX_N_RES]; - xce_resval_t* valptrs[MAX_N_RES]; - char* subnames[MAX_N_RES]; char* resname = NULL; char* resval_str = NULL; int type_override = RESVAL_TYPE_UNKNOWN; @@ -5789,6 +5743,7 @@ int cmd_resupdate(int argc, char* argv[]) optind = 0; opterr = 0; nres = 0; + memset(vals, 0, sizeof(vals)); if (flagspec_build_opts(resupdate_flags, RESUPDATE_NFLAGS, long_opts, optstring, sizeof(optstring)) @@ -5798,46 +5753,36 @@ int cmd_resupdate(int argc, char* argv[]) return -EINVAL; } - /* -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_long(argc, argv, optstring, long_opts, NULL)) != -1) { switch (opt) { case 'n': if (resname != NULL) { - if (resval_str == NULL) { - fprintf( - stderr, - "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]; - /* Tracked independently of vals[nres].name: set_resval() - * leaves .name unset on the Value_u_svalue fallback path - * (see its own comment), but `resname` here is always the - * exact name that was actually submitted for this pair. */ - subnames[nres] = resname; - nres++; - } + rc = resupdate_add(vals, &nres, resname, resval_str, + type_override); resname = NULL; resval_str = NULL; type_override = RESVAL_TYPE_UNKNOWN; + if (rc < 0) { + goto out; + } } resname = strdup(optarg); break; case 'v': - if (resname != NULL && resval_str == NULL) { - resval_str = optarg; - } else { + if (resname == NULL) { fprintf(stderr, "ignore res value `%s' as no res name found\n", optarg); + } else if (resval_str != NULL) { + /* bug-1712: this case used to print "no res name found" */ + fprintf(stderr, + "ignoring duplicate value `%s' for res `%s' (it " + "already has value `%s')\n", + optarg, resname, resval_str); + } else { + resval_str = optarg; } break; case 't': @@ -5859,31 +5804,38 @@ int cmd_resupdate(int argc, char* argv[]) } break; case '?': - return flagspec_reject( + rc = flagspec_reject( "resupdate", resupdate_flags, RESUPDATE_NFLAGS, argv[optind - 1], "resupdate -n -v [-t ] " "[-n -v [-t ] ...]"); + FREEUP(resname); + goto out; } } - /* 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]; - subnames[nres] = resname; - nres++; + /* bug-1707: the trailing -n was dropped without a word when it had no + * -v; it now gets the same warning and default as a mid-list one */ + if (resname != NULL) { + rc = resupdate_add(vals, &nres, resname, resval_str, type_override); + if (rc < 0) { + goto out; } } + if (nres == 0) { + fprintf(stderr, + "resupdate: no resource given (use -n -v )\n"); + rc = -EINVAL; + goto out; + } + { resupdate_call_t* call = (resupdate_call_t*)calloc(1, sizeof(*call)); if (call == NULL) { - return -ENOMEM; + rc = -ENOMEM; + goto out; } call->conn = active_ctx->ctx; @@ -5898,6 +5850,8 @@ int cmd_resupdate(int argc, char* argv[]) rc = xcetest_run_interruptible(resupdate_thunk, call); if (rc == -EINTR) { + /* the still-running worker thread owns `call`, whose vals[] + * point at the same names/values -- leave them allocated */ errno = EINTR; perror("resupdate"); return rc; @@ -5909,26 +5863,28 @@ int cmd_resupdate(int argc, char* argv[]) printf("resource update ok!\n"); - /* The update RPC's own reply can't carry per-name accept/reject - * (see the comment above resupdate_report_one()) -- verify each - * submitted pair individually via a follow-up query instead. An - * interrupt during one of THOSE follow-up queries stops this - * verification pass immediately and is reported as this command's - * own failure (Requirement 1.3), rather than silently skipping the - * remaining pairs' verification. */ for (i = 0; i < nres; i++) { int report_rc - = resupdate_report_one(subnames[i], vals[i].host, vals[i].val); + = resupdate_report_one(vals[i].name, vals[i].host, vals[i].val); if (report_rc == -EINTR) { - return report_rc; + return report_rc; /* same ownership rule as above */ } } + rc = 0; } else { - SET_ERROR(-rc); - perror("xcecli_update_resval"); + /* the RPC's failure used to be printed and then reported as + * success (return 0) */ + cmdutil_rpc_error("xcecli_update_resval", rc); } - return 0; + +out: + for (int i = 0; i < nres; i++) { + free(vals[i].name); + resval_fill_release(&vals[i]); + } + return rc; +#undef MAX_N_RES } typedef struct { @@ -5967,8 +5923,7 @@ int cmd_join_sched(int argc, char* argv[]) } if (rc < 0) { - SET_ERROR(-rc); - perror("xce_join"); + cmdutil_rpc_error("xce_join", rc); return rc; } @@ -6087,8 +6042,7 @@ int cmd_getconf(int argc, char* argv[]) free(call); if (rc < 0) { - SET_ERROR(-rc); - perror("xce_clusterconf"); + cmdutil_rpc_error("xce_clusterconf", rc); return rc; } @@ -6860,7 +6814,7 @@ static int worker_house_keeping(void* arg) rc = xcecli_heartbeat(ctx->ctx, HEARTBEAT_INTERVAL); if (rc < 0) { - perror("xcecli_heartbeat"); + cmdutil_rpc_error("xcecli_heartbeat", rc); } return rc; diff --git a/src/harness/testsuite/test_cmdutil.c b/src/harness/testsuite/test_cmdutil.c new file mode 100644 index 00000000..7a0d3b62 --- /dev/null +++ b/src/harness/testsuite/test_cmdutil.c @@ -0,0 +1,214 @@ +/* + * test_cmdutil.c -- unit tests for cmdutil.c, the xce_test argument and + * RPC-error helpers extracted from test.c so they can be tested without a + * live controller (bug-1702, bug-1703, bug-1705, bug-1708, bug-1710, + * bug-1796). + */ +#include +#include +#include +#include +#include +#include + +#include + +#include "cmdutil.h" +#include "xce/errnox.h" + +void setUp(void) {} +void tearDown(void) {} + +/* ── cmdutil_parse_long (bug-1708 jobkill -s, bug-1710 hello -c) ───────── */ + +static void test_parse_long_accepts_in_range_integer(void) +{ + long v = 0; + + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_long("9", 1, 64, &v)); + TEST_ASSERT_EQUAL_INT(9, v); +} + +static void test_parse_long_rejects_non_numeric(void) +{ + long v = 42; + + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_long("SIGKILL", 1, 64, &v)); + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_long("9x", 1, 64, &v)); + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_long("", 1, 64, &v)); + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_long(NULL, 1, 64, &v)); + TEST_ASSERT_EQUAL_INT_MESSAGE(42, v, "a rejected value must not be stored"); +} + +static void test_parse_long_rejects_out_of_range(void) +{ + long v = 0; + + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_long("0", 1, INT_MAX, &v)); + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_long("-3", 1, INT_MAX, &v)); + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_long("65", 1, 64, &v)); + TEST_ASSERT_EQUAL_INT( + -ERANGE, cmdutil_parse_long("99999999999999999999999", 1, 64, &v)); +} + +/* ── cmdutil_split_host_tasks (bug-1702 jobrun -m) ──────────────────────── */ + +static void test_split_host_tasks_host_and_count(void) +{ + char host[256]; + int n = 0; + + TEST_ASSERT_EQUAL_INT( + 0, cmdutil_split_host_tasks("node01:3", host, sizeof(host), &n)); + TEST_ASSERT_EQUAL_STRING("node01", host); + TEST_ASSERT_EQUAL_INT(3, n); +} + +static void test_split_host_tasks_host_only_defaults_to_one(void) +{ + char host[256]; + int n = 0; + + TEST_ASSERT_EQUAL_INT( + 0, cmdutil_split_host_tasks("node01", host, sizeof(host), &n)); + TEST_ASSERT_EQUAL_STRING("node01", host); + TEST_ASSERT_EQUAL_INT(1, n); +} + +static void test_split_host_tasks_rejects_overlong_host_without_overflow(void) +{ + /* host buffer followed by a canary: the old strncpy wrote past a + * 256-byte stack buffer for a host segment this long */ + struct { + char host[16]; + char canary[16]; + } b; + char arg[64]; + int n = 0; + + memset(&b, 'Z', sizeof(b)); + memset(arg, 'a', 40); + strcpy(arg + 40, ":2"); + + TEST_ASSERT_EQUAL_INT(-ENAMETOOLONG, + cmdutil_split_host_tasks(arg, b.host, sizeof(b.host), &n)); + for (size_t i = 0; i < sizeof(b.canary); i++) { + TEST_ASSERT_EQUAL_CHAR('Z', b.canary[i]); + } +} + +static void test_split_host_tasks_rejects_bad_count_or_empty_host(void) +{ + char host[256]; + int n = 0; + + TEST_ASSERT_EQUAL_INT( + -EINVAL, cmdutil_split_host_tasks("node:x", host, sizeof(host), &n)); + TEST_ASSERT_EQUAL_INT( + -EINVAL, cmdutil_split_host_tasks("node:0", host, sizeof(host), &n)); + TEST_ASSERT_EQUAL_INT( + -EINVAL, cmdutil_split_host_tasks(":2", host, sizeof(host), &n)); +} + +/* ── cmdutil_parse_ntasks (bug-1703 jobsub -n) ──────────────────────────── */ + +static void test_parse_ntasks_single_and_range(void) +{ + uint32_t mn = 0, mx = 0; + + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_ntasks("4", &mn, &mx)); + TEST_ASSERT_EQUAL_UINT32(4, mn); + TEST_ASSERT_EQUAL_UINT32(4, mx); + TEST_ASSERT_EQUAL_INT(0, cmdutil_parse_ntasks("2,8", &mn, &mx)); + TEST_ASSERT_EQUAL_UINT32(2, mn); + TEST_ASSERT_EQUAL_UINT32(8, mx); +} + +static void test_parse_ntasks_rejects_long_min_without_overflow(void) +{ + char arg[160]; + uint32_t mn = 0, mx = 0; + + /* 100 digits before the comma: the old code strncpy'd them into a + * 64-byte stack buffer */ + memset(arg, '1', 100); + strcpy(arg + 100, ",2"); + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_ntasks(arg, &mn, &mx)); +} + +static void test_parse_ntasks_rejects_malformed(void) +{ + uint32_t mn = 0, mx = 0; + + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_ntasks("a,2", &mn, &mx)); + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_ntasks("2,b", &mn, &mx)); + TEST_ASSERT_EQUAL_INT(-EINVAL, cmdutil_parse_ntasks("8,2", &mn, &mx)); + TEST_ASSERT_EQUAL_INT(-ERANGE, cmdutil_parse_ntasks("0", &mn, &mx)); +} + +/* ── cmdutil_default_exdata (bug-1705 jobsub -x) ────────────────────────── */ + +static void test_default_exdata_keeps_caller_value(void) +{ + char* exdata = strdup("{\"custom\":1}"); + char* before = exdata; + + TEST_ASSERT_EQUAL_INT(0, cmdutil_default_exdata(&exdata, "/usr/bin")); + TEST_ASSERT_EQUAL_PTR_MESSAGE(before, exdata, + "-x's exdata must not be replaced or leaked"); + TEST_ASSERT_EQUAL_STRING("{\"custom\":1}", exdata); + free(exdata); +} + +static void test_default_exdata_builds_path_json_when_unset(void) +{ + char* exdata = NULL; + + TEST_ASSERT_EQUAL_INT(0, cmdutil_default_exdata(&exdata, "/usr/bin:/bin")); + TEST_ASSERT_NOT_NULL(exdata); + TEST_ASSERT_NOT_NULL(strstr(exdata, "\"sub_env_vars\"")); + TEST_ASSERT_NOT_NULL(strstr(exdata, "\"PATH\"")); + TEST_ASSERT_NOT_NULL(strstr(exdata, "/usr/bin:/bin")); + free(exdata); +} + +/* ── cmdutil_errstr (bug-1796 stale errno in RPC error messages) ────────── */ + +static void test_errstr_reports_the_rpc_code_not_errno(void) +{ + char buf[128]; + + errno = EINTR; /* stale value left by some earlier call */ + TEST_ASSERT_EQUAL_STRING(strerror(ENOENT), + cmdutil_errstr(-ENOENT, buf, sizeof(buf))); +} + +static void test_errstr_names_xce_error_codes(void) +{ + char buf[128]; + + TEST_ASSERT_EQUAL_STRING("unknown queue", + cmdutil_errstr(-E_XCE_BAD_QUEUE, buf, sizeof(buf))); +} + +int main(void) +{ + UNITY_BEGIN(); + + RUN_TEST(test_parse_long_accepts_in_range_integer); + RUN_TEST(test_parse_long_rejects_non_numeric); + RUN_TEST(test_parse_long_rejects_out_of_range); + RUN_TEST(test_split_host_tasks_host_and_count); + RUN_TEST(test_split_host_tasks_host_only_defaults_to_one); + RUN_TEST(test_split_host_tasks_rejects_overlong_host_without_overflow); + RUN_TEST(test_split_host_tasks_rejects_bad_count_or_empty_host); + RUN_TEST(test_parse_ntasks_single_and_range); + RUN_TEST(test_parse_ntasks_rejects_long_min_without_overflow); + RUN_TEST(test_parse_ntasks_rejects_malformed); + RUN_TEST(test_default_exdata_keeps_caller_value); + RUN_TEST(test_default_exdata_builds_path_json_when_unset); + RUN_TEST(test_errstr_reports_the_rpc_code_not_errno); + RUN_TEST(test_errstr_names_xce_error_codes); + + return UNITY_END(); +} diff --git a/src/harness/testsuite/test_joblist_display.c b/src/harness/testsuite/test_joblist_display.c index 6a482813..19453d00 100644 --- a/src/harness/testsuite/test_joblist_display.c +++ b/src/harness/testsuite/test_joblist_display.c @@ -20,6 +20,12 @@ #include +/* Unity here is built without double support; every value compared below + * is a whole number, so compare as 64-bit integers. */ +#define ASSERT_WHOLE_EQ(expected, actual) \ + TEST_ASSERT_EQUAL_INT64((int64_t)(expected), (int64_t)(actual)) + + #include "xce/ctrlstatus_fmt.h" /* ctrlstatus_format_time_ms -- used directly * below to prove format_job_time() genuinely * delegates to it (byte-identical output), @@ -222,6 +228,43 @@ static void test_format_job_time_buffer_too_small_is_dash(void) void setUp(void) { return; } void tearDown(void) { return; } +/* bug-1777: workerlist showed the join-time attrs->maxswap even when a + * hosts.yaml override gave the worker a different effective maxswap (the + * value the scheduler uses). The workerlist reply carries the effective + * values in resvals_to_sched; prefer them. */ +static void test_effective_attr_prefers_resval(void) +{ + xce_val_t v = { .which = Value_u_fvalue, .u.fvalue = 10000.0 }; + xce_name_val_t nv = { .name = "maxswap", .val = &v }; + xce_name_val_t* l[] = { &nv }; + xce_worker_t w; + + memset(&w, 0, sizeof(w)); + w.nresvals_to_sched = 1; + w.resvals_to_sched = l; + + ASSERT_WHOLE_EQ(10000.0, worker_effective_attr(&w, "maxswap", 1073741824.0)); +} + +static void test_effective_attr_falls_back_to_join_value(void) +{ + xce_val_t v = { .which = Value_u_fvalue, .u.fvalue = 5.0 }; + xce_val_t s = { .which = Value_u_svalue, .u.svalue = "x" }; + xce_name_val_t nv = { .name = "slots", .val = &v }; + xce_name_val_t ns = { .name = "maxtmp", .val = &s }; + xce_name_val_t* l[] = { &nv, &ns }; + xce_worker_t w; + + memset(&w, 0, sizeof(w)); + ASSERT_WHOLE_EQ(42.0, worker_effective_attr(&w, "maxswap", 42.0)); + w.nresvals_to_sched = 2; + w.resvals_to_sched = l; + /* not present, and present but not numeric */ + ASSERT_WHOLE_EQ(42.0, worker_effective_attr(&w, "maxswap", 42.0)); + ASSERT_WHOLE_EQ(7.0, worker_effective_attr(&w, "maxtmp", 7.0)); + ASSERT_WHOLE_EQ(7.0, worker_effective_attr(NULL, "maxtmp", 7.0)); +} + int main(void) { printf("\n>> xce-test-cli-ux task 2.5 remediation: joblist_display " @@ -251,6 +294,8 @@ int main(void) RUN_TEST(test_format_job_time_zero_is_dash); RUN_TEST(test_format_job_time_matches_ctrlstatus_formatter); RUN_TEST(test_format_job_time_buffer_too_small_is_dash); + RUN_TEST(test_effective_attr_prefers_resval); + RUN_TEST(test_effective_attr_falls_back_to_join_value); return UNITY_END(); } diff --git a/src/harness/testsuite/test_join_worker_args.c b/src/harness/testsuite/test_join_worker_args.c index 42872dc4..9f54c879 100644 --- a/src/harness/testsuite/test_join_worker_args.c +++ b/src/harness/testsuite/test_join_worker_args.c @@ -17,8 +17,16 @@ #include #include +#include + #include +/* Unity here is built without double support; every value compared below + * is a whole number, so compare as 64-bit integers. */ +#define ASSERT_WHOLE_EQ(expected, actual) \ + TEST_ASSERT_EQUAL_INT64((int64_t)(expected), (int64_t)(actual)) + + #include "join_worker_args.h" #include "xce/types.h" /* hostattrs_t */ @@ -77,31 +85,43 @@ static void test_join_worker_null_attrs_returns_einval(void) TEST_ASSERT_EQUAL_INT(-EINVAL, rc); } -/* Documents/locks in the CURRENT (still-buggy, unchanged-by-this-task) - * default values of maxcpus/maxswap/maxtmp -- the sibling same-class fields - * task 2.6's implementer correctly declined to touch (a separate, - * recommended follow-up bug covers them). If someone later fixes those - * fields, THIS test's assertions (specific to maxmem) must not be confused - * with assertions on those separate fields. */ -static void test_join_worker_sibling_defaults_unchanged(void) +/* bug-1793: maxswap/maxtmp are bytes-canonical like maxmem (bug-1557); + * their defaults used to read 1024/2048 -- 1 KB of swap, 2 KB of tmp. The + * intended 1024 MB / 2048 MB are 1 GiB / 2 GiB in bytes. maxcpus is a plain + * count and stays 4. */ +static void test_join_worker_swap_tmp_defaults_are_bytes(void) { char* argv[] = { "join" }; hostattrs_t attrs; int rc = join_worker_parse_attrs(1, argv, &attrs); TEST_ASSERT_EQUAL_INT(0, rc); - TEST_ASSERT_EQUAL_INT_MESSAGE( - 4, attrs.maxcpus, - "maxcpus default is unchanged by task 2.6 (bug-1557 scoped to " - "maxmem only)"); - TEST_ASSERT_EQUAL_FLOAT_MESSAGE( - 1024.0f, (float)attrs.maxswap, - "maxswap's still-buggy default is intentionally unchanged by task " - "2.6 -- separate follow-up bug covers it"); - TEST_ASSERT_EQUAL_FLOAT_MESSAGE( - 2048.0f, (float)attrs.maxtmp, - "maxtmp's still-buggy default is intentionally unchanged by task " - "2.6 -- separate follow-up bug covers it"); + TEST_ASSERT_EQUAL_INT(4, attrs.maxcpus); + ASSERT_WHOLE_EQ(1024.0 * 1024 * 1024, attrs.maxswap); + ASSERT_WHOLE_EQ(2048.0 * 1024 * 1024, attrs.maxtmp); +} + +/* bug-1793: byte values above 2 GiB overflowed atoi() into garbage. */ +static void test_join_worker_byte_values_above_int_range(void) +{ + char* argv[] = { "join", "-m", "8589934592", "-s", "4294967296", + "-t", "10737418240" }; + hostattrs_t attrs; + int rc = join_worker_parse_attrs(7, argv, &attrs); + + TEST_ASSERT_EQUAL_INT(0, rc); + ASSERT_WHOLE_EQ(8589934592.0, attrs.maxmem); + ASSERT_WHOLE_EQ(4294967296.0, attrs.maxswap); + ASSERT_WHOLE_EQ(10737418240.0, attrs.maxtmp); +} + +/* A non-numeric byte value is an error, not a silent 0. */ +static void test_join_worker_non_numeric_byte_value_rejected(void) +{ + char* argv[] = { "join", "-s", "lots" }; + hostattrs_t attrs; + + TEST_ASSERT_EQUAL_INT(-EINVAL, join_worker_parse_attrs(3, argv, &attrs)); } /* ------------------------------------------------------------------ */ @@ -123,7 +143,9 @@ int main(void) RUN_TEST(test_join_worker_dash_m_overrides_default); RUN_TEST(test_join_worker_unrecognized_flag_returns_einval); RUN_TEST(test_join_worker_null_attrs_returns_einval); - RUN_TEST(test_join_worker_sibling_defaults_unchanged); + RUN_TEST(test_join_worker_swap_tmp_defaults_are_bytes); + RUN_TEST(test_join_worker_byte_values_above_int_range); + RUN_TEST(test_join_worker_non_numeric_byte_value_rejected); return UNITY_END(); } diff --git a/src/harness/testsuite/test_resval_typeresolve.c b/src/harness/testsuite/test_resval_typeresolve.c index cd4aba71..85bd4972 100644 --- a/src/harness/testsuite/test_resval_typeresolve.c +++ b/src/harness/testsuite/test_resval_typeresolve.c @@ -19,9 +19,18 @@ * which implements the behavior these two cases assert. */ #include +#include + +#include #include +/* Unity here is built without double support; every value compared below + * is a whole number, so compare as 64-bit integers. */ +#define ASSERT_WHOLE_EQ(expected, actual) \ + TEST_ASSERT_EQUAL_INT64((int64_t)(expected), (int64_t)(actual)) + + #include "resval_typeresolve.h" /* ------------------------------------------------------------------ */ @@ -89,6 +98,46 @@ static void test_unrecognized_name_with_override_resolves_requested_type( void setUp(void) { return; } void tearDown(void) { return; } +/* bug-1574: a string resource (type/model) never got its name or value + * set -- the wire encoder then read an uninitialized name pointer and + * crashed xce_test against a live controller. */ +static void test_fill_string_resource_sets_name_and_value(void) +{ + xce_resval_t rv; + + memset(&rv, 0xA5, sizeof(rv)); /* like the old uninitialized stack array */ + TEST_ASSERT_EQUAL_INT(1, resval_fill(&rv, "model", "Intel64", + RESVAL_TYPE_UNKNOWN, "host1")); + TEST_ASSERT_EQUAL_STRING("model", rv.name); + TEST_ASSERT_EQUAL_STRING("host1", rv.host); + TEST_ASSERT_EQUAL_INT(Value_u_svalue, rv.val->which); + TEST_ASSERT_EQUAL_STRING("Intel64", rv.val->u.svalue); + resval_fill_release(&rv); + TEST_ASSERT_NULL(rv.val); +} + +static void test_fill_numeric_resource(void) +{ + xce_resval_t rv; + + TEST_ASSERT_EQUAL_INT(1, resval_fill(&rv, "slots", "4", + RESVAL_TYPE_UNKNOWN, NULL)); + TEST_ASSERT_EQUAL_STRING("slots", rv.name); + TEST_ASSERT_EQUAL_INT(Value_u_fvalue, rv.val->which); + ASSERT_WHOLE_EQ(4.0, rv.val->u.fvalue); + resval_fill_release(&rv); +} + +static void test_fill_string_override_without_value_is_empty_string(void) +{ + xce_resval_t rv; + + TEST_ASSERT_EQUAL_INT(1, resval_fill(&rv, "arch", NULL, Value_u_svalue, NULL)); + TEST_ASSERT_EQUAL_INT(Value_u_svalue, rv.val->which); + TEST_ASSERT_EQUAL_STRING("", rv.val->u.svalue); + resval_fill_release(&rv); +} + int main(void) { printf("\n>> resource-value-verification task 1.1: resval_typeresolve " @@ -100,6 +149,9 @@ int main(void) RUN_TEST(test_known_string_name_resolves_svalue); RUN_TEST(test_unrecognized_name_defaults_to_numeric); RUN_TEST(test_unrecognized_name_with_override_resolves_requested_type); + RUN_TEST(test_fill_string_resource_sets_name_and_value); + RUN_TEST(test_fill_numeric_resource); + RUN_TEST(test_fill_string_override_without_value_is_empty_string); return UNITY_END(); } diff --git a/src/harness/testsuite/test_test_cli.c b/src/harness/testsuite/test_test_cli.c index 149197c1..92ba56cc 100644 --- a/src/harness/testsuite/test_test_cli.c +++ b/src/harness/testsuite/test_test_cli.c @@ -3462,6 +3462,182 @@ static void test_set_list_success_result_on_stdout_not_stderr(void) clitest_buf_free(&err); } +/* ------------------------------------------------------------------ */ +/* xce_test harness bug batch (bug-1702/1703/1707/1708/1709/1710/1712/ */ +/* 1796): input validation and error reporting, no controller running */ +/* ------------------------------------------------------------------ */ + +/* Runs `line` inside context `ctx` ("client"/"worker") with no controller + * reachable, then `echo harness_batch_alive` -- a crash or hang in `line` + * never prints the marker -- then quits. Returns xce_test's exit code; + * `out`/`err` (initialized here, freed by the caller) keep all output. */ +static int batch_run(const char* ctx, const char* line, clitest_buf_t* out, + clitest_buf_t* err) +{ + char xce_test_path[PATH_MAX]; + char* const argv[] = { "xce_test", "--norc", NULL }; + char prompt[64]; + clitest_session_t sess; + int exit_code = -999; + + TEST_ASSERT_EQUAL_INT( + 0, resolve_xce_test_path(xce_test_path, sizeof(xce_test_path))); + clitest_buf_init(out); + clitest_buf_init(err); + TEST_ASSERT_EQUAL_INT(0, clitest_spawn(&sess, xce_test_path, argv)); + TEST_ASSERT_TRUE(clitest_wait_for_text(&sess, out, err, "xce>", false, 5000)); + + snprintf(prompt, sizeof(prompt), "xce/%s>", ctx); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, ctx)); + TEST_ASSERT_TRUE(clitest_wait_for_text(&sess, out, err, prompt, false, 5000)); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, line)); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "echo harness_batch_alive")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, out, err, "harness_batch_alive", false, + 15000), + "xce_test must survive the command and keep reading input"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "quit")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_wait_for_exit(&sess, out, err, 5000, &exit_code)); + clitest_kill_and_reap(&sess); + clitest_close(&sess); + return exit_code; +} + +static void batch_free(clitest_buf_t* out, clitest_buf_t* err) +{ + clitest_buf_free(out); + clitest_buf_free(err); +} + +/* bug-1702: a -m host segment longer than jobrun's 256-byte host buffer + * overflowed it (ASAN stack-buffer-overflow); now a usage error. */ +static void test_jobrun_overlong_host_rejected_without_overflow(void) +{ + char line[512]; + char host[301]; + clitest_buf_t out, err; + + memset(host, 'a', 300); + host[300] = '\0'; + snprintf(line, sizeof(line), "jobrun -j 1-1-1-1 -m %s:2", host); + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", line, &out, &err)); + TEST_ASSERT_NOT_NULL(strstr(err.data, "is longer than 255 characters")); + batch_free(&out, &err); +} + +/* bug-1703: a -n min longer than jobsub's 64-byte buffer overflowed it + * (ASAN stack-buffer-overflow); now a usage error. */ +static void test_jobsub_overlong_ntasks_rejected_without_overflow(void) +{ + char line[512]; + char digits[101]; + clitest_buf_t out, err; + + memset(digits, '1', 100); + digits[100] = '\0'; + snprintf(line, sizeof(line), "jobsub -u u -q normal -c ls -n %s,2", digits); + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", line, &out, &err)); + TEST_ASSERT_NOT_NULL(strstr(err.data, "jobsub: -n expects")); + batch_free(&out, &err); +} + +/* bug-1708: `-s SIGKILL` became signal 0 and the kill was still sent. */ +static void test_jobkill_non_numeric_signal_rejected(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT( + 1, batch_run("client", "jobkill -j 1-1-1-1 -s SIGKILL -y", &out, &err)); + TEST_ASSERT_NOT_NULL(strstr(err.data, "jobkill: -s expects a signal number")); + TEST_ASSERT_NULL_MESSAGE(strstr(err.data, "xcecli_jobctrl"), + "a rejected -s must not reach the RPC"); + batch_free(&out, &err); +} + +/* bug-1709: jobinfo exited 0 with no -j and with a malformed -j. */ +static void test_jobinfo_without_valid_jobid_fails(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", "jobinfo", &out, &err)); + TEST_ASSERT_NOT_NULL(strstr(err.data, "jobinfo: no jobid given")); + batch_free(&out, &err); + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", "jobinfo -j garbage", &out, &err)); + TEST_ASSERT_NOT_NULL(strstr(err.data, "jobinfo: invalid jobid 'garbage'")); + batch_free(&out, &err); +} + +/* bug-1710: hello -c 0 sent nothing and still reported success. */ +static void test_hello_nonpositive_count_rejected(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", "hello -c 0", &out, &err)); + TEST_ASSERT_NOT_NULL(strstr(err.data, "hello: -c expects a count >= 1")); + batch_free(&out, &err); +} + +/* bug-1707: a trailing -n with no -v was dropped without a word. */ +static void test_resupdate_trailing_name_without_value_warns(void) +{ + clitest_buf_t out, err; + + batch_run("worker", "resupdate -n slots -v 4 -n mem", &out, &err); + TEST_ASSERT_NOT_NULL( + strstr(err.data, "res `mem' has no value specified, default will be used")); + batch_free(&out, &err); +} + +/* bug-1712: a second -v for the same resource was reported as "no res + * name found". */ +static void test_resupdate_duplicate_value_named_correctly(void) +{ + clitest_buf_t out, err; + + batch_run("worker", "resupdate -n slots -v 4 -v 8", &out, &err); + TEST_ASSERT_NOT_NULL( + strstr(err.data, "ignoring duplicate value `8' for res `slots'")); + TEST_ASSERT_NULL(strstr(err.data, "no res name found")); + batch_free(&out, &err); +} + +/* resupdate returned 0 even when its RPC failed (found with bug-1707). */ +static void test_resupdate_rpc_failure_fails_the_session(void) +{ + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT(1, batch_run("worker", "resupdate -n slots -v 4", &out, &err)); + TEST_ASSERT_NULL(strstr(out.data, "resource update ok!")); + batch_free(&out, &err); +} + +/* bug-1796: RPC failures printed perror() of whatever errno an earlier, + * unrelated call left behind -- with no controller, qctrl said "Connection + * refused" and jobkill "Invalid argument" for the same failure (piped: + * "Inappropriate ioctl for device", from isatty()). They must report the + * RPC's own return code, which here is -EINVAL for both. */ +static void test_rpc_error_reports_rpc_code_not_stale_errno(void) +{ + char want[128]; + clitest_buf_t out, err; + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", "qctrl -q bogus -a", &out, &err)); + snprintf(want, sizeof(want), "xcecli_queuectrl: %s\n", strerror(EINVAL)); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(err.data, want), err.data); + batch_free(&out, &err); + + TEST_ASSERT_EQUAL_INT(1, batch_run("client", "jobkill -j 1-1-1-1 -y", &out, &err)); + snprintf(want, sizeof(want), "xcecli_jobctrl: %s\n", strerror(EINVAL)); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(err.data, want), err.data); + batch_free(&out, &err); +} + void test_test_cli(void) { UnitySetTestFile(__FILE__); @@ -3491,4 +3667,13 @@ void test_test_cli(void) RUN_TEST(test_expect_jobs_malformed_count_diagnostic_on_stderr_not_stdout); RUN_TEST(test_wait_primary_invalid_timeout_diagnostic_on_stderr_not_stdout); RUN_TEST(test_set_list_success_result_on_stdout_not_stderr); + RUN_TEST(test_jobrun_overlong_host_rejected_without_overflow); + RUN_TEST(test_jobsub_overlong_ntasks_rejected_without_overflow); + RUN_TEST(test_jobkill_non_numeric_signal_rejected); + RUN_TEST(test_jobinfo_without_valid_jobid_fails); + RUN_TEST(test_hello_nonpositive_count_rejected); + RUN_TEST(test_resupdate_trailing_name_without_value_warns); + RUN_TEST(test_resupdate_duplicate_value_named_correctly); + RUN_TEST(test_resupdate_rpc_failure_fails_the_session); + RUN_TEST(test_rpc_error_reports_rpc_code_not_stale_errno); }