Copilot commented on code in PR #3507:
URL: https://github.com/apache/brpc/pull/3507#discussion_r3958348814
##########
src/brpc/ubshm/ub_endpoint.h:
##########
@@ -43,6 +43,23 @@ DECLARE_int32(ub_poller_num);
DECLARE_bool(ub_edisp_unsched);
DECLARE_bool(ub_disable_bthread);
+enum UbrDataFormat {
+ UBR_DATA_FORMAT_NONE = 0,
+ UBR_DATA_FORMAT_LEGACY_64 = 1,
+};
+
+struct HelloFormatExtension {
+ // The V3 format extension is a fixed-size frame. A different wire size
+ // requires negotiation through a new hello version.
+ static const uint16_t WIRE_SIZE = 4;
Review Comment:
`WIRE_SIZE` is used as a byte-count for buffers/read/write sizes, but it’s
declared as `uint16_t` and as `static const` rather than a compile-time
constant with an appropriate size type. Consider making it `static constexpr
size_t WIRE_SIZE = 4;` (or `uint32_t`/`uint16_t` only if you intentionally want
to constrain all size usages), which avoids implicit narrowing/widening in I/O
calls and clarifies intent.
##########
src/brpc/ubshm/ub_endpoint.cpp:
##########
@@ -406,22 +428,56 @@ void* UBShmEndpoint::ProcessHandshakeAtClient(void* arg) {
return nullptr;
}
- if (remote_msg.msg_len > HELLO_MSG_LEN_MIN) {
- // TODO: Read Hello Message customized data
- // Just for future use, should not happen now
- }
-
+ UbrDataFormat selected_format = UBR_DATA_FORMAT_NONE;
if (!HelloNegotiationValid(remote_msg)) {
LOG(WARNING) << "Fail to negotiate with server, fallback to tcp:"
<< s->description();
ub_transport->_ub_state = UBShmTransport::UB_OFF;
} else {
- ep->_state = C_MAP_REMOTE_SHM;
- if (ep->_ub_ring->UbrMapRemoteShm(&local_trx_shm, shm_name) < 0) {
- LOG(WARNING) << "Fail to map the remote shm, fallback to tcp:" <<
s->description();
+ HelloFormatExtension local_extension = {
+ HelloFormatExtension::WIRE_SIZE, UBR_DATA_FORMAT_LEGACY_64};
+ local_extension.Serialize(data);
+ ep->_state = C_FORMAT_SEND;
+ if (ep->WriteToFd(data, HelloFormatExtension::WIRE_SIZE) < 0) {
Review Comment:
The new V3 format negotiation path (send/recv extension +
validation/fallback) is central behavioral logic but currently only has unit
tests for extension (de)serialization. Please add handshake-level tests that
exercise: (1) V3↔V3 success selecting `LEGACY_64` and setting
`_negotiated_data_format`, (2) V3↔V3 negotiation failure on `NONE`, wrong
`extension_len`, and unknown format id causing TCP fallback and leaving
`_negotiated_data_format` as `NONE`, and (3) V3↔V2 no extension bytes exchanged
(stream alignment) with TCP fallback.
##########
src/brpc/ubshm/ub_endpoint.cpp:
##########
@@ -397,7 +419,7 @@ void* UBShmEndpoint::ProcessHandshakeAtClient(void* arg) {
}
HelloMessage remote_msg;
remote_msg.Deserialize(data);
- if (remote_msg.msg_len < HELLO_MSG_LEN_MIN) {
+ if (remote_msg.msg_len != HELLO_MSG_LEN_MIN) {
LOG(WARNING) << "Fail to parse Hello Message length from server:"
<< s->description();
Review Comment:
This log message is now emitted when `msg_len` is successfully parsed but is
*invalid* (not exactly 64). To reduce confusion during debugging, update the
wording to reflect an invalid/unsupported hello length (e.g., 'Invalid Hello
Message length' and include the parsed value).
##########
src/brpc/ubshm/ub_endpoint.cpp:
##########
@@ -406,22 +428,56 @@ void* UBShmEndpoint::ProcessHandshakeAtClient(void* arg) {
return nullptr;
}
- if (remote_msg.msg_len > HELLO_MSG_LEN_MIN) {
- // TODO: Read Hello Message customized data
- // Just for future use, should not happen now
- }
-
+ UbrDataFormat selected_format = UBR_DATA_FORMAT_NONE;
if (!HelloNegotiationValid(remote_msg)) {
LOG(WARNING) << "Fail to negotiate with server, fallback to tcp:"
<< s->description();
ub_transport->_ub_state = UBShmTransport::UB_OFF;
} else {
- ep->_state = C_MAP_REMOTE_SHM;
- if (ep->_ub_ring->UbrMapRemoteShm(&local_trx_shm, shm_name) < 0) {
- LOG(WARNING) << "Fail to map the remote shm, fallback to tcp:" <<
s->description();
+ HelloFormatExtension local_extension = {
+ HelloFormatExtension::WIRE_SIZE, UBR_DATA_FORMAT_LEGACY_64};
+ local_extension.Serialize(data);
+ ep->_state = C_FORMAT_SEND;
+ if (ep->WriteToFd(data, HelloFormatExtension::WIRE_SIZE) < 0) {
+ const int saved_errno = errno;
+ PLOG(WARNING) << "Fail to send format extension to server:"
+ << s->description();
+ s->SetFailed(saved_errno,
+ "Fail to complete ubring handshake from %s: %s",
+ s->description().c_str(), berror(saved_errno));
+ ep->_state = FAILED;
+ return nullptr;
+ }
+
+ ep->_state = C_FORMAT_WAIT;
+ if (ep->ReadFromFd(data, HelloFormatExtension::WIRE_SIZE) < 0) {
+ const int saved_errno = errno;
+ PLOG(WARNING) << "Fail to read format extension from server:"
+ << s->description();
+ s->SetFailed(saved_errno,
+ "Fail to complete ubring handshake from %s: %s",
+ s->description().c_str(), berror(saved_errno));
+ ep->_state = FAILED;
+ return nullptr;
+ }
+ HelloFormatExtension remote_extension;
+ remote_extension.Deserialize(data);
+ if (remote_extension.extension_len != HelloFormatExtension::WIRE_SIZE
||
+ remote_extension.format_id == UBR_DATA_FORMAT_NONE ||
+ remote_extension.format_id != local_extension.format_id) {
+ LOG(WARNING) << "Fail to negotiate data format with server, "
+ << "fallback to tcp:" << s->description();
Review Comment:
The new V3 format negotiation path (send/recv extension +
validation/fallback) is central behavioral logic but currently only has unit
tests for extension (de)serialization. Please add handshake-level tests that
exercise: (1) V3↔V3 success selecting `LEGACY_64` and setting
`_negotiated_data_format`, (2) V3↔V3 negotiation failure on `NONE`, wrong
`extension_len`, and unknown format id causing TCP fallback and leaving
`_negotiated_data_format` as `NONE`, and (3) V3↔V2 no extension bytes exchanged
(stream alignment) with TCP fallback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
