Copilot commented on code in PR #3507:
URL: https://github.com/apache/brpc/pull/3507#discussion_r3951528915
##########
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;
Review Comment:
The same failure-handling pattern (capture `errno`, `PLOG`,
`SetFailed(...)`, set `FAILED`, return) appears multiple times across
client/server read/write steps. Consider extracting a small helper (e.g.,
`FailHandshake(Socket*, UBShmEndpoint*, err, context_message)`) to reduce
duplication and keep future handshake changes consistent.
##########
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;
Review Comment:
`remote_extension` is default-initialized (uninitialized fields) and then
deserialized. For consistency with the changes that explicitly value-initialize
`HelloMessage local_msg = {};`, consider also value-initializing this struct
(`HelloFormatExtension remote_extension = {};`). This reduces the risk of
accidentally using indeterminate values if `Deserialize` ever changes or
early-exits in the future.
##########
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 a compile-time constant; using `static constexpr uint16_t
WIRE_SIZE = 4;` avoids potential ODR/address-taking pitfalls and is the
idiomatic C++ way to express such constants.
--
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]
