From 2eeb1649d4a82d640b8ca4ad5263f98ee34e871c Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Sat, 22 Feb 2025 14:08:27 +0000 Subject: [PATCH 1/2] lib: nb: fix oper-state queries that involve choice/case nodes Signed-off-by: Christian Hopps (cherry picked from commit b9ff10b0863a81e95ca72778a2f1230b47ee2fae) # Conflicts: # lib/northbound_oper.c --- lib/northbound_oper.c | 91 ++++++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/lib/northbound_oper.c b/lib/northbound_oper.c index 5f38c970c77e..82d200a0056d 100644 --- a/lib/northbound_oper.c +++ b/lib/northbound_oper.c @@ -138,6 +138,11 @@ nb_op_create_yield_state(const char *xpath, struct yang_translator *translator, ys = XCALLOC(MTYPE_NB_YIELD_STATE, sizeof(*ys)); ys->xpath = darr_strdup_cap(xpath, (size_t)XPATH_MAXLEN); + /* remove trailing '/'s */ + while (darr_len(ys->xpath) > 1 && ys->xpath[darr_len(ys->xpath) - 2] == '/') { + darr_setlen(ys->xpath, darr_len(ys->xpath) - 1); + *darr_last(ys->xpath) = 0; + } ys->xpath_orig = darr_strdup(xpath); ys->translator = translator; ys->flags = flags; @@ -395,8 +400,7 @@ static enum nb_error nb_op_ys_finalize_node_info(struct nb_op_yield_state *ys, /* Assert that we are walking the rightmost branch */ assert(!inner->parent || inner == inner->parent->child->prev); - if (CHECK_FLAG(inner->schema->nodetype, - LYS_CASE | LYS_CHOICE | LYS_CONTAINER)) { + if (CHECK_FLAG(inner->schema->nodetype, LYS_CONTAINER)) { /* containers have only zero or one child on a branch of a tree */ inner = ((struct lyd_node_inner *)inner)->child; assert(!inner || inner->prev == inner); @@ -511,6 +515,13 @@ static enum nb_error nb_op_ys_init_node_infos(struct nb_op_yield_state *ys) * -- save the prefix length. */ inner = node; +<<<<<<< HEAD +======= + prevlen = 0; + xplen = strlen(xpath); + darr_free(ys->xpath); + ys->xpath = xpath; +>>>>>>> b9ff10b08 (lib: nb: fix oper-state queries that involve choice/case nodes) for (i = len; i > 0; i--, inner = &inner->parent->node) { ni = &ys->node_infos[i - 1]; ni->inner = inner; @@ -817,10 +828,15 @@ static const struct lysc_node *nb_op_sib_first(struct nb_op_yield_state *ys, * base of the user query, return the next schema node from the query * string (schema_path). */ +<<<<<<< HEAD if (darr_last(ys->node_infos) != NULL && !CHECK_FLAG(darr_last(ys->node_infos)->schema->nodetype, LYS_CASE | LYS_CHOICE)) assert(darr_last(ys->node_infos)->schema == parent); +======= + if (last != NULL) + assert(last->schema == parent); +>>>>>>> b9ff10b08 (lib: nb: fix oper-state queries that involve choice/case nodes) if (darr_lasti(ys->node_infos) < ys->query_base_level) return ys->schema_path[darr_lasti(ys->node_infos) + 1]; @@ -896,7 +912,8 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume) if (!walk_stem_tip) return NB_ERR_NOT_FOUND; - if (ys->schema_path[0]->nodetype == LYS_CHOICE) { + if (ys->schema_path[0]->parent && + CHECK_FLAG(ys->schema_path[0]->parent->nodetype, LYS_CHOICE|LYS_CASE)) { flog_err(EC_LIB_NB_OPERATIONAL_DATA, "%s: unable to walk root level choice node from module: %s", __func__, ys->schema_path[0]->module->name); @@ -1002,8 +1019,12 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume) LYS_LEAF | LYS_LEAFLIST | LYS_CONTAINER)) xpath_child = nb_op_get_child_path(ys->xpath, sib, xpath_child); - else if (CHECK_FLAG(sib->nodetype, LYS_CASE | LYS_CHOICE)) + else if (CHECK_FLAG(sib->nodetype, LYS_CASE | LYS_CHOICE)) { darr_in_strdup(xpath_child, ys->xpath); + len = darr_last(ys->node_infos)->xpath_len; + darr_setlen(xpath_child, len + 1); + xpath_child[len] = 0; + } nn = sib->priv; @@ -1619,22 +1640,22 @@ static enum nb_error nb_op_ys_init_schema_path(struct nb_op_yield_state *ys, * NOTE: appears to be a bug in nb_node linkage where parent can be NULL, * or I'm misunderstanding the code, in any case we use the libyang * linkage to walk which works fine. - * - * XXX: we don't actually support choice/case yet, they are container - * types in the libyang schema, but won't be in data so our length - * checking gets messed up. */ - for (sn = nblast->snode, count = 0; sn; count++, sn = sn->parent) + for (sn = nblast->snode, count = 0; sn; sn = sn->parent) { if (sn != nblast->snode) assert(CHECK_FLAG(sn->nodetype, - LYS_CONTAINER | LYS_LIST | - LYS_CHOICE | LYS_CASE)); + LYS_CONTAINER | LYS_LIST | LYS_CHOICE | LYS_CASE)); + if (!CHECK_FLAG(sn->nodetype, LYS_CHOICE | LYS_CASE)) + count++; + } /* create our arrays */ darr_append_n(ys->schema_path, count); darr_append_n(ys->query_tokens, count); darr_append_nz(ys->non_specific_predicate, count); - for (sn = nblast->snode; sn; sn = sn->parent) - ys->schema_path[--count] = sn; + for (sn = nblast->snode; sn; sn = sn->parent) { + if (!CHECK_FLAG(sn->nodetype, LYS_CHOICE | LYS_CASE)) + ys->schema_path[--count] = sn; + } /* * Now tokenize the query string and get pointers to each token @@ -1653,50 +1674,42 @@ static enum nb_error nb_op_ys_init_schema_path(struct nb_op_yield_state *ys, int nlen = strlen(name); int mnlen = 0; - /* - * Technically the query_token for choice/case should probably be pointing at - * the child (leaf) rather than the parent (container), however, - * we only use these for processing list nodes so KISS. - */ - if (CHECK_FLAG(ys->schema_path[i]->nodetype, - LYS_CASE | LYS_CHOICE)) { - ys->query_tokens[i] = ys->query_tokens[i - 1]; - continue; - } - + s2 = s; while (true) { - s2 = strstr(s, name); + /* skip past any module name prefix */ + s2 = strstr(s2, name); if (!s2) goto error; - if (s2[-1] == ':') { + if (s2 > s && s2[-1] == ':') { mnlen = strlen(modname) + 1; - if (ys->query_tokstr > s2 - mnlen || - strncmp(s2 - mnlen, modname, mnlen - 1)) - goto error; + if (s2 - s < mnlen || strncmp(s2 - mnlen, modname, mnlen - 1)) { + /* No match of module prefix, advance and try again */ + s2 += strlen(name); + continue; + } s2 -= mnlen; nlen += mnlen; } - s = s2; - if ((i == 0 || s[-1] == '/') && - (s[nlen] == 0 || s[nlen] == '[' || s[nlen] == '/')) + if ((i == 0 || s2[-1] == '/') && + (s2[nlen] == 0 || s2[nlen] == '[' || s2[nlen] == '/')) { + s = s2; break; - /* - * Advance past the incorrect match, must have been - * part of previous predicate. - */ - s += nlen; + } + /* No exact match at end, advance and try again */ + s2 += strlen(name); } /* NUL terminate previous token and save this one */ - if (i > 0) + if (i > 0) { + assert(s[-1] == '/'); s[-1] = 0; + } ys->query_tokens[i] = s; s += nlen; } - /* NOTE: need to subtract choice/case nodes when these are supported */ ys->query_base_level = darr_lasti(ys->schema_path); return NB_OK; From ef4b24d82cbacb76b40a7dc951203773c3c086a6 Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Sat, 22 Feb 2025 14:09:38 +0000 Subject: [PATCH 2/2] tests: add unit-test for choice/case node queries Signed-off-by: Christian Hopps (cherry picked from commit 73df7da40a107eab52a6819e6325d950ba569220) # Conflicts: # tests/lib/northbound/test_oper_data.in # tests/lib/northbound/test_oper_data.refout --- tests/lib/northbound/test_oper_data.in | 10 +++ tests/lib/northbound/test_oper_data.refout | 76 ++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/tests/lib/northbound/test_oper_data.in b/tests/lib/northbound/test_oper_data.in index f7c44cad3154..0195ab4cd130 100644 --- a/tests/lib/northbound/test_oper_data.in +++ b/tests/lib/northbound/test_oper_data.in @@ -1,2 +1,12 @@ show yang operational-data /frr-test-module:frr-test-module +<<<<<<< HEAD +======= +show yang operational-data /frr-test-module:frr-test-module/vrfs/vrf[name='vrf0']/routes/route[2] +show yang operational-data /frr-test-module:frr-test-module/vrfs/vrf[name='vrf0']/routes/route[3]/interface +show yang operational-data /frr-test-module:frr-test-module/vrfs/vrf[name='vrf0']/routes/route[10] +show yang operational-data /frr-test-module:frr-test-module/c1value +show yang operational-data /frr-test-module:frr-test-module/c2cont +show yang operational-data /frr-test-module:frr-test-module/c2cont/ +show yang operational-data /frr-test-module:frr-test-module/c2cont/c2value +>>>>>>> 73df7da40 (tests: add unit-test for choice/case node queries) test rpc diff --git a/tests/lib/northbound/test_oper_data.refout b/tests/lib/northbound/test_oper_data.refout index 7c565641431c..bfa9eaa3bbce 100644 --- a/tests/lib/northbound/test_oper_data.refout +++ b/tests/lib/northbound/test_oper_data.refout @@ -119,6 +119,82 @@ test# show yang operational-data /frr-test-module:frr-test-module } } } +<<<<<<< HEAD +======= +test# show yang operational-data /frr-test-module:frr-test-module/vrfs/vrf[name='vrf0']/routes/route[2] +{ + "frr-test-module:frr-test-module": { + "vrfs": { + "vrf": [ + { + "name": "vrf0", + "routes": { + "route": [ + { + "prefix": "10.0.0.1/32", + "next-hop": "172.16.0.1", + "interface": "eth1", + "metric": 1 + } + ] + } + } + ] + } + } +} +test# show yang operational-data /frr-test-module:frr-test-module/vrfs/vrf[name='vrf0']/routes/route[3]/interface +{ + "frr-test-module:frr-test-module": { + "vrfs": { + "vrf": [ + { + "name": "vrf0", + "routes": { + "route": [ + { + "interface": "eth2" + } + ] + } + } + ] + } + } +} +test# show yang operational-data /frr-test-module:frr-test-module/vrfs/vrf[name='vrf0']/routes/route[10] +{} +test# show yang operational-data /frr-test-module:frr-test-module/c1value +{ + "frr-test-module:frr-test-module": { + "c1value": 21 + } +} +test# show yang operational-data /frr-test-module:frr-test-module/c2cont +{ + "frr-test-module:frr-test-module": { + "c2cont": { + "c2value": 2868969987 + } + } +} +test# show yang operational-data /frr-test-module:frr-test-module/c2cont/ +{ + "frr-test-module:frr-test-module": { + "c2cont": { + "c2value": 2868969987 + } + } +} +test# show yang operational-data /frr-test-module:frr-test-module/c2cont/c2value +{ + "frr-test-module:frr-test-module": { + "c2cont": { + "c2value": 2868969987 + } + } +} +>>>>>>> 73df7da40 (tests: add unit-test for choice/case node queries) test# test rpc vrf testname data testdata test#