[prev in list] [next in list] [prev in thread] [next in thread]
List: squid-dev
Subject: Re: [squid-dev] [PATCH] Remove SquidList / link_list
From: Alex Rousskov <rousskov () measurement-factory ! com>
Date: 2016-04-23 16:49:45
Message-ID: 571BA7A9.1030806 () measurement-factory ! com
[Download RAW message or body]
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.
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic