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(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 #include 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::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::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 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 ¬es = 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 ¬es = 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 ¬es = 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; }