[ping] RFR(M): 8186072: dll_build_name returns true even if file is missing.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Aug 29 08:29:15 UTC 2017
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?
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