InitNetwork IPAddress related fixes #980
Merged
Conversation
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
returning by value calls CIPAddress::CIPAddress (const CIPAddress &rAddress) which assert (rAddress.m_bValid) so any unset ip will fail in debug builds
Reviewer's GuideRefactor IP address handling by changing CConfig getters to return const references, load IP properties only when present, and enhance CMiniDexed network initialization with explicit DHCP/static logic, detailed logging, and safe defaults for gateway and DNS. Sequence diagram for improved network initialization in CMiniDexed::InitNetworksequenceDiagram
participant CMiniDexed
participant CConfig
participant CNetSubSystem
CMiniDexed->>CConfig: GetNetworkDHCP()
alt DHCP enabled
CMiniDexed->>CConfig: GetNetworkHostname()
CMiniDexed->>CNetSubSystem: new (0,0,0,0,hostname, type)
else Static IP and Subnet set
CMiniDexed->>CConfig: GetNetworkIPAddress()
CMiniDexed->>CConfig: GetNetworkSubnetMask()
CMiniDexed->>CConfig: GetNetworkDefaultGateway()
CMiniDexed->>CConfig: GetNetworkDNSServer()
CMiniDexed->>CConfig: GetNetworkHostname()
CMiniDexed->>CNetSubSystem: new (ip, mask, gw, dns, hostname, type)
else Neither set
CMiniDexed->>CConfig: GetNetworkHostname()
CMiniDexed->>CNetSubSystem: new (0,0,0,0,hostname, type)
end
CMiniDexed->>CNetSubSystem: Initialize(false)
alt Initialization fails
CMiniDexed->>CMiniDexed: Log error
end
Class diagram for updated CConfig IP address handlingclassDiagram
class CConfig {
+bool GetNetworkDHCP() const
+const char* GetNetworkType() const
+const char* GetNetworkHostname() const
+const CIPAddress& GetNetworkIPAddress() const
+const CIPAddress& GetNetworkSubnetMask() const
+const CIPAddress& GetNetworkDefaultGateway() const
+const CIPAddress& GetNetworkDNSServer() const
+bool GetSyslogEnabled() const
+const CIPAddress& GetNetworkSyslogServerIPAddress() const
+bool GetNetworkFTPEnabled() const
-CIPAddress m_INetworkIPAddress
-CIPAddress m_INetworkSubnetMask
-CIPAddress m_INetworkDefaultGateway
-CIPAddress m_INetworkDNSServer
-CIPAddress m_INetworkSyslogServerIPAddress
}
class CIPAddress {
+void Set(const u8* pIP)
+bool IsSet() const
+bool IsNull() const
+void Format(CString* out) const
+u32 Get() const
}
CConfig --> CIPAddress : uses
Class diagram for updated CMiniDexed::InitNetwork logicclassDiagram
class CMiniDexed {
+void UpdateNetwork()
+bool InitNetwork()
-CConfig* m_pConfig
-CNetSubSystem* m_pNet
}
class CNetSubSystem {
+CNetSubSystem(u32 ip, u32 mask, u32 gw, u32 dns, const char* hostname, NetDeviceType type)
+bool Initialize(bool)
}
class CConfig {
+bool GetNetworkDHCP() const
+const char* GetNetworkHostname() const
+const CIPAddress& GetNetworkIPAddress() const
+const CIPAddress& GetNetworkSubnetMask() const
+const CIPAddress& GetNetworkDefaultGateway() const
+const CIPAddress& GetNetworkDNSServer() const
}
CMiniDexed --> CConfig : uses
CMiniDexed --> CNetSubSystem : creates
CConfig --> CIPAddress : uses
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 @soyersoyer - I've reviewed your changes - here's some feedback:
- The fallback branch for when neither DHCP nor static IP is set duplicates the DHCP initialization path; consider refactoring the CNetSubSystem creation into a helper to reduce code duplication.
- m_pNet is managed with a raw pointer—consider using std::unique_ptr (or another RAII approach) to ensure proper cleanup and avoid potential memory leaks.
- In CConfig::Load, missing IP properties leave previous CIPAddress values intact; explicitly clear or reset those members when a property is absent to prevent stale data on reload.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fallback branch for when neither DHCP nor static IP is set duplicates the DHCP initialization path; consider refactoring the CNetSubSystem creation into a helper to reduce code duplication.
- m_pNet is managed with a raw pointer—consider using std::unique_ptr (or another RAII approach) to ensure proper cleanup and avoid potential memory leaks.
- In CConfig::Load, missing IP properties leave previous CIPAddress values intact; explicitly clear or reset those members when a property is absent to prevent stale data on reload.
## Individual Comments
### Comment 1
<location> `src/config.cpp:209` </location>
<code_context>
- m_INetworkIPAddress = m_Properties.GetIPAddress("NetworkIPAddress") != 0;
- m_INetworkSubnetMask = m_Properties.GetIPAddress("NetworkSubnetMask") != 0;
- m_INetworkDefaultGateway = m_Properties.GetIPAddress("NetworkDefaultGateway") != 0;
+ if (const u8 *pIP = m_Properties.GetIPAddress("NetworkIPAddress")) m_INetworkIPAddress.Set (pIP);
+ if (const u8 *pIP = m_Properties.GetIPAddress("NetworkSubnetMask")) m_INetworkSubnetMask.Set (pIP);
+ if (const u8 *pIP = m_Properties.GetIPAddress("NetworkDefaultGateway")) m_INetworkDefaultGateway.Set (pIP);
</code_context>
<issue_to_address>
Assignment logic for IP addresses now only sets values if present, but does not clear them if absent.
This may cause outdated values to persist if properties are removed at runtime. Please reset these member variables to a default state before setting them conditionally.
</issue_to_address>
### Comment 2
<location> `src/minidexed.cpp:2424` </location>
<code_context>
m_pNet = new CNetSubSystem(0, 0, 0, 0, m_pConfig->GetNetworkHostname(), NetDeviceType);
- else
+ }
+ else if (m_pConfig->GetNetworkIPAddress().IsSet() && m_pConfig->GetNetworkSubnetMask().IsSet())
+ {
+ CString IPString, SubnetString;
</code_context>
<issue_to_address>
Default gateway and DNS server are set to 0 if not present, which may have unintended network effects.
If 0 is not a valid value for these parameters, use an explicit 'unset' indicator or confirm that CNetSubSystem treats 0 correctly.
Suggested implementation:
```cpp
// Use INADDR_NONE as an explicit 'unset' indicator for gateway and DNS if not set
m_pNet = new CNetSubSystem(
m_pConfig->GetNetworkIPAddress().Get(),
m_pConfig->GetNetworkSubnetMask().Get(),
m_pConfig->GetNetworkDefaultGateway().IsSet() ? m_pConfig->GetNetworkDefaultGateway().Get() : INADDR_NONE,
m_pConfig->GetNetworkDNSServer().IsSet() ? m_pConfig->GetNetworkDNSServer().Get() : INADDR_NONE,
m_pConfig->GetNetworkHostname(), NetDeviceType);
```
- Make sure to `#include <arpa/inet.h>` or the appropriate header for `INADDR_NONE` at the top of your file if not already present.
- If your codebase uses a different constant for "unset" (e.g., `-1` or a custom value), replace `INADDR_NONE` with that value.
- Confirm that `CNetSubSystem` treats `INADDR_NONE` as "unset" for gateway and DNS server.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Build for testing: |
Owner
|
Thanks @soyersoyer. Please have a look at #980 (review), is there something to it? |
Contributor
Author
Yes, but it won't match the style of the file, so no. For example, unique_ptr could be used, but then the code would have to be rewritten everywhere so that it would not be inconsistent. |
Owner
|
Hi @soyersoyer, why was this closed? Since this PR doesn't change the behavior or performance format and is, there is no reason to close it imho. |
|
Build for testing: |
Contributor
Author
|
I didn't want to keep track of all the open PRs anymore, so I closed them. You can leave it open. |
Owner
|
Thanks @soyersoyer |
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.
This fixes the IP address setting error by improving IP address handling.
Summary by Sourcery
Improve IP address handling by fixing configuration loading, switching getters to return const references, and enhancing network initialization to robustly select between DHCP and static addressing with detailed logging.
Enhancements: