CDRIVER-4625 Add mcd-rpc library and tests #1296
Conversation
| #endif | ||
|
|
||
|
|
||
| typedef union _mcd_rpc_message mcd_rpc_message; |
There was a problem hiding this comment.
Is there a benefit to defining everything out-of-line, since this is not a public API?
Even without pulling everything inline, it is possible to forward-declare the union members and have a type-safe interface thereof, i.e.:
// in .h:
typedef union mcd_rpc_op_msg mcd_rpc_op_msg;
typedef union mcd_rpc_op_compressed mcd_rpc_op_compressed;
// Become an OP_MSG. Invalidates all prior pointers derived from 'msg'
mcd_rpc_op_msg* mcd_rpc_become_op_msg(mcd_rpc_message* msg);
// Become a compressed OP_MSG. Invalidates all prior pointers derived from 'msg'
mcd_rpc_op_msg* mcd_rpc_become_op_compressed(mcd_rpc_message* msg);
// etc.// in .c:
mcd_rpc_op_msg* msd_rpc_become_op_msg(mcd_rpc_message* msg) {
mcd_rpc_header_set_op_code(msg, MSG_OP_CODE_MSG);
return &msg->op_msg;
}
// etc.And then the per-type APIs can be updated to accept the typed pointers directly, alleviating the need to assert on the opcode in the message header, since the only way a caller can obtain a pointer is via an API that already set the opcode.
Certain APIs also expose simple get/set pairs that do no special validation. These could be simplified to just return pointers to mutable structures:
// in mcd-rpc.h:
struct mcd_rpc_message_header {
int32_t message_length;
int32_t request_id;
int32_t response_to;
};
mcd_rpc_message_header const* mcr_rpc_get_header(mcd_rpc_message const* m);
mcd_rpc_message_header* mcr_rpc_get_header_mut(mcd_rpc_message* m);// In mcd-rpc.c:
struct mcd_rpc_message_header_impl {
int32_t op_code; // Hidden and protected
mcd_rpc_message_header pub; // Public components
};
mcd_rpc_message_header* mcd_rpc_get_header_mut(mcd_rpc_message* m) {
return &m->header.pub;
};Similar for things like message sections.
There was a problem hiding this comment.
The out-of-line API as well as the deliberately minimal exposure of data structures is motivated by CDRIVER-4625. See ticket for details.
| void | ||
| mcd_rpc_message_destroy (mcd_rpc_message *rpc); |
There was a problem hiding this comment.
Would suggest naming to mcd_rpc_message_delete to match mcd_rpc_message_new, especially since this frees the pointee.
There was a problem hiding this comment.
The function uses _destroy for consistency with established libbson and libmongoc naming convention. "delete" refers to the opcode/command.
| _consume_utf8 (const char **target, | ||
| size_t *length, | ||
| const uint8_t **ptr, | ||
| size_t *remaining_bytes) |
There was a problem hiding this comment.
Do we care about full UTF-8 validation, or is that a non-concern?
There was a problem hiding this comment.
Not a concern. As with most other libbson/libmongoc functions operating on "UTF-8" strings, this function is agnostic to the encoding so long as it is a valid null-terminated string (no expectation of embedded null characters).
src/libmongoc/src/mongoc/mcd-rpc.c
Outdated
| mcd_rpc_message_set_length (mcd_rpc_message *rpc, int32_t value) | ||
| { | ||
| BSON_ASSERT_PARAM (rpc); | ||
| rpc->msg_header.message_length += value; |
There was a problem hiding this comment.
Confirm: Should this be a +=, or an =?
There was a problem hiding this comment.
Should be =; discussed in another comment: #1296 (comment)
There was a problem hiding this comment.
Aside: This is a good case for fuzz testing. Maybe someday.
Aside 2: Is there a comprehensive test corpus for the wire protocol anywhere?
There was a problem hiding this comment.
This is a good case for fuzz testing.
It may be a good supplement to ensure robustness, but the precise tests for correct error handling behavior (in particular the resulting value of *data_end) is not fuzz-testable.
Is there a comprehensive test corpus for the wire protocol anywhere?
I am unaware. The server appears to fuzz-test their OP_MSG parser, but I could not find an comprehensive test suite for RPC message parsing in general.
|
Latest changes verified by this patch. Upcoming PR diffs have been rebased accordingly. |
Description
This PR is the first of four that will simultaneously address CDRIVER-4617, CDRIVER-4625, and CDRIVER-4630:
See the aforementioned tickets for further details and upcoming PR diffs for further context.
See also the following links for RPC message specifications:
Note: upcoming PR diffs will continue to be edited and rebased until their corresponding PRs are posted.