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.
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
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> >
> >
> >
> >