[squid-dev] [PATCH] renaming StoreEntryStream to PackableStream

Amos Jeffries squid3 at treenet.co.nz
Fri Aug 21 10:38:31 UTC 2015


So PackableStream is really just a rename of StoreEntryStream BUT with
some implicit new properties from the underlying type change:

* lack of Store.h dependency

* ability to stream into a MemBuf if its creator desires that

This patch demonstrates that by doing the upgrade in-place and re-typing
all StoreEntryStream objects.

It includes some extra polishing of the *StreamBuf logics to remove
styling and redundant code issues identified in the previous cycles of
discussion. But otherwise is essentially a drop-in replacement.

Amos

-------------- next part --------------
=== modified file 'src/Makefile.am'
--- src/Makefile.am	2015-08-03 03:50:25 +0000
+++ src/Makefile.am	2015-08-19 15:28:32 +0000
@@ -504,41 +504,40 @@
 
 EXTRA_squid_SOURCES = \
 	$(all_AUTHMODULES) \
 	ConfigOption.h \
 	$(DELAY_POOL_ALL_SOURCE) \
 	htcp.cc \
 	htcp.h \
 	ipc.cc \
 	ipc_win32.cc \
 	ProfStats.cc \
 	LeakFinder.cc \
 	LeakFinder.h \
 	$(SNMP_ALL_SOURCE) \
 	$(UNLINKDSOURCE) \
 	$(WIN32_ALL_SOURCE) \
 	$(LOADABLE_MODULES_SOURCES)
 
 noinst_HEADERS = \
 	client_side_request.cci \
 	MemBuf.h \
-	StoreEntryStream.h \
 	String.cci \
 	SquidString.h \
 	SquidTime.h
 
 BUILT_SOURCES = \
 	cf_gen_defines.cci \
 	cf_parser.cci \
 	err_type.cc \
 	err_detail_type.cc \
 	globals.cc \
 	hier_code.cc \
 	icp_opcode.cc \
 	lookup_t.cc \
 	repl_modules.cc \
 	swap_log_op.cc
 
 CLEANFILES += $(BUILT_SOURCES)
 
 nodist_squid_SOURCES = \
 	$(BUILT_SOURCES)
@@ -2849,44 +2848,44 @@
 	tests/stub_libsslsquid.cc \
 	HttpBody.h \
 	HttpBody.cc \
 	tests/stub_HttpReply.cc \
 	tests/stub_HttpRequest.cc \
 	tests/stub_libcomm.cc \
 	tests/stub_MemStore.cc \
 	mime.h \
 	tests/stub_mime.cc \
 	tests/stub_Port.cc \
 	tests/stub_stat.cc \
 	tests/stub_store_client.cc \
 	tests/stub_store_stats.cc \
 	store_rebuild.h \
 	tests/stub_store_rebuild.cc \
 	tests/stub_store_swapout.cc \
 	tools.h \
 	Transients.cc \
 	tests/stub_tools.cc \
 	tests/stub_UdsOp.cc \
+	tests/testPackableStream.cc \
+	tests/testPackableStream.h \
 	tests/testStore.cc \
 	tests/testStore.h \
-	tests/testStoreEntryStream.cc \
-	tests/testStoreEntryStream.h \
 	tests/testStoreController.cc \
 	tests/testStoreController.h \
 	tests/testStoreHashIndex.cc \
 	tests/testStoreHashIndex.h \
 	tests/testStoreSupport.cc \
 	tests/testStoreSupport.h \
 	tests/TestSwapDir.cc \
 	tests/TestSwapDir.h \
 	tests/stub_time.cc \
 	url.cc \
 	wordlist.h \
 	wordlist.cc
 
 nodist_tests_testStore_SOURCES= \
 	$(TESTSOURCES) \
 	SquidMath.cc \
 	SquidMath.h \
 	swap_log_op.cc
 
 tests_testStore_LDADD= \

=== modified file 'src/SBufStatsAction.cc'
--- src/SBufStatsAction.cc	2015-03-19 12:12:08 +0000
+++ src/SBufStatsAction.cc	2015-08-19 15:16:42 +0000
@@ -1,76 +1,76 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 #include "squid.h"
+#include "base/PackableStream.h"
 #include "ipc/Messages.h"
 #include "ipc/TypedMsgHdr.h"
 #include "mgr/Registration.h"
 #include "SBufDetailedStats.h"
 #include "SBufStatsAction.h"
-#include "StoreEntryStream.h"
 
 SBufStatsAction::SBufStatsAction(const Mgr::CommandPointer &cmd_):
     Action(cmd_)
 { } //default constructor is OK for data member
 
 SBufStatsAction::Pointer
 SBufStatsAction::Create(const Mgr::CommandPointer &cmd)
 {
     return new SBufStatsAction(cmd);
 }
 
 void
 SBufStatsAction::add(const Mgr::Action& action)
 {
     sbdata += dynamic_cast<const SBufStatsAction&>(action).sbdata;
     mbdata += dynamic_cast<const SBufStatsAction&>(action).mbdata;
     sbsizesatdestruct += dynamic_cast<const SBufStatsAction&>(action).sbsizesatdestruct;
     mbsizesatdestruct += dynamic_cast<const SBufStatsAction&>(action).mbsizesatdestruct;
 }
 
 void
 SBufStatsAction::collect()
 {
     sbdata = SBuf::GetStats();
     mbdata = MemBlob::GetStats();
     sbsizesatdestruct = *collectSBufDestructTimeStats();
     mbsizesatdestruct = *collectMemBlobDestructTimeStats();
 }
 
 static void
 statHistSBufDumper(StoreEntry * sentry, int, double val, double size, int count)
 {
     if (count == 0)
         return;
     storeAppendPrintf(sentry, "\t%d-%d\t%d\n", static_cast<int>(val), static_cast<int>(val+size), count);
 }
 
 void
 SBufStatsAction::dump(StoreEntry* entry)
 {
-    StoreEntryStream ses(entry);
+    PackableStream ses(*entry);
     ses << "\n\n\nThese statistics are experimental; their format and contents "
         "should not be relied upon, they are bound to change as "
         "the SBuf feature is evolved\n";
     sbdata.dump(ses);
     mbdata.dump(ses);
     ses << "\n";
     ses << "SBuf size distribution at destruct time:\n";
     sbsizesatdestruct.dump(entry,statHistSBufDumper);
     ses << "MemBlob capacity distribution at destruct time:\n";
     mbsizesatdestruct.dump(entry,statHistSBufDumper);
 }
 
 void
 SBufStatsAction::pack(Ipc::TypedMsgHdr& msg) const
 {
     msg.setType(Ipc::mtCacheMgrResponse);
     msg.putPod(sbdata);
     msg.putPod(mbdata);
 }
 

=== modified file 'src/base/Makefile.am'
--- src/base/Makefile.am	2015-08-03 02:08:22 +0000
+++ src/base/Makefile.am	2015-08-19 15:08:32 +0000
@@ -11,28 +11,29 @@
 noinst_LTLIBRARIES = libbase.la
 
 libbase_la_SOURCES = \
 	AsyncCall.cc \
 	AsyncCall.h \
 	AsyncCbdataCalls.h \
 	AsyncJob.h \
 	AsyncJob.cc \
 	AsyncJobCalls.h \
 	AsyncCallQueue.cc \
 	AsyncCallQueue.h \
 	CbcPointer.h \
 	CbDataList.h \
 	CharacterSet.h \
 	CharacterSet.cc \
 	InstanceId.h \
 	Lock.h \
 	LookupTable.h \
 	LruMap.h \
 	Packable.h \
+	PackableStream.h \
 	RegexPattern.cc \
 	RegexPattern.h \
 	RunnersRegistry.cc \
 	RunnersRegistry.h \
 	Subscription.h \
 	TextException.cc \
 	TextException.h \
 	TidyPointer.h

=== renamed file 'src/StoreEntryStream.h' => 'src/base/PackableStream.h'
--- src/StoreEntryStream.h	2015-01-13 07:25:36 +0000
+++ src/base/PackableStream.h	2015-08-20 08:18:33 +0000
@@ -1,102 +1,82 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
-#ifndef SQUID_STORE_ENTRY_STREAM_H
-#define SQUID_STORE_ENTRY_STREAM_H
+#ifndef SQUID_SRC_BASE_PACKABLESTREAM_H
+#define SQUID_SRC_BASE_PACKABLESTREAM_H
 
-#include "Store.h"
+#include "base/Packable.h"
 
 #include <ostream>
 
-/*
- * This class provides a streambuf interface for writing
- * to StoreEntries. Typical use is via a StoreEntryStream
- * rather than direct manipulation
+/**
+ * Provides a streambuf interface for writing to Packable objects.
+ * Typical use is via a PackableStream rather than direct manipulation
  */
-
-class StoreEntryStreamBuf : public std::streambuf
+class PackableStreamBuf : public std::streambuf
 {
-
 public:
-    StoreEntryStreamBuf(StoreEntry *anEntry) : theEntry(anEntry) {
-        theEntry->lock("StoreEntryStreamBuf");
-        theEntry->buffer();
-    }
-
-    ~StoreEntryStreamBuf() {
-        theEntry->unlock("StoreEntryStreamBuf");
-    }
+    PackableStreamBuf(Packable &p) : buf_(p) {}
+    ~PackableStreamBuf() = default;
 
 protected:
-    /* flush the current buffer and the character that is overflowing
+    /** flush the current buffer and the character that is overflowing
      * to the store entry.
      */
-    virtual int_type overflow(int_type aChar = traits_type::eof()) {
+    virtual int_type overflow(int_type aChar = traits_type::eof()) override {
         std::streamsize pending(pptr() - pbase());
 
-        if (pending && sync ())
+        if (pending && sync())
             return traits_type::eof();
 
         if (aChar != traits_type::eof()) {
-            // NP: cast because GCC promotes int_type to 32-bit type
-            //     std::basic_streambuf<char>::int_type {aka int}
-            //     despite the definition with 8-bit type value.
-            char chars[1] = {char(aChar)};
-
-            if (aChar != traits_type::eof())
-                theEntry->append(chars, 1);
+            const char C = static_cast<char>(aChar);
+            lowAppend(&C, 1);
         }
 
-        pbump (-pending);  // Reset pptr().
+        pbump(-pending);  // Reset pptr().
         return aChar;
     }
 
-    /* push the buffer to the store */
-    virtual int sync() {
+    /** push the buffer to the store */
+    virtual int sync() override {
         std::streamsize pending(pptr() - pbase());
-
-        if (pending)
-            theEntry->append(pbase(), pending);
-
-        theEntry->flush();
-
+        lowAppend(pbase(), pending);
         return 0;
     }
 
-    /* write multiple characters to the store entry
+    /** write multiple characters to the store entry
      * - this is an optimisation method.
      */
-    virtual std::streamsize xsputn(const char * chars, std::streamsize number) {
-        if (number)
-            theEntry->append(chars, number);
-
+    virtual std::streamsize xsputn(const char * chars, std::streamsize number) override {
+        lowAppend(chars, number);
         return number;
     }
 
 private:
-    StoreEntry *theEntry;
+    void lowAppend(const char *s, const std::streamsize n) {buf_.append(s,n);}
 
+    Packable &buf_;
 };
 
-class StoreEntryStream : public std::ostream
+class PackableStream : public std::ostream
 {
 
 public:
     /* create a stream for writing text etc into theEntry */
     // See http://www.codecomments.com/archive292-2005-2-396222.html
-    StoreEntryStream(StoreEntry *entry): std::ostream(0), theBuffer(entry) {
+    PackableStream(Packable &p) : std::ostream(0), theBuffer(p) {
         rdbuf(&theBuffer); // set the buffer to now-initialized theBuffer
         clear(); //clear badbit set by calling init(0)
     }
 
 private:
-    StoreEntryStreamBuf theBuffer;
+    PackableStreamBuf theBuffer;
 };
 
-#endif /* SQUID_STORE_ENTRY_STREAM_H */
+#endif /* SQUID_SRC_BASE_PACKABLESTREAM_H */
 

=== modified file 'src/mem/old_api.cc'
--- src/mem/old_api.cc	2015-07-27 05:21:06 +0000
+++ src/mem/old_api.cc	2015-08-19 15:17:20 +0000
@@ -1,53 +1,52 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 /* DEBUG: section 13    High Level Memory Pool Management */
 
 #include "squid.h"
 #include "acl/AclDenyInfoList.h"
 #include "acl/AclNameList.h"
+#include "base/PackableStream.h"
 #include "CacheDigest.h"
 #include "ClientInfo.h"
 #include "disk.h"
 #include "dlink.h"
 #include "event.h"
 #include "icmp/net_db.h"
 #include "md5.h"
 #include "mem/forward.h"
 #include "mem/Pool.h"
 #include "MemBuf.h"
 #include "memMeter.h"
 #include "mgr/Registration.h"
 #include "SquidConfig.h"
 #include "SquidList.h"
 #include "SquidTime.h"
 #include "Store.h"
-#include "StoreEntryStream.h"
 
 #include <iomanip>
-#include <ostream>
 
 /* forward declarations */
 static void memFree2K(void *);
 static void memFree4K(void *);
 static void memFree8K(void *);
 static void memFree16K(void *);
 static void memFree32K(void *);
 static void memFree64K(void *);
 
 /* module globals */
 const size_t squidSystemPageSize=getpagesize();
 
 /* local prototypes */
 static void memStringStats(std::ostream &);
 
 /* module locals */
 static MemAllocator *MemPools[MEM_MAX];
 static double xm_time = 0;
 static double xm_deltat = 0;
 
@@ -126,41 +125,41 @@
     stream << std::setw(20) << std::left << "Other Strings";
 
     stream << std::right << "\t ";
 
     stream << xpercentInt(StrCountMeter.level - pooled_count, StrCountMeter.level) << "\t ";
 
     stream << xpercentInt(StrVolumeMeter.level - pooled_volume, StrVolumeMeter.level) << "\n\n";
 }
 
 static void
 memBufStats(std::ostream & stream)
 {
     stream << "Large buffers: " <<
            HugeBufCountMeter.level << " (" <<
            HugeBufVolumeMeter.level / 1024 << " KB)\n";
 }
 
 void
 Mem::Stats(StoreEntry * sentry)
 {
-    StoreEntryStream stream(sentry);
+    PackableStream stream(*sentry);
     Report(stream);
     memStringStats(stream);
     memBufStats(stream);
 #if WITH_VALGRIND
     if (RUNNING_ON_VALGRIND) {
         long int leaked = 0, dubious = 0, reachable = 0, suppressed = 0;
         stream << "Valgrind Report:\n";
         stream << "Type\tAmount\n";
         debugs(13, DBG_IMPORTANT, "Asking valgrind for memleaks");
         VALGRIND_DO_LEAK_CHECK;
         debugs(13, DBG_IMPORTANT, "Getting valgrind statistics");
         VALGRIND_COUNT_LEAKS(leaked, dubious, reachable, suppressed);
         stream << "Leaked\t" << leaked << "\n";
         stream << "Dubious\t" << dubious << "\n";
         stream << "Reachable\t" << reachable << "\n";
         stream << "Suppressed\t" << suppressed << "\n";
     }
 #endif
     stream.flush();
 }

=== modified file 'src/ssl/context_storage.cc'
--- src/ssl/context_storage.cc	2015-04-16 04:10:51 +0000
+++ src/ssl/context_storage.cc	2015-08-19 15:17:08 +0000
@@ -1,52 +1,52 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 #include "squid.h"
+#include "base/PackableStream.h"
 #include "mgr/Registration.h"
 #include "ssl/context_storage.h"
 #include "Store.h"
-#include "StoreEntryStream.h"
 
 #include <limits>
 #if HAVE_OPENSSL_SSL_H
 #include <openssl/ssl.h>
 #endif
 
 Ssl::CertificateStorageAction::CertificateStorageAction(const Mgr::Command::Pointer &aCmd)
     :   Mgr::Action(aCmd)
 {}
 
 Ssl::CertificateStorageAction::Pointer
 Ssl::CertificateStorageAction::Create(const Mgr::Command::Pointer &aCmd)
 {
     return new CertificateStorageAction(aCmd);
 }
 
 void Ssl::CertificateStorageAction::dump (StoreEntry *sentry)
 {
-    StoreEntryStream stream(sentry);
+    PackableStream stream(*sentry);
     const char delimiter = '\t';
     const char endString = '\n';
     // Page title.
     stream << "Cached ssl certificates statistic.\n";
     // Title of statistic table.
     stream << "Port" << delimiter << "Max mem(KB)" << delimiter << "Cert number" << delimiter << "KB/cert" << delimiter << "Mem used(KB)" << delimiter << "Mem free(KB)" << endString;
 
     // Add info for each port.
     for (std::map<Ip::Address, LocalContextStorage *>::iterator i = TheGlobalContextStorage.storage.begin(); i != TheGlobalContextStorage.storage.end(); ++i) {
         stream << i->first << delimiter;
         LocalContextStorage & ssl_store_policy(*(i->second));
         stream << ssl_store_policy.memLimit() / 1024 << delimiter;
         stream << ssl_store_policy.entries() << delimiter;
         stream << SSL_CTX_SIZE / 1024 << delimiter;
         stream << ssl_store_policy.size() / 1024 << delimiter;
         stream << ssl_store_policy.freeMem() / 1024 << endString;
     }
     stream << endString;
     stream.flush();
 }

=== renamed file 'src/tests/testStoreEntryStream.cc' => 'src/tests/testPackableStream.cc'
--- src/tests/testStoreEntryStream.cc	2015-01-13 07:25:36 +0000
+++ src/tests/testPackableStream.cc	2015-08-21 10:12:37 +0000
@@ -1,60 +1,52 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
 #include "squid.h"
+#include "base/PackableStream.h"
 #include "CapturingStoreEntry.h"
 #include "Store.h"
-#include "StoreEntryStream.h"
 #include "testStore.h"
-#include "testStoreEntryStream.h"
+#include "testPackableStream.h"
 
 #include <iomanip>
 #include <cppunit/TestAssert.h>
 
-CPPUNIT_TEST_SUITE_REGISTRATION( testStoreEntryStream );
+CPPUNIT_TEST_SUITE_REGISTRATION( testPackableStream );
 
 /* init memory pools */
 
-void testStoreEntryStream::setUp()
+void testPackableStream::setUp()
 {
     Mem::Init();
 }
 
+// TODO: test streaming to a MemBuf as well.
+
 void
-testStoreEntryStream::testGetStream()
+testPackableStream::testGetStream()
 {
     /* Setup a store root so we can create a StoreEntry */
     StorePointer aStore (new TestStore);
     Store::Root(aStore);
 
     CapturingStoreEntry * anEntry = new CapturingStoreEntry();
     {
-        StoreEntryStream stream(anEntry); // locks and unlocks/deletes anEntry
-        CPPUNIT_ASSERT_EQUAL(1, anEntry->_buffer_calls);
-        CPPUNIT_ASSERT_EQUAL(0, anEntry->_flush_calls);
+        anEntry->lock("test");
+        PackableStream stream(*anEntry);
 
         stream.setf(std::ios::fixed);
         stream << 123456 << std::setprecision(1) << 77.7;
         stream << " some text" << std::setw(4) << "!" << '.';
-        CPPUNIT_ASSERT_EQUAL(1, anEntry->_buffer_calls);
-
-        const int preFlushCount = anEntry->_flush_calls;
-        // may have already flushed
-        CPPUNIT_ASSERT(preFlushCount >= 0);
         stream.flush();
-        // flushed at least once more
-        CPPUNIT_ASSERT(anEntry->_flush_calls > preFlushCount);
-
-        CPPUNIT_ASSERT_EQUAL(1, anEntry->_buffer_calls);
+        CPPUNIT_ASSERT_EQUAL(String("12345677.7 some text   !."), anEntry->_appended_text);
 
-        CPPUNIT_ASSERT_EQUAL(String("12345677.7 some text   !."),
-                             anEntry->_appended_text);
+        delete anEntry; // does the unlock()
     }
     Store::Root(NULL);
 }
 

=== renamed file 'src/tests/testStoreEntryStream.h' => 'src/tests/testPackableStream.h'
--- src/tests/testStoreEntryStream.h	2015-01-13 07:25:36 +0000
+++ src/tests/testPackableStream.h	2015-08-19 15:23:47 +0000
@@ -1,32 +1,32 @@
 /*
  * Copyright (C) 1996-2015 The Squid Software Foundation and contributors
  *
  * Squid software is distributed under GPLv2+ license and includes
  * contributions from numerous individuals and organizations.
  * Please see the COPYING and CONTRIBUTORS files for details.
  */
 
-#ifndef SQUID_SRC_TEST_STORE_ENTRY_STREAM_H
-#define SQUID_SRC_TEST_STORE_ENTRY_STREAM_H
+#ifndef SQUID_SRC_TESTS_TESTPACKABLESTREAM_H
+#define SQUID_SRC_TESTS_TESTPACKABLESTREAM_H
 
 #include <cppunit/extensions/HelperMacros.h>
 
 /*
- * test StoreEntryStream
+ * test PackableStream
  */
 
-class testStoreEntryStream : public CPPUNIT_NS::TestFixture
+class testPackableStream : public CPPUNIT_NS::TestFixture
 {
-    CPPUNIT_TEST_SUITE( testStoreEntryStream );
+    CPPUNIT_TEST_SUITE( testPackableStream );
     CPPUNIT_TEST( testGetStream );
     CPPUNIT_TEST_SUITE_END();
 
 public:
     void setUp();
 
 protected:
     void testGetStream();
 };
 
-#endif
+#endif /* SQUID_SRC_TESTS_TESTPACKABLESTREAM_H */
 



More information about the squid-dev mailing list