On 5/7/25 3:41 PM, Martin Morgenstern via dev wrote:
> Extract two functions from the inner loop of jsonrpc_recv():
>
> - a function that receives message chunks from the RPC layer, and
> - a function that assembles a complete message if parsing is complete.
>
> This way, we can reuse some of the logic for a new set of functions in a
> later patch. Furthermore, this streamlines the error handling in
> jsonrpc_recv(), because we can handle the results of the two functions
> separately.
>
> Signed-off-by: Martin Morgenstern <martin.morgenst...@cloudandheat.com>
> ---
> lib/jsonrpc.c | 145 +++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 96 insertions(+), 49 deletions(-)
>
Hi, Martin. Thnkas for the patches! And sorry for the late reply.
See some comments below.
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index 90723a42b..a352e01fb 100644
> --- a/lib/jsonrpc.c
> +++ b/lib/jsonrpc.c
> @@ -309,6 +309,96 @@ jsonrpc_send(struct jsonrpc *rpc, struct jsonrpc_msg
> *msg)
> return rpc->status;
> }
>
> +/* Receives a message chunk from 'rpc' and feeds it into the JSON parser.
> + *
> + * If successful, returns 0.
> + *
> + * Otherwise, it returns:
> + *
> + * - EAGAIN: No data has been received.
> + *
> + * - EOF: The remote end closed the connection gracefully.
> + *
> + * - Otherwise an errno value that represents an error fatal to the
> + * connection. 'rpc' will not send or receive any more messages.
> + * */
> +static int
> +jsonrpc_recv_chunk(struct jsonrpc *rpc)
> +{
> + size_t n, used;
> +
> + /* Fill our input buffer if it's empty. */
> + if (byteq_is_empty(&rpc->input)) {
> + size_t chunk;
> + int retval;
> +
> + byteq_fast_forward(&rpc->input);
> + chunk = byteq_headroom(&rpc->input);
> + retval = stream_recv(rpc->stream, byteq_head(&rpc->input), chunk);
> + if (retval < 0) {
> + if (retval == -EAGAIN) {
> + return EAGAIN;
> + } else {
> + VLOG_WARN_RL(&rl, "%s: receive error: %s",
> + rpc->name, ovs_strerror(-retval));
> + jsonrpc_error(rpc, -retval);
> + return rpc->status;
> + }
> + } else if (retval == 0) {
> + jsonrpc_error(rpc, EOF);
> + return EOF;
> + }
> + byteq_advance_head(&rpc->input, retval);
> + }
> +
> + /* We have some input. Feed it into the JSON parser. */
> + if (!rpc->parser) {
> + rpc->parser = json_parser_create(0);
> + }
> + n = byteq_tailroom(&rpc->input);
> + used = json_parser_feed(rpc->parser,
> + (char *) byteq_tail(&rpc->input), n);
> + byteq_advance_tail(&rpc->input, used);
> +
> + return 0;
> +}
> +
> +/* Get the JSON-RPC message from 'rpc' if JSON parsing is complete.
> + *
> + * On success, stores the message in '*msgp' and returns 0. The caller
> + * takes ownership of '*msgp' and must eventually destroy it with
> + * jsonrpc_msg_destroy().
> + *
> + * Otherwise, it returns:
> + *
> + * - ENODATA: JSON parsing is not yet complete and we need to feed more
> + * data into the parser.
The ENODATA errno doesn't exist on FreeBSD, that's why you see CirrusCI
failure reported in patchwork.
This made me think, however, why we need several custom error codes for
this function and the other ones that call it. And I think we can get
rid of it.
There should not be a real reason to split the large jsonrpc_recv()
function. We can re-use it for the _until() variant with just a slight
modification of the loop. As seen in the second patch of this set, the
both variants are practically the same function with just for() replaced
with while(). So, what we can do is to replace the current loop with a
do {} while(); with an extra condition, e.g.:
i = 0;
do {
...
} while (deadline ? time_msec() < deadline : i++ < 50);
This should cover both cases and reduce code duplication as we'll not
have two copies of essentially the same function.
Additionally, this should allow us to get rid of ENODATA return value
as we'll not need to decompose the function.
I see that some function higher in the stack are also relying on the
ENODATA return value indirectly to try again, however, I think, higher
level functions should not be concerned about this. The CS/IDL code
is manipulating entire jsonrpc messages, so it should only keep going
when there was a full message received on jsonrpc level. And the
jsonrpc level should ensure that it will keep trying to receive the
message until there is data to receive and it's not the deadline yet.
The jsonrpc session may return zero when it's an echo message, but it
may be something to solve on the jsonrpc session level. I'll comment
on that in the later patches.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
