Hi Alan,
I made a new webrev, added the comments and the 2 other modification.
It's now : http://cr.openjdk.java.net/~zhouyx/7159982/webrev.02/
On Thu, Apr 12, 2012 at 4:24 PM, Alan Bateman <alan.bate...@oracle.com>wrote:
> On 12/04/2012 06:40, Sean Chou wrote:
>
>> Hi Alan,
>>
>> Many thanks.
>>
>> I updated the patch, ZIP_Open frees the error message and set "Zip
>> file open error".
>>
>> The new webrev is : http://cr.openjdk.java.net/~**
>> zhouyx/7159982/webrev.01/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.01/><
>> http://cr.openjdk.java.net/%**7Ezhouyx/7159982/webrev.01/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.01/>
>> >
>>
>>
>> Please take a look once more.
>>
> This looks much better. I think we'll need to add comments to the ZIP_*
> functions so that it's clear to anyone using them when they need to free
> the error message and then they don't.
>
> One implementation nit at zip_util.c L876 where it should check if pmsg is
> NULL and I think the tests should be reversed so that its:
>
> if (file != NULL && pmsg != NULL && *pmsg != NULL) { ... }
>
> One other minor nit is L875 where there is a space on either side of the
> "*", best to keep the style consistent.
>
> -Alan.
>
--
Best Regards,
Sean Chou
