Skip to content

Commit

Permalink
update -DMERCURY_USE_XDR=ON code to pass unit tests
Browse files Browse the repository at this point in the history
Mercury configured with -DMERCURY_USE_XDR=ON fails to pass the unit tests.
The main issue is that mercury was not properly accounting for the data
padding that xdr uses.

When encoding, xdr pads all data added to its buffer to the next multiple
of BYTES_PER_XDR_UNIT (4 bytes, see /usr/include/rpc/xdr.h) and provides
a macro RNDUP(s) to round "s" up to the next multiple of BYTES_PER_XDR_UNIT.

The native mercury code (when not using xdr) does not pad data.  When
mercury is compiled to use xdr it does not account for the padding xdr
uses and gets out of sync with xdr.  Given an hg_proc_t "p" we need to
make sure that hg_proc_get_size_used(p) is always equal to
xdr_getpos(hg_proc_get_xdr_ptr(p)) after encoding a chunk.

To fix this we:
 - modify the xdr version of HG_PROC_TYPE() in mercury_proc.h to round
   up the size using RNDUP().
 - modify the xdr version of HG_PROC_BYTES() to mercury_proc.h to round
   up the byte chunk size using RNDUP()

Note that the mercury checksum in this case does not cover the padding
(since the calls to HG_PROC_CHECKSUM_UPDATE() are on the orig data buffer,
and that buffer is not padded.   This is not an issue, since the padding
is thrown away at decode time.

In addition, we change the xdr call in HG_PROC_BYTES() from xdr_bytes()
to xdr_opaque() which better matches the intent of HG_PROC_BYTES().
For reference: xdr_opaque(xdr, char *data, size) copies RNDUP(size)
bytes (the data plus padding) to the buffer.  xdr_bytes(xdr, char **data,
u_int *size, UINT_MAX) copies both a uint32_t length and RNDUP(size)
bytes (the data plus padding) the buffer.  HG_PROC_BYTES() does not
need xdr to encode the length in the buffer (matches the non-xdr case).

Remove code in mercury.c that forces payload_size to be the entire
buffer.  I do not think xdr requires the entire buffer to be payload
if mercury accounts for the BYTES_PER_XDR_UNIT padding.

Unit tests attempt to verify the expected payload length by manually
computing it and comparing it to what is received.  Update the tests
to handle BYTES_PER_XDR_UNIT padding when compiled with xdr enabled.
  • Loading branch information
chuckcranor authored and soumagne committed Dec 19, 2024
1 parent a0d3ac8 commit 93244fa
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 11 deletions.
8 changes: 8 additions & 0 deletions Testing/unit/hg/mercury_rpc_cb.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,16 @@ HG_TEST_RPC_CB(hg_test_rpc_open, handle)
struct hg_unit_info *info = (struct hg_unit_info *) HG_Class_get_data(
HG_Get_info(handle)->hg_class);
hg_size_t payload_size = HG_Get_input_payload_size(handle);
#ifdef HG_HAS_XDR
size_t expected_string_payload_size = /* note xdr rounding rules */
sizeof(uint64_t) + /* length */
RNDUP(strlen(HG_TEST_RPC_PATH) + 1) + /* string data, inc \0 at end */
RNDUP(sizeof(uint8_t)) + /* is_const */
RNDUP(sizeof(uint8_t)); /* is_owned */
#else
size_t expected_string_payload_size =
strlen(HG_TEST_RPC_PATH) + sizeof(uint64_t) + 3;
#endif

HG_TEST_CHECK_ERROR(
payload_size != sizeof(rpc_handle_t) + expected_string_payload_size,
Expand Down
8 changes: 8 additions & 0 deletions Testing/unit/hg/test_rpc.c
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,16 @@ hg_test_rpc_input(hg_handle_t handle, hg_addr_t addr, hg_id_t rpc_id,
rpc_open_in_t in_struct = {
.handle = rpc_open_handle, .path = HG_TEST_RPC_PATH};
hg_size_t payload_size;
#ifdef HG_HAS_XDR
size_t expected_string_payload_size = /* note xdr rounding rules */
sizeof(uint64_t) + /* length */
RNDUP(strlen(HG_TEST_RPC_PATH) + 1) + /* string data, inc \0 at end */
RNDUP(sizeof(uint8_t)) + /* is_const */
RNDUP(sizeof(uint8_t)); /* is_owned */
#else
size_t expected_string_payload_size =
strlen(HG_TEST_RPC_PATH) + sizeof(uint64_t) + 3;
#endif
unsigned int flag;
int rc;

Expand Down
5 changes: 0 additions & 5 deletions src/mercury.c
Original file line number Diff line number Diff line change
Expand Up @@ -781,13 +781,8 @@ hg_set_struct(struct hg_private_handle *hg_handle,
ret = hg_header_proc(HG_ENCODE, buf, buf_size, hg_header);
HG_CHECK_SUBSYS_HG_ERROR(rpc, error, ret, "Could not process header");

#ifdef HG_HAS_XDR
/* XDR requires entire buffer payload */
*payload_size = buf_size;
#else
/* Only send the actual size of the data, not the entire buffer */
*payload_size = hg_proc_get_size_used(proc) + header_offset;
#endif

return HG_SUCCESS;

Expand Down
12 changes: 6 additions & 6 deletions src/mercury_proc.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,14 @@ typedef enum { HG_CRC16, HG_CRC32, HG_CRC64, HG_NOHASH } hg_proc_hash_t;
#ifdef HG_HAS_XDR
# define HG_PROC_TYPE(proc, type, data, label, ret) \
do { \
HG_PROC_CHECK_SIZE(proc, sizeof(type), label, ret); \
HG_PROC_CHECK_SIZE(proc, RNDUP(sizeof(type)), label, ret); \
\
if (xdr_##type(hg_proc_get_xdr_ptr(proc), data) == 0) { \
ret = HG_PROTOCOL_ERROR; \
goto label; \
} \
\
HG_PROC_UPDATE(proc, sizeof(type)); \
HG_PROC_UPDATE(proc, RNDUP(sizeof(type))); \
HG_PROC_CHECKSUM_UPDATE(proc, data, sizeof(type)); \
} while (0)
#else
Expand Down Expand Up @@ -147,15 +147,15 @@ typedef enum { HG_CRC16, HG_CRC32, HG_CRC64, HG_NOHASH } hg_proc_hash_t;
#ifdef HG_HAS_XDR
# define HG_PROC_BYTES(proc, data, size, label, ret) \
do { \
HG_PROC_CHECK_SIZE(proc, size, label, ret); \
HG_PROC_CHECK_SIZE(proc, RNDUP(size), label, ret); \
\
if (xdr_bytes(hg_proc_get_xdr_ptr(proc), (char **) &data, \
(u_int *) &size, UINT_MAX) == 0) { \
if (xdr_opaque(hg_proc_get_xdr_ptr(proc), (char *) data, size) == \
0) { \
ret = HG_PROTOCOL_ERROR; \
goto label; \
} \
\
HG_PROC_UPDATE(proc, size); \
HG_PROC_UPDATE(proc, RNDUP(size)); \
HG_PROC_CHECKSUM_UPDATE(proc, data, size); \
} while (0)
#else
Expand Down

0 comments on commit 93244fa

Please sign in to comment.