From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 15 Jul 2021 05:58:19 +0000 (+0000)
Subject: Remove bogus "found KEY_PRIVATE" WARNINGs (#862)
X-Git-Tag: SQUID_6_0_1~311
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8a07d123ef7e986b601731fdaf4c93cc32471c73;p=thirdparty%2Fsquid.git

Remove bogus "found KEY_PRIVATE" WARNINGs (#862)

... triggered by private bumped StoreEntry unlock()ed in ~ServerBump().

The WARNINGs were added long time ago (commit fc8b9fc) because, AFAICT,
earlier Store code expected StoreEntry owners to release() uncachable
entries, including KEY_PRIVATE ones, right before unlocking them.
However, there is no compile-time enforcement of that expectation, and
unlocking code does not always know whether the entry is cachable (as
ServerBump constructor/destructor RAII code illustrates).

This change stops tying release and unlocking decisions/actions together
but makes sure that idle KEY_PRIVATE entries are still released (because
we do not want to index unneeded/unusable Store entries).
---

diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc
index 13056db48e..b18dce21f5 100644
--- a/src/clients/FtpRelay.cc
+++ b/src/clients/FtpRelay.cc
@@ -160,8 +160,8 @@ Ftp::Relay::Relay(FwdState *const fwdState):
     savedReply.lastReply = NULL;
     savedReply.replyCode = 0;
 
-    // Nothing we can do at request creation time can mark the response as
-    // uncachable, unfortunately. This prevents "found KEY_PRIVATE" WARNINGs.
+    // Prevent the future response from becoming public and being shared/cached
+    // because FTP does not support response cachability and freshness checks.
     entry->releaseRequest();
     AsyncCall::Pointer call = asyncCall(9, 4, "Ftp::Relay::Abort", cbdataDialer(&Relay::HandleStoreAbort, this));
     entry->registerAbortCallback(call);
diff --git a/src/store.cc b/src/store.cc
index 6cb14e4934..61266c2ee4 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -512,9 +512,6 @@ StoreEntry::doAbandon(const char *context)
         return;
     }
 
-    if (EBIT_TEST(flags, KEY_PRIVATE))
-        debugs(20, DBG_IMPORTANT, "WARNING: " << __FILE__ << ":" << __LINE__ << ": found KEY_PRIVATE");
-
     Store::Root().handleIdleEntry(*this); // may delete us
 }
 
diff --git a/src/store/Controller.cc b/src/store/Controller.cc
index f40eeab29a..03c2fc7680 100644
--- a/src/store/Controller.cc
+++ b/src/store/Controller.cc
@@ -276,6 +276,10 @@ Store::Controller::dereferenceIdle(StoreEntry &e, bool wantsLocalMemory)
     if (EBIT_TEST(e.flags, ENTRY_SPECIAL))
         return true;
 
+    // idle private entries cannot be reused
+    if (EBIT_TEST(e.flags, KEY_PRIVATE))
+        return false;
+
     bool keepInStoreTable = false; // keep only if somebody needs it there
 
     // Notify the fs that we are not referencing this object any more. This
@@ -703,6 +707,9 @@ Store::Controller::handleIdleEntry(StoreEntry &e)
 
     debugs(20, 5, HERE << "keepInLocalMemory: " << keepInLocalMemory);
 
+    // formerly known as "WARNING: found KEY_PRIVATE"
+    assert(!EBIT_TEST(e.flags, KEY_PRIVATE));
+
     // TODO: move this into [non-shared] memory cache class when we have one
     if (keepInLocalMemory) {
         e.setMemStatus(IN_MEMORY);