Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chopps/oper choice case #15131

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Chopps/oper choice case #15131

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32a4c40
lib: implement missing YANG choice/case statements.
choppsv1 Jan 11, 2024
7b7725f
lib: change type of `inner` to `struct lyd_node *`, cleaner code
choppsv1 Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 68 additions & 41 deletions lib/northbound_oper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,10 @@ struct nb_op_node_info {
* @xpath: current xpath representing the node_info stack.
* @xpath_orig: the original query string from the user
* @node_infos: the container stack for the walk from root to current
* @schema_path: the schema nodes for each node in the query string.
# @query_tokstr: the query string tokenized with NUL bytes.
* @schema_path: the schema nodes along the path indicated by the query string.
* this will include the choice and case nodes which are not
* present in the query string.
* @query_tokstr: the query string tokenized with NUL bytes.
* @query_tokens: the string pointers to each query token (node).
* @non_specific_predicate: tracks if a query_token is non-specific predicate.
* @walk_root_level: The topmost specific node, +1 is where we start walking.
Expand Down Expand Up @@ -204,6 +206,14 @@ static void ys_pop_inner(struct nb_op_yield_state *ys)
ys_trim_xpath(ys);
}

static void ys_free_inner(struct nb_op_yield_state *ys,
struct nb_op_node_info *ni)
{
if (!CHECK_FLAG(ni->schema->nodetype, LYS_CASE | LYS_CHOICE))
lyd_free_tree(&ni->inner->node);
ni->inner = NULL;
}

static void nb_op_get_keys(struct lyd_node_inner *list_node,
struct yang_list_keys *keys)
{
Expand Down Expand Up @@ -254,8 +264,7 @@ static bool __move_back_to_next(struct nb_op_yield_state *ys, int i)
* The i'th node has been lost after a yield so trim it from the tree
* now.
*/
lyd_free_tree(&ni->inner->node);
ni->inner = NULL;
ys_free_inner(ys, ni);
ni->list_entry = NULL;

/*
Expand Down Expand Up @@ -296,7 +305,7 @@ static void nb_op_resume_data_tree(struct nb_op_yield_state *ys)
ni = &ys->node_infos[i];
nn = ni->schema->priv;

if (CHECK_FLAG(ni->schema->nodetype, LYS_CONTAINER))
if (!CHECK_FLAG(ni->schema->nodetype, LYS_LIST))
continue;

assert(ni->list_entry != NULL ||
Expand Down Expand Up @@ -418,7 +427,8 @@ 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->node == inner->parent->child->prev);

if (CHECK_FLAG(inner->schema->nodetype, LYS_CONTAINER)) {
if (CHECK_FLAG(inner->schema->nodetype,
LYS_CASE | LYS_CHOICE | 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->node);
Expand Down Expand Up @@ -502,17 +512,19 @@ static enum nb_error nb_op_ys_init_node_infos(struct nb_op_yield_state *ys)
ret = NB_ERR_NOT_FOUND;
return ret;
}
while (node &&
!CHECK_FLAG(node->schema->nodetype, LYS_CONTAINER | LYS_LIST))

/* Move up to the container if on a leaf currently. */
if (node &&
!CHECK_FLAG(node->schema->nodetype, LYS_CONTAINER | LYS_LIST))
node = &node->parent->node;
assert(CHECK_FLAG(node->schema->nodetype, LYS_CONTAINER | LYS_LIST));
if (!node)
return NB_ERR_NOT_FOUND;

inner = (struct lyd_node_inner *)node;
for (len = 1; inner->parent; len++)
inner = inner->parent;


darr_append_nz_mt(ys->node_infos, len, MTYPE_NB_NODE_INFOS);

/*
Expand Down Expand Up @@ -761,7 +773,7 @@ static const struct lysc_node *nb_op_sib_next(struct nb_op_yield_state *ys,
/*
* If the node info stack is shorter than the schema path then we are
* doign specific query still on the node from the schema path (should
* match) so just return NULL.
* match) so just return NULL (i.e., don't process siblings)
*/
if (darr_len(ys->schema_path) > darr_len(ys->node_infos))
return NULL;
Expand Down Expand Up @@ -826,7 +838,10 @@ 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).
*/
assert(darr_last(ys->node_infos) != NULL && darr_last(ys->node_infos)->schema == parent);
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 (darr_lasti(ys->node_infos) < ys->query_base_level)
return ys->schema_path[darr_lasti(ys->node_infos) + 1];

Expand Down Expand Up @@ -937,28 +952,30 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume)
if (!sib) {
/*
* We've reached the end of the siblings inside a
* containing node; either a container or a specific
* list node entry.
* containing node; either a container, case, choice, or
* a specific list node entry.
*
* We handle container node inline; however, for lists
* we are only done with a specific entry and need to
* move to the next element on the list so we drop down
* into the switch for that case.
* We handle case/choice/container node inline; however,
* for lists we are only done with a specific entry and
* need to move to the next element on the list so we
* drop down into the switch for that case.
*/

/* Grab the containing node. */
sib = ni->schema;

if (sib->nodetype == LYS_CONTAINER) {
if (CHECK_FLAG(sib->nodetype,
LYS_CASE | LYS_CHOICE | LYS_CONTAINER)) {
/* If we added an empty container node (no
* children) and it's not a presence container
* or it's not backed by the get_elem callback,
* remove the node from the tree.
*/
if (!lyd_child(&ni->inner->node) &&
if (sib->nodetype == LYS_CONTAINER &&
!lyd_child(&ni->inner->node) &&
!nb_op_empty_container_ok(sib, ys->xpath,
ni->list_entry))
lyd_free_tree(&ni->inner->node);
ys_free_inner(ys, ni);

/* If we have returned to our original walk base,
* then we are done with the walk.
Expand Down Expand Up @@ -995,11 +1012,13 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume)
*/
}

/* TODO: old code checked for "first" here and skipped if set */
if (CHECK_FLAG(sib->nodetype,
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))
darr_in_strdup(xpath_child, ys->xpath);

nn = sib->priv;

switch (sib->nodetype) {
Expand Down Expand Up @@ -1032,27 +1051,31 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume)
goto done;
sib = nb_op_sib_next(ys, sib);
continue;
case LYS_CASE:
case LYS_CHOICE:
case LYS_CONTAINER:
if (CHECK_FLAG(nn->flags, F_NB_NODE_CONFIG_ONLY)) {
sib = nb_op_sib_next(ys, sib);
continue;
}

node = NULL;
err = lyd_new_inner(&ni->inner->node, sib->module,
sib->name, false, &node);
if (err) {
ret = NB_ERR_RESOURCE;
goto done;
if (sib->nodetype != LYS_CONTAINER) {
/* Case/choice use parent inner. */
node = &ni->inner->node;
} else {
err = lyd_new_inner(&ni->inner->node,
sib->module, sib->name,
false, &node);
if (err) {
ret = NB_ERR_RESOURCE;
goto done;
}
}

/* push this container node on top of the stack */
/* push this choice/container node on top of the stack */
ni = darr_appendz(ys->node_infos);
ni->inner = (struct lyd_node_inner *)node;
ni->schema = node->schema;
ni->niters = 0;
ni->nents = 0;
ni->has_lookup_next = false;
ni->schema = sib;
ni->lookup_next_ok = ni[-1].lookup_next_ok;
ni->list_entry = ni[-1].list_entry;

Expand Down Expand Up @@ -1279,7 +1302,7 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume)
lysc_is_key((*darr_last(ys->schema_path)))) &&
/* is this at or below the base? */
darr_ilen(ys->node_infos) <= ys->query_base_level)
lyd_free_tree(&ni->inner->node);
ys_free_inner(ys, ni);


if (!list_entry) {
Expand Down Expand Up @@ -1433,12 +1456,6 @@ static enum nb_error __walk(struct nb_op_yield_state *ys, bool is_resume)
sib = nb_op_sib_first(ys, sib);
continue;

case LYS_CHOICE:
/* Container type with no data */
/*FALLTHROUGH*/
case LYS_CASE:
/* Container type with no data */
/*FALLTHROUGH*/
default:
/*FALLTHROUGH*/
case LYS_ANYXML:
Expand Down Expand Up @@ -1534,8 +1551,7 @@ static void nb_op_trim_yield_state(struct nb_op_yield_state *ys)

DEBUGD(&nb_dbg_events, "NB oper-state: deleting tree at level %d", i);
__free_siblings(&ni->inner->node);
lyd_free_tree(&ni->inner->node);
ni->inner = NULL;
ys_free_inner(ys, ni);

while (--i > 0) {
DEBUGD(&nb_dbg_events,
Expand Down Expand Up @@ -1648,6 +1664,17 @@ 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;
}

while (true) {
s2 = strstr(s, name);
if (!s2)
Expand Down
27 changes: 27 additions & 0 deletions tests/lib/northbound/test_oper_data.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,25 @@ static struct yang_data *frr_test_module_vrfs_vrf_routes_route_active_get_elem(
return NULL;
}


/*
* XPath: /frr-test-module:frr-test-module/c1value
*/
static struct yang_data *
frr_test_module_c1value_get_elem(struct nb_cb_get_elem_args *args)
{
return yang_data_new_uint8(args->xpath, 21);
}

/*
* XPath: /frr-test-module:frr-test-module/c2cont/c2value
*/
static struct yang_data *
frr_test_module_c2cont_c2value_get_elem(struct nb_cb_get_elem_args *args)
{
return yang_data_new_uint32(args->xpath, 0xAB010203);
}

/* clang-format off */
const struct frr_yang_module_info frr_test_module_info = {
.name = "frr-test-module",
Expand Down Expand Up @@ -243,6 +262,14 @@ const struct frr_yang_module_info frr_test_module_info = {
.xpath = "/frr-test-module:frr-test-module/vrfs/vrf/routes/route/active",
.cbs.get_elem = frr_test_module_vrfs_vrf_routes_route_active_get_elem,
},
{
.xpath = "/frr-test-module:frr-test-module/c1value",
.cbs.get_elem = frr_test_module_c1value_get_elem,
},
{
.xpath = "/frr-test-module:frr-test-module/c2cont/c2value",
.cbs.get_elem = frr_test_module_c2cont_c2value_get_elem,
},
{
.xpath = NULL,
},
Expand Down
4 changes: 4 additions & 0 deletions tests/lib/northbound/test_oper_data.refout
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ test# show yang operational-data /frr-test-module:frr-test-module
}
}
]
},
"c1value": 21,
"c2cont": {
"c2value": 2868969987
}
Comment on lines +116 to 119
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks wrong that we simultaneously have values from two different cases of a choise in our result. I think it's a violation of the RFC https://datatracker.ietf.org/doc/html/rfc8342#section-5.3. It says about operational datastore:

   Only semantic constraints MAY be violated.  These are the YANG
   "when", "must", "mandatory", "unique", "min-elements", and
   "max-elements" statements; and the uniqueness of key values.

   Syntactic constraints MUST NOT be violated, including hierarchical
   organization, identifiers, and type-based constraints.  If a node in
   <operational> does not meet the syntactic constraints, then it
   MUST NOT be returned, and some other mechanism should be used to flag
   the error.

This seems like a hierarchical organization to me that should not be violated. Should we use lyd_validate_all with LYD_VALIDATE_OPERATIONAL option somewhere at the end to make sure we don't produce an incorrect operational tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is on purpose. We should trust the backend to get this right, or it's a bug in the backend. I don't see a reason to extra-police this at the cost of CPU time and complexity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine. We can always add a validation later if we decide that it's necessary. Merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think about the fact that the test case violated the standard, but then figured it was also OK since the testing framework here is too simple and it highlighted the fact that the backend could do this. I have more tests i did locally that we can work back in once we have some implemented models with real choice/case stements. :)

}
}
Expand Down
18 changes: 18 additions & 0 deletions yang/frr-test-module.yang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,23 @@ module frr-test-module {
}
}
}
choice achoice {
description "a choice statement";
case case1 {
leaf c1value {
type uint8;
description "A uint8 value for case 1";
}
}
case case2 {
container c2cont {
description "case 2 container";
leaf c2value {
type uint32;
description "A uint32 value for case 2";
}
}
}
}
}
}