[squid-dev] [PATCH] Remove SquidList / link_list

[Alex Rousskov]

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:

>     safe_free(walker->_data);
...
> -    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>


Thank you,

Alex.