apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Garrett Rooney <roo...@electricjellyfish.net>
Subject Re: [PATCH] fixing bogus apr_date_parse_rfc()
Date Fri, 12 Aug 2005 02:12:00 GMT
Maxime Petazzoni wrote:
>>Would it be possible for you to rework your test for this problem into 
>>something that would fit in the APR-Util test suite?  I'm thinking that 
>>it shouldn't be all that hard to add the appropriate tests to 
>>testdate.c, but since you have the data that actually shows the problem 
>>and I don't, it's a lot easier for you to do it than for me to do it...
> Unfortunately, I'll be off the Internet for about ten days, starting in
> 2 hours. I can rework the test program in the meanwhile, but you'll only
> get it on August 20th or 21st.
> I don't know if I'll be able to have your answer, so I'll be working on
> it. If you can't wait that long for integrating the patch, you can build
> the needed test material by downloading a copy of the dev@httpd mailing
> list with :
> rsync -avr svn.apache.org::public-arch/httpd.apache.org/dev/ dev/
> July 2005 (200507.gz) shows the problem in particular with the mod_mbox
> development plan thread, where Roy's answer is put before the original
> message from me.
> Thanks for your interest in the patch, and see you in ten days!

Well, I got it running, and it seems to work fine, although it's hard 
for me to say for sure, since I'm not exactly clear what your test 
program would do in case of an error...

One thing that seems odd, in the test output it seems to result in a 
time of 0 for "Date: Fri,  1 Jul 2005 11:34:25 -0400", I assume because 
of the extra space before the 1...  I don't know if that was there 
before, but it would be nice to handle it.

You also managed to insert some tabs into the source in your patch, 
generally we try to stick to spaces in the APR code.

Also, I can't help but notice that the 'if (gmtstr)' block in your 
version of the code seems somewhat less defensive than the original. 
The original version does a bunch of sanity checking to ensure that the 
string isn't empty before going into the switch, where your code doesn't 
seem to do that.  Is there a particular reason you removed that check? 
Again, I don't know much about this code, so it's quite possible that it 
isn't needed, but without a specific reason removing those kind of 
checks kind of gives me the creeps.

I guess what I'm saying is that the patch seems reasonable, but I'd like 
some more specific examples (hopefully in the form of an easily runnable 
test case that fails before the patch and passes after it) and to know 
the reasons behind the removal of those sanity checks.



View raw message