# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: rousskov@measurement-factory.com-20080922055237-\
#   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: rousskov@measurement-factory.com-20080920050047-\
#   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