Merge pull request #18683 from LabNConsulting/chopps/notif-improve

Improve notification selectors (sort, eliminate dups)

lib/darr.c

@@ -158,3 +158,68 @@ void *__darr_insert_n(void *a, uint at, uint count, size_t esize, bool zero,
 	return a;
 #undef _a_at
 }
+
+int _darr_search_floor(const void *a, size_t esize, const void *key, bool *equal,
+		       darr_search_cmpf cmpf)
+{
+	struct darr_metadata *dm;
+
+	if (equal)
+		*equal = false;
+
+	if (!a)
+		return -1;
+
+	dm = (struct darr_metadata *)a - 1;
+
+	int len = dm->len;
+	int low = 0, high = len - 1;
+	int floor = -1;
+
+#define _a_at(i) ((void *)((char *)a + ((i)*esize)))
+	while (low <= high) {
+		int mid = low + (high - low) / 2;
+		int cmp = cmpf(_a_at(mid), key);
+
+		if (!cmp) {
+			if (equal)
+				*equal = true;
+			return mid;
+		} else if (cmp < 0) {
+			floor = mid;
+			low = mid + 1;
+		} else {
+			high = mid - 1;
+		}
+	}
+
+	return floor;
+#undef _a_at
+}
+
+int _darr_search(const void *a, size_t esize, const void *key, darr_search_cmpf cmpf)
+{
+	bool equal;
+	int i;
+
+	i = _darr_search_floor(a, esize, key, &equal, cmpf);
+	if (!equal)
+		return -1;
+	return i;
+}
+
+uint _darr_search_ceil(const void *a, size_t esize, const void *key, bool *equal,
+		       darr_search_cmpf cmpf)
+{
+	uint i;
+
+	i = _darr_search_floor(a, esize, key, equal, cmpf);
+	if (*equal)
+		return i;
+	return i + 1;
+}
+
+int darr_strings_cmp(const char **a, const char *key)
+{
+	return strcmp(*a, key);
+}

lib/darr.h

@@ -64,6 +64,9 @@
  *  - darr_strlen
  *  - darr_strlen_fixup
  *  - darr_strnul
+ *  - darr_str_search
+ *  - darr_str_search_ceil
+ *  - darr_str_search_floor
  *  - darr_sprintf, darr_vsprintf
  */
 /*
@@ -342,8 +345,10 @@ void *__darr_resize(void *a, uint count, size_t esize, struct memtype *mt);
 
 #define _darr_insert_n(A, I, N, Z, MT)                                                             \
 	({                                                                                         \
-		(A) = __darr_insert_n(A, I, N, _darr_esize(A), Z, MT);         \
-		&(A)[I];                                                       \
+		uint _ins_i = (I);                                                                 \
+		uint _ins_n = (N);                                                                 \
+		(A) = __darr_insert_n(A, _ins_i, _ins_n, _darr_esize(A), Z, MT);                   \
+		&(A)[_ins_i];                                                                      \
 	})
 /**
  * Insert N uninitialized elements in the array at index `I`.
@@ -364,9 +369,9 @@ void *__darr_resize(void *a, uint count, size_t esize, struct memtype *mt);
  *      A pointer to the first inserted element in the array.
  */
 #define darr_insert_n(A, I, N)	       _darr_insert_n(A, I, N, false, MTYPE_DARR)
-#define darr_insert_n_mt(A, I, N)  _darr_insert_n(A, I, N, false, MT)
+#define darr_insert_n_mt(A, I, N, MT)  _darr_insert_n(A, I, N, false, MT)
 #define darr_insert_nz(A, I, N)	       _darr_insert_n(A, I, N, true, MTYPE_DARR)
-#define darr_insert_nz_mt(A, I, N) _darr_insert_n(A, I, N, true, MT)
+#define darr_insert_nz_mt(A, I, N, MT) _darr_insert_n(A, I, N, true, MT)
 
 /**
  * Insert an uninitialized element in the array at index `I`.
@@ -387,9 +392,9 @@ void *__darr_resize(void *a, uint count, size_t esize, struct memtype *mt);
  *      A pointer to the element in the array.
  */
 #define darr_insert(A, I)	  _darr_insert_n(A, I, 1, false, MTYPE_DARR)
-#define darr_insert_mt(A, I)  _darr_insert_n(A, I, 1, false, MT)
+#define darr_insert_mt(A, I, MT)  _darr_insert_n(A, I, 1, false, MT)
 #define darr_insertz(A, I)	  _darr_insert_n(A, I, 1, true, MTYPE_DARR)
-#define darr_insertz_mt(A, I) _darr_insert_n(A, I, 1, true, MT)
+#define darr_insertz_mt(A, I, MT) _darr_insert_n(A, I, 1, true, MT)
 
 /**
  * Remove `N` elements from the array starting at index `I`.
@@ -786,6 +791,63 @@ void *__darr_resize(void *a, uint count, size_t esize, struct memtype *mt);
 		d;                                                             \
 	})
 
+/*
+ * darr_search_{floor,ceil}() functions - search for key in sorted arrays
+ */
+typedef int (*darr_search_cmpf)(const void *ep, const void *key);
+extern int darr_strings_cmp(const char **a, const char *key);
+extern int _darr_search(const void *a, size_t esize, const void *key, darr_search_cmpf cmpf);
+extern uint _darr_search_ceil(const void *a, size_t esize, const void *key, bool *equal,
+			      darr_search_cmpf cmpf);
+extern int _darr_search_floor(const void *a, size_t esize, const void *key, bool *equal,
+			      darr_search_cmpf cmpf);
+
+/**
+ * darr_str_search() - Find exact key in array of strings.
+ *
+ * Args:
+ *	A: array of string pointers
+ *	K: key string
+ *
+ * Return:
+ *	The index of the string which matches the key or -1 for no match.
+ */
+#define darr_str_search(A, K)                                                                      \
+	_darr_search((A), _darr_esize(A), (K), (darr_search_cmpf)darr_strings_cmp)
+
+/**
+ * darr_str_search_ceil() - Find least elm greater than or equal to the key
+ *
+ * Args:
+ *	A: array of string pointers
+ *	K: key string
+ *	E: Ptr to bool, set to true if element matching key is found
+ *
+ * Return:
+ *	The index of the least element that is greater than or equal to the @K
+ *	string. @E is set to true if equal otherwise false. The return value can
+ *	be passed directly to darr_insert().
+ */
+#define darr_str_search_ceil(A, K, E)                                                              \
+	_darr_search_ceil((A), _darr_esize(A), (K), (E), (darr_search_cmpf)darr_strings_cmp)
+
+/**
+ * darr_str_search_floor() - Find greatest elm less than or equal to the key
+ *
+ * Args:
+ *	A: array of string pointers
+ *	K: key string
+ *	E: Ptr to bool, set to true if element matching key is found
+ *
+ * Return:
+ *	The index of the greatest element that is less than or equal to the @K
+ *	string. @E is set to true if equal otherwise false. If used with
+ *	darr_insert() then the index should be passed +1 because darr_insert()
+ *	inserts *before* the given index.
+ */
+#define darr_str_search_floor(A, K, E)                                                             \
+	_darr_search_floor((A), _darr_esize(A), (K), (E), (darr_search_cmpf)darr_strings_cmp)
+
 /**
  * Iterate over array `A` using a pointer to each element in `P`.
  *

lib/mgmt_be_client.c

@@ -313,7 +313,7 @@ static int be_client_send_error(struct mgmt_be_client *client, uint64_t txn_id,
 }
 
 static int __send_notification(struct mgmt_be_client *client, const char *xpath,
-			       const struct lyd_node *tree, uint8_t op)
+			       const struct lyd_node *tree, uint8_t op, uint64_t refer_id)
 {
 	struct mgmt_msg_notify_data *msg = NULL;
 	// LYD_FORMAT format = LYD_LYB;
@@ -335,6 +335,7 @@ static int __send_notification(struct mgmt_be_client *client, const char *xpath,
 	msg = mgmt_msg_native_alloc_msg(struct mgmt_msg_notify_data, 0, MTYPE_MSG_NATIVE_NOTIFY);
 	msg->code = MGMT_MSG_CODE_NOTIFY;
 	msg->result_type = format;
+	msg->refer_id = refer_id;
 	msg->op = op;
 
 	mgmt_msg_native_xpath_encode(msg, xpath);
@@ -368,7 +369,7 @@ int mgmt_be_send_ds_delete_notification(const char *path)
 				path);
 		return 1;
 	}
-	return __send_notification(__be_client, path, NULL, NOTIFY_OP_DS_DELETE);
+	return __send_notification(__be_client, path, NULL, NOTIFY_OP_DS_DELETE, 0);
 }
 
 /**
@@ -381,7 +382,7 @@ int mgmt_be_send_ds_patch_notification(const char *path, const struct lyd_node *
 				path);
 		return 1;
 	}
-	return __send_notification(__be_client, path, patch, NOTIFY_OP_DS_PATCH);
+	return __send_notification(__be_client, path, patch, NOTIFY_OP_DS_PATCH, 0);
 }
 
 /**
@@ -394,7 +395,7 @@ int mgmt_be_send_ds_replace_notification(const char *path, const struct lyd_node
 				path);
 		return 1;
 	}
-	return __send_notification(__be_client, path, tree, NOTIFY_OP_DS_REPLACE);
+	return __send_notification(__be_client, path, tree, NOTIFY_OP_DS_REPLACE, 0);
 }
 
 /**
@@ -404,7 +405,7 @@ int mgmt_be_send_ds_replace_notification(const char *path, const struct lyd_node
  */
 static int mgmt_be_send_notification(void *__client, const char *path, const struct lyd_node *tree)
 {
-	__send_notification(__client, path, tree, NOTIFY_OP_NOTIFICATION);
+	__send_notification(__client, path, tree, NOTIFY_OP_NOTIFICATION, 0);
 	return 0;
 }
 

lib/northbound_notif.c

@@ -692,15 +692,28 @@ static void nb_notif_set_walk_timer(void)
 
 void nb_notif_set_filters(const char **selectors, bool replace)
 {
+	// struct nb_node **np, **nb_nodes;
 	const char **csp;
+	bool exists;
+	int before;
 
-	if (replace) {
+	if (replace)
 		darr_free_free(nb_notif_filters);
-		nb_notif_filters = selectors;
-		return;
-	}
-	darr_foreach_p (selectors, csp)
+
+	/* Add in sorted, eliminating duplicates */
+	darr_foreach_p (selectors, csp) {
+		if (!darr_len(nb_notif_filters)) {
 			*darr_append(nb_notif_filters) = *csp;
+			continue;
+		}
+		exists = false;
+		before = darr_str_search_ceil(nb_notif_filters, *csp, &exists);
+		if (exists)
+			darr_free(*csp);
+		else
+			*darr_insert(nb_notif_filters, before) = *csp;
+	}
+
 	darr_free(selectors);
 }
 

mgmtd/mgmt_fe_adapter.c

@@ -119,8 +119,8 @@ static uint64_t mgmt_fe_ns_string_remove_session(struct ns_string_head *head,
 		if (!node)
 			continue;
 		list_delete_node(ns->sessions, node);
-		clients |= mgmt_be_interested_clients(ns->s, MGMT_BE_XPATH_SUBSCR_TYPE_OPER);
 		if (list_isempty(ns->sessions)) {
+			clients |= mgmt_be_interested_clients(ns->s, MGMT_BE_XPATH_SUBSCR_TYPE_OPER);
 			ns_string_del(head, ns);
 			mgmt_fe_free_ns_string(ns);
 		}
@@ -1642,6 +1642,7 @@ static void fe_adapter_handle_notify_select(struct mgmt_fe_session_ctx *session,
 {
 	struct mgmt_msg_notify_select *msg = __msg;
 	uint64_t req_id = msg->req_id;
+	struct nb_node **nb_nodes;
 	const char **selectors = NULL;
 	const char **new;
 	const char **sp;
@@ -1656,6 +1657,19 @@ static void fe_adapter_handle_notify_select(struct mgmt_fe_session_ctx *session,
 			return;
 		}
 	}
+
+	/* Validate all selectors, they need to resolve to actual northbound_nodes */
+	darr_foreach_p (selectors, sp) {
+		nb_nodes = nb_nodes_find(*sp);
+		if (!nb_nodes) {
+			fe_adapter_send_error(session, req_id, false, -EINVAL,
+					      "Selector doesn't resolve to a node: %s", *sp);
+			darr_free_free(selectors);
+			return;
+		}
+		darr_free(nb_nodes);
+	}
+
 	if (DEBUG_MODE_CHECK(&mgmt_debug_fe, DEBUG_MODE_ALL)) {
 		selstr = frrstr_join(selectors, darr_len(selectors), ", ");
 		if (!selstr)
@@ -1932,6 +1946,11 @@ void mgmt_fe_adapter_send_notify(struct mgmt_msg_notify_data *msg, size_t msglen
 		}
 	}
 
+	/*
+	 * XXX if `is_root` should only send to each session one time, the code
+	 * below will send multiple times if a session has multiple selectors.
+	 */
+
 	frr_each (ns_string, &mgmt_fe_ns_strings, ns) {
 		if (!is_root) {
 			len = strlen(ns->s);

tests/lib/test_darr.c

@@ -50,6 +50,9 @@
  * [x] - darr_strlen
  * [x] - darr_strlen_fixup
  * [x] - darr_strnul
+ * [x] - darr_str_search
+ * [x] - darr_str_search_ceil
+ * [x] - darr_str_search_floor
  * [ ] - darr_vsprintf
  */
 
@@ -329,6 +332,8 @@ static void test_string(void)
 	const char **strings = NULL;
 	uint sum = 0;
 	uint i;
+	bool b;
+	int idx;
 
 	i = darr_strlen(da1);
 	assert(i == 0);
@@ -462,6 +467,126 @@ static void test_string(void)
 
 	darr_free_free(strings);
 	assert(strings == NULL);
+
+	add = darr_strdup("5");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("2");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("9");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("3");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	i = darr_str_search_ceil(strings, "0", &b);
+	assert(!b);
+	assert(i == 0);
+	i = darr_str_search_ceil(strings, "1", &b);
+	assert(!b);
+	assert(i == 0);
+	i = darr_str_search_ceil(strings, "2", &b);
+	assert(b);
+	assert(i == 0);
+	i = darr_str_search_ceil(strings, "3", &b);
+	assert(b);
+	assert(i == 1);
+	i = darr_str_search_ceil(strings, "4", &b);
+	assert(!b);
+	assert(i == 2);
+	i = darr_str_search_ceil(strings, "5", &b);
+	assert(b);
+	assert(i == 2);
+	i = darr_str_search_ceil(strings, "6", &b);
+	assert(!b);
+	assert(i == 3);
+	i = darr_str_search_ceil(strings, "7", &b);
+	assert(!b);
+	assert(i == 3);
+	i = darr_str_search_ceil(strings, "8", &b);
+	assert(!b);
+	assert(i == 3);
+	i = darr_str_search_ceil(strings, "9", &b);
+	assert(b);
+	assert(i == 3);
+	i = darr_str_search_ceil(strings, "X", &b);
+	assert(!b);
+	assert(i == 4);
+
+	assert(!strcmp(strings[0], "2"));
+	assert(!strcmp(strings[1], "3"));
+	assert(!strcmp(strings[2], "5"));
+	assert(!strcmp(strings[3], "9"));
+
+	darr_free_free(strings);
+	assert(strings == NULL);
+
+	/* -------------------- */
+	/* Test sorted prefixes */
+	/* -------------------- */
+
+	add = darr_strdup("/foo");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("/bar");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("/abc");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("/xyz/abc");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	add = darr_strdup("/foo/bar");
+	i = darr_str_search_ceil(strings, add, &b);
+	assert(!b);
+	*darr_insert(strings, i) = add;
+
+	i = darr_str_search_ceil(strings, "/abc", &b);
+	assert(i == 0 && b);
+	i = darr_str_search_ceil(strings, "/bar", &b);
+	assert(i == 1 && b);
+	i = darr_str_search_ceil(strings, "/foo", &b);
+	assert(i == 2 && b);
+	i = darr_str_search_ceil(strings, "/foo/bar", &b);
+	assert(i == 3 && b);
+	i = darr_str_search_ceil(strings, "/xyz/abc", &b);
+	assert(i == 4 && b);
+
+	idx = darr_str_search(strings, "/abc");
+	assert(idx == 0);
+	idx = darr_str_search(strings, "/abc/123");
+	assert(idx == -1);
+	idx = darr_str_search(strings, "/xyz");
+	assert(idx == -1);
+	idx = darr_str_search(strings, "/xyz/abc");
+	assert(idx == 4);
+
+	assert(!strcmp(strings[0], "/abc"));
+	assert(!strcmp(strings[1], "/bar"));
+	assert(!strcmp(strings[2], "/foo"));
+	assert(!strcmp(strings[3], "/foo/bar"));
+	assert(!strcmp(strings[4], "/xyz/abc"));
+
+	darr_free_free(strings);
+	assert(strings == NULL);
 }
 
 int main(int argc, char **argv)