On 29/08/2017 8:11 PM, Lindenmaier, Goetz wrote:
> Hi,
>
> Thomas, thanks for looking at the new webrev ad hoc.
>
>>> 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.
>>
>> That seems a much bigger change. My question before pushing this is:
>> have we in any way reduced the size of path that we may accept on some
>> platforms? If we have that would be bad.
> No. I nowhere change the size of the buffer passed into this method,
> and that's the limit.
Thanks for confirming - I will push this now.
David
-----
> On posix, paths no more contain // where concatenated, so the
> paths supported can contain one more char in this case. This is an
> improvement.
> The buffer passed in usually has size MAX_PATH, so the checks for
> overflow should never cause a problem. They are mostly there
> for safety. Allocation of the memory rather would reduce memory
> consumption, but I think it's not that relevant. Overall, I think it's
> a mismatch that some functions allocate the memory, others
> require a buffer ... but that's really out of scope.
>
> Best regards,
> Goetz.
>
>
>
>
>>
>>> 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?
>>
>> IIUC the existing windows code supports this. So yes c:foo.dll is a
>> reference to foo.dll in whatever the current directory on drive c: is.
>> As for a usecase ... perhaps a way to workaround long paths or
>> historical path format restrictions (ie no spaces) ? But the main thing
>> is to not break what we currently support.
>>
>> Thanks,
>> David
>>
>>> Kind Regards, Thomas
>>>
>>>
>>> On Tue, Aug 29, 2017 at 10:41 AM, David Holmes
>> <david.holmes at oracle.com
>>> <mailto: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/
>>> <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
>>> <mailto:david.holmes at oracle.com>]
>>> Sent: Dienstag, 29. August 2017 09:53
>>> 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: [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-
>>> <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
>>> <mailto:david.holmes at oracle.com>>
>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>> <mailto: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
>>> <mailto:david.holmes at oracle.com>]
>>> Sent: Montag, 28. August 2017 07:38
>>> 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: [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-
>>> <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
>>> <mailto:thomas.stuefe at gmail.com>]
>>> Sent: Dienstag, 22. August 2017 17:30
>>> 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.
>>>
>>> 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>
>>> <mailto: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-
>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>> dllBuildName/webrev.04
>>>
>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>> <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>
>>> <mailto: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>
>>> <mailto:hotspot- <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,
>>> >
>>> > could I please get a second
>>> review?
>>> >
>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName-
>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName->
>>> hs/webrev.04
>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>> <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>
>>> <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>
>>> <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- <mailto:hotspot->
>>> runtime-dev at openjdk.java.net
>>> <mailto: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>>
>>>
>>> <mailto: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->
>>> <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->
>>> <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->
>>> <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->
>>> <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>>
>>> > >
>>> <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>>
>>> > >
>>> <mailto: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- <mailto:hotspot->
>>> runtime-dev at openjdk.java.net
>>> <mailto:runtime-dev at openjdk.java.net>>
>>> <mailto:hotspot-runtime-
>>> <mailto:hotspot-runtime->
>>>
>>> <mailto:hotspot- <mailto:hotspot->
>>>
>>> runtime->
>>> > > dev at openjdk.java.net
>>> <mailto:dev at openjdk.java.net>
>>> <mailto: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>>
>>>
>>> <mailto: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->
>>> <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->
>>> <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->
>>> <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->
>>> <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->
>>> <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->
>>> <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->
>>> <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.ht
>>> <http://os_posix.cpp.udiff.ht>ml
>>>
>>> > >
>>> <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.02/src/os/posix/vm/os_posix.cpp.udiff.ht
>>> <http://os_posix.cpp.udiff.ht>ml>
>>>
>>> > >
>>> > >
>>> > >
>>> > > 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->
>>> <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->
>>> <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->>
>>> > >
>>> <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->>
>>> > >
>>> <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
>>> > >
>>> > >
>>> > >
>>> > >
>>> > >
>>>
>>>
>>>
>>>
>>>