Re: cvs commit: squid3/lib MemPool.cc

[Amos Jeffries]

Alex Rousskov wrote:

rousskov2007/05/22 10:40:06 MDT

  Modified files:
lib  MemPool.cc 
  Log:

  Bug #1966 fix: Use rounded String MemPool sizes in the hard-coded pool
  config to avoid warnings that the configured pool size does not match the
  actual size.
  
  Revision  ChangesPath

  1.7   +7 -2  squid3/lib/MemPool.cc



Did you run make check before committing any of the days changes?

I'm getting 7 of 14 core unit tests fail. Two on MemPool.cc:240, the 
rest on silent segfault.



Assertion failed: (aLabel != NULL && aSize) at MemPool.cc:240
FAIL: tests/testString
/bin/bash: line 4: 23339 Aborted (core dumped) ${dir}$tst
FAIL: tests/testCacheManager
Assertion failed: (aLabel != NULL && aSize) at MemPool.cc:240
FAIL: tests/testDiskIO
/bin/bash: line 4: 23351 Aborted (core dumped) ${dir}$tst
FAIL: tests/testEvent
/bin/bash: line 4: 23358 Aborted (core dumped) ${dir}$tst
FAIL: tests/testEventLoop
/bin/bash: line 4: 23371 Aborted (core dumped) ${dir}$tst
FAIL: tests/testHttpRequest

Amos

Re: cvs commit: squid3/lib MemPool.cc

[Henrik Nordstrom]

tor 2007-05-24 klockan 00:59 +1200 skrev Amos Jeffries:
> > 
> 
> Did you run make check before committing any of the days changes?
> 
> I'm getting 7 of 14 core unit tests fail. Two on MemPool.cc:240, the 
> rest on silent segfault.

> Assertion failed: (aLabel != NULL && aSize) at MemPool.cc:240

Hmm...

(gdb) p StrPoolsAttrs
$3 = {{name = 0x509d01 "Short Strings", obj_size = 0}, {name = 0x509d0f
"Medium Strings", obj_size = 0}, {
name = 0x509d1e "Long Strings", obj_size = 0}}

doesn't look quite right.

Looks like C++ magics strikes again and Mem::Init is called (via the
Initer test class) before StrPoolsAttrs has been assigned..

Mem::Init needs to be called via the setUp() method, not from an
automatic instantiation constructor..


Index: src/tests/testCacheManager.cc
===
RCS file: /cvsroot/squid/squid3/src/tests/testCacheManager.cc,v
retrieving revision 1.2
diff -u -p -r1.2 testCacheManager.cc
--- src/tests/testCacheManager.cc   18 May 2007 06:41:33 -
1.2
+++ src/tests/testCacheManager.cc   23 May 2007 20:47:42 -
@@ -17,12 +17,10 @@ shut_down(int)
 
 /* init memory pools */
 
-struct Initer
+void testCacheManager::setUp()
 {
-Initer() {Mem::Init();}
-};
-
-static Initer ensure_mempools;
+Mem::Init();
+}
 
 /*
  * Test creating a CacheManager
Index: src/tests/testCacheManager.h
===
RCS file: /cvsroot/squid/squid3/src/tests/testCacheManager.h,v
retrieving revision 1.1
diff -u -p -r1.1 testCacheManager.h
--- src/tests/testCacheManager.h29 May 2006 00:15:09 -
1.1
+++ src/tests/testCacheManager.h23 May 2007 20:47:42 -
@@ -16,6 +16,7 @@ class testCacheManager : public CPPUNIT_
 CPPUNIT_TEST_SUITE_END();
 
 public:
+void setUp();
 
 protected:
 void testCreate();



signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: cvs commit: squid3/lib MemPool.cc

[Robert Collins]

On Wed, 2007-05-23 at 22:49 +0200, Henrik Nordstrom wrote:
> 
> Mem::Init needs to be called via the setUp() method, not from an
> automatic instantiation constructor.. 

Actually I spent quite some time making it work correctly without setUp.
I think manual setup is very annoying and error prone: if we can avoid
it lets do so.

-Rob
-- 
GPG key available at: .


signature.asc
Description: This is a digitally signed message part

Re: cvs commit: squid3/lib MemPool.cc

[Henrik Nordstrom]

ons 2007-05-23 klockan 22:49 +0200 skrev Henrik Nordstrom:

> Mem::Init needs to be called via the setUp() method, not from an
> automatic instantiation constructor..

The tests have been updated, and now pass fine here..

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: cvs commit: squid3/lib MemPool.cc

[Henrik Nordstrom]

tor 2007-05-24 klockan 06:58 +1000 skrev Robert Collins:
> On Wed, 2007-05-23 at 22:49 +0200, Henrik Nordstrom wrote:
> > 
> > Mem::Init needs to be called via the setUp() method, not from an
> > automatic instantiation constructor.. 
> 
> Actually I spent quite some time making it work correctly without setUp.
> I think manual setup is very annoying and error prone: if we can avoid
> it lets do so.

I disagree.

The initialization should mimic the main program as much as possible. In
squid Mem::Init is called by main(), and setUp() is the closest
equivalence.

Also the two methods is equally manual. Each test needs a list of stuff
to initialize. The only difference is that it's explicitly called by
cppunit and not the compiler. All I did was to remove the static object
which caused the compiler to call the test initialization and moved this
to the test setUp() method instead.

The day Squid is changed to not do manual setup of these things the
tests obviously no longer need (or should) do it either. But that's a
separate question.

Imho doing heavy dependent stuff from static constructors is very
dangerous from the simple fact that you don't know for sure in which
order these is called. It's both link order and compiler dependent, and
for this reason extremely error prone. It's in the area of C++ magics
best avoided. Keep static constructors as simple as possible.

Regards
Henrik


signature.asc
Description: Detta är en digitalt signerad	meddelandedel

Re: cvs commit: squid3/lib MemPool.cc

[Alex Rousskov]

On Thu, 2007-05-24 at 00:59 +1200, Amos Jeffries wrote:
> Alex Rousskov wrote:
> > rousskov2007/05/22 10:40:06 MDT
> > 
> >   Modified files:
> > lib  MemPool.cc 
> >   Log:
> >   Bug #1966 fix: Use rounded String MemPool sizes in the hard-coded pool
> >   config to avoid warnings that the configured pool size does not match the
> >   actual size.
> >   
> >   Revision  ChangesPath
> >   1.7   +7 -2  squid3/lib/MemPool.cc
> > 
> 
> Did you run make check before committing any of the days changes?

Nope. I never do. Will try to remember next time.

Sorry,

Alex.
P.S. Henrik, thanks for fixing the test cases!