protocol: Make T_DATUM_PROTOCOL_HEADER serialisation explicit#185
protocol: Make T_DATUM_PROTOCOL_HEADER serialisation explicit#185luke-jr wants to merge 1 commit intoOCEAN-xyz:masterfrom
Conversation
Should improve portability In particular, this fixes big endian systems, as well as: warning: bit-field ___ of type ___ has a different storage size than the preceding bit-field and will not be packed under the Microsoft ABI [-Wms-bitfield-padding]
luke-jr
added
the
portability
There was a problem hiding this comment.
Pull request overview
This PR makes T_DATUM_PROTOCOL_HEADER wire serialization/deserialization explicit (fixed 4-byte little-endian value), improving portability across big-endian systems and avoiding MS ABI bit-field packing warnings.
Changes:
- Replace packed/bitfield-based header wire layout with explicit pack/unpack helpers (
datum_header_pk/datum_header_upk). - Introduce
T_DATUM_PROTOCOL_HEADER_WIRE_BYTESand update send/recv paths to use the explicit 4-byte wire header. - Remove the old in-place XOR-on-struct helper that depended on casting the header to
uint32_t.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/datum_protocol.h |
Updates header struct fields and defines the fixed wire header size. |
src/datum_protocol.c |
Adds explicit header pack/unpack functions and switches protocol I/O to use them. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((n + (sizeof(wire_h) - protocol_state)) != sizeof(wire_h)) { | ||
| if ((n+protocol_state) > 4) { | ||
| DLOG_DEBUG("recv() issue. too many header bytes. protocol_state=%d, n=%d, errno=%d (%s)", protocol_state, n, errno, strerror(errno)); | ||
| break_again = true; break; |
There was a problem hiding this comment.
This check uses a hard-coded header size (4). Since the header size is now abstracted via wire_h/T_DATUM_PROTOCOL_HEADER_WIRE_BYTES, use sizeof(wire_h) (or the macro) here too to avoid future inconsistencies if the wire header size changes.
| } | ||
| if (n != sizeof(T_DATUM_PROTOCOL_HEADER)) { | ||
| if (n != sizeof(wire_h)) { | ||
| if (n > 4) { |
There was a problem hiding this comment.
This branch still hard-codes 4 as the maximum header byte count. For consistency with the rest of this state machine, replace it with sizeof(wire_h) (or T_DATUM_PROTOCOL_HEADER_WIRE_BYTES).
| if (n > 4) { | |
| if (n > sizeof(wire_h)) { |
| } | ||
|
|
||
| protocol_state = sizeof(T_DATUM_PROTOCOL_HEADER) - n - (sizeof(T_DATUM_PROTOCOL_HEADER) - protocol_state); // should give us a state equal to the number of. consoluted to show the process. (compiler optimizes) | ||
| protocol_state = sizeof(wire_h) - n - (sizeof(wire_h) - protocol_state); // should give us a state equal to the number of. consoluted to show the process. (compiler optimizes) |
There was a problem hiding this comment.
Typo in comment: “consoluted” should be “convoluted”.
| protocol_state = sizeof(wire_h) - n - (sizeof(wire_h) - protocol_state); // should give us a state equal to the number of. consoluted to show the process. (compiler optimizes) | |
| protocol_state = sizeof(wire_h) - n - (sizeof(wire_h) - protocol_state); // should give us a state equal to the number of. convoluted to show the process. (compiler optimizes) |
Should improve portability
In particular, this fixes big endian systems, as well as:
warning: bit-field ___ of type ___ has a different storage size than the preceding bit-field and will not be packed under the Microsoft ABI [-Wms-bitfield-padding]