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.

Sign up for GitHub

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

Maintenance: convert ICP HttpRequest* to smart Pointer #1804

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+25 −34
Open

Maintenance: convert ICP HttpRequest* to smart Pointer #1804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01fa0cc
Use Pointer in class ICPState
yadij Apr 27, 2024
38d322f
Convert several HttpRequest* to smart Pointer
yadij Apr 28, 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
10 changes: 5 additions & 5 deletions src/ICP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@

#include "base/RefCount.h"
#include "comm/forward.h"
#include "http/forward.h"
#include "icp_opcode.h"
#include "ip/Address.h"
#include "LogTags.h"
#include "store_key_md5.h"
#include "StoreClient.h"

class AccessLogEntry;
class HttpRequest;

typedef RefCount<AccessLogEntry> AccessLogEntryPointer;

Expand Down Expand Up @@ -62,14 +62,14 @@ class ICPState: public StoreClient
{

public:
ICPState(icp_common_t &aHeader, HttpRequest *aRequest);
ICPState(icp_common_t &, const HttpRequestPointer &);
~ICPState() override;

/// whether the cache contains the requested entry
bool isHit() const;

icp_common_t header;
HttpRequest *request;
HttpRequestPointer request;
int fd;

Ip::Address from;
Expand All @@ -90,10 +90,10 @@ extern Comm::ConnectionPointer icpOutgoingConn;
extern Ip::Address theIcpPublicHostID;

/// \ingroup ServerProtocolICPAPI
HttpRequest* icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from);
HttpRequestPointer icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from);

/// \ingroup ServerProtocolICPAPI
bool icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request);
bool icpAccessAllowed(Ip::Address &, const HttpRequestPointer &);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep the first parameter name. It is very useful to a human reader of this declaration.

Suggested change
bool icpAccessAllowed(Ip::Address &, const HttpRequestPointer &);
bool icpAccessAllowed(Ip::Address &from, const HttpRequestPointer &);


/// \ingroup ServerProtocolICPAPI
void icpCreateAndSend(icp_opcode, int flags, char const *url, int reqnum, int pad, int fd, const Ip::Address &from, AccessLogEntryPointer);
Expand Down
27 changes: 9 additions & 18 deletions src/icp_v2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,16 @@ icp_common_t::getOpCode() const

/* ICPState */

ICPState::ICPState(icp_common_t &aHeader, HttpRequest *aRequest):
ICPState::ICPState(icp_common_t &aHeader, const HttpRequestPointer &aRequest):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you recall from previous discussions, I think we should use Foo::Pointer1 whenever that name has to be visible (for other valid reasons).

This suggestion illustrates the change but this change request applies to most other (if not all other) PR changes as well.

Suggested change
ICPState::ICPState(icp_common_t &aHeader, const HttpRequestPointer &aRequest):
ICPState::ICPState(icp_common_t &aHeader, const HttpRequest::Pointer &aRequest):

I do not insist on these changes.

Footnotes

  1. Instead of a FooPointer forward declaration. ↩

header(aHeader),
request(aRequest),
fd(-1),
url(nullptr)
{
HTTPMSGLOCK(request);
}
{}

ICPState::~ICPState()
{
safe_free(url);
HTTPMSGUNLOCK(request);
}

bool
Expand Down Expand Up @@ -191,7 +188,7 @@ ICPState::loggingTags() const
void
ICPState::fillChecklist(ACLFilledChecklist &checklist) const
{
checklist.setRequest(request);
checklist.setRequest(request.getRaw());
icpSyncAle(al, from, url, 0, 0);
checklist.al = al;
}
Expand All @@ -205,7 +202,7 @@ class ICP2State: public ICPState
{

public:
ICP2State(icp_common_t & aHeader, HttpRequest *aRequest):
ICP2State(icp_common_t & aHeader, const HttpRequestPointer &aRequest):
ICPState(aHeader, aRequest),rtt(0),src_rtt(0),flags(0) {}

~ICP2State() override;
Expand Down Expand Up @@ -439,14 +436,14 @@ icpDenyAccess(Ip::Address &from, char *url, int reqnum, int fd)
}

bool
icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request)
icpAccessAllowed(Ip::Address &from, const HttpRequestPointer &icp_request)
{
if (!Config.accessList.icp) {
debugs(12, 2, "Access Denied due to lack of ICP access rules.");
return false;
}

ACLFilledChecklist checklist(Config.accessList.icp, icp_request, nullptr);
ACLFilledChecklist checklist(Config.accessList.icp, icp_request.getRaw(), nullptr);
checklist.src_addr = from;
checklist.my_addr.setNoAddr();
const auto &answer = checklist.fastCheck();
Expand All @@ -457,7 +454,7 @@ icpAccessAllowed(Ip::Address &from, HttpRequest * icp_request)
return false;
}

HttpRequest *
HttpRequestPointer
icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from)
{
if (strpbrk(url, w_space)) {
Expand All @@ -467,12 +464,11 @@ icpGetRequest(char *url, int reqnum, int fd, Ip::Address &from)
}

const auto mx = MasterXaction::MakePortless<XactionInitiator::initIcp>();
auto *result = HttpRequest::FromUrlXXX(url, mx);
HttpRequestPointer result = HttpRequest::FromUrlXXX(url, mx);
if (!result)
icpCreateAndSend(ICP_ERR, 0, url, reqnum, 0, fd, from, nullptr);

return result;

}

static void
Expand All @@ -483,16 +479,13 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header)
uint32_t flags = 0;
/* We have a valid packet */
char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t);
HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from);
const auto icp_request = icpGetRequest(url, header.reqnum, fd, from);

if (!icp_request)
return;

HTTPMSGLOCK(icp_request);

if (!icpAccessAllowed(from, icp_request)) {
icpDenyAccess(from, url, header.reqnum, fd);
HTTPMSGUNLOCK(icp_request);
return;
}
#if USE_ICMP
Expand Down Expand Up @@ -536,8 +529,6 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header)
}

icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, state.al);
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend removing "Maintenance: " from PR title because these changes might (positively) affect current or future code if, for example, icpCreateAndSend() throws. I do not insist on this change.


HTTPMSGUNLOCK(icp_request);
}

void
Expand Down
5 changes: 2 additions & 3 deletions src/icp_v3.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class ICP3State: public ICPState
{

public:
ICP3State(icp_common_t &aHeader, HttpRequest *aRequest) :
ICP3State(icp_common_t &aHeader, const HttpRequestPointer &aRequest) :
ICPState(aHeader, aRequest) {}

~ICP3State() override = default;
Expand All @@ -36,14 +36,13 @@ doV3Query(int fd, Ip::Address &from, char *buf, icp_common_t header)
{
/* We have a valid packet */
char *url = buf + sizeof(icp_common_t) + sizeof(uint32_t);
HttpRequest *icp_request = icpGetRequest(url, header.reqnum, fd, from);
const auto icp_request = icpGetRequest(url, header.reqnum, fd, from);

if (!icp_request)
return;

if (!icpAccessAllowed(from, icp_request)) {
icpDenyAccess (from, url, header.reqnum, fd);
delete icp_request;
return;
}

Expand Down
8 changes: 4 additions & 4 deletions src/refresh.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,13 +579,13 @@ refreshCheckHTTP(const StoreEntry * entry, HttpRequest * request)
}

/// \see int refreshCheckHTTP(const StoreEntry * entry, HttpRequest * request)
int
refreshCheckICP(const StoreEntry * entry, HttpRequest * request)
bool
refreshCheckICP(const StoreEntry *entry, const HttpRequestPointer &request)
{
int reason = refreshCheck(entry, request, 30);
int reason = refreshCheck(entry, request.getRaw(), 30);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int reason = refreshCheck(entry, request.getRaw(), 30);
const auto reason = refreshCheck(entry, request.getRaw(), 30);

++ refreshCounts[rcICP].total;
++ refreshCounts[rcICP].status[reason];
return (reason < 200) ? 0 : 1;
return !(reason < 200);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are going to make this condition look different from its duplicates (in refresh.cc context), then please simplify it:

Suggested change
return !(reason < 200);
return reason >= 200;

Alternatively, keep the original look-and-feel (until all such conditions are fixed in refresh.cc):

Suggested change
return !(reason < 200);
return (reason < 200) ? false : true;

}

#if USE_HTCP
Expand Down
2 changes: 1 addition & 1 deletion src/refresh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
void refreshAddToList(const char *, int, time_t, int, time_t);
bool refreshIsCachable(const StoreEntry *);
int refreshCheckHTTP(const StoreEntry *, HttpRequest *);
int refreshCheckICP(const StoreEntry *, HttpRequest *);
bool refreshCheckICP(const StoreEntry *, const HttpRequestPointer &);
int refreshCheckHTCP(const StoreEntry *, HttpRequest *);
int refreshCheckDigest(const StoreEntry *, time_t delta);
time_t getMaxAge(const char *url);
Expand Down
7 changes: 4 additions & 3 deletions src/tests/stub_icp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "squid.h"
#include "AccessLogEntry.h"
#include "comm/Connection.h"
#include "HttpRequest.h"
#include "ICP.h"

#define STUB_API "icp_*.cc"
Expand All @@ -19,7 +20,7 @@ icp_common_t::icp_common_t(char *, unsigned int) STUB
void icp_common_t::handleReply(char *, Ip::Address &) STUB
icp_common_t *icp_common_t::CreateMessage(icp_opcode, int, const char *, int, int) STUB_RETVAL(nullptr)
icp_opcode icp_common_t::getOpCode() const STUB_RETVAL(ICP_INVALID)
ICPState::ICPState(icp_common_t &, HttpRequest *) STUB
ICPState::ICPState(icp_common_t &, const HttpRequestPointer &) STUB
ICPState::~ICPState() STUB
bool ICPState::confirmAndPrepHit(const StoreEntry &) const STUB_RETVAL(false)
LogTags *ICPState::loggingTags() const STUB_RETVAL(nullptr)
Expand All @@ -29,8 +30,8 @@ Comm::ConnectionPointer icpIncomingConn;
Comm::ConnectionPointer icpOutgoingConn;
Ip::Address theIcpPublicHostID;

HttpRequest* icpGetRequest(char *, int, int, Ip::Address &) STUB_RETVAL(nullptr)
bool icpAccessAllowed(Ip::Address &, HttpRequest *) STUB_RETVAL(false)
HttpRequestPointer icpGetRequest(char *, int, int, Ip::Address &) STUB_RETVAL(nullptr)
bool icpAccessAllowed(Ip::Address &, const HttpRequestPointer &) STUB_RETVAL(false)
void icpCreateAndSend(icp_opcode, int, char const *, int, int, int, const Ip::Address &, AccessLogEntryPointer) STUB
icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID)
void icpDenyAccess(Ip::Address &, char *, int, int) STUB
Expand Down
Loading