fix: Enhance cooperation status management#662
Merged
re2zero merged 2 commits intolinuxdeepin:masterfrom Aug 7, 2025
Merged
Conversation
- Updated ServiceManager to set "share_connect_ip" to an empty string. - Added methods in HandleIpcService to update and retrieve cooperation status. - Enhanced HandleRpcService to check current cooperation status before accepting new connection requests. - Implemented busy status handling in NetworkUtil for share requests. - Updated ShareHelper to manage cooperation status during connection and disconnection events. Log: Enhance cooperation status management for better handling of connection requests and status updates.
Reviewer's GuideCentralize and enhance cooperation status management by introducing IPC interfaces, adding status checks in RPC logic, extending NetworkUtil for busy handling and status tracking, integrating status updates in ShareHelper, and resetting initial share_connect_ip in ServiceManager. Sequence diagram for handling incoming share connection requests with busy statussequenceDiagram
participant Requester as Requesting Device
participant ShareHelper as ShareHelper
participant NetworkUtil as NetworkUtil
participant CompatWrapper as CompatWrapper (optional)
participant IPC as HandleIpcService (optional)
Requester->>ShareHelper: Send connection request
ShareHelper->>NetworkUtil: isCurrentlyCooperating()
NetworkUtil->>ShareCooperationServiceManager: Check server/client running
alt ENABLE_COMPAT
NetworkUtil->>CompatWrapper: getCurrentCooperationStatus()
CompatWrapper->>IPC: getCurrentCooperationStatus()
IPC-->>CompatWrapper: Return status
end
NetworkUtil-->>ShareHelper: Return cooperation status
alt Already cooperating
ShareHelper->>NetworkUtil: replyShareRequestBusy(senderIp)
NetworkUtil->>Requester: Send BUSY_COOPERATING rejection
else Not cooperating
ShareHelper->>...: Proceed with normal connection flow
end
ER diagram for cooperation status and share_connect_ip changeserDiagram
NODE_PEER_INFO {
string username
string hostname
string ipv4
string share_connect_ip
int port
string os_type
string mode_type
}
NODE_PEER_INFO ||--o{ COOPERATION_STATUS : has
COOPERATION_STATUS {
int status
}
Class diagram for enhanced cooperation status managementclassDiagram
class NetworkUtil {
+void replyShareRequestBusy(QString targetIp)
+bool isCurrentlyCooperating()
+void updateCooperationStatus(int status)
}
class ShareHelper {
+void notifyConnectRequest(QString info)
+void handleConnectResult(int result, QString clientprint)
+void disconnectToDevice(DeviceInfoPointer info)
+void handleDisConnectResult(QString devName)
}
class HandleIpcService {
+void updateCooperationStatus(int status)
+bool getCurrentCooperationStatus()
}
class HandleRpcService {
+void handleRemoteShareConnect(co::Json &info)
}
class ServiceManager {
+fastring genPeerInfo()
}
NetworkUtil --|> ShareHelper : used by
NetworkUtil -- HandleIpcService : IPC calls
ShareHelper -- NetworkUtil : uses
HandleRpcService -- HandleIpcService : may call for status
ServiceManager ..> NetworkUtil : sets share_connect_ip
HandleIpcService <..> Comshare : updates/gets status
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @re2zero - I've reviewed your changes - here's some feedback:
- Replace hard-coded status codes (e.g., 6 and 0) and string literals like "BUSY_COOPERATING" with named enums or constants to improve readability and reduce maintenance risk.
- Consider consolidating the cooperation-status check logic in NetworkUtil and the RPC/IPC handlers into a shared utility or service to avoid duplication and ensure consistent behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replace hard-coded status codes (e.g., 6 and 0) and string literals like "BUSY_COOPERATING" with named enums or constants to improve readability and reduce maintenance risk.
- Consider consolidating the cooperation-status check logic in NetworkUtil and the RPC/IPC handlers into a shared utility or service to avoid duplication and ensure consistent behavior.
## Individual Comments
### Comment 1
<location> `src/lib/cooperation/core/net/networkutil.cpp:120` </location>
<code_context>
+ int resultCode;
+ if (agree) {
+ resultCode = SHARE_CONNECT_COMFIRM;
+ } else if (req.nick == "BUSY_COOPERATING") {
+ WLOG << "Device is busy, use ERR_CONNECTED to show appropriate message";
+ // Device is busy, use ERR_CONNECTED to show appropriate message
</code_context>
<issue_to_address>
Using a magic string for busy status may reduce maintainability.
Consider replacing the hardcoded "BUSY_COOPERATING" string with a constant or enum to improve clarity and reduce maintenance risks.
</issue_to_address>
### Comment 2
<location> `src/lib/cooperation/core/net/networkutil.cpp:870` </location>
<code_context>
+ // 直接发送忙碌拒绝消息
+ ApplyMessage msg;
+ msg.flag = REPLY_REJECT;
+ msg.nick = "BUSY_COOPERATING"; // Special marker for busy status
+ msg.host = CooperationUtil::localIPAddress().toStdString();
+ msg.fingerprint = ""; // No fingerprint needed for busy response
</code_context>
<issue_to_address>
Consider using a constant for the busy status marker.
Using a shared constant or enum for "BUSY_COOPERATING" will reduce duplication and make updates easier.
Suggested implementation:
```cpp
msg.nick = BUSY_COOPERATING_NICK; // Special marker for busy status
```
You also need to:
1. Define `BUSY_COOPERATING_NICK` in a suitable header file (e.g., `networkutil.h` or a shared constants header), for example:
```cpp
constexpr const char* BUSY_COOPERATING_NICK = "BUSY_COOPERATING";
```
2. Include that header in `networkutil.cpp` if it is not already included.
</issue_to_address>
### Comment 3
<location> `src/compat/plugins/daemon/core/service/rpc/handlerpcservice.cpp:267` </location>
<code_context>
+ DLOG << "Current Comshare status:" << currentStatus << " isCurrentlyCooperating:" << isCurrentlyCooperating;
+
+ // 如果正在协同，直接拒绝新的连接请求
+ if (isCurrentlyCooperating) {
+ ShareConnectReply reply;
+ reply.ip = Util::getFirstIp();
+ reply.appName = lo.tarAppname; // 本地（被请求方）app名称
+ reply.tarAppname = lo.appName; // 远端（请求方）app名称
+ reply.reply = SHARE_CONNECT_ERR_CONNECTED;
+
+ // 发送给远端控制方
+ SendRpcService::instance()->doSendProtoMsg(APPLY_SHARE_CONNECT_RES,
+ lo.tarAppname.c_str(), reply.as_json().str().c_str());
+ return;
+ }
+
</code_context>
<issue_to_address>
Immediate return on cooperation may prevent further checks.
Returning early bypasses the DiscoveryJob fallback check, which may cause issues if Comshare status is outdated. Please review whether both checks are needed or if their order should be adjusted.
</issue_to_address>
### Comment 4
<location> `src/compat/plugins/daemon/core/service/rpc/handlerpcservice.cpp:265` </location>
<code_context>
//NodeInfo
NodePeerInfo nodepeer;
nodepeer.from_json(baseJson);
- if (!nodepeer.share_connect_ip.empty()
- && nodepeer.share_connect_ip.compare(nodepeer.ipv4) != 0) {
+ DLOG << "DiscoveryJob baseInfo share_connect_ip:" << nodepeer.share_connect_ip.c_str()
+ << " ipv4:" << nodepeer.ipv4.c_str();
</code_context>
<issue_to_address>
Removed check for share_connect_ip != ipv4 may affect logic.
Consider whether removing the check for share_connect_ip != ipv4 could cause issues if these values are equal, as this may alter the intended logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| int resultCode; | ||
| if (agree) { | ||
| resultCode = SHARE_CONNECT_COMFIRM; | ||
| } else if (req.nick == "BUSY_COOPERATING") { |
There was a problem hiding this comment.
suggestion: Using a magic string for busy status may reduce maintainability.
Consider replacing the hardcoded "BUSY_COOPERATING" string with a constant or enum to improve clarity and reduce maintenance risks.
| // 直接发送忙碌拒绝消息 | ||
| ApplyMessage msg; | ||
| msg.flag = REPLY_REJECT; | ||
| msg.nick = "BUSY_COOPERATING"; // Special marker for busy status |
There was a problem hiding this comment.
suggestion: Consider using a constant for the busy status marker.
Using a shared constant or enum for "BUSY_COOPERATING" will reduce duplication and make updates easier.
Suggested implementation:
msg.nick = BUSY_COOPERATING_NICK; // Special marker for busy status
You also need to:
- Define
BUSY_COOPERATING_NICKin a suitable header file (e.g.,networkutil.hor a shared constants header), for example:constexpr const char* BUSY_COOPERATING_NICK = "BUSY_COOPERATING";
- Include that header in
networkutil.cppif it is not already included.
Comment on lines
+267
to
+276
| if (isCurrentlyCooperating) { | ||
| ShareConnectReply reply; | ||
| reply.ip = Util::getFirstIp(); | ||
| reply.appName = lo.tarAppname; // 本地（被请求方）app名称 | ||
| reply.tarAppname = lo.appName; // 远端（请求方）app名称 | ||
| reply.reply = SHARE_CONNECT_ERR_CONNECTED; | ||
|
|
||
| // 发送给远端控制方 | ||
| SendRpcService::instance()->doSendProtoMsg(APPLY_SHARE_CONNECT_RES, | ||
| lo.tarAppname.c_str(), reply.as_json().str().c_str()); |
There was a problem hiding this comment.
question: Immediate return on cooperation may prevent further checks.
Returning early bypasses the DiscoveryJob fallback check, which may cause issues if Comshare status is outdated. Please review whether both checks are needed or if their order should be adjusted.
Comment on lines
-265
to
-266
| if (!nodepeer.share_connect_ip.empty() | ||
| && nodepeer.share_connect_ip.compare(nodepeer.ipv4) != 0) { |
There was a problem hiding this comment.
question (bug_risk): Removed check for share_connect_ip != ipv4 may affect logic.
Consider whether removing the check for share_connect_ip != ipv4 could cause issues if these values are equal, as this may alter the intended logic.
Update changelog for version 1.1.14 . Log: New v1.1.14
|
TAG Bot TAG: 1.1.14 |
lzwind
approved these changes
Aug 7, 2025
This was referenced Aug 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Log: Enhance cooperation status management for better handling of connection requests and status updates.
Summary by Sourcery
Improve cooperation status management by centralizing status updates, implementing busy-state handling to reject overlapping share requests, and synchronizing status across NetworkUtil, ShareHelper, and IPC/RPC services
New Features:
Enhancements: