[ping] RFR(M): 8186072: dll_build_name returns true even if file is missing.
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Aug 29 09:44:26 UTC 2017
Hi,
I am fine with this change going in in the current form. Lets get this
patch in.
I see some issues, but they can be addressed in follow up items:
1) os::path_seperator() should really return a char, not a string. Most
callers seem to reference os::path_seperator()[0].
2) If the user provided buffer is too small, we will fail, which looks like
the dll could not have been located. I am not sure we have to be shy with
allocating memory - internally, we malloc a buffer for assembling the
filename, and then os::splitpath() will malloc a whole bunch of arrays too.
So I think we could just return the dll path location in a malloced buffer
and require the caller to free.
3) I do not understand ':' as a file separator on windows. So, a path is
allowed to contain e.g. "C:;" ? Which would mean "a path relative to the
current directory currently active on drive C". If I am not mistaken. Do we
want to support this, what is the use case?
Kind Regards, Thomas
On Tue, Aug 29, 2017 at 10:41 AM, David Holmes <david.holmes at oracle.com>
wrote:
> On 29/08/2017 6:29 PM, Lindenmaier, Goetz wrote:
>
>> Hi David,
>>
>> I fixed the indentation and added you as reviewer.
>> I replaced the webrev in-place:
>> http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.05/
>> The new code went through all our testing ... except for some ppc/s390
>> builds
>> that failed because of an other change pushed to hs tonight. But that
>> should
>> not matter, it all passed with the jdk10/jdk10 testing.
>>
>> Would you mind sponsoring?
>>
>
> No problem - but can we get Thomas to sign-off on this latest version
> please.
>
>
> Thanks,
> David
>
> Best regards,
>> Goetz.
>>
>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Dienstag, 29. August 2017 09:53
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: [ping] RFR(M): 8186072: dll_build_name returns true even if
>>> file
>>> is missing.
>>>
>>> Hi Goetz,
>>>
>>> On 29/08/2017 4:18 PM, Lindenmaier, Goetz wrote:
>>>
>>>> Hi,
>>>>
>>>> this is a webrev with merged windows and posix implementations:
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>> dllBuildName/webrev.05/
>>>
>>> I like the look of this.
>>>
>>> There are a couple of indention nits in os.cpp:
>>>
>>> 247 static bool conc_path_file_and_check(char *buffer, char
>>> *printbuffer, size_t printbuflen,
>>> 248 const char* pname, char lastchar,
>>> const char* fname) {
>>>
>>>
>>> 251 const char *filesep = (WINDOWS_ONLY(lastchar == ':' ||) lastchar
>>> == os::file_separator()[0]) ?
>>> 252 "" : os::file_separator();
>>>
>>>
>>> Thanks,
>>> David
>>>
>>> Best regards,
>>>> Goetz
>>>>
>>>> -----Original Message-----
>>>>> From: Lindenmaier, Goetz
>>>>> Sent: Montag, 28. August 2017 12:10
>>>>> To: 'David Holmes' <david.holmes at oracle.com>
>>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>>> Subject: RE: [ping] RFR(M): 8186072: dll_build_name returns true even
>>>>> if
>>>>>
>>>> file
>>>
>>>> is missing.
>>>>>
>>>>> Hi,
>>>>>
>>>>> this are the changes needed to make the windows dll_locate_lib
>>>>> universally applicable. I also merge the three similar jio_snprintf
>>>>> calls into one method.
>>>>> I do some gymnastics to avoid another buffer of MAX_PATH_LEN
>>>>> at the first call to conc_path_file_and_check.
>>>>> I'll test this tonight.
>>>>>
>>>>> Best regards,
>>>>> Goetz.
>>>>>
>>>>> diff -r e09e4eb985c5 src/os/windows/vm/os_windows.cpp
>>>>> --- a/src/os/windows/vm/os_windows.cpp Thu Aug 17 17:26:02
>>>>>
>>>> 2017
>>>
>>>> +0200
>>>>> +++ b/src/os/windows/vm/os_windows.cpp Mon Aug 28 12:02:26
>>>>>
>>>> 2017
>>>
>>>> +0200
>>>>> @@ -1205,6 +1205,17 @@
>>>>> return GetFileAttributes(filename) != INVALID_FILE_ATTRIBUTES;
>>>>> }
>>>>>
>>>>> +bool conc_path_file_and_check(char *buffer, char *printbuffer, size_t
>>>>> printbuflen,
>>>>> + const char* pname, char lastchar, const
>>>>> char* fname) {
>>>>> + char *filesep = (WINDOWS_ONLY(lastchar == ':' ||) lastchar ==
>>>>> os::file_seperator()[0]) ? "" : os::file_separator();
>>>>> + int ret = jio_snprintf(printbuffer, printbuflen, "%s%s%s", path,
>>>>> filesep,
>>>>> fullfname);
>>>>> + if (ret != -1) {
>>>>> + struct stat statbuf;
>>>>> + return os::stat(buffer, &statbuf) == 0;
>>>>> + }
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> bool os::dll_locate_lib(char *buffer, size_t buflen,
>>>>> const char* pname, const char* fname) {
>>>>> bool retval = false;
>>>>> @@ -1220,11 +1231,8 @@
>>>>> if (p != NULL) {
>>>>> const size_t plen = strlen(buffer);
>>>>> const char lastchar = buffer[plen - 1];
>>>>> - char *filesep = (lastchar == ':' || lastchar == '\\') ? "" :
>>>>> "\\";
>>>>> - int ret = jio_snprintf(&buffer[plen], buflen - plen, "%s%s",
>>>>> filesep,
>>>>> fullfname);
>>>>> - if (ret != -1) {
>>>>> - retval = file_exists(buffer);
>>>>> - }
>>>>> + retval = conc_path_file_and_check(buffer, &buffer[plen],
>>>>> buflen -
>>>>> plen,
>>>>> + "", lastchar, fullfname);
>>>>> }
>>>>> } else if (strchr(pname, *os::path_separator()) != NULL) {
>>>>> int n;
>>>>> @@ -1238,12 +1246,8 @@
>>>>> continue; // skip the empty path values
>>>>> }
>>>>> const char lastchar = path[plen - 1];
>>>>> - char *filesep = (lastchar == ':' || lastchar == '\\') ? ""
>>>>> : "\\";
>>>>> - int ret = jio_snprintf(buffer, buflen, "%s%s%s", path,
>>>>> filesep,
>>>>> fullfname);
>>>>> - if (ret != -1 && file_exists(buffer)) {
>>>>> - retval = true;
>>>>> - break;
>>>>> - }
>>>>> + retval = conc_path_file_and_check(buffer, buffer, buflen,
>>>>> path,
>>>>> lastchar, fullfname);
>>>>> + if (retval) break;
>>>>> }
>>>>> // release the storage
>>>>> for (int i = 0; i < n; i++) {
>>>>> @@ -1255,11 +1259,7 @@
>>>>> }
>>>>> } else {
>>>>> const char lastchar = pname[pnamelen-1];
>>>>> - char *filesep = (lastchar == ':' || lastchar == '\\') ? "" :
>>>>> "\\";
>>>>> - int ret = jio_snprintf(buffer, buflen, "%s%s%s", pname, filesep,
>>>>> fullfname);
>>>>> - if (ret != -1) {
>>>>> - retval = file_exists(buffer);
>>>>> - }
>>>>> + retval = conc_path_file_and_check(buffer, buffer, buflen, path,
>>>>>
>>>> lastchar,
>>>
>>>> fullfname);
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>>> Sent: Montag, 28. August 2017 07:38
>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>>>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>>>> Subject: Re: [ping] RFR(M): 8186072: dll_build_name returns true even
>>>>>> if
>>>>>>
>>>>> file
>>>>>
>>>>>> is missing.
>>>>>>
>>>>>> Hi Goetz,
>>>>>>
>>>>>> On 25/08/2017 12:19 AM, Lindenmaier, Goetz wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I please need a second review and a sponsor:
>>>>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>
>>>>>> dllBuildName/webrev.04
>>>>>
>>>>>>
>>>>>>> To update my description of the change to the status after Thomas'
>>>>>>>
>>>>>> review:
>>>>>
>>>>>>
>>>>>>> dll_build_name builds the proper path to a library given a list of
>>>>>>> paths
>>>>>>>
>>>>>> separated by
>>>>>>
>>>>>>> path_seperator and a library name. It adds in the platform specific
>>>>>>>
>>>>>> endings
>>>>>
>>>>>> etc.
>>>>>>
>>>>>>> It is documented to return whether the file exists, but only does so
>>>>>>> if a
>>>>>>>
>>>>>> path_seperator
>>>>>>
>>>>>>> exists in the path.
>>>>>>> Especially if the path is empty, it just returns ‘true’ without
>>>>>>> checking.
>>>>>>>
>>>>>>> Dll_build_name is usually used before calling dll_load. If dll_load
>>>>>>> does
>>>>>>>
>>>>>> not
>>>>>
>>>>>> get a full path it searches
>>>>>>
>>>>>>> in well known unix/windows locations. This is intended in the two
>>>>>>> cases
>>>>>>>
>>>>>> where dll_build_name
>>>>>>
>>>>>>> is called with an empty path.
>>>>>>>
>>>>>>> I renamed dll_build_name to dll_locate_lib and changed it's behavior
>>>>>>> to
>>>>>>>
>>>>>> always return
>>>>>>
>>>>>>> a full path to the lib, inserting current working directory if no
>>>>>>> path is
>>>>>>>
>>>>>> given.
>>>
>>>> For the use case where "" was actually passed to the function, I added
>>>>>>>
>>>>>> a
>>>
>>>> new function
>>>>>>
>>>>>>> (reusing the old function name) dll_build_name that just adds system
>>>>>>>
>>>>>> dependent prefix and suffix
>>>>>>
>>>>>>> to the name.
>>>>>>> I merged all unix implementations to the posix os branch.
>>>>>>>
>>>>>>
>>>>>> I started to look at this and have applied the patch to run through
>>>>>> some
>>>>>> basic testing. The overall approach seems reasonable. But it is hard
>>>>>> to
>>>>>> track all the details - in particular whether there were any subtle
>>>>>> differences across the "posix" systems?
>>>>>>
>>>>>> I'm wondering what, if any, significant differences exist between the
>>>>>> Windows and POSIX versions? I would hope the platform differences
>>>>>>
>>>>> could
>>>
>>>> easily be hidden behind macros (for path separator, library suffix etc).
>>>>>> Then perhaps this could just go in shared code (os.hpp, os.cpp)?
>>>>>>
>>>>>> That aside, in the Windows code shouldn't the hardwired .dll strings
>>>>>> actually be JNI_LIB_SUFFIX?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> Best regards,
>>>>>>> Goetz.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>>>>>>>> Sent: Dienstag, 22. August 2017 17:30
>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>>>>>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>>>>>> Subject: Re: RFR(M): 8186072: dll_build_name returns true even if
>>>>>>>> file
>>>>>>>>
>>>>>>> is
>>>
>>>> missing.
>>>>>>>>
>>>>>>>> Looks good.
>>>>>>>>
>>>>>>>> ..Thomas
>>>>>>>>
>>>>>>>> On Tue, Aug 22, 2017 at 4:33 PM, Lindenmaier, Goetz
>>>>>>>> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com>
>>>>>>>>
>>>>>>>
>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> I mistyped the path to webrev, this should work:
>>>>>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> dllBuildName/webrev.04
>>>>>>>>
>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>
>>>>>>> dllBuildName/webrev.04>
>>>>>>>>
>>>>>>>> Sorry,
>>>>>>>> Goetz
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> > -----Original Message-----
>>>>>>>> > From: Lindenmaier, Goetz
>>>>>>>> > Sent: Dienstag, 22. August 2017 15:48
>>>>>>>> > To: 'Thomas Stüfe' <thomas.stuefe at gmail.com
>>>>>>>> <mailto:thomas.stuefe at gmail.com> >
>>>>>>>> > Cc: hotspot-runtime-dev at openjdk.java.net <mailto:hotspot-
>>>>>>>>
>>>>>>> runtime-
>>>>>>
>>>>>>> dev at openjdk.java.net>
>>>>>>>> > Subject: RE: RFR(M): 8186072: dll_build_name returns true
>>>>>>>> even if
>>>>>>>> file is
>>>>>>>> > missing.
>>>>>>>> >
>>>>>>>> > Hi,
>>>>>>>> >
>>>>>>>> > could I please get a second review?
>>>>>>>> > http://cr.openjdk.java.net/~go
>>>>>>>> etz/wr17/8186072-dllBuildName-
>>>>>>>> hs/webrev.04 <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>>
>>>>>>> dllBuildName-
>>>>>>
>>>>>>> hs/webrev.04>
>>>>>>>> >
>>>>>>>> > I had to update the webrev because of a problem on
>>>>>>>> windows.
>>>>>>>> > @Thomas I had edited os.hpp, but not saved :(
>>>>>>>> >
>>>>>>>> > Best regards,
>>>>>>>> > Goetz.
>>>>>>>> >
>>>>>>>> > PS: Didn't double-check the webrev as cr server is slow.
>>>>>>>> >
>>>>>>>> > > -----Original Message-----
>>>>>>>> > > From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com
>>>>>>>> <mailto:thomas.stuefe at gmail.com> ]
>>>>>>>> > > Sent: Donnerstag, 17. August 2017 19:54
>>>>>>>> > > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com
>>>>>>>> <mailto:goetz.lindenmaier at sap.com> >
>>>>>>>> > > Cc: hotspot-runtime-dev at openjdk.java.net <mailto:
>>>>>>>> hotspot-
>>>>>>>> runtime-dev at openjdk.java.net>
>>>>>>>> > > Subject: Re: RFR(M): 8186072: dll_build_name returns
>>>>>>>> true even
>>>>>>>>
>>>>>>> if
>>>>>>
>>>>>>> file is
>>>>>>>> > > missing.
>>>>>>>> > >
>>>>>>>> > > Hi Goetz,
>>>>>>>> > >
>>>>>>>> > > On Thu, Aug 17, 2017 at 6:03 PM, Lindenmaier, Goetz
>>>>>>>> > > <goetz.lindenmaier at sap.com
>>>>>>>> <mailto:goetz.lindenmaier at sap.com>
>>>>>>>>
>>>>>>> <mailto:goetz.lindenmaier at sap.com
>>>>>>
>>>>>>> <mailto:goetz.lindenmaier at sap.com> > > wrote:
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Hi Thomas,
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > I adapted the comments in os.hpp.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > If I move the call to dll_build_name out of
>>>>>>>> dll_locate_lib
>>>>>>>> > >
>>>>>>>> > > I have to do a lot of coding in all the places
>>>>>>>> where it is called.
>>>>>>>> > >
>>>>>>>> > > That seems not useful to me.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Fixed the type to size_t.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > One could merge posix/windows if putting the check
>>>>>>>> for ‘:’
>>>>>>>> > >
>>>>>>>> > > into a WINDOWS_ONLY() I guess. The check for \
>>>>>>>> could be
>>>>>>>> > >
>>>>>>>> > > done in posix as well, if using file_seperator().
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > * Not your change, but: why does the code in
>>>>>>>>
>>>>>>> os::dll_locate_lib()
>>>>>>
>>>>>>> even
>>>>>>>> > >
>>>>>>>> > > * differentiate between a PATH containing no
>>>>>>>> os::path_separator()
>>>>>>>> > >
>>>>>>>> > > * and a path containing os::path_separator()?
>>>>>>>> > >
>>>>>>>> > > I assume this was done to avoid all the allocations
>>>>>>>> and copying
>>>>>>>>
>>>>>>> of
>>>>>>
>>>>>>> the
>>>>>>>> > > path.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Also adapted the comment in jvmtiExport.cpp.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > New webrev:
>>>>>>>> > >
>>>>>>>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > dllBuildName/webrev.03/
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > dllBuildName/webrev.03/>
>>>>>>>> > >
>>>>>>>> > > incremental diff:
>>>>>>>> > >
>>>>>>>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > dllBuildName/webrev.03/diffs-incremental.patch
>>>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > dllBuildName/webrev.03/diffs-incremental.patch>
>>>>>>>> > >
>>>>>>>> > > (fixed indentation on windows)
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Best regards,
>>>>>>>> > >
>>>>>>>> > > Goetz.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Comments in os.hpp seem unchanged ?
>>>>>>>> > >
>>>>>>>> > > But looks fine otherwise. I do not need another webrev.
>>>>>>>> > >
>>>>>>>> > > Thanks, Thomas
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com
>>>>>>>> <mailto:thomas.stuefe at gmail.com>
>>>>>>>> > > <mailto:thomas.stuefe at gmail.com
>>>>>>>> <mailto:thomas.stuefe at gmail.com> > ]
>>>>>>>> > > Sent: Thursday, August 17, 2017 3:48 PM
>>>>>>>> > > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com
>>>>>>>> <mailto:goetz.lindenmaier at sap.com>
>>>>>>>> > > <mailto:goetz.lindenmaier at sap.com
>>>>>>>> <mailto:goetz.lindenmaier at sap.com> > >
>>>>>>>> > > Cc: hotspot-runtime-dev at openjdk.java.net <mailto:
>>>>>>>> hotspot-
>>>>>>>> runtime-dev at openjdk.java.net> <mailto:hotspot-runtime-
>>>>>>>>
>>>>>>> <mailto:hotspot-
>>>>>>
>>>>>>> runtime->
>>>>>>>> > > dev at openjdk.java.net <mailto:dev at openjdk.java.net> >
>>>>>>>> > > Subject: Re: RFR(M): 8186072: dll_build_name
>>>>>>>> returns true
>>>>>>>>
>>>>>>> even
>>>>>>
>>>>>>> if file
>>>>>>>> > > is missing.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Hi Goetz,
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > On Thu, Aug 17, 2017 at 1:35 PM, Lindenmaier, Goetz
>>>>>>>> > > <goetz.lindenmaier at sap.com
>>>>>>>> <mailto:goetz.lindenmaier at sap.com>
>>>>>>>>
>>>>>>> <mailto:goetz.lindenmaier at sap.com
>>>>>>
>>>>>>> <mailto:goetz.lindenmaier at sap.com> > > wrote:
>>>>>>>> > >
>>>>>>>> > > Hi Thomas,
>>>>>>>> > >
>>>>>>>> > > I reworked the whole thing.
>>>>>>>> > >
>>>>>>>> > > First, there is dll_build_name. It just
>>>>>>>> does <name> ->
>>>>>>>> > > lib<name>.so.
>>>>>>>> > >
>>>>>>>> > > Second, I renamed the legacy dll_build_name
>>>>>>>> to
>>>>>>>> dll_locate_lib.
>>>>>>>> > >
>>>>>>>> > > I merged all the unix variants to one in
>>>>>>>> os_posix.
>>>>>>>> > >
>>>>>>>> > > I removed the buffer overflow check at the
>>>>>>>> top.
>>>>>>>> > > It's too restrictive because the path
>>>>>>>> argument
>>>>>>>> > > can contain several paths. I added the
>>>>>>>> overflow
>>>>>>>> > > checks into the single cases.
>>>>>>>> > >
>>>>>>>> > > Also, I first assemble the pure name using
>>>>>>>> the new, simple
>>>>>>>> > > dll_build_name. This is for reuse and
>>>>>>>> readability.
>>>>>>>> > >
>>>>>>>> > > In case of an empty directory, I use
>>>>>>>> get_current_directory
>>>>>>>> > > to complete the path as indicated by the
>>>>>>>> original
>>>>>>>> > > documentation
>>>>>>>> > > where it was called with "".
>>>>>>>> > > Dll_locate_lib now always returns a name
>>>>>>>> with a full path if
>>>>>>>> > > the file exists.
>>>>>>>> > >
>>>>>>>> > > Also, on windows, I think I fixed a bug by
>>>>>>>> reversing the
>>>>>>>>
>>>>>>> order
>>>>>>
>>>>>>> > > of checks. A path list ending in ':' or '\'
>>>>>>>> would not have
>>>>>>>> > > been recognized.
>>>>>>>> > >
>>>>>>>> > > On Bsd, I removed JNI_LIB_* because that
>>>>>>>> already is
>>>>>>>>
>>>>>>> defined
>>>>>>
>>>>>>> > > in jvm_bsh.h
>>>>>>>> > >
>>>>>>>> > > New webrev:
>>>>>>>> > > http://cr.openjdk.java.net/~g
>>>>>>>> oetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > dllBuildName/webrev.02/
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > dllBuildName/webrev.02/>
>>>>>>>> > >
>>>>>>>> > > Best regards,
>>>>>>>> > > Goetz.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > I like this better than before. Remarks:
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>> dllBuildName/webrev.02/src/share/vm/runtime/os.hpp.udiff.html
>>>>>>
>>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>> dllBuildName/webrev.02/src/share/vm/runtime/os.hpp.udiff.html>
>>>>>>
>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > + // Builds the platform-specific name of a
>>>>>>>> library.
>>>>>>>> > >
>>>>>>>> > > + // Returns false on __buffer overflow__.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Hopefully not! :D
>>>>>>>> > >
>>>>>>>> > > How about: "Returns false no truncation" instead.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > + // Builds a platform-specific full library path
>>>>>>>> given an ld path
>>>>>>>> and lib
>>>>>>>> > > name.
>>>>>>>> > >
>>>>>>>> > > + // Returns true if the buffer contains a full
>>>>>>>> path to an existing
>>>>>>>> file,
>>>>>>>> > > false
>>>>>>>> > >
>>>>>>>> > > + // otherwise. If pathname is empty, checks the
>>>>>>>> current
>>>>>>>> directory.
>>>>>>>> > >
>>>>>>>> > > + static bool dll_locate_lib(char*
>>>>>>>> buffer, size_t size,
>>>>>>>> > >
>>>>>>>> > > const char*
>>>>>>>> pathname, const char*
>>>>>>>> fname);
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Might be worth mentioning that "fname" is the
>>>>>>>> unadorned
>>>>>>>>
>>>>>>> library
>>>>>>
>>>>>>> > > name, e.g. "verify" for libverify.so or verify.dll.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Would the following alternative be valid:
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > one could make dll_locate_lib take the real file
>>>>>>>> name, and let
>>>>>>>> caller
>>>>>>>> > > use dll_build_name() to build the libary name first
>>>>>>>> before handing
>>>>>>>>
>>>>>>> it
>>>>>>
>>>>>>> to
>>>>>>>> > > dll_locate_lib(). In that case, dll_locate_lib() could
>>>>>>>> be renamed to
>>>>>>>>
>>>>>>> a
>>>>>>
>>>>>>> generic
>>>>>>>> > > "find_file_in_path" because it would work for any kind
>>>>>>>> of file.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > As an added bonus, there would be no need to create
>>>>>>>> a
>>>>>>>> temporary
>>>>>>>> > > array in dll_build_name/dll_locate_lib, and no need to
>>>>>>>> call free()
>>>>>>>>
>>>>>>> so
>>>>>>
>>>>>>> no
>>>>>>>> > > cleanup-related control flow changes in these functions.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > =====
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>>>
>>>>>> dllBuildName/webrev.02/src/os/windows/vm/os_windows.cpp.udiff.html
>>>
>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>> dllBuildName/webrev.02/src/os/windows/vm/os_windows.cpp.udiff.html>
>>>
>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > + int fullfnamelen = strlen(JNI_LIB_PREFIX) +
>>>>>>>> strlen(fname) +
>>>>>>>> > > strlen(JNI_LIB_SUFFIX);
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > int -> size_t (does that even compile without
>>>>>>>> warning?)
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > + // Check current working directory.
>>>>>>>> > >
>>>>>>>> > > + const char* p = get_current_directory(buffer,
>>>>>>>> buflen);
>>>>>>>> > >
>>>>>>>> > > + if (p != NULL &&
>>>>>>>> > >
>>>>>>>> > > + strlen(buffer) + 1 + fullfnamelen + 1 <=
>>>>>>>> buflen) {
>>>>>>>> > >
>>>>>>>> > > + strcat(buffer, "\\");
>>>>>>>> > >
>>>>>>>> > > + strcat(buffer, fullfname);
>>>>>>>> > >
>>>>>>>> > > + retval = file_exists(buffer);
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Small nit: I'd use jio_snprintf instead of strcat.
>>>>>>>> Functionally
>>>>>>>> identical but
>>>>>>>> > > will make scanners (e.g. coverity) happy. One could
>>>>>>>> then avoid
>>>>>>>>
>>>>>>> the
>>>>>>
>>>>>>> length
>>>>>>>> > > calculation and rely on jio_snprintf truncation:
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > const char* p = get_current_directory(buffer,
>>>>>>>> buflen);
>>>>>>>> > >
>>>>>>>> > > if (p != NULL) {
>>>>>>>> > >
>>>>>>>> > > const size_t end = strlen(p);
>>>>>>>> > >
>>>>>>>> > > if (jio_snprintf(end, buflen - end, "\\%s",
>>>>>>>> fullname) != -1) {
>>>>>>>> > >
>>>>>>>> > > retval = file_exists(buffer);
>>>>>>>> > >
>>>>>>>> > > }
>>>>>>>> > >
>>>>>>>> > > }
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > --
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Not your change, but: why does the code in
>>>>>>>> os::dll_locate_lib()
>>>>>>>> even
>>>>>>>> > > differentiate between a PATH containing no
>>>>>>>> os::path_separator()
>>>>>>>> and a path
>>>>>>>> > > containing os::path_separator()?
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Would the former not be just a PATH with only one
>>>>>>>> directory
>>>>>>>>
>>>>>>> and
>>>>>>
>>>>>>> hence
>>>>>>>> > > need no special treatment?
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > =====
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>> dllBuildName/webrev.02/src/os/posix/vm/os_posix.cpp.udiff.html
>>>>>>
>>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>> dllBuildName/webrev.02/src/os/posix/vm/os_posix.cpp.udiff.html>
>>>>>>
>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Could os::dll_locate_lib be consolidated between
>>>>>>>> windows and
>>>>>>>> unix?
>>>>>>>> > > Seems to be the implementation is almost identical.
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > ====
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>>> dllBuildName/webrev.02/src/share/vm/prims/jvmtiExport.cpp.
>>>>> udiff.html
>>>>>
>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > >
>>>>>>>>
>>>>>>>>
>>>>>> dllBuildName/webrev.02/src/share/vm/prims/jvmtiExport.cpp.udiff.html>
>>>
>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > + // not found - try library path
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Proposal: "not found - try OS default library path"
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Find some comments inline:
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > > Especially if the path is empty, it
>>>>>>>> just returns 'true'.
>>>>>>>> > > > Dll_build_name is usually used
>>>>>>>> before calling dll_load.
>>>>>>>> If
>>>>>>>> > > dll_load does not get a full path it searches
>>>>>>>> > > > in well known unix/windows
>>>>>>>> locations. This is intended
>>>>>>>> in
>>>>>>>> > > the two cases where dll_build_name
>>>>>>>> > > > is called with an empty path.
>>>>>>>> > > >
>>>>>>>> > > > So, for both cases (thread.cpp,
>>>>>>>> jvmtiExport.cpp),
>>>>>>>> > > >
>>>>>>>> > > > before, we would call
>>>>>>>> os::dll_build_name() with an empty
>>>>>>>> > > string for the path
>>>>>>>> > > > which, for relative paths, would result
>>>>>>>> in feeding that path
>>>>>>>> > > unexpanded to
>>>>>>>> > > > dlopen(), which would use whatever the OS
>>>>>>>> does in those
>>>>>>>> > > cases (LIBPATH,
>>>>>>>> > > > LD_LIBRARY_PATH, PATH on windows). Note
>>>>>>>> that this
>>>>>>>>
>>>>>>> does
>>>>>>
>>>>>>> > > not necessarily
>>>>>>>> > > > include searching the current directory.
>>>>>>>> > > Right. With changed dll_biuld_name it's
>>>>>>>> again exactly as
>>>>>>>> > > before.
>>>>>>>> > >
>>>>>>>> > > > With your change, we now use
>>>>>>>> java.library.path, which is
>>>>>>>> not
>>>>>>>> > > necessarily the
>>>>>>>> > > > same?
>>>>>>>> > > You are right, I oversaw that
>>>>>>>> java.library.path can be
>>>>>>>> > > overwritten. Initially,
>>>>>>>> > > it's set to the right thing.
>>>>>>>> > >
>>>>>>>> > > > (BTW, I think the old comments in
>>>>>>>> thread.cpp and
>>>>>>>> > > jniExport.cpp were wrong:"//
>>>>>>>> > > > Try the local directory" - if "local"
>>>>>>>> means "current", this is
>>>>>>>> not
>>>>>>>> > > what did
>>>>>>>> > > > happen).
>>>>>>>> > > Right, I tried to adapt them, did I miss
>>>>>>>> one?
>>>>>>>> > >
>>>>>>>> > > > I added a second variant of
>>>>>>>> dll_build_name without
>>>>>>>>
>>>>>>> the
>>>>>>
>>>>>>> > > path argument that adds the path
>>>>>>>> > > > from system property java.lang.path
>>>>>>>> and use that in
>>>>>>>> these
>>>>>>>> > > two cases.
>>>>>>>> > > > I changed the original function to
>>>>>>>> actually check file
>>>>>>>> > > availability in all cases,
>>>>>>>> > > > and to check . if the path is empty.
>>>>>>>> > > > I think that may be a bit confusing. We
>>>>>>>> would then have
>>>>>>>> three
>>>>>>>> > > options:
>>>>>>>> > > >
>>>>>>>> > > > - call os::dll_build_name with a real
>>>>>>>> "<aa>;<bb>;.." PATH
>>>>>>>> and
>>>>>>>> > > get a file name
>>>>>>>> > > > resolved from that path
>>>>>>>> > > > - call os::dll_build_name with "" for the
>>>>>>>> PATH and get OS
>>>>>>>> dll
>>>>>>>> > > resolution
>>>>>>>> > > No, in that case, as I called
>>>>>>>> file_exists(), it would only work if
>>>>>>>> > > the dll is in the
>>>>>>>> > > current working directory. But I changed
>>>>>>>> this now, anyways.
>>>>>>>> > >
>>>>>>>> > > > - call your new overloaded version of
>>>>>>>>
>>>>>>> os::dll_build_name(),
>>>>>>
>>>>>>> > > which uses -
>>>>>>>> > > > Djava.library.path.
>>>>>>>> > > >
>>>>>>>> > > > Please review this change. I please
>>>>>>>> need a sponsor.
>>>>>>>> > > > http://cr.openjdk.java.net/~g
>>>>>>>> oetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-> >
>>>>>>>> > > > dllBuildName/webrev.01/
>>>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>>>>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-> >
>>>>>>>> > > > dllBuildName/webrev.01/>
>>>>>>>> > >
>>>>>>>> > > >
>>>>>>>> > > > Best regards,
>>>>>>>> > > > Goetz.
>>>>>>>> > > >
>>>>>>>> > > >
>>>>>>>> > > >
>>>>>>>> > > >
>>>>>>>> > > > Kind Regards, Thomas
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > > Best Regards, Thomas
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>> > >
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
More information about the hotspot-runtime-dev
mailing list