Added Comm close handler for the data channel of FtpStateData
transaction in preparation for officially dropping connect callbacks for
closing descriptors.
The data channel can be opened and closed a few times and the descriptor
must be kept in sync with the close handler. I factored out the
open/closing code into a simple FtpChannel class. That class is now used
for both FTP control and data channels.
Similar code appears to work in my tests and more tests will be done
tomorrow. The changes resolve one XXX discussion regarding FTP not
having a close handler for the data channel. On the other hand, adding a
second close handler attached to the same transaction is not a trivial
change as the side-effects of Squid cleanup code are often illusive.
For example, I suspect that FTP cleanup code does not close or even
check the control channel. I added a DBG_IMPORTANT statement to test
whether the control channel remains open. Or should that be an assert()?
I think that only one out of the two callbacks can be dialed because the
first close handler will invalidate the transaction object.
Thank you,
Alex.
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: [EMAIL PROTECTED]
# pfkm6wl99m441nxg
# target_branch: file:///home/rousskov/programs/bazaar/repos/squid\
# /trunk/
# testament_sha1: d5563ceef1d013380fc02cdc7f704ff302907793
# timestamp: 2008-09-21 23:55:08 -0600
# base_revision_id: [EMAIL PROTECTED]
# wydtvpav5cw32a8e
#
# Begin patch
=== modified file 'src/ftp.cc'
--- src/ftp.cc 2008-07-22 12:33:41 +0000
+++ src/ftp.cc 2008-09-22 05:52:37 +0000
@@ -122,6 +122,24 @@
/// \ingroup ServerProtocolFTPInternal
typedef void (FTPSM) (FtpStateData *);
+/// common code for FTP control and data channels
+// does not own the channel descriptor, which is managed by FtpStateData
+class FtpChannel {
+public:
+ FtpChannel(): fd(-1) {}
+
+ /// called after the socket is opened, sets up close handler
+ void opened(int aFd, const AsyncCall::Pointer &aCloser);
+
+ void close(); /// clears the close handler and calls comm_close
+ void clear(); /// just resets fd and close handler
+
+ int fd; /// channel descriptor; \todo: remove because the closer has it
+
+private:
+ AsyncCall::Pointer closer; /// Comm close handler callback
+};
+
/// \ingroup ServerProtocolFTPInternal
class FtpStateData : public ServerStateData
{
@@ -159,9 +177,10 @@
char *old_filepath;
char typecode;
- struct
+ // \todo: optimize ctrl and data structs member order, to minimize size
+ /// FTP control channel info; the channel is opened once per transaction
+ struct CtrlChannel: public FtpChannel
{
- int fd;
char *buf;
size_t size;
size_t offset;
@@ -171,9 +190,9 @@
int replycode;
} ctrl;
- struct
+ /// FTP data channel info; the channel may be opened/closed a few times
+ struct DataChannel: public FtpChannel
{
- int fd;
MemBuf *readBuf;
char *host;
u_short port;
@@ -183,7 +202,6 @@
struct _ftp_flags flags;
private:
- AsyncCall::Pointer closeHandler;
CBDATA_CLASS(FtpStateData);
public:
@@ -222,7 +240,8 @@
static CNCB ftpPasvCallback;
static PF ftpDataWrite;
void ftpTimeout(const CommTimeoutCbParams &io);
- void ftpSocketClosed(const CommCloseCbParams &io);
+ void ctrlClosed(const CommCloseCbParams &io);
+ void dataClosed(const CommCloseCbParams &io);
void ftpReadControlReply(const CommIoCbParams &io);
void ftpWriteCommandCallback(const CommIoCbParams &io);
void ftpAcceptDataConnection(const CommAcceptCbParams &io);
@@ -238,6 +257,7 @@
virtual bool doneWithServer() const;
virtual bool haveControlChannel(const char *caller_name) const;
+ AsyncCall::Pointer dataCloser(); /// creates a Comm close callback
private:
// BodyConsumer for HTTP: consume request body.
@@ -395,11 +415,21 @@
ftpReadMkdir /* SENT_MKDIR */
};
-void
-FtpStateData::ftpSocketClosed(const CommCloseCbParams &io)
-{
- ctrl.fd = -1;
- deleteThis("FtpStateData::ftpSocketClosed");
+/// handler called by Comm when FTP control channel is closed unexpectedly
+void
+FtpStateData::ctrlClosed(const CommCloseCbParams &io)
+{
+ ctrl.clear();
+ deleteThis("FtpStateData::ctrlClosed");
+}
+
+/// handler called by Comm when FTP data channel is closed unexpectedly
+void
+FtpStateData::dataClosed(const CommCloseCbParams &io)
+{
+ data.clear();
+ failed(ERR_FTP_FAILURE, 0); // or is it better to call abortTransaction()?
+ /* failed closes ctrl.fd and frees ftpState */
}
FtpStateData::FtpStateData(FwdState *theFwdState) : AsyncJob("FtpStateData"), ServerStateData(theFwdState)
@@ -408,8 +438,6 @@
debugs(9, 3, HERE << "'" << url << "'" );
statCounter.server.all.requests++;
statCounter.server.ftp.requests++;
- ctrl.fd = theFwdState->server_fd;
- data.fd = -1;
theSize = -1;
mdtm = -1;
@@ -419,9 +447,9 @@
flags.rest_supported = 1;
typedef CommCbMemFunT<FtpStateData, CommCloseCbParams> Dialer;
- closeHandler = asyncCall(9, 5, "FtpStateData::ftpSocketClosed",
- Dialer(this,&FtpStateData::ftpSocketClosed));
- comm_add_close_handler(ctrl.fd, closeHandler);
+ AsyncCall::Pointer closer = asyncCall(9, 5, "FtpStateData::ctrlClosed",
+ Dialer(this, &FtpStateData::ctrlClosed));
+ ctrl.opened(theFwdState->server_fd, closer);
if (request->method == METHOD_PUT)
flags.put = 1;
@@ -436,10 +464,11 @@
reply_hdr = NULL;
}
- if (data.fd > -1) {
- int fd = data.fd;
- data.fd = -1;
- comm_close(fd);
+ data.close();
+
+ if (ctrl.fd >= 0) {
+ debugs(9, DBG_IMPORTANT, HERE << "Internal bug: FtpStateData left " <<
+ "control FD " << ctrl.fd << " open");
}
if (ctrl.buf) {
@@ -1209,14 +1238,9 @@
debugs(9, 3,HERE);
/* Connection closed; transfer done. */
- if (data.fd > -1) {
- /**
- * Close data socket so it does not occupy resources while
- * we wait.
- */
- comm_close(data.fd);
- data.fd = -1;
- }
+
+ /// Close data channel, if any, to conserve resources while we wait.
+ data.close();
/* expect the "transfer complete" message on the control socket */
/*
@@ -2453,12 +2477,8 @@
return;
}
- /** \par
- * Closes any old FTP-Data connection which may exist. */
- if (ftpState->data.fd >= 0) {
- comm_close(ftpState->data.fd);
- ftpState->data.fd = -1;
- }
+ /// Closes any old FTP-Data connection which may exist. */
+ ftpState->data.close();
/** \par
* Checks for previous EPSV/PASV failures on this server/session.
@@ -2499,17 +2519,7 @@
return;
}
- /*
- * No comm_add_close_handler() here. If we have both ctrl and
- * data FD's call ftpSocketClosed() upon close, then we have
- * to delete the close handler which did NOT get called
- * to prevent ftpSocketClosed() getting called twice.
- * Instead we'll always call comm_close() on the ctrl FD.
- *
- * XXX this should not actually matter if the ftpState is cbdata
- * managed correctly and comm close handlers are cbdata fenced
- */
- ftpState->data.fd = fd;
+ ftpState->data.opened(fd, ftpState->dataCloser());
/** \par
* Send EPSV (ALL,2,1) or PASV on the control channel.
@@ -2715,15 +2725,9 @@
struct addrinfo *AI = NULL;
int on = 1;
int x = 0;
- /*
- * Tear down any old data connection if any. We are about to
- * establish a new one.
- */
- if (ftpState->data.fd > 0) {
- comm_close(ftpState->data.fd);
- ftpState->data.fd = -1;
- }
+ /// Close old data channel, if any. We may open a new one below.
+ ftpState->data.close();
/*
* Set up a listen socket on the same local address as the
@@ -2772,7 +2776,7 @@
return -1;
}
- ftpState->data.fd = fd;
+ ftpState->data.opened(fd, ftpState->dataCloser());
ftpState->data.port = comm_local_port(fd);
ftpState->data.host = NULL;
return fd;
@@ -2962,9 +2966,8 @@
/**\par
* Replace the Listen socket with the accepted data socket */
- comm_close(data.fd);
-
- data.fd = io.nfd;
+ data.close();
+ data.opened(io.nfd, dataCloser());
data.port = io.details.peer.GetPort();
io.details.peer.NtoA(data.host,SQUIDHOSTNAMELEN);
@@ -3830,16 +3833,10 @@
if (ctrl.fd > -1) {
fwd->unregister(ctrl.fd);
- comm_remove_close_handler(ctrl.fd, closeHandler);
- closeHandler = NULL;
- comm_close(ctrl.fd);
- ctrl.fd = -1;
+ ctrl.close();
}
- if (data.fd > -1) {
- comm_close(data.fd);
- data.fd = -1;
- }
+ data.close();
}
/**
@@ -3895,3 +3892,47 @@
fwd->handleUnregisteredServerEnd();
deleteThis("FtpStateData::abortTransaction");
}
+
+/// creates a data channel Comm close callback
+AsyncCall::Pointer
+FtpStateData::dataCloser()
+{
+ typedef CommCbMemFunT<FtpStateData, CommCloseCbParams> Dialer;
+ return asyncCall(9, 5, "FtpStateData::dataClosed",
+ Dialer(this, &FtpStateData::dataClosed));
+}
+
+/// configures the channel with a descriptor and registers a close handler
+void
+FtpChannel::opened(int aFd, const AsyncCall::Pointer &aCloser)
+{
+ assert(fd < 0);
+ assert(closer == NULL);
+
+ assert(aFd >= 0);
+ assert(aCloser != NULL);
+
+ fd = aFd;
+ closer = aCloser;
+ comm_add_close_handler(fd, closer);
+}
+
+/// planned close: removes the close handler and calls comm_close
+void
+FtpChannel::close()
+{
+ if (fd >= 0) {
+ comm_remove_close_handler(fd, closer);
+ closer = NULL;
+ comm_close(fd); // we do not expect to be called back
+ fd = -1;
+ }
+}
+
+/// just resets fd and close handler
+void
+FtpChannel::clear()
+{
+ fd = -1;
+ closer = NULL;
+}
# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWWgl90AABjrfgERQcXf////n
3oS////6YAxHw83Ac6DKF6xBpez0e2poBoBBKWzbANNSbQlD1NMMo2ppkZMTIAAGgyNGh6TAlCZA
ImKaTJB6IHqaHqGgANPU0ABkCUTVPT1QTIxpBkMjEGIDRoBoDINGIJCgSTTU00NqekG0Q9QAAAA0
AABxkyaGIxNGARgJhAGAmmjTI0AwkSEAmITAg1Mp6aMQ00jSeppskHqZGjTI1ECJPprFg+ywTESr
ViM8VoPk78gaG5CAP8mdj/rWsjGw2x+36ULYxlkV6UyNOkoSZVD2oWrtOcKSq/N/N2Fuq4s9Iyq5
7fCXRb4MkMyOIpAxSGOvwlLTL1T+q6zOvlxgC9vngV7F0sW5peO6j1yqmWa4KyKp6biVZSVdHvuO
F26hThph+g2i4nYzhuSEt4xISwSErN3n0aBsmcA/Y/M46eryM0vEPzOQ022IWFFom/9kLPsy5TKD
6PBA6GJxZWoVZrtJMVFaY4TMMyCjo8nHZ1ctMLNjEvSxM0WGZOzeUtiYldsvwQe6CrYqHuYwToSC
k1MXUw3WLwgWHAUyTefm1464JL+sdnj0z5NW28QekYz7Fk5aGvdboF4jgVGFGqakVirbbl7XohSB
40lIIJ8xXhpKqqSFmzA0rTm/9kv8WlR5is6aoXl0Izrj4s0IYsPfSx+ZhUmN23vV5pdh0K5m+TLS
xsbd/1V1PDFSyaRrpv8PrmpSm5YDZVUtIvPOpcVCfXeUMXL6yiby1roXKxClZzpj0ys5SFbbNY0a
hOISYqkx4PCKG2ri7OC+oZTeemcNyLBKWBcTS3TW3JesOPLjaz9kDL8fs91/I50kFCVhBScnk/Cy
Jew1YtdiRSngdRM+WLZAfe0lAbBi5jhpYbISgbMcZlIOKYVw66yYSZBzHwmhbi5UVx8ZeXepsZ0a
zIrFa1nWWxcOfnMHXuMzwOBydP0vA2DliasekWJ/HBIMzAW5L1d5xQiRMR5C9J5oKttfdBFmmWqv
jfaLZuBtXVoRJdBNR3nQAvkxNoPiw3NyTaZIG02MOfs5d3tES0bIiPgWB8GlY0iTBcWPYQl4FFUk
iqQtpJe0rU/EXAp2lFAI3Eu9yFooCPfgVJAwn7gx83I8ByMiH314RE4UwocbZ3isu87TS+sYguyI
+OnerGlBORUywNYQVZJ/pKBSouVKFJdDyLSF22TsHMzFRloaSsZeIvFWKxsyv1batspWjVNAQvUX
IAoookMiOV3KADkKI5cTBXUmSWMiKaCaDGlS4pUck6jpV3uOE1INCqJJxSZaqJdOkweikSY1rnRo
xgSRC5sJ2pTRzHN0RllQ4uRAnI2JTYYGEOpIMklGdddSM6Uk9BIMx5TVosqElcWUkqh1iH3iZRTg
dFZmRGUdCUFIkpAcTSOzzoPSKjRJkVisx9oGrB6LFUvjAiObYUL1gL06RQX2i2GorWeFZzngPsTd
PEJg0S3VckvTLJcSm7vM6+RPmVnI6ELmDGLsblS2mDHCEjBwZcmMdQlueHsFokJlIua66ltzl1jE
yz4ZicPpIcHMkzgobiQKM4lKiyUZUi1lhKQIbaNpPaJ798FLC7p2k8R3GlYmgsNLq2Z65e5SFkI1
zJbIyJF6QSNhaKZkRMWreKhcHIRUKzf9nn32jpfomBtQPgKQxNo2mSIxFuRRhmBV/TQgL+H2SBsk
/tabJThRmewPWul94qDJB8IEfgMJEL4/lJG8PMuDK4cxRSkHKoqg/Ac4XjpGHIA4qSIxyCRFlvoI
9f1NnNssLoZ8FBe0wbPn8q/UB76I3o7vVCvKIlMbYxiJAGLq1NCj+o/1WP6eZ+5QQy4ICpIkA35R
viBCC61HC5lKE3hD+D3TrkcPPcQPxSKGEJZjBFG4IxvInEQiUX1tIgdtzDkeBWCpUEBRXAhJywGg
V5CUFPD/HsqbDBpDbGxsaF8SQjxKB8xh2kF596sSXc0dpfJrxPrHIOd5r6CR3YVTDeS3rrjnuDb/
SBgpQOSYRXVDaFraNKA0d+JJUU1uoHUjWJRVWvaEkIrLqDD4dSQ02MQxDEYgfaxKs0VGWChe7eXi
N30Os8vH0HEWzma+hYfInxYZOZ/dc8sB7GisAOk6dpTcQGBLYW485awDGBEp+QnnMzye6bPcgWyC
G7boKd7cQtzQQ2NtCpRKU9JIeyC9WwnZYqswxoRWFBiKA9rS5I1G1GI2kidlkWOWhQ6+ZaUd6Xhz
8DVBtpaSzHIyVopSMjjGNKrEmFhqD+xsqkbitGrCoVCZbbYI8e/IFmfMaY71JkiDm/A5zb6C0yuO
my1HeGlAVGU7xrzUeXRgz2cfbhzbuA9uzDr1VLuiFPQ05Fb2eLW7KOIZRFOcD8tcDg/z3ntJnYXX
azI8Q+oV9YVlaGJama5GZWl4lUhWoRqNy249AOVxXKUIulFtNk7ZIKDCQmM8gtxNxrMrTQyD7gvl
chKhlBI+bNLAWYaGMFzTQia1iiEjfeZs1QNEJQnGEi2SJg0aIe2UmyhdKBhUAy6Q4L+gwr2cgwRN
WCx06C9ZVcRDAKcCo5ElmK4wM84INoxm855HkMyPeYe9CGdvt90epylMbj3s1MlzAzMNMqfke4qO
oU1ALPw9h5HmMpmDPI/UtBh8LQV0eY3eA2Da6YGcFpHvnjLDaSYxoZS/u4BEAhozt80EyZX1bxCl
2CP5Luq7l7CXLgegRu2CL0c5zHgFm4RlBgw51YlhstXglrgl1unf2mWoVku0gtOQTwuGDEtiKCMw
2+IB2efYIqObkai6+Bl1dZ0JeYtEpdCXAViSJdptbueGM5AwmxJjJiqFiKRMTUxcLFMKyWKUxTFC
w6RK8bZafE8VdRJHoHcjuaQ2hNDQNIYmk022FZePiQXEiI4bQLkI7hdgusUxWNGetEiE8Q/ZCJSQ
ims7zrICkyDBJEVKw9YUiL3oWRtWlU2V1SjbFvMC18B5UFaiWSTQEWwybQNJJ3yXbthLdHlQYAXB
qJUWEkMXHIxOiUSILwCsstekXNujnQqa6FjdsaDYI9IqpUOLCEaxJoM+pgix4iqAxRHIN+QZRA9z
kMkg0DkiHxbcSFwpJBvqunX+JchF3oSRpVls+e8bFQaEpcGaORyNQt6sD1hy1IVrA5CulU47aYzP
A0GgGY1c6KirbRCoxjSbTT1yKMGNsRGJtDSiNRyzvUuUwb6CKuf1GFRpGhESdqFOcl7NJkSNIxSM
7dCLcELQi+qQFVAOz9Tz5zQQDhMhOzO+AbBsNpMiQn0NQcymtrUixMGuXzXLVInh0dDuMesUmMFm
G8aOHc6GRA5qIgqiAbQE/lcmb6g2hjKhABEixJF4pV16zSpAwwAWAmXTHUlxLIwsQigicHUKWHRO
BcTApEywnxwlWaMbwYQjNbGk0mcMYOIKACwfwSIgoMSqXJia80aszcMHLU7HBPKj45A4nErKmB0q
vpKgwSqiohCGGIHzAaTEqzyol4tFQFi1EnJGhijMxI7FQOO8RkpzXpP40mnSjLme4aEcYypRJHX/
wjSIZRfQUIRreT5hRFQBwRsRgsRE1Q2rHNoX0FPwVRgJpHSG1GmHkaCqUyCaSP5ioQe0R9yzQTqs
1ivNYsw1/vJEP/4u5IpwoSDQS+6A
