<i18n dev> Fix for small leak in TimeZone_md.c
Volker Simonis
volker.simonis at gmail.com
Thu Sep 10 13:20:39 UTC 2015
Hi Christoph,
the change looks good. Thanks for doing this clean-up.
I'll sponsor the change.
Regards,
Volker
On Wed, Sep 9, 2015 at 3:08 PM, Langer, Christoph
<christoph.langer at sap.com> wrote:
> Hi Dmitry,
>
> let me comment on your suggestions:
> 663 it might be better to use str*r*chr
> -> a TZ variable could look like TZ="MET-1METDST,M3.5.0/02:00:00,M10.5.0/03:00:00". So there could be multiple instances of the character ',' and I'm only interested in the first part of TZ, e.g. in "MET-1METDST" in this example case. So why should I want to use strrchr?
>
> 666 memcpy(tz_buf, tz, tz_len+1);
> 667 is not necessary
> -> well, in the case of a TZ variable like above, tz_len would be 11. If I now copy 12 characters, I don't copy a trailing 0 but the ',' character that strchr found. So I have to memcpy exactly tz_len characters and set the trailing 0 myself.
>
> As for the P4 CR - I can do it, when I'm editor eventually :-)
>
> Can I consider this change as reviewed then?
>
> Thanks
> Christoph
>
> -----Original Message-----
> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
> Sent: Mittwoch, 9. September 2015 09:12
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: jdk9-dev at openjdk.java.net; i18n-dev at openjdk.java.net; Roger Riggs <Roger.Riggs at oracle.com>
> Subject: Re: Fix for small leak in TimeZone_md.c
>
> Christoph,
>
> Looks good for me.
>
> 663 it might be better to use str*r*chr
> 666 memcpy(tz_buf, tz, tz_len+1);
> 667 is not necessary
>
>> - The part following line 323 for Solaris32 can probably be removed
>> but I don't want to be the guy that does this
>
> OK. Could you file a P4 CR to get it removed?
>
> -Dmitry
>
> On 2015-09-08 17:06, Langer, Christoph wrote:
>> Hi Dmitry,
>>
>> thanks for your review.
>>
>> - I followed your suggestions to replace #ifdef with #if defined() at all places
>> - I changed the usage of readdir_r to readdir64_r, as it is done in UnixFileSystem_md.c as well. So we can remove the define from line 129 in the original TimeZone_md.c
>> - The part following line 323 for Solaris32 can probably be removed but I don't want to be the guy that does this
>> - 664 and 666 - I changed the construct a bit and I'm using strchr and memcpy now :-)
>> - 678: changed to use strcpy as suggested
>>
>> Here is the new webrev: http://cr.openjdk.java.net/~simonis/webrevs/2015/8134505.v1/
>>
>> Best regards,
>> Christoph
>>
>> -----Original Message-----
>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>> Sent: Samstag, 5. September 2015 19:47
>> To: Langer, Christoph <christoph.langer at sap.com>; Roger Riggs <Roger.Riggs at oracle.com>
>> Cc: jdk9-dev at openjdk.java.net; i18n-dev at openjdk.java.net
>> Subject: Re: Fix for small leak in TimeZone_md.c
>>
>> Lander,
>>
>> Changes looks good, few nits below. Please fill free to ignore comments
>> that is out of scope of your work.
>>
>>
>> 53 I would prefer don't mix #if defined and #ifdef styles.
>> Could you please use #if defined(_AIX)
>>
>> #if defined(_AIX) need not to be inside #else block, please move
>> it below #endif
>>
>>
>> 128 Do you know the platform that fall to #else here?
>> I guess we can remove this define.
>>
>> 323 JDK 9 doesn't support Solaris 32bit so this code could be removed.
>>
>> 664 It's better to use memcpy here and copy string with terminating \0
>> in one line, as you already know the length.
>>
>> 666: It might be easier to use strrchr here (not really important).
>>
>> 678: It's better to use plain strcpy (or ever sprintf) here because you
>> checked length above.
>>
>> 779: Could you change #ifdef __linux__ to #if defined(__linux__) to keep
>> one style.
>>
>> -Dmitry
>>
>>
>> On 2015-09-03 16:39, Langer, Christoph wrote:
>>> Here is the updated webrev: http://cr.openjdk.java.net/~simonis/webrevs/2015/8134505/
>>>
>>> -----Original Message-----
>>> From: Langer, Christoph
>>> Sent: Donnerstag, 3. September 2015 12:25
>>> To: 'Roger Riggs' <Roger.Riggs at Oracle.com>; jdk9-dev at openjdk.java.net
>>> Cc: 'i18n-dev at openjdk.java.net' <i18n-dev at openjdk.java.net>
>>> Subject: RE: Fix for small leak in TimeZone_md.c
>>>
>>> Hi Roger,
>>>
>>> thanks for your comments.
>>>
>>> I agree with you and I will remove the platform specifics around line 770.
>>>
>>> As for the suggestions about line 804 and beyond, it would perhaps be nicer to have mutually exclusive sections. However, then I would have to add the common, "not _AIX and not __solaris__", code also to the "__solaris__" part for the case that tz does not equal "localtime", because then the common handling would apply as well. So, to avoid code duplication I'd prefer to leave it as it is.
>>>
>>> Best regards
>>> Christoph
>>>
>>>
>>> -----Original Message-----
>>> From: jdk9-dev [mailto:jdk9-dev-bounces at openjdk.java.net] On Behalf Of Roger Riggs
>>> Sent: Dienstag, 1. September 2015 23:46
>>> To: jdk9-dev at openjdk.java.net
>>> Subject: Re: Fix for small leak in TimeZone_md.c
>>>
>>> Hi,
>>>
>>> Some comments:
>>>
>>> - Line 770: the platform specific conditional compilation can be
>>> removed, if *tz is zero, its not a real tz anyway.
>>>
>>> - Line 804: " } else" having the "else" inside the conditional
>>> compilation makes the code more fragile.
>>> Perhaps just return javatz and not have to figure out whether those
>>> other conditions apply.
>>>
>>> - It might be clearer if the AIX, Solaris, and linux tail of the code
>>> was mutually exclusive.
>>>
>>> i.e.
>>>
>>> #if defined(_AIX)
>>> /* On AIX do the platform to Java mapping. */
>>> javatz = mapPlatformToJavaTimezone(java_home_dir, tz);
>>> if (freetz != NULL) {
>>> free((void *) freetz);
>>> }
>>> #elif defined(__solaris__)
>>> /* Solaris might use localtime, so handle it here. */
>>> if (strcmp(tz, "localtime") == 0) {
>>> javatz = getSolarisDefaultZoneID();
>>> if (freetz != NULL) {
>>> free((void *) freetz);
>>> }
>>> }
>>> #else /* otherwise, not _AIX and not __solaris__ */
>>> if (freetz == NULL) {
>>> /* strdup if we are still working on getenv result. */
>>> javatz = strdup(tz);
>>> } else if (freetz != tz) {
>>> /* strdup and free the old buffer, if we moved the pointer. */
>>> javatz = strdup(tz);
>>> free((void *) freetz);
>>> } else {
>>> /* we are good if we already work on a freshly allocated
>>> buffer. */
>>> javatz = tz;
>>> }
>>> #endif
>>>
>>> $.02, Roger
>>>
>>> On 8/26/2015 5:34 PM, Langer, Christoph wrote:
>>>> Hi Andrew,
>>>>
>>>> today I have posted the following RFR in i18n-dev:
>>>>
>>>> when working on TimeZone_md.c lately, I found it worthwhile to do some cleanup on it. Please review my changes.
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8134505
>>>> Webrev: http://cr.openjdk.java.net/~goetz/webrevs/8134505-timeZone/webrev.01/
>>>>
>>>> I basically did the following things:
>>>> - clean up some typos and comment errors
>>>> - added #include "TimeZone_md.h"
>>>> - split up the platform specific define sections into an #ifdef <platform1> #elif <platform2> #elif … #endif construct
>>>> - moved some AIX coding from the #ifdef block at the bottom of the file into the first AIX specific block
>>>> - AIX function “mapPlatformToJavaTimezone”: use a dynamic malloced buffer instead of a fixed length buffer
>>>> - refactor function “findJavaTZ_md” to make it more straightforward and to avoid unnecessary mallocs and don’t forget necessary frees
>>>>
>>>> I’m also wondering if the “if (tz == NULL || *tz == '\0') {“ of line 770 could be used for all platforms instead of Solaris and AIX only. The other platforms will only do a check if TZ is NULL but not if it is an empty string.
>>>>
>>>> I did not follow the suggestion to split up "findJavaTZ_md" in platform versions, though.
>>>>
>>>> I'd be grateful for your feedback.
>>>>
>>>> Thanks
>>>> Christoph
>>>
>>
>>
>
>
> --
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.
More information about the i18n-dev
mailing list