Merge pull request #17772 from LabNConsulting/chopps/fix-oper-walk

improve error handling of operational state walk callback

lib/mgmt_be_client.c

@@ -908,8 +908,15 @@ static enum nb_error be_client_send_tree_data_batch(const struct lyd_node *tree,
 		more = true;
 		ret = NB_OK;
 	}
-	if (ret != NB_OK)
+	if (ret != NB_OK) {
+		if (be_client_send_error(client, args->txn_id, args->req_id, false, -EINVAL,
+					 "BE client %s txn-id %Lu error fetching oper state %d",
+					 client->name, args->txn_id, ret))
+			ret = NB_ERR;
+		else
+			ret = NB_OK;
 		goto done;
+	}
 
 	tree_msg = mgmt_msg_native_alloc_msg(struct mgmt_msg_tree_data, 0,
 					     MTYPE_MSG_NATIVE_TREE_DATA);
@@ -924,20 +931,15 @@ static enum nb_error be_client_send_tree_data_batch(const struct lyd_node *tree,
 				     (LYD_PRINT_SHRINK | LYD_PRINT_WD_EXPLICIT |
 				      LYD_PRINT_WITHSIBLINGS));
 	if (err) {
-		ret = NB_ERR;
-		goto done;
+		mgmt_msg_native_free_msg(tree_msg);
+		/* We will be called again to send the error */
+		return NB_ERR;
 	}
 	(void)be_client_send_native_msg(client, tree_msg,
 					mgmt_msg_native_get_msg_len(tree_msg),
 					false);
-done:
 	mgmt_msg_native_free_msg(tree_msg);
-	if (ret)
-		be_client_send_error(client, args->txn_id, args->req_id, false,
-				     -EINVAL,
-				     "BE client %s txn-id %" PRIu64
-				     " error fetching oper state %d",
-				     client->name, args->txn_id, ret);
+done:
 	if (ret != NB_OK || !more)
 		XFREE(MTYPE_MGMTD_BE_GT_CB_ARGS, args);
 	return ret;

lib/northbound.h

@@ -797,16 +797,19 @@ typedef int (*nb_oper_data_cb)(const struct lysc_node *snode,
  *        error.
  *
  * If nb_op_iterate_yielding() was passed with @should_batch set then this
- * callback will be invoked during each portion (batch) of the walk.
+ * callback will be invoked during each portion (batch) of the walk with @ret
+ * set to NB_YIELD.
  *
  * The @tree is read-only and should not be modified or freed.
  *
- * If this function returns anything but NB_OK then the walk will be terminated.
- * and this function will not be called again regardless of if @ret was
- * `NB_YIELD` or not.
+ * When @ret is NB_YIELD and this function returns anything but NB_OK then the
+ * walk will be terminated, and this function *will* be called again with @ret
+ * set the non-NB_OK return value it just returned. This allows the callback
+ * have a single bit of code to send an error message and do any cleanup for any
+ * type of failure, whether that failure was from itself or from the infra code.
  *
- * Return: NB_OK to continue or complete the walk normally, otherwise an error
- * to immediately terminate the walk.
+ * Return: NB_OK or an error during handling of @ret == NB_YIELD otherwise the
+ * value is ignored.
  */
 /* Callback function used by nb_oper_data_iter_yielding(). */
 typedef enum nb_error (*nb_oper_data_finish_cb)(const struct lyd_node *tree,

lib/northbound_oper.c

@@ -1561,17 +1561,13 @@ static void nb_op_walk_continue(struct event *thread)
 
 	ret = __walk(ys, true);
 	if (ret == NB_YIELD) {
-		if (nb_op_yield(ys) != NB_OK) {
-			if (ys->should_batch)
-				goto stopped;
-			else
-				goto finish;
-		}
-		return;
+		ret = nb_op_yield(ys);
+		if (ret == NB_OK)
+			return;
 	}
 finish:
+	assert(ret != NB_YIELD);
 	(*ys->finish)(ys_root_node(ys), ys->finish_arg, ret);
-stopped:
 	nb_op_free_yield_state(ys, false);
 }
 
@@ -1630,6 +1626,13 @@ static void nb_op_trim_yield_state(struct nb_op_yield_state *ys)
 	       (int)darr_lasti(ys->node_infos));
 }
 
+/**
+ * nb_op_yield() - Yield during the walk.
+ * @ys: the yield state tracking the walk.
+ *
+ * Return: Any error from the `ys->finish` callback which should terminate the
+ * walk. Otherwise if `ys->should_batch` == false always returns NB_OK.
+ */
 static enum nb_error nb_op_yield(struct nb_op_yield_state *ys)
 {
 	enum nb_error ret;
@@ -1842,17 +1845,13 @@ void *nb_oper_walk(const char *xpath, struct yang_translator *translator,
 
 	ret = nb_op_walk_start(ys);
 	if (ret == NB_YIELD) {
-		if (nb_op_yield(ys) != NB_OK) {
-			if (ys->should_batch)
-				goto stopped;
-			else
-				goto finish;
-		}
-		return ys;
+		ret = nb_op_yield(ys);
+		if (ret == NB_OK)
+			return ys;
 	}
-finish:
+
+	assert(ret != NB_YIELD);
 	(void)(*ys->finish)(ys_root_node(ys), ys->finish_arg, ret);
-stopped:
 	nb_op_free_yield_state(ys, false);
 	return NULL;
 }

mgmtd/mgmt_fe_adapter.c

@@ -1237,8 +1237,8 @@ static void fe_adapter_handle_session_req(struct mgmt_fe_client_adapter *adapter
 	struct mgmt_fe_session_ctx *session;
 	uint64_t client_id;
 
-	__dbg("Got session-req creating: %u for refer-id %" PRIu64 " from '%s'",
-	      msg->refer_id == 0, msg->refer_id, adapter->name);
+	__dbg("Got session-req is create %u req-id %Lu for refer-id %Lu from '%s'",
+	      msg->refer_id == 0, msg->req_id, msg->refer_id, adapter->name);
 
 	if (msg->refer_id) {
 		uint64_t session_id = msg->refer_id;

tools/checkpatch.pl

@@ -6285,13 +6285,14 @@ sub process {
 		while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
 			my $string = substr($rawline, $-[1], $+[1] - $-[1]);
 			$string =~ s/%%/__/g;
-			# check for %L
-			if ($show_L && $string =~ /%[\*\d\.\$]*L([diouxX])/) {
-				WARN("PRINTF_L",
-				     "\%L$1 is non-standard C, use %ll$1\n" . $herecurr);
-				$show_L = 0;
-			}
-			# check for %Z
+                        # check for %L
+                        # OK in FRR
+                        # if ($show_L && $string =~ /%[\*\d\.\$]*L([diouxX])/) {
+                        #       WARN("PRINTF_L",
+                        #            "\%L$1 is non-standard C, use %ll$1\n" . $herecurr);
+                        #       $show_L = 0;
+                        # }
+                        # check for %Z
 			if ($show_Z && $string =~ /%[\*\d\.\$]*Z([diouxX])/) {
 				WARN("PRINTF_Z",
 				     "%Z$1 is non-standard C, use %z$1\n" . $herecurr);