Skip to content

Commit

Permalink
Fixed: [Missing/no namespace error in YANG augments with default valu…
Browse files Browse the repository at this point in the history 
…es](#354)

Tests: ensure all netconf requests with identityref have declared namespaces.
  * This is part of fixing [Yang identityref XML encoding is not general](#90)
  • Loading branch information
@olofhagsand
olofhagsand committed Aug 18, 2022
1 parent b748d68 commit 74da966
Show file tree
Hide file tree
Showing 12 changed files with 177 additions and 104 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Expected: September 2022

### Corrected Bugs

* Fixed: [Missing/no namespace error in YANG augments with default values](https://github.com/clicon/clixon/issues/354)
* Fixed: [Validation of mandatory in choice/case does not work in some cases](https://github.com/clicon/clixon/issues/349)

## 5.8.0
Expand Down
117 changes: 88 additions & 29 deletions lib/src/clixon_datastore_write.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,27 @@ attr_ns_value(cxobj *x,
}

/*! When new body is added, some needs type lookup is made and namespace checked
*
* This includes identityrefs, paths
* This code identifies x0 as an identityref, looks at the _body_ string and ensures the right
* namespace is inserted in x1.
* @param[in] x1 Base xml tree (can be NULL in add scenarios)
* @param[in] x0 XML tree which modifies base
* @param[in] x0p Parent of x0
* @param[in] x1bstr Body string of x1
* @param[in] y Yang of x0 (and x1)
* @param[out] cbret Initialized cligen buffer. Contains return XML if retval is 0.
* @retval -1 Error
* @retval 0 Failed (cbret set)
* @retval 1 OK
*/
static int
check_body_namespace(cxobj *x0,
cxobj *x0p,
cxobj *x1,
cxobj *x1p,
char *x1bstr,
yang_stmt *y)
yang_stmt *y,
cbuf *cbret)
{
int retval = -1;
char *prefix = NULL;
Expand All @@ -151,56 +162,102 @@ check_body_namespace(cxobj *x0,
cxobj *xa;
cxobj *x;
int isroot;
cbuf *cberr = NULL;
int ret;

/* XXX: need to identify root better than hiereustics and strcmp,... */
isroot = xml_parent(x1p)==NULL &&
strcmp(xml_name(x1p), DATASTORE_TOP_SYMBOL) == 0 &&
xml_prefix(x1p)==NULL;
isroot = xml_parent(x0p)==NULL &&
strcmp(xml_name(x0p), DATASTORE_TOP_SYMBOL) == 0 &&
xml_prefix(x0p)==NULL;
if (nodeid_split(x1bstr, &prefix, NULL) < 0)
goto done;
if (prefix == NULL)
goto ok; /* skip */
if (xml2ns(x0, prefix, &ns0) < 0)
if (xml2ns(x1, prefix, &ns0) < 0)
goto done;
if (xml2ns(x1, prefix, &ns1) < 0)
if (xml2ns(x0, prefix, &ns1) < 0)
goto done;
if (ns0 != NULL){
if (ns1){
if (strcmp(ns0, ns1)){
clicon_err(OE_YANG, EFAULT, "identity namespace collision: %s: %s vs %s", x1bstr, ns0, ns1);
if (ns0 != NULL && ns1 != NULL){ /* namespace exists in both x1 and x0 */
if (strcmp(ns0, ns1)){
/* prefixes in x1 and x0 refers to different namespaces
* XXX return netconf error instead
bad-attribue?
*/
if ((cberr = cbuf_new()) == NULL){
clicon_err(OE_UNIX, errno, "cbuf_new");
goto done;
}
cprintf(cberr, "identityref: \"%s\": namespace collision %s vs %s", x1bstr, ns0, ns1);
if (netconf_invalid_value(cbret, "application", cbuf_get(cberr)) < 0)
goto done;
goto fail;
}
}
else if (ns0 != NULL && ns1 == NULL){ /* namespace exists in x0 but not in x1: add it to x1*/
if (isroot)
x = x0;
else
x = x0p;
if (nscache_set(x, prefix, ns0) < 0)
goto done;
/* Create xmlns attribute to x0 XXX same code ^*/
if (prefix){
if ((xa = xml_new(prefix, x, CX_ATTR)) == NULL)
goto done;
if (xml_prefix_set(xa, "xmlns") < 0)
goto done;
}
else{
if (isroot)
x = x1;
else
x = x1p;
if (nscache_set(x, prefix, ns0) < 0)
if ((xa = xml_new("xmlns", x, CX_ATTR)) == NULL)
goto done;
}
if (xml_value_set(xa, ns0) < 0)
goto done;
xml_sort(x); /* Ensure attr is first / XXX xml_insert? */
}
#if 0
else if (ns0 == NULL && ns1 != NULL){ /* namespace exists in x1 but not in x0: OK (but request is realy invalid */
}
#endif
else{ /* Namespace does not exist in x0: error */
#ifdef IDENTITYREF_KLUDGE
if (ns1 == NULL){
if ((ret = yang_find_namespace_by_prefix(y, prefix, &ns0)) < 0)
goto done;
/* Create xmlns attribute to x1 XXX same code ^*/
if (prefix){
if ((xa = xml_new(prefix, x, CX_ATTR)) == NULL)
if (ret == 0){ /* no such namespace in yang */
;
}
else{ /* Add it according to the kludge,... */
if ((xa = xml_new(prefix, x0, CX_ATTR)) == NULL)
goto done;
if (xml_prefix_set(xa, "xmlns") < 0)
goto done;

}
else{
if ((xa = xml_new("xmlns", x, CX_ATTR)) == NULL)
if (xml_value_set(xa, ns0) < 0)
goto done;
}
if (xml_value_set(xa, ns0) < 0)
goto done;
xml_sort(x); /* Ensure attr is first / XXX xml_insert? */
}
#else
if ((cberr = cbuf_new()) == NULL){
clicon_err(OE_UNIX, errno, "cbuf_new");
goto done;
}
cprintf(cberr, "identityref: \"%s\": prefix \"%s\" has no associated namespace", x1bstr, prefix);
if (netconf_invalid_value(cbret, "application", cbuf_get(cberr)) < 0)
goto done;
goto fail;
#endif
}
ok:
retval = 0;
retval = 1;
done:
if (cberr)
cbuf_free(cberr);
if (prefix)
free(prefix);
return retval;
fail:
retval = 0;
goto done;
}

/*! Check yang when condition between a new xml x1 and old x0
Expand Down Expand Up @@ -602,13 +659,15 @@ text_modify(clicon_handle h,
}
restype = yang_argument_get(yrestype);
/* Differentiate between an empty type (NULL) and an empty string "" */
if (x1bstr==NULL && strcmp(restype,"string")==0)
if (x1bstr==NULL && strcmp(restype, "string")==0)
x1bstr="";
if (x1bstr){
if (strcmp(restype, "identityref") == 0){
x1bstr = clixon_trim2(x1bstr, " \t\n");
if (check_body_namespace(x1, x0, x0p, x1bstr, y0) < 0)
if ((ret = check_body_namespace(x0, x0p, x1, x1bstr, y0, cbret)) < 0)
goto done;
if (ret == 0)
goto fail;
}
else{
/* Some bodies strip pretty-printed here, unsure where to do this,.. */
Expand Down
71 changes: 38 additions & 33 deletions lib/src/clixon_xml_map.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -651,10 +651,11 @@ xml_namespace_change(cxobj *x,
return retval;
}

int
/*!
*/
static int
xml_default_create1(yang_stmt *y,
cxobj *xt,
int top,
cxobj **xcp)
{
int retval = -1;
Expand All @@ -678,12 +679,6 @@ xml_default_create1(yang_stmt *y,
}
else{ /* Namespace does not exist in target, must add it w xmlns attr.
use source prefix */
if (!top){
if ((prefix = yang_find_myprefix(y)) == NULL){
clicon_err(OE_UNIX, errno, "strdup");
goto done;
}
}
if (add_namespace(xc, xc, prefix, namespace) < 0)
goto done;
/* Add prefix to x, if any */
Expand Down Expand Up @@ -718,7 +713,7 @@ xml_default_create(yang_stmt *y,
char *str;
cg_var *cv;

if (xml_default_create1(y, xt, top, &xc) < 0)
if (xml_default_create1(y, xt, &xc) < 0)
goto done;
xml_flag_set(xc, XML_FLAG_DEFAULT);
if ((xb = xml_new("body", xc, CX_BODY)) == NULL)
Expand Down Expand Up @@ -869,7 +864,7 @@ xml_default1(yang_stmt *yt,
if (create){
/* Retval shows there is a default value need to create the
* container */
if (xml_default_create1(yc, xt, top, &xc) < 0)
if (xml_default_create1(yc, xt, &xc) < 0)
goto done;
xml_sort(xt);
/* Then call it recursively */
Expand Down Expand Up @@ -923,6 +918,7 @@ xml_default(cxobj *xt,
* @param[in] state If set expand defaults also for state data, otherwise only config
* @retval 0 OK
* @retval -1 Error
* @see xml_global_defaults
*/
int
xml_default_recurse(cxobj *xn,
Expand Down Expand Up @@ -989,6 +985,7 @@ xml_global_defaults_create(cxobj *xt,
* @retval 0 OK
* @retval -1 Error
* Uses cache?
* @see xml_default_recurse
*/
int
xml_global_defaults(clicon_handle h,
Expand Down Expand Up @@ -1567,28 +1564,31 @@ xml_merge1(cxobj *x0, /* the target */
cxobj *x1, /* the source */
char **reason)
{
int retval = -1;
char *x1cname; /* child name */
cxobj *x0c; /* base child */
cxobj *x0b; /* base body */
cxobj *x1c; /* mod child */
char *x1bstr; /* mod body string */
yang_stmt *yc; /* yang child */
cbuf *cbr = NULL; /* Reason buffer */
int ret;
int i;
int retval = -1;
char *x1cname; /* child name */
cxobj *x0c; /* base child */
cxobj *x0b; /* base body */
cxobj *x1c; /* mod child */
char *x1bstr; /* mod body string */
yang_stmt *yc; /* yang child */
cbuf *cbr = NULL; /* Reason buffer */
int ret;
int i;
merge_twophase *twophase = NULL;
int twophase_len;
int twophase_len;
cvec *nsc = NULL;
cg_var *cv;
char *ns;
char *px;
char *pxe;

assert(x1 && xml_type(x1) == CX_ELMNT);
assert(y0);

if (x1 == NULL || xml_type(x1) != CX_ELMNT || y0 == NULL){
clicon_err(OE_XML, EINVAL, "x1 is NULL or not XML element, or lacks yang spec");
goto done;
}
if (x0 == NULL){
cvec *nsc = NULL;
cg_var *cv;
char *ns;
char *px;
nsc = cvec_dup(nscache_get_all(x1));
if (xml_nsctx_node(x1, &nsc) < 0)
goto done;
if (xml_rm(x1) < 0)
goto done;
/* This is to make the anydata case a little more robust, more could be done */
Expand All @@ -1603,13 +1603,16 @@ xml_merge1(cxobj *x0, /* the target */
while ((cv = cvec_each(nsc, cv)) != NULL){
px = cv_name_get(cv);
ns = cv_string_get(cv);
/* Check if it exists */
if (xml2prefix(x1, ns, NULL) == 0)
/* Check if namespace exists */
if ((ret = xml2prefix(x1, ns, &pxe)) < 0)
goto done;
if (ret == 0 || /* Not exist */
clicon_strcmp(px, pxe) != 0){ /* Exists and not equal (can be NULL) */
if (xmlns_set(x1, px, ns) < 0)
goto done;
xml_sort(x1);
}
}
if (nsc)
cvec_free(nsc);
goto ok;
}
if (yang_keyword_get(y0) == Y_LEAF_LIST || yang_keyword_get(y0) == Y_LEAF){
Expand Down Expand Up @@ -1692,6 +1695,8 @@ xml_merge1(cxobj *x0, /* the target */
ok:
retval = 1;
done:
if (nsc)
cvec_free(nsc);
if (twophase)
free(twophase);
if (cbr)
Expand Down
2 changes: 1 addition & 1 deletion test/test_augment.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS>
<name>e2</name>
<type>mymod:some-new-iftype</type>
<mymod:mandatory-leaf>true</mymod:mandatory-leaf>
<mymod:other>if:fddi</mymod:other>
<mymod:other xmlns:if=\"urn:ietf:params:xml:ns:yang:ietf-interfaces\">if:fddi</mymod:other>
</interface></interfaces></config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"

new "netconf validate ok"
Expand Down
10 changes: 5 additions & 5 deletions test/test_augment_state.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ cat <<EOF > $fstate
EOF

EXPSTATE=$(cat <<EOF
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><aug:gads xmlns:aug="urn:example:augment">gads</aug:gads></global-state>
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><gads xmlns="urn:example:augment">gads</gads></global-state>
EOF
)

Expand Down Expand Up @@ -184,13 +184,13 @@ new "4. Empty config + optional state, expect global default + optional state"
cat <<EOF > $fstate
<global-state xmlns="urn:example:lib">
<gbos>gbos</gbos>
<aug:gaos xmlns:aug="urn:example:augment">gaos</aug:gaos>
<gaos xmlns="urn:example:augment">gaos</gaos>
</global-state>
EOF

# Note Expect gbds(default) + gbos(optional), the latter given by file above
EXPSTATE=$(cat <<EOF
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><gbos>gbos</gbos><aug:gads xmlns:aug="urn:example:augment">gads</aug:gads><aug:gaos xmlns:aug="urn:example:augment">gaos</aug:gaos></global-state>
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><gbos>gbos</gbos><gads xmlns="urn:example:augment">gads</gads><gaos xmlns="urn:example:augment">gaos</gaos></global-state>
EOF
)

Expand All @@ -209,7 +209,7 @@ cat <<EOF > $fstate
EOF

EXPSTATE=$(cat <<EOF
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><aug:gads xmlns:aug="urn:example:augment">gads</aug:gads></global-state><base-config xmlns="urn:example:lib"><parameter><name>a</name><param-state><lbds>lbds</lbds><aug:lads xmlns:aug="urn:example:augment">lads</aug:lads></param-state></parameter></base-config>
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><gads xmlns="urn:example:augment">gads</gads></global-state><base-config xmlns="urn:example:lib"><parameter><name>a</name><param-state><lbds>lbds</lbds><lads xmlns="urn:example:augment">lads</lads></param-state></parameter></base-config>
EOF
)

Expand Down Expand Up @@ -238,7 +238,7 @@ cat <<EOF > $fstate
EOF

EXPSTATE=$(cat <<EOF
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><aug:gads xmlns:aug="urn:example:augment">gads</aug:gads></global-state><base-config xmlns="urn:example:lib"><parameter><name>a</name><param-state><lbds>lbds</lbds><lbos>lbos</lbos><aug:lads xmlns:aug="urn:example:augment">lads</aug:lads><laos xmlns="urn:example:augment">laos</laos></param-state></parameter></base-config>
<global-state xmlns="urn:example:lib"><gbds>gbds</gbds><gads xmlns="urn:example:augment">gads</gads></global-state><base-config xmlns="urn:example:lib"><parameter><name>a</name><param-state><lbds>lbds</lbds><lbos>lbos</lbos><lads xmlns="urn:example:augment">lads</lads><laos xmlns="urn:example:augment">laos</laos></param-state></parameter></base-config>
EOF
)

Expand Down
Loading

0 comments on commit 74da966

Please sign in to comment.