From 0502ca4218a3fcd188d3512f7a34e066d2163304 Mon Sep 17 00:00:00 2001 From: Olof hagsand Date: Sat, 5 Jan 2019 14:16:56 +0100 Subject: [PATCH] Hand-crafted validation messages removed and replaced with generic validations. --- CHANGELOG.md | 4 +- apps/netconf/netconf_rpc.c | 390 +++---------------------------------- 2 files changed, 30 insertions(+), 364 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36269387..937eeac7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,9 +80,9 @@ ### API changes on existing features (you may need to change your code) * Stricter YANG choice validation leads to enforcement of structures like: `choice c{ mandatory true; leaf x` statements. `x` was not previously enforced. -* CLICON_XML_SORT option (in clixon-config.yang) has been removed and set to true permanently since setting it to false is obsolete. +* Many hand-crafted validation messages have been removed and replaced with generic validations, which may lead to changed rpc-error messages. +* CLICON_XML_SORT option (in clixon-config.yang) has been removed and set to true permanently. Unsorted XML lists leads to slower performance and old obsolete code can be removed. * Strict namespace setting can be a problem when upgrading existing database files, such as startup-db or persistent running-db, or any other saved XML file. - * For backward compatibility, load of startup and running set CLICON_XML_NS_STRICT to false temporarily. * Removed `delete-config` support for candidate db since it is not supported in RFC6241. * Switched the order of `error-type` and `error-tag` in all netconf and restconf error messages to comply to RFC order. * Yang parser is stricter (see above) which may break parsing of existing yang specs. diff --git a/apps/netconf/netconf_rpc.c b/apps/netconf/netconf_rpc.c index 21a02193..ba41648a 100644 --- a/apps/netconf/netconf_rpc.c +++ b/apps/netconf/netconf_rpc.c @@ -135,20 +135,10 @@ netconf_get_config(clicon_handle h, { cxobj *xfilter; /* filter */ int retval = -1; - char *source; char *ftype = NULL; cxobj *xfilterconf; cxobj *xconf; - if ((source = netconf_get_target(xn, "source")) == NULL){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "source" - ""); - goto ok; - } /* ie ... */ if ((xfilter = xpath_first(xn, "filter")) != NULL) ftype = xml_find_value(xfilter, "type"); @@ -187,7 +177,6 @@ netconf_get_config(clicon_handle h, "type" ""); } - ok: /* netconf error is not fatal */ retval = 0; done: return retval; @@ -223,11 +212,9 @@ get_edit_opts(cxobj *xn, if ((optstr = xml_body(x)) != NULL){ if (strcmp(optstr, "test-then-set") == 0) *testopt = TEST_THEN_SET; - else - if (strcmp(optstr, "set") == 0) + else if (strcmp(optstr, "set") == 0) *testopt = SET; - else - if (strcmp(optstr, "test-only") == 0) + else if (strcmp(optstr, "test-only") == 0) *testopt = TEST_ONLY; else goto parerr; @@ -311,52 +298,18 @@ netconf_edit_config(clicon_handle h, { int retval = -1; int optret; - enum operation_type operation = OP_MERGE; enum test_option testopt = TEST_THEN_SET;/* only supports this */ enum error_option erropt = STOP_ON_ERROR; /* only supports this */ - cxobj *x; - cxobj *xfilter; - char *ftype = NULL; - char *target; /* db */ - /* must have target, and it should be candidate */ - if ((target = netconf_get_target(xn, "target")) == NULL || - strcmp(target, "candidate")){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "target" - ""); - goto ok; - } - /* CLICON addition, eg /> */ - if ((xfilter = xpath_first(xn, "filter")) != NULL) { - if ((ftype = xml_find_value(xfilter, "type")) != NULL) - if (strcmp(ftype,"restconf")){ - xml_parse_va(xret, NULL, "" - "invalid-value" - "protocol" - "error" - ""); - goto ok; - } - } - if ((x = xpath_first(xn, "default-operation")) != NULL){ - if (xml_operation(xml_body(x), &operation) < 0){ - xml_parse_va(xret, NULL, "" - "invalid-value" - "protocol" - "error" - ""); - goto ok; - } - } if ((optret = get_edit_opts(xn, &testopt, &erropt, xret)) < 0) goto done; if (optret == 0) /* error in opt parameters */ goto ok; - /* not supported opts */ + /* These constraints are clixon-specific since :validate should + * support all testopts, and erropts should be supported + * And therefore extends the validation + * (implement the features before removing these checks) + */ if (testopt!=TEST_THEN_SET || erropt!=STOP_ON_ERROR){ xml_parse_va(xret, NULL, "" "operation-not-supported" @@ -364,158 +317,15 @@ netconf_edit_config(clicon_handle h, "error" ""); goto ok; - } - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - ok: - retval = 0; - done: - return retval; -} - -/*! Netconf copy configuration - - - - - - - - - - - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_copy_config(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval = -1; - char *source; - char *target; /* filenames */ - - if ((source = netconf_get_target(xn, "source")) == NULL){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "source" - ""); - goto ok; - } - if ((target = netconf_get_target(xn, "target")) == NULL){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "target" - ""); - goto ok; - } - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - ok: - retval = 0; - done: - return retval; -} - -/*! Delete configuration - - - - - - Delete a configuration datastore. The - configuration datastore cannot be deleted. - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_delete_config(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - char *target; /* filenames */ - int retval = -1; - - if ((target = netconf_get_target(xn, "target")) == NULL || - strcmp(target, "running")==0){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "target" - ""); - goto ok; - } - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - ok: - retval = 0; - done: - return retval; -} - - -/*! Lock a database - - - - - - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_lock(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval = -1; - char *target; - - if ((target = netconf_get_target(xn, "target")) == NULL){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "target" - ""); - goto ok; } if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) goto done; ok: retval = 0; - done: + done: return retval; } -/*! Unlock a database - - - - - - XXX - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_unlock(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - return netconf_lock(h, xn, xret); -} - /*! Get running configuration and device state information * * @@ -579,138 +389,11 @@ netconf_get(clicon_handle h, "type" ""); } - // ok: /* netconf error is not fatal */ retval = 0; done: return retval; } - -/*! Close a (user) session - - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK -*/ -static int -netconf_close_session(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval = -1; - - cc_closed++; - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - retval = 0; - done: - return retval; -} - -/*! Kill other user sessions - - PID - - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_kill_session(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval=-1; - cxobj *xs; - - if ((xs = xpath_first(xn, "//session-id")) == NULL){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "session-id" - ""); - goto ok; - } - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - ok: - retval = 0; - done: - return retval; -} -/*! Check the semantic consistency of candidate - - :validate - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_validate(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval = -1; - char *target; - - if ((target = netconf_get_target(xn, "source")) == NULL){ - xml_parse_va(xret, NULL, "" - "missing-element" - "protocol" - "error" - "target" - ""); - goto ok; - } - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - ok: - retval = 0; - done: - return retval; -} - -/*! Commit candidate -> running - - :candidate - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_commit(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval = -1; - - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - retval = 0; - done: - return retval; -} - -/*! Discard all changes in candidate / revert to running - - :candidate - * @param[in] h clicon handle - * @param[in] xn Sub-tree (under xorig) at ... level. - * @param[out] xret Return XML, error or OK - */ -static int -netconf_discard_changes(clicon_handle h, - cxobj *xn, - cxobj **xret) -{ - int retval = -1; - - if (clicon_rpc_netconf_xml(h, xml_parent(xn), xret, NULL) < 0) - goto done; - retval = 0; - done: - return retval; -} - /*! Called when a notification has happened on backend * and this session has registered for that event. * Filter it and forward it. @@ -993,9 +676,25 @@ netconf_rpc_dispatch(clicon_handle h, if (xml_value_set(xa, username) < 0) goto done; } + /* Many of these calls are now calling generic clicon_rpc_netconf_xml + * directly, since the validation is generic and done before this place + * in the call. Some call however need extra validation, such as the + * filter parameter to get/get-config and tes- err-opts of edit-config. + */ xe = NULL; while ((xe = xml_child_each(xn, xe, CX_ELMNT)) != NULL) { - if (strcmp(xml_name(xe), "get-config") == 0){ + if (strcmp(xml_name(xe), "copy-config") == 0 || + strcmp(xml_name(xe), "delete-config") == 0 || + strcmp(xml_name(xe), "lock") == 0 || + strcmp(xml_name(xe), "unlock") == 0 || + strcmp(xml_name(xe), "kill-session") == 0 || + strcmp(xml_name(xe), "validate") == 0 || /* :validate */ + strcmp(xml_name(xe), "commit") == 0 || /* :candidate */ + strcmp(xml_name(xe), "discard-changes") == 0){ + if (clicon_rpc_netconf_xml(h, xml_parent(xe), xret, NULL) < 0) + goto done; + } + else if (strcmp(xml_name(xe), "get-config") == 0){ if (netconf_get_config(h, xe, xret) < 0) goto done; } @@ -1003,47 +702,14 @@ netconf_rpc_dispatch(clicon_handle h, if (netconf_edit_config(h, xe, xret) < 0) goto done; } - else if (strcmp(xml_name(xe), "copy-config") == 0){ - if (netconf_copy_config(h, xe, xret) < 0) - goto done; - } - else if (strcmp(xml_name(xe), "delete-config") == 0){ - if (netconf_delete_config(h, xe, xret) < 0) - goto done; - } - else if (strcmp(xml_name(xe), "lock") == 0) { - if (netconf_lock(h, xe, xret) < 0) - goto done; - } - else if (strcmp(xml_name(xe), "unlock") == 0){ - if (netconf_unlock(h, xe, xret) < 0) - goto done; - } else if (strcmp(xml_name(xe), "get") == 0){ if (netconf_get(h, xe, xret) < 0) goto done; } else if (strcmp(xml_name(xe), "close-session") == 0){ - if (netconf_close_session(h, xe, xret) < 0) - goto done; - } - else if (strcmp(xml_name(xe), "kill-session") == 0) { - if (netconf_kill_session(h, xe, xret) < 0) - goto done; - } - /* Validate capability :validate */ - else if (strcmp(xml_name(xe), "validate") == 0){ - if (netconf_validate(h, xe, xret) < 0) - goto done; - } - /* Candidate configuration capability :candidate */ - else if (strcmp(xml_name(xe), "commit") == 0){ - if (netconf_commit(h, xe, xret) < 0) - goto done; - } - else if (strcmp(xml_name(xe), "discard-changes") == 0){ - if (netconf_discard_changes(h, xe, xret) < 0) - goto done; + cc_closed++; + if (clicon_rpc_netconf_xml(h, xml_parent(xe), xret, NULL) < 0) + goto done; } /* RFC 5277 :notification */ else if (strcmp(xml_name(xe), "create-subscription") == 0){