Copilot commented on code in PR #3507:
URL: https://github.com/apache/brpc/pull/3507#discussion_r3950799082
##########
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();
ub_transport->_ub_state = UBShmTransport::UB_OFF;
} else {
Review Comment:
This introduces new handshake behavior (client-side fallback conditions
based on extension length, `NONE`, and mismatched/unknown formats). The current
tests cover extension (de)serialization, but don’t appear (in this diff) to
exercise the end-to-end handshake negotiation branches that drive `_ub_state`
and `_negotiated_data_format`. Consider adding a unit/integration test that
simulates a V3 peer returning `NONE` or an unknown `format_id` and asserts TCP
fallback plus negotiated-format state remains `NONE`.
##########
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-size for buffers and I/O lengths; defining it
as `static constexpr size_t` (or at least `uint32_t`) avoids repeated implicit
conversions to `size_t` in calls like `ReadFromFd/WriteToFd` and is more
idiomatic for “byte size” constants. This can also prevent compiler warnings on
some platforms/toolchains.
--
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]
