Hi Thomas,
On 29/08/2017 7:44 PM, Thomas Stüfe wrote:
> Hi,
>
> I am fine with this change going in in the current form. Lets get this
> patch in.
Now I'm questioning some things ...
> I see some issues, but they can be addressed in follow up items:
>
> 1) os::path_seperator() should really return a char, not a string. Most
> callers seem to reference os::path_seperator()[0].
I've often wondered about that too but that's a separate RFE.
> 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.
> 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
> > >
> > >
> > >
> > >
> > >
>
>
>
>
>