[squid-dev] [PATCH] Remove SquidList / link_list
rousskov at measurement-factory.com
Sat Apr 23 16:49:45 UTC 2016
On 04/23/2016 06:20 AM, Amos Jeffries wrote:
> On 15/04/2016 12:31 a.m., Amos Jeffries wrote:
>> This patch replaces the remaining use of Squid custom link_list type
>> with STL std::queue or std::list templates. Removing the now unneeded
>> custom type completely.
>> It builds on the previous libmem old_api cleanup patch and has yet to be
>> run tested, though the unit tests we have for the types all pass.
> Testing on real traffic found a few bugs, which are fixed in the
> attached patch.
Thank you for testing. I saw a couple of these bugs (before I noticed
the updated patch) but probably not all of them. Here is one more red flag:
> - heap_walk = (HeapPurgeData *)xcalloc(1, sizeof(*heap_walk));
> - walker->_data = heap_walk;
> + walker->_data = new HeapPurgeData;
A new/free conflict? Please run the patched Squid through valgrind.
> Though that means the cache unit tests do not actually
> cover I/O to a file using store API, which is a fairly critical part of
> cache testing IMO.
<rant>In my experience, current cache unit tests do more harm than good:
I spend a lot more time maintaining those test cases than I save time by
learning about the bugs they find earlier than black box testing would
reveal them. I am not sure whether this should be fixed by improving
those unit tests or removing them, but I suspect it might be the latter
-- the Store API is just too far from where it needs to be to allow for
good unit tests.</rant>
More information about the squid-dev