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.samers...@oracle.com] Sent: Mittwoch, 9. September 2015 09:12 To: Langer, Christoph <christoph.lan...@sap.com> Cc: jdk9-...@openjdk.java.net; i18n-dev@openjdk.java.net; Roger Riggs <roger.ri...@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.samers...@oracle.com] > Sent: Samstag, 5. September 2015 19:47 > To: Langer, Christoph <christoph.lan...@sap.com>; Roger Riggs > <roger.ri...@oracle.com> > Cc: jdk9-...@openjdk.java.net; i18n-dev@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.ri...@oracle.com>; jdk9-...@openjdk.java.net >> Cc: 'i18n-dev@openjdk.java.net' <i18n-dev@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-boun...@openjdk.java.net] On Behalf Of Roger >> Riggs >> Sent: Dienstag, 1. September 2015 23:46 >> To: jdk9-...@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.
