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