Merge AccessLogEntry 'helperNotes' and 'configNotes' members to 'notes' member

There is not any need to store notes added using Note cfg option and notes added
from helper to separated member. This patch merge them to the same 
AccessLogEntry::note member.
 
This is a Measurement Factory project

=== modified file 'src/AccessLogEntry.h'
--- src/AccessLogEntry.h	2013-04-29 13:31:05 +0000
+++ src/AccessLogEntry.h	2013-05-02 09:13:01 +0000
@@ -215,45 +215,43 @@
         char *last_meta;
     } adapt;
 #endif
 
     // Why is this a sub-class and not a set of real "private:" fields?
     // It looks like its duplicating HTTPRequestMethod anyway!
     // TODO: shuffle this to the relevant protocol section OR replace with request->method
     class Private
     {
 
     public:
         Private() : method_str(NULL) {}
 
         const char *method_str;
     } _private;
     HierarchyLogEntry hier;
     HttpReply *reply;
     HttpRequest *request; //< virgin HTTP request
     HttpRequest *adapted_request; //< HTTP request after adaptation and redirection
 
-    // TODO: merge configNotes and helperNotes
-    /// key:value pairs set by note.
-    NotePairs::Pointer configNotes;
+    /// key:value pairs set by squid.conf note directive and
     /// key=value pairs returned from URL rewrite/redirect helper
-    NotePairs::Pointer helperNotes;
+    NotePairs::Pointer notes;
 
 #if ICAP_CLIENT
     /** \brief This subclass holds log info for ICAP part of request
      *  \todo Inner class declarations should be moved outside
      */
     class IcapLogEntry
     {
     public:
         IcapLogEntry() : reqMethod(Adaptation::methodNone), bytesSent(0), bytesRead(0),
                 bodyBytesRead(-1), request(NULL), reply(NULL),
                 outcome(Adaptation::Icap::xoUnknown), trTime(0),
                 ioTime(0), resStatus(Http::scNone), processingTime(0) {}
 
         Ip::Address hostAddr; ///< ICAP server IP address
         String serviceName;        ///< ICAP service name
         String reqUri;             ///< ICAP Request-URI
         Adaptation::Icap::ICAP::Method reqMethod; ///< ICAP request method
         int64_t bytesSent;       ///< number of bytes sent to ICAP server so far
         int64_t bytesRead;       ///< number of bytes read from ICAP server so far
         /**

=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc	2013-04-29 13:31:05 +0000
+++ src/HttpRequest.cc	2013-04-30 10:44:03 +0000
@@ -41,49 +41,47 @@
 #include "globals.h"
 #include "gopher.h"
 #include "http.h"
 #include "HttpHdrCc.h"
 #include "HttpHeaderRange.h"
 #include "HttpRequest.h"
 #include "log/Config.h"
 #include "MemBuf.h"
 #include "SquidConfig.h"
 #include "Store.h"
 #include "URL.h"
 
 #if USE_AUTH
 #include "auth/UserRequest.h"
 #endif
 #if ICAP_CLIENT
 #include "adaptation/icap/icap_log.h"
 #endif
 
 HttpRequest::HttpRequest() :
-        HttpMsg(hoRequest),
-        helperNotes(NULL)
+        HttpMsg(hoRequest)
 {
     init();
 }
 
 HttpRequest::HttpRequest(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath) :
-        HttpMsg(hoRequest),
-        helperNotes(NULL)
+        HttpMsg(hoRequest)
 {
     static unsigned int id = 1;
     debugs(93,7, HERE << "constructed, this=" << this << " id=" << ++id);
     init();
     initHTTP(aMethod, aProtocol, aUrlpath);
 }
 
 HttpRequest::~HttpRequest()
 {
     clean();
     debugs(93,7, HERE << "destructed, this=" << this);
 }
 
 void
 HttpRequest::initHTTP(const HttpRequestMethod& aMethod, AnyP::ProtocolType aProtocol, const char *aUrlpath)
 {
     method = aMethod;
     protocol = aProtocol;
     urlpath = aUrlpath;
 }
@@ -151,41 +149,41 @@
     safe_free(canonical);
 
     safe_free(vary_headers);
 
     urlpath.clean();
 
     header.clean();
 
     if (cache_control) {
         delete cache_control;
         cache_control = NULL;
     }
 
     if (range) {
         delete range;
         range = NULL;
     }
 
     myportname.clean();
 
-    helperNotes = NULL;
+    notes = NULL;
 
     tag.clean();
 #if USE_AUTH
     extacl_user.clean();
     extacl_passwd.clean();
 #endif
     extacl_log.clean();
 
     extacl_message.clean();
 
 #if USE_ADAPTATION
     adaptHistory_ = NULL;
 #endif
 #if ICAP_CLIENT
     icapHistory_ = NULL;
 #endif
 }
 
 void
 HttpRequest::reset()
@@ -211,41 +209,40 @@
     copy->host_addr = host_addr;
 
     copy->port = port;
     // urlPath handled in ctor
     copy->canonical = canonical ? xstrdup(canonical) : NULL;
 
     // range handled in hdrCacheInit()
     copy->ims = ims;
     copy->imslen = imslen;
     copy->hier = hier; // Is it safe to copy? Should we?
 
     copy->errType = errType;
 
     // XXX: what to do with copy->peer_login?
 
     copy->lastmod = lastmod;
     copy->vary_headers = vary_headers ? xstrdup(vary_headers) : NULL;
     // XXX: what to do with copy->peer_domain?
 
     copy->myportname = myportname;
-    copy->helperNotes = helperNotes;
     copy->tag = tag;
 #if USE_AUTH
     copy->extacl_user = extacl_user;
     copy->extacl_passwd = extacl_passwd;
 #endif
     copy->extacl_log = extacl_log;
     copy->extacl_message = extacl_message;
 
     assert(copy->inheritProperties(this));
 
     return copy;
 }
 
 bool
 HttpRequest::inheritProperties(const HttpMsg *aMsg)
 {
     const HttpRequest* aReq = dynamic_cast<const HttpRequest*>(aMsg);
     if (!aReq)
         return false;
 
@@ -260,40 +257,41 @@
 #if USE_ADAPTATION
     adaptHistory_ = aReq->adaptHistory();
 #endif
 #if ICAP_CLIENT
     icapHistory_ = aReq->icapHistory();
 #endif
 
     // This may be too conservative for the 204 No Content case
     // may eventually need cloneNullAdaptationImmune() for that.
     flags = aReq->flags.cloneAdaptationImmune();
 
     errType = aReq->errType;
     errDetail = aReq->errDetail;
 #if USE_AUTH
     auth_user_request = aReq->auth_user_request;
 #endif
 
     // main property is which connection the request was received on (if any)
     clientConnectionManager = aReq->clientConnectionManager;
 
+    notes = aReq->notes;
     return true;
 }
 
 /**
  * Checks the first line of an HTTP request is valid
  * currently just checks the request method is present.
  *
  * NP: Other errors are left for detection later in the parse.
  */
 bool
 HttpRequest::sanityCheckStartLine(MemBuf *buf, const size_t hdr_len, Http::StatusCode *error)
 {
     // content is long enough to possibly hold a reply
     // 2 being magic size of a 1-byte request method plus space delimiter
     if ( buf->contentSize() < 2 ) {
         // this is ony a real error if the headers apparently complete.
         if (hdr_len > 0) {
             debugs(58, 3, HERE << "Too large request header (" << hdr_len << " bytes)");
             *error = Http::scInvalidHeader;
         }

=== modified file 'src/HttpRequest.h'
--- src/HttpRequest.h	2013-04-29 13:31:05 +0000
+++ src/HttpRequest.h	2013-05-02 09:13:54 +0000
@@ -186,41 +186,41 @@
 
     HierarchyLogEntry hier;
 
     int dnsWait; ///< sum of DNS lookup delays in milliseconds, for %dt
 
     err_type errType;
     int errDetail; ///< errType-specific detail about the transaction error
 
     char *peer_login;		/* Configured peer login:password */
 
     char *peer_host;           /* Selected peer host*/
 
     time_t lastmod;		/* Used on refreshes */
 
     const char *vary_headers;	/* Used when varying entities are detected. Changes how the store key is calculated */
 
     char *peer_domain;		/* Configured peer forceddomain */
 
     String myportname; // Internal tag name= value from port this requests arrived in.
 
-    NotePairs::Pointer helperNotes;         ///< collection of meta notes associated with this request by helper lookups.
+    NotePairs::Pointer notes; ///< annotations added by the note directive and helpers
 
     String tag;			/* Internal tag for this request */
 
     String extacl_user;		/* User name returned by extacl lookup */
 
     String extacl_passwd;	/* Password returned by extacl lookup */
 
     String extacl_log;		/* String to be used for access.log purposes */
 
     String extacl_message;	/* String to be used for error page purposes */
 
 #if FOLLOW_X_FORWARDED_FOR
     String x_forwarded_for_iterator; /* XXX a list of IP addresses */
 #endif /* FOLLOW_X_FORWARDED_FOR */
 
 public:
     bool multipartRangeRequest() const;
 
     bool parseFirstLine(const char *start, const char *end);
 

=== modified file 'src/Notes.cc'
--- src/Notes.cc	2013-04-29 13:31:05 +0000
+++ src/Notes.cc	2013-05-16 16:12:50 +0000
@@ -12,40 +12,41 @@
  *  sources; see the CREDITS file for full details.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
  *  the Free Software Foundation; either version 2 of the License, or
  *  (at your option) any later version.
  *
  *  This program is distributed in the hope that it will be useful,
  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
  *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  *  GNU General Public License for more details.
  *
  *  You should have received a copy of the GNU General Public License
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA.
  *
  */
 
 #include "squid.h"
 #include "globals.h"
+#include "AccessLogEntry.h"
 #include "acl/FilledChecklist.h"
 #include "acl/Gadgets.h"
 #include "ConfigParser.h"
 #include "HttpRequest.h"
 #include "HttpReply.h"
 #include "SquidConfig.h"
 #include "Store.h"
 #include "StrList.h"
 
 #include <algorithm>
 #include <string>
 
 Note::Value::~Value()
 {
     aclDestroyAclList(&aclList);
 }
 
 Note::Value::Pointer
 Note::addValue(const String &value)
 {
@@ -191,20 +192,33 @@
     }
 }
 
 bool
 NotePairs::hasPair(const char *key, const char *value) const
 {
     for (Vector<NotePairs::Entry *>::const_iterator  i = entries.begin(); i != entries.end(); ++i) {
         if ((*i)->name.cmp(key) == 0 || (*i)->value.cmp(value) == 0)
             return true;
     }
     return false;
 }
 
 void
 NotePairs::append(const NotePairs *src)
 {
     for (Vector<NotePairs::Entry *>::const_iterator  i = src->entries.begin(); i != src->entries.end(); ++i) {
         entries.push_back(new NotePairs::Entry((*i)->name.termedBuf(), (*i)->value.termedBuf()));
     }
 }
+
+
+NotePairs &
+SyncNotes(AccessLogEntry &ale, HttpRequest &request)
+{
+    if (!ale.notes) {
+        assert(!request.notes);
+        ale.notes = request.notes = new NotePairs;
+    } else {
+        assert(ale.notes == request.notes);
+    }
+    return *ale.notes;
+}

=== modified file 'src/Notes.h'
--- src/Notes.h	2013-04-29 13:31:05 +0000
+++ src/Notes.h	2013-05-16 16:12:23 +0000
@@ -157,21 +157,27 @@
      * Return true if the key/value pair is already stored
      */
     bool hasPair(const char *key, const char *value) const;
 
     /**
      * Convert NotePairs list to a string consist of "Key: Value"
      * entries separated by sep string.
      */
     const char *toString(const char *sep = "\r\n") const;
 
     /**
      * True if there are not entries in the list
      */
     bool empty() const {return entries.empty();}
 
     Vector<NotePairs::Entry *> entries;	  ///< The key/value pair entries
 };
 
 MEMPROXY_CLASS_INLINE(NotePairs::Entry);
 
+class AccessLogEntry;
+/**
+ * Keep in sync HttpRequest and the corresponding AccessLogEntry objects
+ */
+NotePairs &SyncNotes(AccessLogEntry &ale, HttpRequest &request);
+
 #endif

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2013-04-29 13:31:05 +0000
+++ src/client_side.cc	2013-05-16 16:27:34 +0000
@@ -676,47 +676,49 @@
     al->cache.code = logType;
 
     al->cache.msec = tvSubMsec(start_time, current_time);
 
     if (request)
         prepareLogWithRequestDetails(request, al);
 
     if (getConn() != NULL && getConn()->clientConnection != NULL && getConn()->clientConnection->rfc931[0])
         al->cache.rfc931 = getConn()->clientConnection->rfc931;
 
 #if USE_SSL && 0
 
     /* This is broken. Fails if the connection has been closed. Needs
      * to snarf the ssl details some place earlier..
      */
     if (getConn() != NULL)
         al->cache.ssluser = sslGetUserEmail(fd_table[getConn()->fd].ssl);
 
 #endif
 
-    /*Add meta headers*/
+    /*Add notes*/
+    // The al->notes and request->notes must point to the same object.
+    // Enable the following assertion to check for possible bugs.
+    // assert(request->notes == al->notes);
     typedef Notes::iterator ACAMLI;
     for (ACAMLI i = Config.notes.begin(); i != Config.notes.end(); ++i) {
         if (const char *value = (*i)->match(request, al->reply)) {
-            if (al->configNotes == NULL)
-                al->configNotes = new NotePairs;
-            al->configNotes->add((*i)->key.termedBuf(), value);
+            NotePairs &notes = SyncNotes(*al, *request);
+            notes.add((*i)->key.termedBuf(), value);
             debugs(33, 3, HERE << (*i)->key.termedBuf() << " " << value);
         }
     }
 
     ACLFilledChecklist *checklist = clientAclChecklistCreate(Config.accessList.log, this);
 
     if (al->reply) {
         checklist->reply = al->reply;
         HTTPMSGLOCK(checklist->reply);
     }
 
     if (!Config.accessList.log || checklist->fastCheck() == ACCESS_ALLOWED) {
         if (request) {
             al->adapted_request = request;
             HTTPMSGLOCK(al->adapted_request);
         }
         accessLogLog(al, checklist);
         if (request)
             updateCounters();
 

=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc	2013-04-29 13:31:05 +0000
+++ src/client_side_request.cc	2013-05-16 16:27:21 +0000
@@ -1242,44 +1242,42 @@
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *)data;
 
     if (!calloutContext->httpStateIsValid())
         return;
 
     calloutContext->clientStoreIdDone(result);
 }
 
 void
 ClientRequestContext::clientRedirectDone(const HelperReply &reply)
 {
     HttpRequest *old_request = http->request;
     debugs(85, 5, HERE << "'" << http->uri << "' result=" << reply);
     assert(redirect_state == REDIRECT_PENDING);
     redirect_state = REDIRECT_DONE;
 
     // Put helper response Notes into the transaction state record (ALE) eventually
     // do it early to ensure that no matter what the outcome the notes are present.
     if (http->al != NULL) {
-        if (!http->al->helperNotes)
-            http->al->helperNotes = new NotePairs;
-        http->al->helperNotes->append(&reply.notes);
-        old_request->helperNotes = http->al->helperNotes;
+        NotePairs &notes = SyncNotes(*http->al, *old_request);
+        notes.append(&reply.notes);
     }
 
     switch (reply.result) {
     case HelperReply::Unknown:
     case HelperReply::TT:
         // Handler in redirect.cc should have already mapped Unknown
         // IF it contained valid entry for the old URL-rewrite helper protocol
         debugs(85, DBG_IMPORTANT, "ERROR: URL rewrite helper returned invalid result code. Wrong helper? " << reply);
         break;
 
     case HelperReply::BrokenHelper:
         debugs(85, DBG_IMPORTANT, "ERROR: URL rewrite helper: " << reply << ", attempt #" << (redirect_fail_count+1) << " of 2");
         if (redirect_fail_count < 2) { // XXX: make this configurable ?
             ++redirect_fail_count;
             // reset state flag to try redirector again from scratch.
             redirect_done = false;
         }
         break;
 
     case HelperReply::Error:
@@ -1365,44 +1363,42 @@
 
     assert(http->uri);
 
     http->doCallouts();
 }
 
 /**
  * This method handles the different replies from StoreID helper.
  */
 void
 ClientRequestContext::clientStoreIdDone(const HelperReply &reply)
 {
     HttpRequest *old_request = http->request;
     debugs(85, 5, "'" << http->uri << "' result=" << reply);
     assert(store_id_state == REDIRECT_PENDING);
     store_id_state = REDIRECT_DONE;
 
     // Put helper response Notes into the transaction state record (ALE) eventually
     // do it early to ensure that no matter what the outcome the notes are present.
     if (http->al != NULL) {
-        if (!http->al->helperNotes)
-            http->al->helperNotes = new NotePairs;
-        http->al->helperNotes->append(&reply.notes);
-        old_request->helperNotes = http->al->helperNotes;
+        NotePairs &notes = SyncNotes(*http->al, *old_request);
+        notes.append(&reply.notes);
     }
 
     switch (reply.result) {
     case HelperReply::Unknown:
     case HelperReply::TT:
         // Handler in redirect.cc should have already mapped Unknown
         // IF it contained valid entry for the old helper protocol
         debugs(85, DBG_IMPORTANT, "ERROR: storeID helper returned invalid result code. Wrong helper? " << reply);
         break;
 
     case HelperReply::BrokenHelper:
         debugs(85, DBG_IMPORTANT, "ERROR: storeID helper: " << reply << ", attempt #" << (store_id_fail_count+1) << " of 2");
         if (store_id_fail_count < 2) { // XXX: make this configurable ?
             ++store_id_fail_count;
             // reset state flag to try StoreID again from scratch.
             store_id_done = false;
         }
         break;
 
     case HelperReply::Error:

=== modified file 'src/format/Format.cc'
--- src/format/Format.cc	2013-04-29 13:31:05 +0000
+++ src/format/Format.cc	2013-04-30 15:57:15 +0000
@@ -1032,66 +1032,58 @@
             break;
 
         case LFT_SSL_USER_CERT_ISSUER:
             if (X509 *cert = al->cache.sslClientCert.get()) {
                 if (X509_NAME *issuer = X509_get_issuer_name(cert)) {
                     X509_NAME_oneline(issuer, tmp, sizeof(tmp));
                     out = tmp;
                 }
             }
             break;
 #endif
         case LFT_NOTE:
             if (fmt->data.string) {
 #if USE_ADAPTATION
                 Adaptation::History::Pointer ah = al->request->adaptHistory();
                 if (ah != NULL && ah->metaHeaders != NULL) {
                     if (const char *meta = ah->metaHeaders->find(fmt->data.string))
                         sb.append(meta);
                 }
 #endif
-                if (al->helperNotes != NULL) {
-                    if (const char *note = al->helperNotes->find(fmt->data.string)) {
-                        if (sb.size())
-                            sb.append(", ");
-                        sb.append(note);
-                    }
-                }
-                if (al->configNotes != NULL) {
-                    if (const char *note = al->configNotes->find(fmt->data.string)) {
+                if (al->notes != NULL) {
+                    if (const char *note = al->notes->find(fmt->data.string)) {
                         if (sb.size())
                             sb.append(", ");
                         sb.append(note);
                     }
                 }
                 out = sb.termedBuf();
                 quote = 1;
             } else {
 #if USE_ADAPTATION
                 Adaptation::History::Pointer ah = al->request->adaptHistory();
                 if (ah != NULL && ah->metaHeaders != NULL && !ah->metaHeaders->empty())
                     sb.append(ah->metaHeaders->toString());
 #endif
-                if (al->helperNotes != NULL && !al->helperNotes->empty())
-                    sb.append(al->helperNotes->toString());
-                if (al->configNotes != NULL && !al->configNotes->empty())
-                    sb.append(al->configNotes->toString());
+                if (al->notes != NULL && !al->notes->empty())
+                    sb.append(al->notes->toString());
+
                 out = sb.termedBuf();
                 quote = 1;
             }
             break;
 
         case LFT_PERCENT:
             out = "%";
 
             break;
         }
 
         if (dooff) {
             snprintf(tmp, sizeof(tmp), "%0*" PRId64, fmt->zero && fmt->widthMin >= 0 ? fmt->widthMin : 0, outoff);
             out = tmp;
 
         } else if (doint) {
             snprintf(tmp, sizeof(tmp), "%0*ld", fmt->zero && fmt->widthMin >= 0 ? fmt->widthMin : 0, outint);
             out = tmp;
         }
 


