diff --git a/src/include/resreq.h b/src/include/resreq.h index bebda5a0..7040c814 100644 --- a/src/include/resreq.h +++ b/src/include/resreq.h @@ -191,6 +191,7 @@ struct select_context_t_ { int err_pos; int has_error; char errmsg[256]; /* fixed-size — no alloc/free in grammar actions */ + int unknown_resource; /* bug-1887: the error was an unknown resource name */ int depth; /* current nesting depth — guard vs SELECT_MAX_DEPTH */ xce_select_node_t* root; /* result root set by grammar top-level action */ }; diff --git a/src/lib/resreq.c b/src/lib/resreq.c index 55542d7e..fad1e46a 100644 --- a/src/lib/resreq.c +++ b/src/lib/resreq.c @@ -325,7 +325,9 @@ static int parse_select(xce_primary_resreq_t* primary, char* selectString, *errmsg = strdup(ctx.errmsg); else get_parse_errmsg(errmsg, selectString, ctx.err_pos); - rc = -EINVAL; + /* bug-1887: -ENOENT tells a select naming an unregistered resource + * apart from a malformed one (-EINVAL) */ + rc = ctx.unknown_resource ? -ENOENT : -EINVAL; goto cleanup_parser; } @@ -535,7 +537,14 @@ int parse_resreq(xce_resreq_t* resreq, char* data, var resmgr, char** errmsg) xce_resreq_parse(p, NULL); if (ctx.has_error) { - get_parse_errmsg(errmsg, ctx.input, ctx.err_pos); + /* bug-1887: a primary's own message (select/rusage) names the + * failing clause; otherwise report the grammar error position */ + if (ctx.errmsg != NULL) { + *errmsg = ctx.errmsg; + ctx.errmsg = NULL; + } else { + get_parse_errmsg(errmsg, ctx.input, ctx.err_pos); + } rc = -EINVAL; /* FIXME: need to freeup partial data in resreqs */ } diff --git a/src/lib/testsuite/test_resreq.c b/src/lib/testsuite/test_resreq.c index b82cc5ff..264c4ed8 100644 --- a/src/lib/testsuite/test_resreq.c +++ b/src/lib/testsuite/test_resreq.c @@ -6,6 +6,12 @@ #include +#include + +#include "kvmgr.h" +#include "resobj.h" +#include "xstring.h" + #include "resreq.h" #include "utils.h" @@ -519,6 +525,86 @@ void test_check_rusage_syntax(void) } } +/* bug-1887: a primary whose select clause is malformed fails the whole + * resreq with that clause's message; it used to be accepted with no select + * at all, matching every host. */ +static void expect_resreq_error(var resmgr, char* str, const char* needle) +{ + xce_resreq_t resreq; + char* errmsg = NULL; + int rc; + + memset(&resreq, 0, sizeof(resreq)); + rc = parse_resreq(&resreq, str, resmgr, &errmsg); + TEST_ASSERT_TRUE_MESSAGE(rc < 0, str); + TEST_ASSERT_NOT_NULL_MESSAGE(errmsg, str); + TEST_ASSERT_NOT_NULL_MESSAGE(strstr(errmsg, needle), errmsg); + free(errmsg); + kill_resreq(&resreq); +} + +/* a resource manager of the kind the scheduler parses against (an xkvmgr + * of resobjs); make_test_resmgr()'s plain kvtab does not resolve selects */ +static var make_select_resmgr(void) +{ + kvmgr_params_t params = { .key_type = KVMGR_KEY_STRING, + .nslots = 64, + .delta = 8, + .datakiller = (destroy_fn_t)dereference }; + xce_resource_t tmpl; + var mgr = makeInstance(&xkvmgr_type, ¶ms); + + TEST_ASSERT_NOT_NULL(mgr); + memset(&tmpl, 0, sizeof(tmpl)); + tmpl.name = (char*)"type"; + tmpl.valtype = XCE_VAL_STRING; + TEST_ASSERT_NOT_NULL(makeResource(mgr, &tmpl)); + tmpl.name = (char*)"mem"; + tmpl.valtype = XCE_VAL_NUMERIC; + TEST_ASSERT_NOT_NULL(makeResource(mgr, &tmpl)); + + return mgr; +} + +void test_resreq_primary_select_error_fails_parse(void) +{ + var resmgr; + xce_resreq_t resreq; + char* errmsg = NULL; + int rc; + + TEST_ASSERT_TRUE(xstring_table_init(64) >= 0); + resreq_init(1); /* as the scheduler core does: select node table */ + resmgr = make_select_resmgr(); + + expect_resreq_error(resmgr, "select[type==] rusage[mem=1]", "Error"); + expect_resreq_error(resmgr, "select[mem>]", "Error"); + expect_resreq_error(resmgr, "2*{select[mem>1 &&]}", "Error"); + + /* an unregistered resource name is still dropped with the select, not + * an error (jobs rely on it; see xce_resreq.peg) */ + memset(&resreq, 0, sizeof(resreq)); + rc = parse_resreq(&resreq, "select[nosuch>1]", resmgr, &errmsg); + TEST_ASSERT_TRUE(rc >= 0); + TEST_ASSERT_NULL(errmsg); + TEST_ASSERT_NULL(resreq.u.primary->select); + kill_resreq(&resreq); + + /* a valid select still parses, and keeps its select */ + memset(&resreq, 0, sizeof(resreq)); + rc = parse_resreq(&resreq, "select[type==any] rusage[mem=1]", resmgr, + &errmsg); + TEST_ASSERT_TRUE_MESSAGE(rc >= 0, (errmsg != NULL) ? errmsg : "(null)"); + TEST_ASSERT_NULL(errmsg); + TEST_ASSERT_EQUAL_INT(XCE_RESREQ_PRIMARY, resreq.tag); + TEST_ASSERT_NOT_NULL(resreq.u.primary->select); + kill_resreq(&resreq); + + destroy(resmgr); + resreq_fini(); + xstring_table_fini(); +} + void test_resreq(void) { printf("\n>> resreq test\n\n"); @@ -538,4 +624,5 @@ void test_resreq(void) RUN_TEST(test_parse_section_explicit_select_with_span); RUN_TEST(test_parse_section_explicit_select_rusage_span); RUN_TEST(test_parse_section_span_not_in_select); + RUN_TEST(test_resreq_primary_select_error_fails_parse); } diff --git a/src/lib/xce_resreq.peg b/src/lib/xce_resreq.peg index 1628f33b..f0252cbb 100644 --- a/src/lib/xce_resreq.peg +++ b/src/lib/xce_resreq.peg @@ -2,6 +2,7 @@ %value "char *" %auxil "resreq_context_t *" %source { + #include #include #include #include @@ -108,7 +109,18 @@ primary_resreq <- < (! ('{' / '}' ) .)* > { auxil->primary = primary; free(auxil->errmsg); /* bug-1884: an earlier primary's message */ auxil->errmsg = NULL; - parse_resreq_primary(primary, (char *)$0, auxil->resmgr, &(auxil->errmsg)); + /* bug-1887: a primary whose select/rusage is malformed fails the + * whole resreq (it was kept without that clause). A select naming + * an unregistered resource (-ENOENT) is still dropped, as before -- + * jobs rely on it, the scheduler's own "type == any" default among + * them when no "type" resource is registered. */ + { + int prc = parse_resreq_primary(primary, (char *)$0, auxil->resmgr, + &(auxil->errmsg)); + if ((prc < 0) && (prc != -ENOENT)) { + auxil->has_error = 1; + } + } } } diff --git a/src/lib/xce_select.peg b/src/lib/xce_select.peg index 9fe83207..5057eed3 100644 --- a/src/lib/xce_select.peg +++ b/src/lib/xce_select.peg @@ -48,6 +48,7 @@ snprintf(auxil->errmsg, sizeof(auxil->errmsg), "unknown resource '%s'", name); } + auxil->unknown_resource = 1; auxil->has_error = 1; return NULL; } diff --git a/src/scheduler/testsuite/test_snapshot_save_load.c b/src/scheduler/testsuite/test_snapshot_save_load.c index 18cf570d..619d30c4 100644 --- a/src/scheduler/testsuite/test_snapshot_save_load.c +++ b/src/scheduler/testsuite/test_snapshot_save_load.c @@ -1335,12 +1335,10 @@ test_snapshot_load_leaves_schedobj_null_when_resreq_fails_to_parse(void) /* An unbalanced/unterminated compound-resreq brace: the top-level PEG * grammar (xce_resreq.peg) requires a full input match through EOF; a * lone unterminated '{' cannot complete as alter/compound/primary and - * leaves trailing unconsumed input, so parse_resreq() genuinely fails - * (unlike plain non-brace garbage, which the grammar's permissive - * primary_resreq rule accepts as a literal select expression and only - * fails later, silently, inside parse_resreq_primary() without - * propagating an error -- confirmed empirically while writing this - * test). */ + * leaves trailing unconsumed input, so parse_resreq() genuinely fails. + * (Plain non-brace garbage is taken as a select expression; before + * bug-1887 its failure inside parse_resreq_primary() was dropped + * silently, now a malformed select fails the parse too.) */ bad_job = make_job_with_resreq(404, JOB_STAT_PEND, "normal", "alice", "proj1", "{");