[squid-dev] [PATCH] Bug 3803 : IDENT upgrade pt1
Amos Jeffries
squid3 at treenet.co.nz
Tue Sep 30 07:58:25 UTC 2014
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Begin the process of conversion for IdentStateData to an AsyncJob.
Referencing it as a 'job' below
* convert the object from CBDATA struct to a class with
CBDATA_CLASS2() API.
* Bug 3803 is caused by a lack of proper cleanup and consistent exit
actions terminating the job. Take the core logic changes from the
tested bug patch and;
1) define a swanSong() method to cleanup the memory allocated
2) define a deleteThis() method to emulate AsyncJob::deleteThis()
* Locate all code paths leveraging conn->close() to trigger cleanup
via the connection close handler and convert to explicit deleteThis()
with excuse. Including a few which were not but need to in order to
terminate the job correctly as fixed in bug 3803 patch.
The actions performed are nearly identical to the original code. The
differences are that many code paths now omit an AsyncCall step going
via the Comm close handler, and that all paths terminating the IDENT
lookup now go through swanSong() cleanup.
Further cleanup converting to a full AsyncJob is not included, since
there is an explicit hash of running IdentStateData object pointers
being used in the old code.
Amos
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (MingW32)
iQEcBAEBAgAGBQJUKmKfAAoJELJo5wb/XPRjp7MIAMyTlRRFS4Wc8juQkBEdFk2T
XJtxm9ADG27CzkRhlbxs3XHqx7t9691ndBRUH2akwFWL7PElGX9YUKpH5E4q+SZe
qka4f2wK+aL8+siX6uGWgkZQruUY75x/ZEcGyf9qBngt/zkDxfAULtTH4Jmp8pXl
8qo7ZgzccDfzXg7ojX3pHOeaaqZgpSzuPylw4f3h0x1Nr3o9sUb0UYCO8spxQpwz
Q6tnmpTa0qxHusdRdDf7WtJHB3QU+5fEskwpKoTVTL8KoofBv/1z5eW4DphjSFmI
e13AysTzXXKdMktyFBhyJPTRGRI8PjBrj+lSoA9rQLNXZz5BEhQOg6dIgT3d3z0=
=8iNi
-----END PGP SIGNATURE-----
-------------- next part --------------
=== modified file 'src/ident/Ident.cc'
--- src/ident/Ident.cc 2014-09-13 13:59:43 +0000
+++ src/ident/Ident.cc 2014-09-29 13:36:32 +0000
@@ -19,236 +19,259 @@
#include "CommCalls.h"
#include "globals.h"
#include "ident/Config.h"
#include "ident/Ident.h"
#include "MemBuf.h"
namespace Ident
{
#define IDENT_PORT 113
#define IDENT_KEY_SZ 50
#define IDENT_BUFSIZE 4096
typedef struct _IdentClient {
IDCB *callback;
void *callback_data;
struct _IdentClient *next;
} IdentClient;
-typedef struct _IdentStateData {
+class IdentStateData
+{
+public:
+ /* AsyncJob API emulated */
+ void deleteThis(const char *aReason);
+ void swanSong();
+
+ /// notify all waiting IdentClient callbacks
+ void notify(const char *result);
+
hash_link hash; /* must be first */
Comm::ConnectionPointer conn;
MemBuf queryMsg; ///< the lookup message sent to IDENT server
IdentClient *clients;
char buf[IDENT_BUFSIZE];
-} IdentStateData;
+
+private:
+ CBDATA_CLASS2(IdentStateData);
+};
+
+CBDATA_CLASS_INIT(IdentStateData);
// TODO: make these all a series of Async job calls. They are self-contained callbacks now.
static IOCB ReadReply;
static IOCB WriteFeedback;
static CLCB Close;
static CTCB Timeout;
static CNCB ConnectDone;
static hash_table *ident_hash = NULL;
static void ClientAdd(IdentStateData * state, IDCB * callback, void *callback_data);
-static void identCallback(IdentStateData * state, char *result);
} // namespace Ident
Ident::IdentConfig Ident::TheConfig;
-/**** PRIVATE FUNCTIONS ****/
+void
+Ident::IdentStateData::deleteThis(const char *aReason)
+{
+ swanSong();
+ delete this;
+}
void
-Ident::identCallback(IdentStateData * state, char *result)
+Ident::IdentStateData::swanSong()
{
- IdentClient *client;
+ if (clients != NULL)
+ notify(NULL);
+
+ if (!Comm::IsConnOpen(conn)) {
+ comm_remove_close_handler(conn->fd, Ident::Close, this);
+ conn->close();
+ }
- if (result && *result == '\0')
- result = NULL;
+ queryMsg.clean();
- while ((client = state->clients)) {
+ hash_remove_link(ident_hash, (hash_link *) this);
+ xfree(hash.key);
+}
+
+void
+Ident::IdentStateData::notify(const char *result)
+{
+ while (IdentClient *client = clients) {
void *cbdata;
- state->clients = client->next;
+ clients = client->next;
if (cbdataReferenceValidDone(client->callback_data, &cbdata))
client->callback(result, cbdata);
xfree(client);
}
}
void
Ident::Close(const CommCloseCbParams ¶ms)
{
IdentStateData *state = (IdentStateData *)params.data;
- identCallback(state, NULL);
- state->conn->close();
- hash_remove_link(ident_hash, (hash_link *) state);
- xfree(state->hash.key);
- cbdataFree(state);
+ state->deleteThis("connection closed");
}
void
Ident::Timeout(const CommTimeoutCbParams &io)
{
debugs(30, 3, HERE << io.conn);
- io.conn->close();
+ IdentStateData *state = (IdentStateData *)io.data;
+ state->deleteThis("timeout");
}
void
Ident::ConnectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, int xerrno, void *data)
{
IdentStateData *state = (IdentStateData *)data;
if (status != Comm::OK) {
if (status == Comm::TIMEOUT)
debugs(30, 3, "IDENT connection timeout to " << state->conn->remote);
- Ident::identCallback(state, NULL);
+ state->deleteThis(status == Comm::TIMEOUT ? "connect timeout" : "connect error");
return;
}
- assert(conn != NULL && conn == state->conn);
-
/*
* see if any of our clients still care
*/
IdentClient *c;
for (c = state->clients; c; c = c->next) {
if (cbdataReferenceValid(c->callback_data))
break;
}
if (c == NULL) {
- /* no clients care */
- conn->close();
+ state->deleteThis("client(s) aborted");
return;
}
+ assert(conn != NULL && conn == state->conn);
comm_add_close_handler(conn->fd, Ident::Close, state);
AsyncCall::Pointer writeCall = commCbCall(5,4, "Ident::WriteFeedback",
CommIoCbPtrFun(Ident::WriteFeedback, state));
Comm::Write(conn, &state->queryMsg, writeCall);
AsyncCall::Pointer readCall = commCbCall(5,4, "Ident::ReadReply",
CommIoCbPtrFun(Ident::ReadReply, state));
comm_read(conn, state->buf, IDENT_BUFSIZE, readCall);
AsyncCall::Pointer timeoutCall = commCbCall(5,4, "Ident::Timeout",
CommTimeoutCbPtrFun(Ident::Timeout, state));
commSetConnTimeout(conn, Ident::TheConfig.timeout, timeoutCall);
}
void
Ident::WriteFeedback(const Comm::ConnectionPointer &conn, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data)
{
debugs(30, 5, HERE << conn << ": Wrote IDENT request " << len << " bytes.");
// TODO handle write errors better. retry or abort?
if (flag != Comm::OK) {
debugs(30, 2, HERE << conn << " err-flags=" << flag << " IDENT write error: " << xstrerr(xerrno));
- conn->close();
+ IdentStateData *state = (IdentStateData *)data;
+ state->deleteThis("write error");
}
}
void
Ident::ReadReply(const Comm::ConnectionPointer &conn, char *buf, size_t len, Comm::Flag flag, int xerrno, void *data)
{
IdentStateData *state = (IdentStateData *)data;
char *ident = NULL;
char *t = NULL;
assert(buf == state->buf);
assert(conn->fd == state->conn->fd);
if (flag != Comm::OK || len <= 0) {
- state->conn->close();
+ state->deleteThis("read error");
return;
}
/*
* XXX This isn't really very tolerant. It should read until EOL
* or EOF and then decode the answer... If the reply is fragmented
* then this will fail
*/
buf[len] = '\0';
if ((t = strchr(buf, '\r')))
*t = '\0';
if ((t = strchr(buf, '\n')))
*t = '\0';
debugs(30, 5, HERE << conn << ": Read '" << buf << "'");
if (strstr(buf, "USERID")) {
if ((ident = strrchr(buf, ':'))) {
while (xisspace(*++ident));
- Ident::identCallback(state, ident);
+ if (ident && *ident == '\0')
+ ident = NULL;
+ state->notify(ident);
}
}
- state->conn->close();
+ state->deleteThis("completed");
}
void
Ident::ClientAdd(IdentStateData * state, IDCB * callback, void *callback_data)
{
IdentClient *c = (IdentClient *)xcalloc(1, sizeof(*c));
IdentClient **C;
c->callback = callback;
c->callback_data = cbdataReference(callback_data);
for (C = &state->clients; *C; C = &(*C)->next);
*C = c;
}
-CBDATA_TYPE(IdentStateData);
-
-/**** PUBLIC FUNCTIONS ****/
-
/*
* start a TCP connection to the peer host on port 113
*/
void
Ident::Start(const Comm::ConnectionPointer &conn, IDCB * callback, void *data)
{
IdentStateData *state;
char key1[IDENT_KEY_SZ];
char key2[IDENT_KEY_SZ];
char key[IDENT_KEY_SZ];
conn->local.toUrl(key1, IDENT_KEY_SZ);
conn->remote.toUrl(key2, IDENT_KEY_SZ);
snprintf(key, IDENT_KEY_SZ, "%s,%s", key1, key2);
if (!ident_hash) {
Init();
}
if ((state = (IdentStateData *)hash_lookup(ident_hash, key)) != NULL) {
ClientAdd(state, callback, data);
return;
}
- CBDATA_INIT_TYPE(IdentStateData);
- state = cbdataAlloc(IdentStateData);
+ state = new IdentStateData;
state->hash.key = xstrdup(key);
// copy the conn details. We dont want the original FD to be re-used by IDENT.
state->conn = conn->copyDetails();
// NP: use random port for secure outbound to IDENT_PORT
state->conn->local.port(0);
state->conn->remote.port(IDENT_PORT);
// build our query from the original connection details
state->queryMsg.init();
state->queryMsg.Printf("%d, %d\r\n", conn->remote.port(), conn->local.port());
ClientAdd(state, callback, data);
hash_join(ident_hash, &state->hash);
AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone", CommConnectCbPtrFun(Ident::ConnectDone, state));
AsyncJob::Start(new Comm::ConnOpener(state->conn, call, Ident::TheConfig.timeout));
}
void
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ident_mk1_s35.patch.sig
Type: application/octet-stream
Size: 287 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20140930/415c83fd/attachment.obj>
More information about the squid-dev
mailing list