Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: implement Ip::Address::asText() #1739

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
c673207
Implement SBuf wrapper for toStr
kinkie Mar 13, 2024
b97f3ed
Add unit tests for toStrAsSBuf
kinkie Mar 13, 2024
49b1423
convert all uses in Header.cc
kinkie Mar 13, 2024
8979a90
convert peer_sourcehash.cc
kinkie Mar 13, 2024
7e6b316
Source maintenance
kinkie Mar 15, 2024
1ab9f10
Merge branch 'master' into sbuf-ip-address-tostr
kinkie Apr 2, 2024
5f2ee67
Fix heap buffer overead in ConfigParser::UnQuote() (#1763)
xiaoxiaoafeifei Apr 2, 2024
0a15787
Do not blame cache_peer for CONNECT errors (#1772)
rousskov Apr 2, 2024
95bd8d1
Fix const-correctness of ACLHTTPHeaderData::match() parameter (#1771)
rousskov Apr 3, 2024
ba29475
Fix eCAP header includes (#1753)
yadij Apr 5, 2024
1b46e26
Maintenance: Remove several unused files (#1773)
yadij Apr 5, 2024
6105648
Have SQUID_CHECK_LIB_WORKS do state SAVE/RESTORE (#1774)
yadij Apr 7, 2024
468faa7
NoNewGlobals for MemBlob::Stats (#1749)
kinkie Apr 7, 2024
bf14fcc
Refactor and improve ErrorState::Dump (#1730)
kinkie Apr 8, 2024
a422967
Add AtMostOnce stream manipulator (#1742)
kinkie Apr 8, 2024
06644aa
NoNewGlobals for HttpHdrCc:ccLookupTable (#1750)
kinkie Apr 8, 2024
343f04b
Feature-complete, tests to do
kinkie Apr 9, 2024
6007f79
Upgrade Acl::Node::name to SBuf; remove AclMatchedName global (#1766)
rousskov Apr 8, 2024
beee7cb
Maintenance: update --with-tdb detection (#1776)
yadij Apr 9, 2024
96489ce
Merge remote-tracking branch 'upstream/master' into sbuf-ip-address-t…
kinkie Apr 9, 2024
899724a
Make stream-based operations first class
kinkie Apr 9, 2024
794cad1
fix bug in bracketed and withPort
kinkie Apr 10, 2024
c8f7293
Unit tests complete against old implementations
kinkie Apr 10, 2024
c73c17f
source maintenance
kinkie Apr 10, 2024
a38723a
AddressText::str is same as ToSBuf(AddressText)
kinkie Apr 10, 2024
4de23d0
Add Ip::Address::asText()
kinkie Apr 10, 2024
abf7d4d
remove toStrAsSBuf
kinkie Apr 10, 2024
9a19f31
fix wrong API name
kinkie Apr 11, 2024
15ca319
do not custom forward-declare class Address
kinkie Apr 11, 2024
e29fa69
Fix peer_sourcehash.cc
kinkie Apr 11, 2024
ec14be4
Revert formatting change
kinkie Apr 11, 2024
88402b1
Remove modifiers from constructor
kinkie Apr 11, 2024
80dd4bf
Document AddressText methods
kinkie Apr 11, 2024
8545ac0
minimize changes in testIpAddress
kinkie Apr 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -2388,12 +2388,12 @@ check_PROGRAMS += tests/testIpAddress
tests_testIpAddress_SOURCES = \
tests/testIpAddress.cc
nodist_tests_testIpAddress_SOURCES = \
tests/stub_SBuf.cc \
tests/stub_debug.cc \
tests/stub_libmem.cc \
tests/stub_tools.cc
tests_testIpAddress_LDADD = \
ip/libip.la \
sbuf/libsbuf.la \
base/libbase.la \
$(LIBCPPUNIT_LIBS) \
$(COMPAT_LIB) \
Expand Down
42 changes: 42 additions & 0 deletions src/ip/Address.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "debug/Stream.h"
#include "ip/Address.h"
#include "ip/tools.h"
#include "sbuf/Stream.h"
#include "util.h"

#include <cassert>
Expand Down Expand Up @@ -1043,3 +1044,44 @@ Ip::Address::getInAddr(struct in_addr &buf) const
return false;
}

Ip::AddressText::AddressText(const Address &ip) :
ip_(ip)
{}

std::ostream&
Ip::AddressText::print(std::ostream& os) const
{
if (printBrackets_ && ip_.isIPv6())
os << '[';

/* some external code may have blindly memset a parent. */
/* that's okay, our default is known */
if ( ip_.isAnyAddr() ) {
if (ip_.isIPv6())
os << "::";
if (ip_.isIPv4())
os << "0.0.0.0";
} else {

char buf[INET6_ADDRSTRLEN];
if (ip_.isIPv6()) {
inet_ntop(AF_INET6, &ip_.mSocketAddr_.sin6_addr, buf, INET6_ADDRSTRLEN);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for errors here? Legacy code doesn't

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for errors here?

We should, eventually.

Legacy code doesn't

This PR may not have to check, but it is too early to tell. For example, if this code does not fix any of the (many) other problems with legacy printing code, then it does not have to fix this problem either.

I cannot offer a formula that will tell us which legacy problems should be solved in this PR, but it feels like this PR should solve most if not all of them (eventually). For now, I recommend adding an XXX comment to mark this problem in the new code and focus on this code callers/users/API. We will polish the implementation details later (but probably in this PR).

} else if (ip_.isIPv4()) {
struct in_addr tmp;
ip_.getInAddr(tmp);
inet_ntop(AF_INET, &tmp, buf, INET6_ADDRSTRLEN);
} else {
debugs(14, DBG_CRITICAL, "corrupted IP address details");
assert(false);
}
os << buf;
}

if (printBrackets_ && ip_.isIPv6())
os << ']';

if (printPort_ && ip_.port() != 0)
os << ":" << ip_.port();

return os;
}
49 changes: 49 additions & 0 deletions src/ip/Address.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#define SQUID_SRC_IP_ADDRESS_H

#include "ip/forward.h"
#include "sbuf/SBuf.h"

#include <iosfwd>
#include <ostream>
Expand All @@ -34,6 +35,49 @@
namespace Ip
{

/**
* Formats an Address for stream output
*
* Normally instantiated via factory method Ip::Address::asText().
* Methods manipulate flags to determine output format
*/
class AddressText
{
public:
explicit AddressText(const Address &ip);

/// if the Address has a port, output it (default: no)
/// if set to true, also print brackets for IPv6 addresses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "if set to true..." feature introduces a method call order dependency (e.g., the second XXX below) and other problems (e.g., the third XXX below). Please avoid that.

Eventually, it would be nice to address both problems:

os << AddressText(ip).withPort().bracketed(false); // XXX: No brackets, as expected, but breaks IPv6 printing.
os << AddressText(ip).bracketed(false).withPort(); // XXX: Brackets added, surprising code reader.

// XXX: The following two code lines produce different output:
os << AddressText(ip).withPort(false); 
os << AddressText(ip).withPort(true).withPort(false); 

AddressText &withPort(bool b = true)
{
printPort_ = b;
if (b)
printBrackets_ = true;
return *this;
};

/// whether to print brackets surrounding the IP portion of IPv6 addresses (default: yes)
/// IPv4 addresses never have brackets
AddressText &bracketed(bool b = true)
rousskov marked this conversation as resolved.
Show resolved Hide resolved
{
printBrackets_ = b;
return *this;
};
/// output to ostream. Normally used through operator <<
std::ostream &print(std::ostream &) const;

private:
const Address &ip_;
bool printPort_ = false;
bool printBrackets_ = true;
};

inline std::ostream &
operator<<(std::ostream &os, const AddressText &at)
{
return at.print(os);
}

/**
* Holds and manipulates IPv4, IPv6, and Socket Addresses.
*/
Expand Down Expand Up @@ -212,6 +256,9 @@ class Address
*/
char* toStr(char *buf, const unsigned int blen, int force = AF_UNSPEC) const;

/// Return object textually representing an Address for stream output
AddressText asText() const { return AddressText(*this); }

/** Return the ASCII equivalent of the address:port combination
* Provides a URL formatted version of the content.
* If buffer is not large enough the data is truncated silently.
Expand Down Expand Up @@ -354,6 +401,8 @@ class Address
static const struct in6_addr v4_anyaddr;
static const struct in6_addr v4_noaddr;
static const struct in6_addr v6_noaddr;

friend class AddressText;
};

inline std::ostream &
Expand Down
10 changes: 4 additions & 6 deletions src/peer_sourcehash.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "neighbors.h"
#include "peer_sourcehash.h"
#include "PeerSelectState.h"
#include "sbuf/Stream.h"
#include "SquidConfig.h"
#include "Store.h"

Expand Down Expand Up @@ -137,28 +138,25 @@ peerSourceHashRegisterWithCacheManager(void)
CachePeer *
peerSourceHashSelectParent(PeerSelector *ps)
{
const char *c;
CachePeer *p = nullptr;
unsigned int user_hash = 0;
unsigned int combined_hash;
double score;
double high_score = 0;
const char *key = nullptr;
char ntoabuf[MAX_IPSTRLEN];

if (SourceHashPeers().empty())
return nullptr;

assert(ps);
HttpRequest *request = ps->request;

key = request->client_addr.toStr(ntoabuf, sizeof(ntoabuf));
const auto key = ToSBuf(request->client_addr.asText());

/* calculate hash key */
debugs(39, 2, "peerSourceHashSelectParent: Calculating hash for " << key);

for (c = key; *c != 0; ++c)
user_hash += ROTATE_LEFT(user_hash, 19) + *c;
for (auto c : key)
user_hash += ROTATE_LEFT(user_hash, 19) + c;

/* select CachePeer */
for (const auto &tp: SourceHashPeers()) {
Expand Down
6 changes: 2 additions & 4 deletions src/proxyp/Header.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,13 @@ ProxyProtocol::Header::getValues(const uint32_t headerType, const char sep) cons
return SBuf();
auto logAddr = sourceAddress;
logAddr.applyClientMask(Config.Addrs.client_netmask);
char ipBuf[MAX_IPSTRLEN];
return SBuf(logAddr.toStr(ipBuf, sizeof(ipBuf)));
return ToSBuf(logAddr.asText());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both new and the old code suffer from the same problem that this PR should address: A reader cannot easily tell whether this code formats the address correctly. For example, I cannot easily tell whether this code prints the port number. The same problem is present in other converted code as well.

To fix this, I suggest adjusting AddressText API so that callers must use

  • either withPort() // implies bracketed IPv6 addresses; bans bracketed() calls
  • or withoutPort() and bracketed(bool) // explicit on both counts

Any better ideas?

}

case Two::htPseudoDstAddr: {
if (!hasAddresses())
return SBuf();
char ipBuf[MAX_IPSTRLEN];
return SBuf(destinationAddress.toStr(ipBuf, sizeof(ipBuf)));
return ToSBuf(destinationAddress.asText());
}

case Two::htPseudoSrcPort: {
Expand Down
67 changes: 59 additions & 8 deletions src/tests/testIpAddress.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include "compat/cppunit.h"
#include "ip/Address.h"
#include "ip/tools.h"
#include "sbuf/SBuf.h"
#include "sbuf/Stream.h"
#include "unitTestMain.h"

#include <cstring>
Expand Down Expand Up @@ -48,6 +50,7 @@ class TestIpAddress : public CPPUNIT_NS::TestFixture
CPPUNIT_TEST(testtoUrl_fromSockAddr);
CPPUNIT_TEST(testgetReverseString);
CPPUNIT_TEST(testMasking);
CPPUNIT_TEST(testAddressText);

CPPUNIT_TEST(testBugNullingDisplay);
CPPUNIT_TEST_SUITE_END();
Expand All @@ -72,6 +75,7 @@ class TestIpAddress : public CPPUNIT_NS::TestFixture
void testtoStr();
void testtoUrl_fromInAddr();
void testtoUrl_fromSockAddr();
void testAddressText();
void testgetReverseString();
void testMasking();

Expand Down Expand Up @@ -465,24 +469,23 @@ void
TestIpAddress::testtoStr()
{
struct in_addr inval;
char buf[MAX_IPSTRLEN];
Ip::Address anIPA;

anIPA.setAnyAddr();

/* test AnyAddr display values */
CPPUNIT_ASSERT( memcmp("::", anIPA.toStr(buf,MAX_IPSTRLEN), 2) == 0 );
CPPUNIT_ASSERT_EQUAL(SBuf("::"), ToSBuf(anIPA.asText().bracketed(false)));

inval.s_addr = htonl(0xC0A8640C);
anIPA = inval;

/* test IP display */
CPPUNIT_ASSERT( memcmp("192.168.100.12",anIPA.toStr(buf,MAX_IPSTRLEN), 14) == 0 );
CPPUNIT_ASSERT_EQUAL(SBuf("192.168.100.12"), ToSBuf(anIPA.asText()));

anIPA.setNoAddr();

/* test NoAddr display values */
CPPUNIT_ASSERT( memcmp("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",anIPA.toStr(buf,MAX_IPSTRLEN), 39) == 0 );
CPPUNIT_ASSERT_EQUAL(SBuf("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"), ToSBuf(anIPA.asText().bracketed(false)));
}

void
Expand Down Expand Up @@ -588,7 +591,6 @@ TestIpAddress::testgetReverseString()
void
TestIpAddress::testMasking()
{
char buf[MAX_IPSTRLEN];
Ip::Address anIPA;
Ip::Address maskIPA;

Expand Down Expand Up @@ -617,8 +619,7 @@ TestIpAddress::testMasking()
CPPUNIT_ASSERT_EQUAL( 80, anIPA.cidr() );

/* BUG Check: test values by display. */
CPPUNIT_ASSERT( anIPA.toStr(buf,MAX_IPSTRLEN) != nullptr );
CPPUNIT_ASSERT( memcmp("ffff:ffff:ffff:ffff:ffff::", buf, 26) == 0 );
CPPUNIT_ASSERT_EQUAL(SBuf("ffff:ffff:ffff:ffff:ffff::"), ToSBuf(anIPA.asText().bracketed(false)) );

/* Test Network Bitmask from Ip::Address */
anIPA.setNoAddr();
Expand All @@ -635,7 +636,7 @@ TestIpAddress::testMasking()
CPPUNIT_ASSERT_EQUAL( (uint32_t)htonl(0xFFFFF000), btest.s_addr );

/* BUG Check failing test. Masked values for display. */
CPPUNIT_ASSERT( memcmp("255.255.240.0",anIPA.toStr(buf,MAX_IPSTRLEN), 13) == 0 );
CPPUNIT_ASSERT_EQUAL(SBuf("255.255.240.0"), ToSBuf(anIPA.asText()));

anIPA.setNoAddr();
maskIPA.setNoAddr();
Expand Down Expand Up @@ -799,6 +800,56 @@ TestIpAddress::testBugNullingDisplay()

}

void
TestIpAddress::testAddressText()
{
std::vector<const char *> testAddresses = {
"192.168.100.12",
"2000:800::45",
"::",
"0.0.0.0",
"::1",
"::ffff:192.168.100.12",
};

std::vector<unsigned short int> testPorts = {
0,
1,
1024,
65535,
};

for (const auto p : testPorts) {
for (const auto& a : testAddresses) {
Ip::Address addr(a);
addr.port(p);

static char buf[MAX_IPSTRLEN];
SBufStream ss;

ss << Ip::AddressText(addr).bracketed(false).withPort(false);
addr.toStr(buf, MAX_IPSTRLEN);
CPPUNIT_ASSERT_EQUAL(SBuf(buf), ss.buf());
ss.clearBuf();

ss << Ip::AddressText(addr).bracketed(true).withPort(true);
addr.toUrl(buf, MAX_IPSTRLEN);
CPPUNIT_ASSERT_EQUAL(SBuf(buf), ss.buf());
ss.clearBuf();

ss << Ip::AddressText(addr).withPort(true);
addr.toUrl(buf, MAX_IPSTRLEN);
CPPUNIT_ASSERT_EQUAL(SBuf(buf), ss.buf());
ss.clearBuf();

ss << Ip::AddressText(addr);
addr.toHostStr(buf, MAX_IPSTRLEN);
CPPUNIT_ASSERT_EQUAL(SBuf(buf), ss.buf());
ss.clearBuf();
}
}
}

int
main(int argc, char *argv[])
{
Expand Down
Loading