Re: A review of the latest Polipo commits

On Mon, Dec 21, 2009 at 06:11:14PM +0100, Juliusz Chroboczek wrote:
> 2. "Remove dead store in..."
> 
> I'm certainly not a big fan of these changes.  There are good reasons
> for these stores, even though clang detects them as dead.
> 
> My policy is that I never write
> 
>   free(p);
> 
> but instead write
> 
>   free(p);
>   p = NULL;
> 
> even when I'm not going to use p afterwards.
> 
> The reason for that is that in the former case, if I use p afterwards
> I'm going to get a mysterious malloc arena corruption.  With the NULL
> assignment, I'm guaranteed to get a reliable crash.
> 
> Similarly, I do
> 
>   close(fd);
>   fd = -1;
> 
> so that the next access through fd gets EBADF.
> 
> I strongly recommend reverting this series of patches.
>
Hmm. This policy is sensible, but it's not easy to tell that
it exists by reading the source code. I think Richard's suggestion
of using macros/functions would be the best compromise. It would
save some code repetition, spare us the clang warnings, and also
make this policy much more self-evident. I'm not planning to
revert Fabian's patch series for now (it only looks like two of
his patches are related to this policy, anyway).
 
> 
> 3. "Don't crash when parsing a malformed Cache-Control: header."
> 
> Yep, definitely my bug.  Thanks for fixing this.
> 
> If you have the time -- perhaps using strtol and proper error checking
> might be a good idea?
>
Sure. I'll put this on my TODO list.
 
> 
> 4. "Fix what looks like a variable mixup in makeDiskEntry()."
> 
> Not an urgent task, but I suggest drastically simplifying this whole
> mess.
> 
Also on my TODO list.

-- 
Christopher Davis
Mangrin Remailer Admin
PGP: 0x0F8DA163

------------------------------------------------------------------------------
This SF.Net email is sponsored by the Verizon Developer Community
Take advantage of Verizon's best-in-class app development support
A streamlined, 14 day to market process makes app distribution fast and easy
Join now and get one step closer to millions of Verizon customers
http://p.sf.net/sfu/verizon-dev2dev