Cleanup: remove goto from clientProcessRequest()

The code path can be better statically analysed without goto statements.

Bonus side effect is that several cases of checking for cleanup code can
be removed entirely.

diff --git a/src/client_side.cc b/src/client_side.cc
index b801ed6d06b57804b8642e343e67858c3b72442b..a138b4f0fb57aed66bb2c64d9d2c16f8330acc2c 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2524,6 +2524,23 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context)
 }
 #endif // USE_OPENSSL
 
+static void
+clientProcessRequestFinished(ConnStateData *conn, const HttpRequest::Pointer &request)
+{
+    /*
+     * DPW 2007-05-18
+     * Moved the TCP_RESET feature from clientReplyContext::sendMoreData
+     * to here because calling comm_reset_close() causes http to
+     * be freed and the above connNoteUseOfBuffer() would hit an
+     * assertion, not to mention that we were accessing freed memory.
+     */
+    if (request != NULL && request->flags.resetTcp && Comm::IsConnOpen(conn->clientConnection)) {
+        debugs(33, 3, HERE << "Sending TCP RST on " << conn->clientConnection);
+        conn->flags.readMore = false;
+        comm_reset_close(conn->clientConnection);
+    }
+}
+
 void
 clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *context, const HttpRequestMethod& method, Http::ProtocolVersion http_ver)
 {
@@ -2543,87 +2560,91 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
         assert(http->request);
         request = http->request;
         notedUseOfBuffer = true;
-        goto doFtpAndHttp;
-    }
+    } else {
 
-    if (context->flags.parsed_ok == 0) {
-        assert(hp);
-        clientStreamNode *node = context->getClientReplyContext();
-        debugs(33, 2, "clientProcessRequest: Invalid Request");
-        conn->quitAfterError(NULL);
-        // setLogUri should called before repContext->setReplyToError
-        setLogUri(http, http->uri,  true);
-        clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
-        assert (repContext);
-        switch (hp->request_parse_status) {
-        case Http::scHeaderTooLarge:
-            repContext->setReplyToError(ERR_TOO_BIG, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL);
-            break;
-        case Http::scMethodNotAllowed:
-            repContext->setReplyToError(ERR_UNSUP_REQ, Http::scMethodNotAllowed, method, http->uri,
-                                        conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL);
-            break;
-        default:
-            repContext->setReplyToError(ERR_INVALID_REQ, hp->request_parse_status, method, http->uri,
-                                        conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL);
+        if (context->flags.parsed_ok == 0) {
+            clientStreamNode *node = context->getClientReplyContext();
+            debugs(33, 2, "clientProcessRequest: Invalid Request");
+            conn->quitAfterError(NULL);
+            // setLogUri should called before repContext->setReplyToError
+            setLogUri(http, http->uri,  true);
+            clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
+            assert (repContext);
+            switch (hp->request_parse_status) {
+            case Http::scHeaderTooLarge:
+                repContext->setReplyToError(ERR_TOO_BIG, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL);
+                break;
+            case Http::scMethodNotAllowed:
+                repContext->setReplyToError(ERR_UNSUP_REQ, Http::scMethodNotAllowed, method, http->uri,
+                                            conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL);
+                break;
+            default:
+                repContext->setReplyToError(ERR_INVALID_REQ, hp->request_parse_status, method, http->uri,
+                                            conn->clientConnection->remote, NULL, conn->in.buf.c_str(), NULL);
+            }
+            assert(context->http->out.offset == 0);
+            context->pullData();
+            connNoteUseOfBuffer(conn, http->req_sz);
+            return;
         }
-        assert(context->http->out.offset == 0);
-        context->pullData();
-        goto finish;
-    }
 
-    if ((request = HttpRequest::CreateFromUrlAndMethod(http->uri, method)) == NULL) {
-        clientStreamNode *node = context->getClientReplyContext();
-        debugs(33, 5, "Invalid URL: " << http->uri);
-        conn->quitAfterError(request.getRaw());
-        // setLogUri should called before repContext->setReplyToError
-        setLogUri(http, http->uri,  true);
-        clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
-        assert (repContext);
-        repContext->setReplyToError(ERR_INVALID_URL, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL);
-        assert(context->http->out.offset == 0);
-        context->pullData();
-        goto finish;
-    }
+        if ((request = HttpRequest::CreateFromUrlAndMethod(http->uri, method)) == NULL) {
+            clientStreamNode *node = context->getClientReplyContext();
+            debugs(33, 5, "Invalid URL: " << http->uri);
+            conn->quitAfterError(request.getRaw());
+            // setLogUri should called before repContext->setReplyToError
+            setLogUri(http, http->uri,  true);
+            clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
+            assert (repContext);
+            repContext->setReplyToError(ERR_INVALID_URL, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL);
+            assert(context->http->out.offset == 0);
+            context->pullData();
+            connNoteUseOfBuffer(conn, http->req_sz);
+            return;
+        }
 
-    /* RFC 2616 section 10.5.6 : handle unsupported HTTP major versions cleanly. */
-    /* We currently only support 0.9, 1.0, 1.1 properly */
-    /* TODO: move HTTP-specific processing into servers/HttpServer and such */
-    if ( (http_ver.major == 0 && http_ver.minor != 9) ||
-            (http_ver.major > 1) ) {
+        /* RFC 2616 section 10.5.6 : handle unsupported HTTP major versions cleanly. */
+        /* We currently only support 0.9, 1.0, 1.1 properly */
+        /* TODO: move HTTP-specific processing into servers/HttpServer and such */
+        if ( (http_ver.major == 0 && http_ver.minor != 9) ||
+                (http_ver.major > 1) ) {
 
-        clientStreamNode *node = context->getClientReplyContext();
-        debugs(33, 5, "Unsupported HTTP version discovered. :\n" << HttpParserHdrBuf(hp));
-        conn->quitAfterError(request.getRaw());
-        // setLogUri should called before repContext->setReplyToError
-        setLogUri(http, http->uri,  true);
-        clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
-        assert (repContext);
-        repContext->setReplyToError(ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, method, http->uri,
-                                    conn->clientConnection->remote, NULL, HttpParserHdrBuf(hp), NULL);
-        assert(context->http->out.offset == 0);
-        context->pullData();
-        goto finish;
-    }
+            clientStreamNode *node = context->getClientReplyContext();
+            debugs(33, 5, "Unsupported HTTP version discovered. :\n" << HttpParserHdrBuf(hp));
+            conn->quitAfterError(request.getRaw());
+            // setLogUri should called before repContext->setReplyToError
+            setLogUri(http, http->uri,  true);
+            clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
+            assert (repContext);
+            repContext->setReplyToError(ERR_UNSUP_HTTPVERSION, Http::scHttpVersionNotSupported, method, http->uri,
+                                        conn->clientConnection->remote, NULL, HttpParserHdrBuf(hp), NULL);
+            assert(context->http->out.offset == 0);
+            context->pullData();
+            connNoteUseOfBuffer(conn, http->req_sz);
+            clientProcessRequestFinished(conn, request);
+            return;
+        }
 
-    /* compile headers */
-    /* we should skip request line! */
-    /* XXX should actually know the damned buffer size here */
-    if (http_ver.major >= 1 && !request->parseHeader(HttpParserHdrBuf(hp), HttpParserHdrSz(hp))) {
-        clientStreamNode *node = context->getClientReplyContext();
-        debugs(33, 5, "Failed to parse request headers:\n" << HttpParserHdrBuf(hp));
-        conn->quitAfterError(request.getRaw());
-        // setLogUri should called before repContext->setReplyToError
-        setLogUri(http, http->uri,  true);
-        clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
-        assert (repContext);
-        repContext->setReplyToError(ERR_INVALID_REQ, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL);
-        assert(context->http->out.offset == 0);
-        context->pullData();
-        goto finish;
+        /* compile headers */
+        /* we should skip request line! */
+        /* XXX should actually know the damned buffer size here */
+        if (http_ver.major >= 1 && !request->parseHeader(HttpParserHdrBuf(hp), HttpParserHdrSz(hp))) {
+            clientStreamNode *node = context->getClientReplyContext();
+            debugs(33, 5, "Failed to parse request headers:\n" << HttpParserHdrBuf(hp));
+            conn->quitAfterError(request.getRaw());
+            // setLogUri should called before repContext->setReplyToError
+            setLogUri(http, http->uri,  true);
+            clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
+            assert (repContext);
+            repContext->setReplyToError(ERR_INVALID_REQ, Http::scBadRequest, method, http->uri, conn->clientConnection->remote, NULL, NULL, NULL);
+            assert(context->http->out.offset == 0);
+            context->pullData();
+            connNoteUseOfBuffer(conn, http->req_sz);
+            clientProcessRequestFinished(conn, request);
+            return;
+        }
     }
 
-doFtpAndHttp:
     // Some blobs below are still HTTP-specific, but we would have to rewrite
     // this entire function to remove them from the FTP code path. Connection
     // setup and body_pipe preparation blobs are needed for FTP.
@@ -2715,7 +2736,9 @@ doFtpAndHttp:
                                     conn->clientConnection->remote, request.getRaw(), NULL, NULL);
         assert(context->http->out.offset == 0);
         context->pullData();
-        goto finish;
+        connNoteUseOfBuffer(conn, http->req_sz);
+        clientProcessRequestFinished(conn, request);
+        return;
     }
 
     if (!chunked && !clientIsContentLengthValid(request.getRaw())) {
@@ -2728,7 +2751,9 @@ doFtpAndHttp:
                                     conn->clientConnection->remote, request.getRaw(), NULL, NULL);
         assert(context->http->out.offset == 0);
         context->pullData();
-        goto finish;
+        connNoteUseOfBuffer(conn, http->req_sz);
+        clientProcessRequestFinished(conn, request);
+        return;
     }
 
     if (request->header.has(HDR_EXPECT)) {
@@ -2743,7 +2768,9 @@ doFtpAndHttp:
                                         conn->clientConnection->remote, request.getRaw(), NULL, NULL);
             assert(context->http->out.offset == 0);
             context->pullData();
-            goto finish;
+            connNoteUseOfBuffer(conn, http->req_sz);
+            clientProcessRequestFinished(conn, request);
+            return;
         }
     }
 
@@ -2764,8 +2791,12 @@ doFtpAndHttp:
     }
 
 #if USE_OPENSSL
-    if (conn->switchedToHttps() && conn->serveDelayedError(context))
-        goto finish;
+    if (conn->switchedToHttps() && conn->serveDelayedError(context)) {
+        if (!notedUseOfBuffer)
+            connNoteUseOfBuffer(conn, http->req_sz);
+        clientProcessRequestFinished(conn, request);
+        return;
+    }
 #endif
 
     /* Do we expect a request-body? */
@@ -2792,14 +2823,17 @@ doFtpAndHttp:
                                         conn->clientConnection->remote, http->request, NULL, NULL);
             assert(context->http->out.offset == 0);
             context->pullData();
-            goto finish;
+            clientProcessRequestFinished(conn, request);
+            return;
         }
 
         if (!isFtp) {
             // We may stop producing, comm_close, and/or call setReplyToError()
             // below, so quit on errors to avoid http->doCallouts()
-            if (!conn->handleRequestBodyData())
-                goto finish;
+            if (!conn->handleRequestBodyData()) {
+                clientProcessRequestFinished(conn, request);
+                return;
+            }
 
             if (!request->body_pipe->productionEnded()) {
                 debugs(33, 5, "need more request body");
@@ -2813,22 +2847,10 @@ doFtpAndHttp:
 
     http->doCallouts();
 
-finish:
     if (!notedUseOfBuffer)
         connNoteUseOfBuffer(conn, http->req_sz);
 
-    /*
-     * DPW 2007-05-18
-     * Moved the TCP_RESET feature from clientReplyContext::sendMoreData
-     * to here because calling comm_reset_close() causes http to
-     * be freed and the above connNoteUseOfBuffer() would hit an
-     * assertion, not to mention that we were accessing freed memory.
-     */
-    if (request != NULL && request->flags.resetTcp && Comm::IsConnOpen(conn->clientConnection)) {
-        debugs(33, 3, HERE << "Sending TCP RST on " << conn->clientConnection);
-        conn->flags.readMore = false;
-        comm_reset_close(conn->clientConnection);
-    }
+    clientProcessRequestFinished(conn, request);
 }
 
 static void