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 5 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
10 changes: 10 additions & 0 deletions src/ip/Address.cc
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,16 @@ Ip::Address::toStr(char* buf, const unsigned int blen, int force) const
return buf;
}

SBuf
Ip::Address::toStrAsSBuf(int force) const
{
SBuf rv;
auto p = rv.rawAppendStart(MAX_IPSTRLEN);
toStr(p, MAX_IPSTRLEN, force);
rv.rawAppendFinish(p,strlen(p));
return rv;
}

unsigned int
Ip::Address::toHostStr(char *buf, const unsigned int blen) const
{
Expand Down
4 changes: 3 additions & 1 deletion 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 Down Expand Up @@ -211,6 +212,7 @@ class Address
\return pointer to buffer received.
*/
char* toStr(char *buf, const unsigned int blen, int force = AF_UNSPEC) const;
SBuf toStrAsSBuf(int force = AF_UNSPEC) const;
Copy link
Contributor

@yadij yadij Mar 15, 2024

Choose a reason for hiding this comment

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

IMO the AsSBuf portion of the naming is more harm than good. We should just be deprecating the char* API and otherwise keeping the naming the same.
[EDIT: ... assuming scope is kept at updating methods, not moving to the alternative API @rousskov proposed]

Suggested change
char* toStr(char *buf, const unsigned int blen, int force = AF_UNSPEC) const;
SBuf toStrAsSBuf(int force = AF_UNSPEC) const;
SBuf toStr(int force = AF_UNSPEC) const;
/// Deprecated. Use the SBuf method instead.
char* toStr(char *buf, const unsigned int blen, int force = AF_UNSPEC) const;

Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree that toStrAsSBuf() is a non-starter as far as spelling is concerned. However, FWIW, following the above renaming suggestion will not address my primary concerns and, hence, will not move this PR forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now gone

Copy link
Contributor

Choose a reason for hiding this comment

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

It is now gone

Still here and still used. I cannot provide requested feedback until this is gone.


/** Return the ASCII equivalent of the address:port combination
* Provides a URL formatted version of the content.
Expand All @@ -220,7 +222,7 @@ class Address
\param len byte length of buffer available for writing.
\return pointer to buffer received.
*/
char* toUrl(char *buf, unsigned int len) const;
char *toUrl(char *buf, unsigned int len) const;
kinkie marked this conversation as resolved.
Show resolved Hide resolved

/** Return a properly hostname formatted copy of the address
* Provides a URL formatted version of the content.
Expand Down
9 changes: 3 additions & 6 deletions src/peer_sourcehash.cc
Original file line number Diff line number Diff line change
Expand Up @@ -137,28 +137,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 = request->client_addr.toStrAsSBuf();
Copy link
Contributor

Choose a reason for hiding this comment

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

This use-case does not need any new API. It can just:

Suggested change
const auto key = request->client_addr.toStrAsSBuf();
const auto key = ToSBuf(request->client_addr);

Copy link
Contributor

@rousskov rousskov Mar 15, 2024

Choose a reason for hiding this comment

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

Do we want to guarantee that two different Squid instances produce the same hash for the same request?

  • If yes, then we may want to avoid changing format/hash to avoid breaking that guarantee for instances running different Squid versions. We could still change if there is a compelling reason to do so, of course, but I do not see such a reason here.
  • Otherwise, we should not covert the IP address to text just to compute a hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point.
I think we should try to preserve behaviour in a major version,
but I wouldn't feel constrained to do the same across
major versions.

At the same time, I would keep this change as its own PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This use-case does not need any new API. It can just:

Adopted

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't feel constrained to do the same across major versions.

Is this hash computation (de facto) standardized across different products (including non-Squid-based caches) or can we assume that this is a Squid-specific concern?

If it is a Squid-specific concern: The "we can change things across versions" logic is sound, but it does not work as well when dealing with distributed cache hierarchies where nodes may run different versions. I would not object to a breaking change like that if others are sure that it is warranted, but in that case, again, we should not covert the IP address to text to compute a hash.

The non-obvious sensitivity of this code to address formatting warrants a C++ comment, especially if we migrate to something implicit like ToSBuf().

I would keep this change as its own PR

Thank you. I assume "this change" is "possible change of peer hash values".


/* 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 logAddr.toStrAsSBuf();
}

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

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

#include <cstring>
Expand Down Expand Up @@ -471,18 +472,20 @@ TestIpAddress::testtoStr()
anIPA.setAnyAddr();

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

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"), anIPA.toStrAsSBuf());

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"), anIPA.toStrAsSBuf());
}

void
Expand Down Expand Up @@ -619,6 +622,7 @@ TestIpAddress::testMasking()
/* 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::"), anIPA.toStrAsSBuf() );

/* Test Network Bitmask from Ip::Address */
anIPA.setNoAddr();
Expand All @@ -636,6 +640,7 @@ TestIpAddress::testMasking()

/* 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"), anIPA.toStrAsSBuf());

anIPA.setNoAddr();
maskIPA.setNoAddr();
Expand Down
Loading