Merge pull request #3457 from opensourcerouting/keyless-lists

lib, tests: add support for keyless YANG lists

lib/northbound.c

@@ -95,6 +95,13 @@ static int nb_node_new_cb(const struct lys_node *snode, void *arg)
 		if (config_only)
 			SET_FLAG(nb_node->flags, F_NB_NODE_CONFIG_ONLY);
 	}
+	if (CHECK_FLAG(snode->nodetype, LYS_LIST)) {
+		struct lys_node_list *slist;
+
+		slist = (struct lys_node_list *)snode;
+		if (slist->keys_size == 0)
+			SET_FLAG(nb_node->flags, F_NB_NODE_KEYLESS_LIST);
+	}
 
 	/*
 	 * Link the northbound node and the libyang schema node with one
@@ -1056,6 +1063,7 @@ static int nb_oper_data_iter_list(const struct nb_node *nb_node,
 {
 	struct lys_node_list *slist = (struct lys_node_list *)nb_node->snode;
 	const void *list_entry = NULL;
+	uint32_t position = 1;
 
 	if (CHECK_FLAG(nb_node->flags, F_NB_NODE_CONFIG_ONLY))
 		return NB_OK;
@@ -1073,19 +1081,31 @@ static int nb_oper_data_iter_list(const struct nb_node *nb_node,
 			/* End of the list. */
 			break;
 
-		/* Obtain the list entry keys. */
-		if (nb_node->cbs.get_keys(list_entry, &list_keys) != NB_OK) {
-			flog_warn(EC_LIB_NB_CB_STATE,
-				  "%s: failed to get list keys", __func__);
-			return NB_ERR;
-		}
+		if (!CHECK_FLAG(nb_node->flags, F_NB_NODE_KEYLESS_LIST)) {
+			/* Obtain the list entry keys. */
+			if (nb_node->cbs.get_keys(list_entry, &list_keys)
+			    != NB_OK) {
+				flog_warn(EC_LIB_NB_CB_STATE,
+					  "%s: failed to get list keys",
+					  __func__);
+				return NB_ERR;
+			}
 
-		/* Build XPath of the list entry. */
-		strlcpy(xpath, xpath_list, sizeof(xpath));
-		for (unsigned int i = 0; i < list_keys.num; i++) {
-			snprintf(xpath + strlen(xpath),
-				 sizeof(xpath) - strlen(xpath), "[%s='%s']",
-				 slist->keys[i]->name, list_keys.key[i]);
+			/* Build XPath of the list entry. */
+			strlcpy(xpath, xpath_list, sizeof(xpath));
+			for (unsigned int i = 0; i < list_keys.num; i++) {
+				snprintf(xpath + strlen(xpath),
+					 sizeof(xpath) - strlen(xpath),
+					 "[%s='%s']", slist->keys[i]->name,
+					 list_keys.key[i]);
+			}
+		} else {
+			/*
+			 * Keyless list - build XPath using a positional index.
+			 */
+			snprintf(xpath, sizeof(xpath), "%s[%u]", xpath_list,
+				 position);
+			position++;
 		}
 
 		/* Iterate over the child nodes. */
@@ -1400,6 +1420,8 @@ bool nb_operation_is_valid(enum nb_operation operation,
 		case LYS_LIST:
 			if (CHECK_FLAG(nb_node->flags, F_NB_NODE_CONFIG_ONLY))
 				return false;
+			if (CHECK_FLAG(nb_node->flags, F_NB_NODE_KEYLESS_LIST))
+				return false;
 			break;
 		default:
 			return false;

lib/northbound.h

@@ -254,7 +254,8 @@ struct nb_callbacks {
 	 * Operational data callback for YANG lists.
 	 *
 	 * The callback function should fill the 'keys' parameter based on the
-	 * given list_entry.
+	 * given list_entry. Keyless lists don't need to implement this
+	 * callback.
 	 *
 	 * list_entry
 	 *    Pointer to list entry.
@@ -272,7 +273,8 @@ struct nb_callbacks {
 	 * Operational data callback for YANG lists.
 	 *
 	 * The callback function should return a list entry based on the list
-	 * keys given as a parameter.
+	 * keys given as a parameter. Keyless lists don't need to implement this
+	 * callback.
 	 *
 	 * parent_list_entry
 	 *    Pointer to parent list entry.
@@ -367,6 +369,8 @@ struct nb_node {
 };
 /* The YANG container or list contains only config data. */
 #define F_NB_NODE_CONFIG_ONLY 0x01
+/* The YANG list doesn't contain key leafs. */
+#define F_NB_NODE_KEYLESS_LIST 0x02
 
 struct frr_yang_module_info {
 	/* YANG module name. */

lib/northbound_confd.c

@@ -138,10 +138,19 @@ static int frr_confd_hkeypath_get_list_entry(const confd_hkeypath_t *kp,
 			nb_node_list = nb_node_list->parent_list;
 
 		/* Obtain list entry. */
-		*list_entry =
-			nb_node_list->cbs.lookup_entry(*list_entry, &keys);
-		if (*list_entry == NULL)
-			return -1;
+		if (!CHECK_FLAG(nb_node_list->flags, F_NB_NODE_KEYLESS_LIST)) {
+			*list_entry = nb_node_list->cbs.lookup_entry(
+				*list_entry, &keys);
+			if (*list_entry == NULL)
+				return -1;
+		} else {
+			unsigned long ptr_ulong;
+
+			/* Retrieve list entry from pseudo-key (string). */
+			if (sscanf(keys.key[0], "%lu", &ptr_ulong) != 1)
+				return -1;
+			*list_entry = (const void *)ptr_ulong;
+		}
 
 		curr_list++;
 	}
@@ -640,7 +649,6 @@ static int frr_confd_data_get_next(struct confd_trans_ctx *tctx,
 {
 	struct nb_node *nb_node;
 	char xpath[BUFSIZ];
-	struct yang_list_keys keys;
 	struct yang_data *data;
 	const void *parent_list_entry, *nb_next;
 	confd_value_t v[LIST_MAXKEYS];
@@ -672,18 +680,53 @@ static int frr_confd_data_get_next(struct confd_trans_ctx *tctx,
 
 	switch (nb_node->snode->nodetype) {
 	case LYS_LIST:
-		memset(&keys, 0, sizeof(keys));
-		if (nb_node->cbs.get_keys(nb_next, &keys) != NB_OK) {
-			flog_warn(EC_LIB_NB_CB_STATE,
-				  "%s: failed to get list keys", __func__);
-			confd_data_reply_next_key(tctx, NULL, -1, -1);
-			return CONFD_OK;
-		}
+		if (!CHECK_FLAG(nb_node->flags, F_NB_NODE_KEYLESS_LIST)) {
+			struct yang_list_keys keys;
 
-		/* Feed keys to ConfD. */
-		for (size_t i = 0; i < keys.num; i++)
-			CONFD_SET_STR(&v[i], keys.key[i]);
-		confd_data_reply_next_key(tctx, v, keys.num, (long)nb_next);
+			memset(&keys, 0, sizeof(keys));
+			if (nb_node->cbs.get_keys(nb_next, &keys) != NB_OK) {
+				flog_warn(EC_LIB_NB_CB_STATE,
+					  "%s: failed to get list keys",
+					  __func__);
+				confd_data_reply_next_key(tctx, NULL, -1, -1);
+				return CONFD_OK;
+			}
+
+			/* Feed keys to ConfD. */
+			for (size_t i = 0; i < keys.num; i++)
+				CONFD_SET_STR(&v[i], keys.key[i]);
+			confd_data_reply_next_key(tctx, v, keys.num,
+						  (long)nb_next);
+		} else {
+			char pointer_str[16];
+
+			/*
+			 * ConfD 6.6 user guide, chapter 6.11 (Operational data
+			 * lists without keys):
+			 * "To support this without having completely separate
+			 * APIs, we use a "pseudo" key in the ConfD APIs for
+			 * this type of list. This key is not part of the data
+			 * model, and completely hidden in the northbound agent
+			 * interfaces, but is used with e.g. the get_next() and
+			 * get_elem() callbacks as if it were a normal key. This
+			 * "pseudo" key is always a single signed 64-bit
+			 * integer, i.e. the confd_value_t type is C_INT64. The
+			 * values can be chosen arbitrarily by the application,
+			 * as long as a key value returned by get_next() can be
+			 * used to get the data for the corresponding list entry
+			 * with get_elem() or get_object() as usual. It could
+			 * e.g. be an index into an array that holds the data,
+			 * or even a memory address in integer form".
+			 *
+			 * Since we're using the CONFD_DAEMON_FLAG_STRINGSONLY
+			 * option, we must convert our pseudo-key (a void
+			 * pointer) to a string before sending it to confd.
+			 */
+			snprintf(pointer_str, sizeof(pointer_str), "%lu",
+				 (unsigned long)nb_next);
+			CONFD_SET_STR(&v[0], pointer_str);
+			confd_data_reply_next_key(tctx, v, 1, (long)nb_next);
+		}
 		break;
 	case LYS_LEAFLIST:
 		data = nb_node->cbs.get_elem(xpath, nb_next);
@@ -792,6 +835,7 @@ static int frr_confd_data_get_next_object(struct confd_trans_ctx *tctx,
 	const void *nb_next;
 #define CONFD_OBJECTS_PER_TIME 100
 	struct confd_next_object objects[CONFD_OBJECTS_PER_TIME + 1];
+	char pseudo_keys[CONFD_OBJECTS_PER_TIME][16];
 	int nobjects = 0;
 
 	frr_confd_get_xpath(kp, xpath, sizeof(xpath));
@@ -844,6 +888,26 @@ static int frr_confd_data_get_next_object(struct confd_trans_ctx *tctx,
 			XMALLOC(MTYPE_CONFD,
 				CONFD_MAX_CHILD_NODES * sizeof(confd_value_t));
 
+		/*
+		 * ConfD 6.6 user guide, chapter 6.11 (Operational data lists
+		 * without keys):
+		 * "In the response to the get_next_object() callback, the data
+		 * provider is expected to provide the key values along with the
+		 * other leafs in an array that is populated according to the
+		 * data model. This must be done also for this type of list,
+		 * even though the key isn't actually in the data model. The
+		 * "pseudo" key must always be the first element in the array".
+		 */
+		if (CHECK_FLAG(nb_node->flags, F_NB_NODE_KEYLESS_LIST)) {
+			confd_value_t *v;
+
+			snprintf(pseudo_keys[j], sizeof(pseudo_keys[j]), "%lu",
+				 (unsigned long)nb_next);
+
+			v = &object->v[nvalues++];
+			CONFD_SET_STR(v, pseudo_keys[j]);
+		}
+
 		/* Loop through list child nodes. */
 		LY_TREE_FOR (nb_node->snode->child, child) {
 			struct nb_node *nb_node_child = child->priv;

tests/lib/northbound/test_oper_data.c

@@ -153,39 +153,6 @@ frr_test_module_vrfs_vrf_routes_route_get_next(const void *parent_list_entry,
 	return node;
 }
 
-static int
-frr_test_module_vrfs_vrf_routes_route_get_keys(const void *list_entry,
-					       struct yang_list_keys *keys)
-{
-	const struct troute *route;
-
-	route = listgetdata((struct listnode *)list_entry);
-
-	keys->num = 1;
-	(void)prefix2str(&route->prefix, keys->key[0], sizeof(keys->key[0]));
-
-	return NB_OK;
-}
-
-static const void *frr_test_module_vrfs_vrf_routes_route_lookup_entry(
-	const void *parent_list_entry, const struct yang_list_keys *keys)
-{
-	const struct tvrf *vrf;
-	const struct troute *route;
-	struct listnode *node;
-	struct prefix prefix;
-
-	yang_str2ipv4p(keys->key[0], &prefix);
-
-	vrf = listgetdata((struct listnode *)parent_list_entry);
-	for (ALL_LIST_ELEMENTS_RO(vrf->routes, node, route)) {
-		if (prefix_same((struct prefix *)&route->prefix, &prefix))
-			return node;
-	}
-
-	return NULL;
-}
-
 /*
  * XPath: /frr-test-module:frr-test-module/vrfs/vrf/routes/route/prefix
  */
@@ -276,8 +243,6 @@ const struct frr_yang_module_info frr_test_module_info = {
 		{
 			.xpath = "/frr-test-module:frr-test-module/vrfs/vrf/routes/route",
 			.cbs.get_next = frr_test_module_vrfs_vrf_routes_route_get_next,
-			.cbs.get_keys = frr_test_module_vrfs_vrf_routes_route_get_keys,
-			.cbs.lookup_entry = frr_test_module_vrfs_vrf_routes_route_lookup_entry,
 		},
 		{
 			.xpath = "/frr-test-module:frr-test-module/vrfs/vrf/routes/route/prefix",

tools/checkpatch.pl

@@ -5886,27 +5886,6 @@ sub process {
 			     "unchecked sscanf return value\n" . "$here\n$stat_real\n");
 		}
 
-# check for simple sscanf that should be kstrto<foo>
-		if ($^V && $^V ge 5.10.0 &&
-		    defined $stat &&
-		    $line =~ /\bsscanf\b/) {
-			my $lc = $stat =~ tr@\n@@;
-			$lc = $lc + $linenr;
-			my $stat_real = raw_line($linenr, 0);
-		        for (my $count = $linenr + 1; $count <= $lc; $count++) {
-				$stat_real = $stat_real . "\n" . raw_line($count, 0);
-			}
-			if ($stat_real =~ /\bsscanf\b\s*\(\s*$FuncArg\s*,\s*("[^"]+")/) {
-				my $format = $6;
-				my $count = $format =~ tr@%@%@;
-				if ($count == 1 &&
-				    $format =~ /^"\%(?i:ll[udxi]|[udxi]ll|ll|[hl]h?[udxi]|[udxi][hl]h?|[hl]h?|[udxi])"$/) {
-					WARN("SSCANF_TO_KSTRTO",
-					     "Prefer kstrto<type> to single variable sscanf\n" . "$here\n$stat_real\n");
-				}
-			}
-		}
-
 # check for new externs in .h files.
 		if ($realfile =~ /\.h$/ &&
 		    $line =~ /^\+\s*(extern\s+)$Type\s*$Ident\s*\(/s) {
@@ -6177,12 +6156,6 @@ sub process {
 			     "consider using a completion\n" . $herecurr);
 		}
 
-# recommend kstrto* over simple_strto* and strict_strto*
-		if ($line =~ /\b((simple|strict)_(strto(l|ll|ul|ull)))\s*\(/) {
-			WARN("CONSIDER_KSTRTO",
-			     "$1 is obsolete, use k$3 instead\n" . $herecurr);
-		}
-
 # check for __initcall(), use device_initcall() explicitly or more appropriate function please
 		if ($line =~ /^.\s*__initcall\s*\(/) {
 			WARN("USE_DEVICE_INITCALL",

yang/frr-test-module.yang

@@ -34,8 +34,6 @@ module frr-test-module {
         }
         container routes {
           list route {
-            key "prefix";
-
             leaf prefix {
               type inet:ipv4-prefix;
             }