Add getter to fetch all IP addresses of a live device as IPAddress objects. #1500
Conversation
- Deprecated `getAddresses()` in `PcapLiveDevice.h`, advising to use `getIPAddresses()` instead for better functionality. - Implemented `getIPAddresses()` in `PcapLiveDevice.cpp`, which returns a vector of `IPAddress` objects for both IPv4 and IPv6 addresses. - Updated `LiveDeviceTests.cpp` to include tests for the new `getIPAddresses()` method, ensuring it correctly identifies and returns IP addresses.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1500 +/- ##
==========================================
+ Coverage 82.09% 82.13% +0.04%
==========================================
Files 273 273
Lines 43542 43568 +26
Branches 9509 9408 -101
==========================================
+ Hits 35747 35786 +39
+ Misses 7157 6952 -205
- Partials 638 830 +192
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| in_addr* ipv4Addr = internal::try_sockaddr2in_addr(address.addr); | ||
| if (ipv4Addr != nullptr) | ||
| { | ||
| results.push_back(IPv4Address(ipv4Addr->s_addr)); |
There was a problem hiding this comment.
Could be emplace_back, but it's also fine now.
There was a problem hiding this comment.
As in this?
| results.push_back(IPv4Address(ipv4Addr->s_addr)); | |
| results.emplace_back(IPv4Address(ipv4Addr->s_addr)); |
There was a problem hiding this comment.
I think push_back(std::move(IPv4Address)) is equal to emplace_back(IPv4Address
how about this?
results.emplace_back(ipv4Addr->s_addr);
But it might be too trivial to distinguish the difference.
There was a problem hiding this comment.
Tested it on VS. That works for the IPv4, due to the uint32_t constructor. The IPv6 gets confused tho coz both v4 and v6 have a constructor from pointer.
There was a problem hiding this comment.
But it might be too trivial to distinguish the difference.
Eh, it can technically save a single move constructor if the compiler does not optimize it away anyway. Personally I think its fine with the push_back for now.
Pcap++/src/PcapLiveDevice.cpp
Outdated
| if (ipv4Addr != nullptr) | ||
| { | ||
| results.push_back(IPv4Address(ipv4Addr->s_addr)); | ||
| PCPP_LOG_DEBUG("IPv4 address found: " << results.back()); |
There was a problem hiding this comment.
Not sure. Added it for consistency as both getIPv4Address and getIPv6Address output address logs.
There was a problem hiding this comment.
This is very old code, I think we can remove the logs 🙂
| std::vector<IPAddress> PcapLiveDevice::getIPAddresses() const | ||
| { | ||
| std::vector<IPAddress> results; | ||
| for (const auto& address : m_Addresses) |
There was a problem hiding this comment.
it might be simpler to use sockaddr2string to convert the address to string, and the call IPAddress c'tor with this string
There was a problem hiding this comment.
Eh, wouldn't that not achieve the same but with extra steps? We add encoding to string and then decoding from string for not much benefit. Atm, most operations are read/check operations.
Maybe it would be more concise and skip the if checks in the current function, but I don't think the current implementatios is that bad.
There was a problem hiding this comment.
What I mean is that you can do something like this which is shorter and more concise:
for (const auto& address : m_Addresses)
{
std::array<char, INET6_ADDRSTRLEN> addrAsString;
internal::sockaddr2string(address.addr, addrAsString.data(), addrAsString.size());
try
{
results.push_back(IPAddress(addrAsString));
}
catch(...) {}
}There was a problem hiding this comment.
True, but looking at the way it will be implemented adds some overhead that is avoided by the current code.
This is the current implementation of the IPAddress string constructor.
IPAddress::IPAddress(const std::string& addrAsString)
{
if (IPv4Address::isValidIPv4Address(addrAsString)) // <-- string parse
{
m_Type = IPv4AddressType;
m_IPv4 = IPv4Address(addrAsString); // <-- potential string parse
}
else if (IPv6Address::isValidIPv6Address(addrAsString)) // <-- string parse
{
m_Type = IPv6AddressType;
m_IPv6 = IPv6Address(addrAsString); // <-- potential string parse
}
else
{
throw std::invalid_argument("Not a valid IP address: " + addrAsString);
}
}
This is between 2 and 3 parses of the string address to get the IP object in addition to the initial encode of the IP to a string. The current implementation, while slightly longer is at most 4 nullptr checks, 2 integer equality checks and a buffer copy.
There was a problem hiding this comment.
ok, we can go with your approach 🙂
Pcap++/src/PcapLiveDevice.cpp
Outdated
| if (ipv4Addr != nullptr) | ||
| { | ||
| results.push_back(IPv4Address(ipv4Addr->s_addr)); | ||
| PCPP_LOG_DEBUG("IPv4 address found: " << results.back()); |
There was a problem hiding this comment.
This is very old code, I think we can remove the logs 🙂
Stage one of #1499
getAddresses()inPcapLiveDevice.h, advising to usegetIPAddresses()instead for better functionality.getIPAddresses()inPcapLiveDevice.cpp, which returns a vector ofIPAddressobjects for both IPv4 and IPv6 addresses.LiveDeviceTests.cppto include tests for the newgetIPAddresses()method, ensuring it correctly identifies and returns IP addresses.