Thanks Ivan, I'll push with these changes.
-Rob
On 24/09/15 16:31, Ivan Gerasimov wrote:
Thanks Rob!
The webrev looks cleaner.
jni_util.h:
Wouldn't it be better to move this block directly to the jni_util_md.c
file?
390 #if defined(LINUX) && (defined(_GNU_SOURCE) || \
391 (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE < 200112L \
392 && defined(_XOPEN_SOURCE) && _XOPEN_SOURCE < 600))
393 extern int __xpg_strerror_r(int, char *, size_t);
394 #endif
I don't see the __xpg_strerror_r() function used anywhere outside that
.c file anyway.
TwoStacksPlainDatagramSocketImpl.c:
It wasn't introduced by your fix, but it seems that a new-line isn't
necessary here:
2218 sprintf(errmsg, "error getting socket option: %s\n", tmpbuf);
In all other places JNU_ThrowByName() uses non-new-line-terminated
messages.
Sincerely yours,
Ivan
On 24.09.2015 17:10, Rob McKenna wrote:
Hi folks,
Uploaded a newer version:
http://cr.openjdk.java.net/~robm/8133249/webrev.02/
To address Rogers question, this needed to be separate from
getLastErrorString because of the return type and the err arg, but I
like the idea of avoiding the creation of a new file. Also, all of the
files that require the new function (now called getErrorString)
already include jni_util.c so it makes sense to put this in
jni_util_md.c with getLastErrorString.
As per Christos' suggestion I've mapped strerr_r to __xpg_strerror_r
instead of using the gnu strerror_r. I'd be interested to hear
feedback on this change.
I've corrected the oversights around the jli sources and
PlainDatagramSocket. The jli stuff shouldn't have been there in the
first place.
Ivan, I've kept the return type. As you note, I should have been using
it in ProcessImpl_md.c and the extra information from the return is
potentially useful.
-Rob
On 23/09/15 14:14, Ivan Gerasimov wrote:
Hi Rob!
On 21.09.2015 18:53, Rob McKenna wrote:
Hi folks,
Requesting a review of this change which switches corelibs usages of
the thread-unsafe strerror over to strerror_r/strerror_s:
http://cr.openjdk.java.net/~robm/8133249/webrev.01/
I think it would be better to have jdk_strerror(errno, tmpbuf,
sizeof(tmpbuf)) instead of jdk_strerror(errno, tmpbuf, (size_t) 1024),
just for the sake of avoiding constant duplication.
I don't see the return code of jdk_strerror() is ever used. I suggest
using it in ProcessImpl_md.c (see below), or just making this function
return void.
jni_util_md.c:
- why can't we just call jdk_strerror(errno, buf, len) and get rid of
the temp buffer?
java_md_common.c:
- commented out #include @ line 26
- the change in JLI_ReportErrorMessageSys() looks incomplete
ProcessImpl_md.c:
- would it make sense to check if jdk_strerror() == EINVAL instead of
comparing the strings?
PlainDatagramSocketImpl.c:
- #include added, but no other changes were made.
TwoStacksPlainDatagramSocketImpl.c:
- buf is declared to be char[1024], but only up to 255 chars are used.
- would be better to move the buf declaration closer to its use, maybe
next to char errmsg[300] line?
jdk_strerror.c:
- minor nit: extra space after strerror_r at lines 46 and 63.
Sincerely yours,
Ivan
