[ping] RFR(M): 8186072: dll_build_name returns true even if file is missing.
David Holmes
david.holmes at oracle.com
Tue Aug 29 07:53:13 UTC 2017
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/~goetz/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/~goetz/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/~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/
>>>>> > > <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