SourceLayout: convert helper stats display to Packable API

Requires unifying the classes Helper::Request queues which incidentally
also brings stateful helpers closer to concurrency support

diff --git a/src/auth/basic/Config.cc b/src/auth/basic/Config.cc
index ec43ab555f2f90346cb6e6e2f1fcd72f506e8095..04387ad786856bca225ece87db94a5394f1290b9 100644 (file)
--- a/src/auth/basic/Config.cc
+++ b/src/auth/basic/Config.cc
@@ -147,7 +147,8 @@ Auth::Basic::Config::parse(Auth::Config * scheme, int n_configured, char *param_
 static void
 authenticateBasicStats(StoreEntry * sentry)
 {
-    helperStats(sentry, basicauthenticators, "Basic Authenticator Statistics");
+    if (basicauthenticators)
+        basicauthenticators->packStatsInto(sentry, "Basic Authenticator Statistics");
 }
 
 char *

diff --git a/src/auth/digest/Config.cc b/src/auth/digest/Config.cc
index ef5b5b350f12ef1faa15eb865ce93856fd154827..49ca98201aa79085ceed130c9fa38675455434bd 100644 (file)
--- a/src/auth/digest/Config.cc
+++ b/src/auth/digest/Config.cc
@@ -643,7 +643,8 @@ Auth::Digest::Config::type() const
 static void
 authenticateDigestStats(StoreEntry * sentry)
 {
-    helperStats(sentry, digestauthenticators, "Digest Authenticator Statistics");
+    if (digestauthenticators)
+        digestauthenticators->packStatsInto(sentry, "Digest Authenticator Statistics");
 }
 
 /* NonceUserUnlink: remove the reference to auth_user and unlink the node from the list */

diff --git a/src/auth/negotiate/Config.cc b/src/auth/negotiate/Config.cc
index f3d8193a5dc48816736c8feb3e9b8e687b89300b..956be3995f6a987b25471df60b56e54ee7d1cdf1 100644 (file)
--- a/src/auth/negotiate/Config.cc
+++ b/src/auth/negotiate/Config.cc
@@ -244,7 +244,8 @@ Auth::Negotiate::Config::fixHeader(Auth::UserRequest::Pointer auth_user_request,
 static void
 authenticateNegotiateStats(StoreEntry * sentry)
 {
-    helperStatefulStats(sentry, negotiateauthenticators, "Negotiate Authenticator Statistics");
+    if (negotiateauthenticators)
+        negotiateauthenticators->packStatsInto(sentry, "Negotiate Authenticator Statistics");
 }
 
 /*

diff --git a/src/auth/ntlm/Config.cc b/src/auth/ntlm/Config.cc
index dbbd47927375944229f8cd7094a45da3775f8244..0f9f7f5bf2fa458d9da684ecd6870250f979f8a9 100644 (file)
--- a/src/auth/ntlm/Config.cc
+++ b/src/auth/ntlm/Config.cc
@@ -224,7 +224,8 @@ Auth::Ntlm::Config::fixHeader(Auth::UserRequest::Pointer auth_user_request, Http
 static void
 authenticateNTLMStats(StoreEntry * sentry)
 {
-    helperStatefulStats(sentry, ntlmauthenticators, "NTLM Authenticator Statistics");
+    if (ntlmauthenticators)
+        ntlmauthenticators->packStatsInto(sentry, "NTLM Authenticator Statistics");
 }
 
 /*

diff --git a/src/external_acl.cc b/src/external_acl.cc
index 6b2e06e67eb369d5f3db47a1ddfd5323279a7594..a2247db9ecad7b20ce0eb49cf0aa0bff5d5a6132 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -1464,7 +1464,8 @@ externalAclStats(StoreEntry * sentry)
     for (external_acl *p = Config.externalAclHelperList; p; p = p->next) {
         storeAppendPrintf(sentry, "External ACL Statistics: %s\n", p->name);
         storeAppendPrintf(sentry, "Cache size: %d\n", p->cache->count);
-        helperStats(sentry, p->theHelper);
+        assert(p->theHelper);
+        p->theHelper->packStatsInto(sentry);
         storeAppendPrintf(sentry, "\n");
     }
 }

diff --git a/src/helper.cc b/src/helper.cc
index 94bbd2143f50850d5f88cc73d6e158846b0530bd..47b1515e044662343128dc265003ab97e4ee782a 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "base/AsyncCbdataCalls.h"
+#include "base/Packable.h"
 #include "comm.h"
 #include "comm/Connection.h"
 #include "comm/Read.h"
@@ -63,7 +64,6 @@ static void helperKickQueue(helper * hlp);
 static void helperStatefulKickQueue(statefulhelper * hlp);
 static void helperStatefulServerDone(helper_stateful_server * srv);
 static void StatefulEnqueue(statefulhelper * hlp, Helper::Request * r);
-static bool helperStartStats(StoreEntry *sentry, void *hlp, const char *label);
 
 CBDATA_CLASS_INIT(helper);
 CBDATA_CLASS_INIT(helper_server);
@@ -471,7 +471,7 @@ void statefulhelper::submit(const char *buf, HLPCB * callback, void *data, helpe
     if ((buf != NULL) && lastserver) {
         debugs(84, 5, "StatefulSubmit with lastserver " << lastserver);
         assert(lastserver->flags.reserved);
-        assert(!(lastserver->request));
+        assert(!lastserver->requests.size());
 
         debugs(84, 5, "StatefulSubmit dispatching");
         helperStatefulDispatch(lastserver, r);
@@ -523,31 +523,21 @@ helperStatefulServerGetData(helper_stateful_server * srv)
     return srv->data;
 }
 
-/**
- * Dump some stats about the helper states to a StoreEntry
- */
 void
-helperStats(StoreEntry * sentry, helper * hlp, const char *label)
+helper::packStatsInto(Packable *p, const char *label) const
 {
-    if (!helperStartStats(sentry, hlp, label))
-        return;
-
-    storeAppendPrintf(sentry, "program: %s\n",
-                      hlp->cmdline->key);
-    storeAppendPrintf(sentry, "number active: %d of %d (%d shutting down)\n",
-                      hlp->childs.n_active, hlp->childs.n_max, (hlp->childs.n_running - hlp->childs.n_active) );
-    storeAppendPrintf(sentry, "requests sent: %d\n",
-                      hlp->stats.requests);
-    storeAppendPrintf(sentry, "replies received: %d\n",
-                      hlp->stats.replies);
-    storeAppendPrintf(sentry, "requests timedout: %d\n",
-                      hlp->stats.timedout);
-    storeAppendPrintf(sentry, "queue length: %d\n",
-                      hlp->stats.queue_size);
-    storeAppendPrintf(sentry, "avg service time: %d msec\n",
-                      hlp->stats.avg_svc_time);
-    storeAppendPrintf(sentry, "\n");
-    storeAppendPrintf(sentry, "%7s\t%7s\t%7s\t%11s\t%11s\t%11s\t%s\t%7s\t%7s\t%7s\n",
+    if (label)
+        p->appendf("%s:\n", label);
+
+    p->appendf("  program: %s\n", cmdline->key);
+    p->appendf("  number active: %d of %d (%d shutting down)\n", childs.n_active, childs.n_max, (childs.n_running - childs.n_active));
+    p->appendf("  requests sent: %d\n", stats.requests);
+    p->appendf("  replies received: %d\n", stats.replies);
+    p->appendf("  requests timedout: %d\n", stats.timedout);
+    p->appendf("  queue length: %d\n", stats.queue_size);
+    p->appendf("  avg service time: %d msec\n", stats.avg_svc_time);
+    p->append("\n",1);
+    p->appendf("%7s\t%7s\t%7s\t%11s\t%11s\t%11s\t%6s\t%7s\t%7s\t%7s\n",
                       "ID #",
                       "FD",
                       "PID",
@@ -559,11 +549,12 @@ helperStats(StoreEntry * sentry, helper * hlp, const char *label)
                       "Offset",
                       "Request");
 
-    for (dlink_node *link = hlp->servers.head; link; link = link->next) {
-        helper_server *srv = (helper_server*)link->data;
+    for (dlink_node *link = servers.head; link; link = link->next) {
+        HelperServerBase *srv = static_cast<HelperServerBase *>(link->data);
+        assert(srv);
         Helper::Request *request = srv->requests.empty() ? NULL : srv->requests.front();
         double tt = 0.001 * (request ? tvSubMsec(request->dispatch_time, current_time) : tvSubMsec(srv->dispatch_time, srv->answer_time));
-        storeAppendPrintf(sentry, "%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c\t%7.3f\t%7d\t%s\n",
+        p->appendf("%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c%c%c\t%7.3f\t%7d\t%s\n",
                           srv->index.value,
                           srv->readPipe->fd,
                           srv->pid,
@@ -573,74 +564,21 @@ helperStats(StoreEntry * sentry, helper * hlp, const char *label)
                           srv->stats.pending ? 'B' : ' ',
                           srv->flags.writing ? 'W' : ' ',
                           srv->flags.closing ? 'C' : ' ',
-                          srv->flags.shutdown ? 'S' : ' ',
-                          tt < 0.0 ? 0.0 : tt,
-                          (int) srv->roffset,
-                          request ? Format::QuoteMimeBlob(request->buf) : "(none)");
-    }
-
-    storeAppendPrintf(sentry, "\nFlags key:\n\n");
-    storeAppendPrintf(sentry, "   B = BUSY\n");
-    storeAppendPrintf(sentry, "   W = WRITING\n");
-    storeAppendPrintf(sentry, "   C = CLOSING\n");
-    storeAppendPrintf(sentry, "   S = SHUTDOWN PENDING\n");
-}
-
-void
-helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label)
-{
-    if (!helperStartStats(sentry, hlp, label))
-        return;
-
-    storeAppendPrintf(sentry, "program: %s\n",
-                      hlp->cmdline->key);
-    storeAppendPrintf(sentry, "number active: %d of %d (%d shutting down)\n",
-                      hlp->childs.n_active, hlp->childs.n_max, (hlp->childs.n_running - hlp->childs.n_active) );
-    storeAppendPrintf(sentry, "requests sent: %d\n",
-                      hlp->stats.requests);
-    storeAppendPrintf(sentry, "replies received: %d\n",
-                      hlp->stats.replies);
-    storeAppendPrintf(sentry, "queue length: %d\n",
-                      hlp->stats.queue_size);
-    storeAppendPrintf(sentry, "avg service time: %d msec\n",
-                      hlp->stats.avg_svc_time);
-    storeAppendPrintf(sentry, "\n");
-    storeAppendPrintf(sentry, "%7s\t%7s\t%7s\t%11s\t%11s\t%6s\t%7s\t%7s\t%7s\n",
-                      "ID #",
-                      "FD",
-                      "PID",
-                      "# Requests",
-                      "# Replies",
-                      "Flags",
-                      "Time",
-                      "Offset",
-                      "Request");
-
-    for (dlink_node *link = hlp->servers.head; link; link = link->next) {
-        helper_stateful_server *srv = (helper_stateful_server *)link->data;
-        double tt = 0.001 * tvSubMsec(srv->dispatch_time, srv->stats.pending ? current_time : srv->answer_time);
-        storeAppendPrintf(sentry, "%7u\t%7d\t%7d\t%11" PRIu64 "\t%11" PRIu64 "\t%c%c%c%c%c\t%7.3f\t%7d\t%s\n",
-                          srv->index.value,
-                          srv->readPipe->fd,
-                          srv->pid,
-                          srv->stats.uses,
-                          srv->stats.replies,
-                          srv->stats.pending ? 'B' : ' ',
-                          srv->flags.closing ? 'C' : ' ',
                           srv->flags.reserved ? 'R' : ' ',
                           srv->flags.shutdown ? 'S' : ' ',
-                          srv->request ? (srv->request->placeholder ? 'P' : ' ') : ' ',
+                          request && request->placeholder ? 'P' : ' ',
                           tt < 0.0 ? 0.0 : tt,
                           (int) srv->roffset,
-                          srv->request ? Format::QuoteMimeBlob(srv->request->buf) : "(none)");
+                          request ? Format::QuoteMimeBlob(request->buf) : "(none)");
     }
 
-    storeAppendPrintf(sentry, "\nFlags key:\n\n");
-    storeAppendPrintf(sentry, "   B = BUSY\n");
-    storeAppendPrintf(sentry, "   C = CLOSING\n");
-    storeAppendPrintf(sentry, "   R = RESERVED\n");
-    storeAppendPrintf(sentry, "   S = SHUTDOWN PENDING\n");
-    storeAppendPrintf(sentry, "   P = PLACEHOLDER\n");
+    p->append("\nFlags key:\n"
+              "   B\tBUSY\n"
+              "   W\tWRITING\n"
+              "   C\tCLOSING\n"
+              "   R\tRESERVED\n"
+              "   S\tSHUTDOWN PENDING\n"
+              "   P\tPLACEHOLDER\n", 101);
 }
 
 void
@@ -743,7 +681,6 @@ static void
 helperServerFree(helper_server *srv)
 {
     helper *hlp = srv->parent;
-    Helper::Request *r;
     int concurrency = hlp->childs.concurrency;
 
     if (!concurrency)
@@ -793,7 +730,7 @@ helperServerFree(helper_server *srv)
 
     while (!srv->requests.empty()) {
         // XXX: re-schedule these on another helper?
-        r = srv->requests.front();
+        Helper::Request *r = srv->requests.front();
         srv->requests.pop_front();
         void *cbdata;
 
@@ -814,7 +751,6 @@ static void
 helperStatefulServerFree(helper_stateful_server *srv)
 {
     statefulhelper *hlp = srv->parent;
-    Helper::Request *r;
 
     if (srv->rbuf) {
         memFreeBuf(srv->rbuf_sz, srv->rbuf);
@@ -857,18 +793,18 @@ helperStatefulServerFree(helper_stateful_server *srv)
         }
     }
 
-    if ((r = srv->request)) {
+    while (!srv->requests.empty()) {
+        // XXX: re-schedule these on another helper?
+        Helper::Request *r = srv->requests.front();
+        srv->requests.pop_front();
         void *cbdata;
 
         if (cbdataReferenceValidDone(r->data, &cbdata)) {
             Helper::Reply nilReply;
-            nilReply.whichServer = srv;
             r->callback(cbdata, nilReply);
         }
 
         delete r;
-
-        srv->request = NULL;
     }
 
     if (srv->data != NULL)
@@ -1051,7 +987,6 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
 {
     char *t = NULL;
     helper_stateful_server *srv = (helper_stateful_server *)data;
-    Helper::Request *r;
     statefulhelper *hlp = srv->parent;
     assert(cbdataReferenceValid(data));
 
@@ -1073,7 +1008,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
 
     srv->roffset += len;
     srv->rbuf[srv->roffset] = '\0';
-    r = srv->request;
+    Helper::Request *r = srv->requests.front();
     debugs(84, DBG_DATA, Raw("accumulated", srv->rbuf, srv->roffset));
 
     if (r == NULL) {
@@ -1087,6 +1022,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
 
     if ((t = strchr(srv->rbuf, hlp->eom))) {
         /* end of reply found */
+        srv->requests.pop_front(); // we already have it in 'r'
         int called = 1;
         int skip = 1;
         debugs(84, 3, "helperStatefulHandleRead: end of reply found");
@@ -1120,7 +1056,6 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
          */
         srv->roffset = 0;
         delete r;
-        srv->request = NULL;
 
         -- srv->stats.pending;
         ++ srv->stats.replies;
@@ -1437,14 +1372,14 @@ helperStatefulDispatch(helper_stateful_server * srv, Helper::Request * r)
         /* and push the queue. Note that the callback may have submitted a new
          * request to the helper which is why we test for the request */
 
-        if (srv->request == NULL)
+        if (!srv->requests.size())
             helperStatefulServerDone(srv);
 
         return;
     }
 
     srv->flags.reserved = true;
-    srv->request = r;
+    srv->requests.push_back(r);
     srv->dispatch_time = current_time;
     AsyncCall::Pointer call = commCbCall(5,5, "helperStatefulDispatchWriteDone",
                                          CommIoCbPtrFun(helperStatefulDispatchWriteDone, hlp));
@@ -1489,22 +1424,6 @@ helperStatefulServerDone(helper_stateful_server * srv)
     }
 }
 
-// TODO: should helper_ and helper_stateful_ have a common parent?
-static bool
-helperStartStats(StoreEntry *sentry, void *hlp, const char *label)
-{
-    if (!hlp) {
-        if (label)
-            storeAppendPrintf(sentry, "%s: unavailable\n", label);
-        return false;
-    }
-
-    if (label)
-        storeAppendPrintf(sentry, "%s:\n", label);
-
-    return true;
-}
-
 void
 helper_server::checkForTimedOutRequests(bool const retry)
 {

diff --git a/src/helper.h b/src/helper.h
index 4991811e1ecf30dd5bbc19b95988425ded28d7cb..2b3dcc95c1e66e9441cb774a633513f7501750aa 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -24,6 +24,8 @@
 #include <list>
 #include <map>
 
+class Packable;
+
 /**
  * Managers a set of individual helper processes with a common queue of requests.
  *
@@ -68,6 +70,10 @@ public:
     /// Submits a request to the helper or add it to the queue if none of
     /// the servers is available.
     void submitRequest(Helper::Request *r);
+
+    /// Dump some stats about the helper state to a Packable object
+    void packStatsInto(Packable *p, const char *label = NULL) const;
+
 public:
     wordlist *cmdline;
     dlink_list servers;
@@ -164,6 +170,9 @@ public:
         bool reserved;
     } flags;
 
+    typedef std::list<Helper::Request *> Requests;
+    Requests requests; ///< requests in order of submission/expiration
+
     struct {
         uint64_t uses;     //< requests sent to this helper
         uint64_t replies;  //< replies received from this helper
@@ -189,9 +198,6 @@ public:
 
     helper *parent;
 
-    typedef std::list<Helper::Request *> Requests;
-    Requests requests; ///< requests in order of submission/expiration
-
     // STL says storing std::list iterators is safe when changing the list
     typedef std::map<uint64_t, Requests::iterator> RequestIndex;
     RequestIndex requestsIndex; ///< maps request IDs to requests
@@ -213,7 +219,6 @@ public:
     /* MemBuf writebuf; */
 
     statefulhelper *parent;
-    Helper::Request *request;
 
     void *data;         /* State data used by the calling routines */
 };
@@ -223,8 +228,6 @@ void helperOpenServers(helper * hlp);
 void helperStatefulOpenServers(statefulhelper * hlp);
 void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data);
 void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver);
-void helperStats(StoreEntry * sentry, helper * hlp, const char *label = NULL);
-void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label = NULL);
 void helperShutdown(helper * hlp);
 void helperStatefulShutdown(statefulhelper * hlp);
 void helperStatefulReleaseServer(helper_stateful_server * srv);

diff --git a/src/redirect.cc b/src/redirect.cc
index 80beba17bcf4f78ed497c021449d7d0a77f8baf6..1fac3fc09ee584d30afd8b90e7a6f82c99e2e6b6 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -200,7 +200,7 @@ redirectStats(StoreEntry * sentry)
         return;
     }
 
-    helperStats(sentry, redirectors, "Redirector Statistics");
+    redirectors->packStatsInto(sentry, "Redirector Statistics");
 
     if (Config.onoff.redirector_bypass)
         storeAppendPrintf(sentry, "\nNumber of requests bypassed "
@@ -215,7 +215,7 @@ storeIdStats(StoreEntry * sentry)
         return;
     }
 
-    helperStats(sentry, storeIds, "StoreId helper Statistics");
+    storeIds->packStatsInto(sentry, "StoreId helper Statistics");
 
     if (Config.onoff.store_id_bypass)
         storeAppendPrintf(sentry, "\nNumber of requests bypassed "

diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc
index 56825f2635f0de0ef7ca237dae9313adfb6d5888..b6608ed702745e1f0280755330c46b431698d678 100644 (file)
--- a/src/tests/stub_helper.cc
+++ b/src/tests/stub_helper.cc
@@ -16,9 +16,8 @@ void helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data) S
 void helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPCB * callback, void *data, helper_stateful_server * lastserver) STUB
 helper::~helper() STUB
 CBDATA_CLASS_INIT(helper);
+void helper::packStatsInto(Packable *p, const char *label) const STUB
 
-void helperStats(StoreEntry * sentry, helper * hlp, const char *label) STUB
-void helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label) STUB
 void helperShutdown(helper * hlp) STUB
 void helperStatefulShutdown(statefulhelper * hlp) STUB
 void helperOpenServers(helper * hlp) STUB