RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Hello, please review this small adjustment to parse_manifest.c . It adds support for extended - length paths (on Window) . (see https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#maximu... For extended length paths ) Additionally , some small memory leaks are fixed in find_file . Bug/webrev : https://bugs.openjdk.java.net/browse/JDK-8217093 http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.0/ Best rergards, Matthias
On 16/01/2019 08:42, Baesken, Matthias wrote:
Hello, please review this small adjustment to parse_manifest.c . It adds support for extended - length paths (on Window) .
(see https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#maximu... For extended length paths )
Additionally , some small memory leaks are fixed in find_file .
Bug/webrev :
If I read this correctly then it will prepend \\?\ which is consistent with what we do in other areas of the platform when presented with a long file name. Have you looked at moving open_jarfile to a platform specific source file?, in this case it might be better if the WIndows specific code were in src/java.base/windows/native/libjli for example. -Alan
Hi Alan, I thought having UNIX+Windows code in this case together at one place would be okay . However, if you prefer separating it, should I put it here ? src/java.base/windows/native/libjli/java_md.c Or do you have a better suggestion ? Best regards, Matthias
From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 16. Januar 2019 09:51 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 16/01/2019 08:42, Baesken, Matthias wrote:
Hello, please review this small adjustment to parse_manifest.c . It adds support for extended - length paths (on Window) .
(see https://docs.microsoft.com/en-us/windows/desktop/fileio/naming- a-file#maximum-path-length-limitation For extended length paths )
Additionally , some small memory leaks are fixed in find_file .
Bug/webrev :
If I read this correctly then it will prepend \\?\ which is consistent with what we do in other areas of the platform when presented with a long file name. Have you looked at moving open_jarfile to a platform specific source file?, in this case it might be better if the WIndows specific code were in src/java.base/windows/native/libjli for example.
-Alan
On 16/01/2019 09:07, Baesken, Matthias wrote:
Hi Alan, I thought having UNIX+Windows code in this case together at one place would be okay . However, if you prefer separating it, should I put it here ?
src/java.base/windows/native/libjli/java_md.c
Or do you have a better suggestion ?
With a few exceptions, the Windows specific code for jli.dll is in src/java.base/windows/native/libji so if it's not too difficult then I think that should be location for the Windows long path support. It's might be parse_manifest_md.c rather than java_md.c as the latter is the platform specific code to go with launcher code in java.c. -Alan
Hi Alan , do you think it is good to start a UNIX parse_manifest_md.c too with just a one-liner in it ? Or do you prefer only having the windows - file with the Windows coding and keeping the UNIX one-liner for open_jarfile in parse_manifest.c ? Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 16. Januar 2019 10:37 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 16/01/2019 09:07, Baesken, Matthias wrote:
Hi Alan, I thought having UNIX+Windows code in this case together at one place would be okay . However, if you prefer separating it, should I put it here ?
src/java.base/windows/native/libjli/java_md.c
Or do you have a better suggestion ?
With a few exceptions, the Windows specific code for jli.dll is in src/java.base/windows/native/libji so if it's not too difficult then I think that should be location for the Windows long path support. It's might be parse_manifest_md.c rather than java_md.c as the latter is the platform specific code to go with launcher code in java.c.
-Alan
On 16/01/2019 14:43, Baesken, Matthias wrote:
Hi Alan , do you think it is good to start a UNIX parse_manifest_md.c too with just a one-liner in it ? Or do you prefer only having the windows - file with the Windows coding and keeping the UNIX one-liner for open_jarfile in parse_manifest.c ?
I think this makes sense. -Alan
Hello Alan, here is a second webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.1/ I moved the Windows-only code into a new file src/java.base/windows/native/libjli/parse_manifest_md.c Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 16. Januar 2019 16:43 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 16/01/2019 14:43, Baesken, Matthias wrote:
Hi Alan , do you think it is good to start a UNIX parse_manifest_md.c too with just a one-liner in it ? Or do you prefer only having the windows - file with the Windows coding and keeping the UNIX one-liner for open_jarfile in parse_manifest.c ?
I think this makes sense.
-Alan
On 23/01/2019 08:29, Baesken, Matthias wrote:
Hello Alan, here is a second webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.1/
I moved the Windows-only code into a new file
src/java.base/windows/native/libjli/parse_manifest_md.c
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ? On the Windows version then your patch is low risk but the retrying after ENOENT is a bit icky. I'm just wondering if you've considered creating an absolute path and adding the prefix when the length is 247 or greater. I also wonder about the share mode specified to CreateFileW where it might be better to specify FILE_SHARE_READ at least so that you don't exclude other processes from also opening the file to read. -Alan
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ?
Hi Alan, do you mean , adding a separate c-file for UNIX containing the function open_jarfile ? I considered this, but did not like the idea to add a separate file for UNIX just for the small function . Your suggestion to use FILE_SHARE_READ is probably the right way to do it , I changed this, second webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.2/ Best Regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Donnerstag, 24. Januar 2019 13:04 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 23/01/2019 08:29, Baesken, Matthias wrote:
Hello Alan, here is a second webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.1/
I moved the Windows-only code into a new file
src/java.base/windows/native/libjli/parse_manifest_md.c
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ?
On the Windows version then your patch is low risk but the retrying after ENOENT is a bit icky. I'm just wondering if you've considered creating an absolute path and adding the prefix when the length is 247 or greater. I also wonder about the share mode specified to CreateFileW where it might be better to specify FILE_SHARE_READ at least so that you don't exclude other processes from also opening the file to read.
-Alan
Hi Matthias, thanks for bringing this enhancement into the OpenJDK. I have a few wishes, though. First of all, I think this method fits perfectly well to jli_util. It does not really have to do with the logic of parsing manifests but it is rather an enhanced open function. So you should declare a function "int JLI_Open (const char* name, int flags);" in jli_util.h. Then, create a platform file jli_util_md.c, at least for Windows (instead of parse_manifest_md.c). I also rather like the idea to symmetrically create a jli_util_md.c file for Unix, although it has very little contents. Alternatively, you can maybe consider using "#define JLI_Open open" in the header file for the Unix version. As for the Windows implementation I have a few stylistic comments: 40 if (fd == -1 && ENOENT == errno) { I'd rather like 40 if (fd == -1 && errno == ENOENT) { 41 #define ELP_PREFIX L"\\\\?\\" Can you move this line out of the function, after the includes? Can you change the indentation of: 64 wfullname_with_prefix = (wchar_t*) malloc((wcslen(ELP_PREFIX) + wfullnamelen + 1) 65 * sizeof(wchar_t)); to: 64 wfullname_with_prefix = (wchar_t*) malloc( 65 (wcslen(ELP_PREFIX) + wfullnamelen + 1) * sizeof(wchar_t)); I also don't like the label/goto syntax so much... don't know if you would want to rewrite this a bit? Maybe you can then also pick up Alan's suggestion to check for the length of 247 and only add the prefix in case it's equal or larger which would save you a malloc operation. Thanks Christoph
-----Original Message----- From: Baesken, Matthias Sent: Donnerstag, 24. Januar 2019 17:19 To: Alan Bateman <Alan.Bateman@oracle.com>; core-libs- dev@openjdk.java.net Cc: Langer, Christoph <christoph.langer@sap.com> Subject: RE: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ?
Hi Alan, do you mean , adding a separate c-file for UNIX containing the function open_jarfile ? I considered this, but did not like the idea to add a separate file for UNIX just for the small function .
Your suggestion to use FILE_SHARE_READ is probably the right way to do it , I changed this, second webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.2/
Best Regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Donnerstag, 24. Januar 2019 13:04 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 23/01/2019 08:29, Baesken, Matthias wrote:
Hello Alan, here is a second webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.1/
I moved the Windows-only code into a new file
src/java.base/windows/native/libjli/parse_manifest_md.c
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ?
On the Windows version then your patch is low risk but the retrying after ENOENT is a bit icky. I'm just wondering if you've considered creating an absolute path and adding the prefix when the length is 247 or greater. I also wonder about the share mode specified to CreateFileW where it might be better to specify FILE_SHARE_READ at least so that you don't exclude other processes from also opening the file to read.
-Alan
On 24/01/2019 16:19, Baesken, Matthias wrote:
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ? Hi Alan, do you mean , adding a separate c-file for UNIX containing the function open_jarfile ? I considered this, but did not like the idea to add a separate file for UNIX just for the small function .
Your suggestion to use FILE_SHARE_READ is probably the right way to do it , I changed this, second webrev :
Christoph Langer's suggestion to use jli_util seems worth exploring to keep the platform specific code together. I still find the call to _open and checking for ENOENT a bit icky. If this change gets pushed then I think we need a follow-up issue created to replace this to use CreateFile consistently. -Alan
Hello Alan / Christoph, new webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.3/ - moved ELP_PREFIX define out of the function - added a check (wfullnamelen > 247) before CreateFileW is attempted - some other formatting (mostly suggested by Christoph) Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Sonntag, 27. Januar 2019 16:57 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Langer, Christoph <christoph.langer@sap.com> Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 24/01/2019 16:19, Baesken, Matthias wrote:
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ? Hi Alan, do you mean , adding a separate c-file for UNIX containing the function open_jarfile ? I considered this, but did not like the idea to add a separate file for UNIX just for the small function .
Your suggestion to use FILE_SHARE_READ is probably the right way to do it , I changed this, second webrev :
Christoph Langer's suggestion to use jli_util seems worth exploring to keep the platform specific code together.
I still find the call to _open and checking for ENOENT a bit icky. If this change gets pushed then I think we need a follow-up issue created to replace this to use CreateFile consistently.
-Alan
Hi Matthias, thanks for the update. I still think you should avoid introducing a file parse_manifest_md.c. As per my previous mail, you should think of a JLI_open function and then you can put its implementation into the Windows version of java_md.c. You can take example on the implementation of JLI_Snprintf which has coding on Windows and for Unix it's just a #define to snprintf. Would it be possible to add a jtreg testcase that shows/tests the issue? Best regards Christoph
-----Original Message----- From: Baesken, Matthias Sent: Montag, 28. Januar 2019 12:38 To: Alan Bateman <Alan.Bateman@oracle.com>; core-libs- dev@openjdk.java.net Cc: Langer, Christoph <christoph.langer@sap.com> Subject: RE: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Hello Alan / Christoph, new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.3/
- moved ELP_PREFIX define out of the function - added a check (wfullnamelen > 247) before CreateFileW is attempted - some other formatting (mostly suggested by Christoph)
Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Sonntag, 27. Januar 2019 16:57 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Langer, Christoph <christoph.langer@sap.com> Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 24/01/2019 16:19, Baesken, Matthias wrote:
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ? Hi Alan, do you mean , adding a separate c-file for UNIX containing the function open_jarfile ? I considered this, but did not like the idea to add a separate file for UNIX just for the small function .
Your suggestion to use FILE_SHARE_READ is probably the right way to do it , I changed this, second webrev :
Christoph Langer's suggestion to use jli_util seems worth exploring to keep the platform specific code together.
I still find the call to _open and checking for ENOENT a bit icky. If this change gets pushed then I think we need a follow-up issue created to replace this to use CreateFile consistently.
-Alan
you should think of a JLI_open function and then you can put its implementation into the Windows version of java_md.c.
Hi Christoph, I like this idea . New webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.4/
Would it be possible to add a jtreg testcase that shows/tests the issue?
Maybe I could put something into the existing jdk/test/jdk/tools/launcher tests. Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Montag, 28. Januar 2019 14:23 To: Baesken, Matthias <matthias.baesken@sap.com>; Alan Bateman <Alan.Bateman@oracle.com>; core-libs-dev@openjdk.java.net Subject: RE: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Hi Matthias,
thanks for the update.
I still think you should avoid introducing a file parse_manifest_md.c. As per my previous mail, you should think of a JLI_open function and then you can put its implementation into the Windows version of java_md.c. You can take example on the implementation of JLI_Snprintf which has coding on Windows and for Unix it's just a #define to snprintf.
Would it be possible to add a jtreg testcase that shows/tests the issue?
Best regards Christoph
-----Original Message----- From: Baesken, Matthias Sent: Montag, 28. Januar 2019 12:38 To: Alan Bateman <Alan.Bateman@oracle.com>; core-libs- dev@openjdk.java.net Cc: Langer, Christoph <christoph.langer@sap.com> Subject: RE: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Hello Alan / Christoph, new webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.3/
- moved ELP_PREFIX define out of the function - added a check (wfullnamelen > 247) before CreateFileW is attempted - some other formatting (mostly suggested by Christoph)
Best regards, Matthias
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Sonntag, 27. Januar 2019 16:57 To: Baesken, Matthias <matthias.baesken@sap.com>; core-libs- dev@openjdk.java.net Cc: Langer, Christoph <christoph.langer@sap.com> Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 24/01/2019 16:19, Baesken, Matthias wrote:
Did you consider adding a Unix version of open_jarfile to avoid #ifdef WIN32 ? Hi Alan, do you mean , adding a separate c-file for UNIX containing the function open_jarfile ? I considered this, but did not like the idea to add a separate file for UNIX just for the small function .
Your suggestion to use FILE_SHARE_READ is probably the right way to do it , I changed this, second webrev :
Christoph Langer's suggestion to use jli_util seems worth exploring to keep the platform specific code together.
I still find the call to _open and checking for ENOENT a bit icky. If this change gets pushed then I think we need a follow-up issue created to replace this to use CreateFile consistently.
-Alan
On 28/01/2019 16:09, Baesken, Matthias wrote:
you should think of a JLI_open function and then you can put its implementation into the Windows version of java_md.c.
Hi Christoph, I like this idea .
New webrev :
This is the one. Looks good ( and clean ). -Chris.
Hi Matthias,
New webrev :
This is the one. Looks good ( and clean ).
I agree, this version would be ok for me to be pushed. It would be good ,though, if there was a test for this (long paths on Windows). I also like Alan's suggestion to open a follow up bug to explore using CreateFile on Windows right away rather than trying open. Looking at libjava/libnet/libnio, it's all CreateFileW on Windows... Best regards Christoph
Hi Christoph+Alan, I opened https://bugs.openjdk.java.net/browse/JDK-8218547 JDK-8218547 : use CreateFile without open on Windows in jdk native code To check for the usage of CreateFile (CreateFileW) without open . Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Dienstag, 29. Januar 2019 09:59 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: Chris Hegarty <chris.hegarty@oracle.com>; core-libs- dev@openjdk.java.net Subject: RE: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Hi Matthias,
New webrev :
This is the one. Looks good ( and clean ).
I agree, this version would be ok for me to be pushed. It would be good ,though, if there was a test for this (long paths on Windows).
I also like Alan's suggestion to open a follow up bug to explore using CreateFile on Windows right away rather than trying open. Looking at libjava/libnet/libnio, it's all CreateFileW on Windows...
Best regards Christoph
On 29/01/2019 08:58, Langer, Christoph wrote:
Hi Matthias,
New webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.4/ This is the one. Looks good ( and clean ). I agree, this version would be ok for me to be pushed. It would be good ,though, if there was a test for this (long paths on Windows).
I also like Alan's suggestion to open a follow up bug to explore using CreateFile on Windows right away rather than trying open. Looking at libjava/libnet/libnio, it's all CreateFileW on Windows... I think the right fix is to convert a relative path into absolute path, apply the prefix for long paths, and then use CreateFileW consistently. It's probably okay to get there in steps but I can't tell from the current webrev if it works with relative paths or not.
-Alan
Hi Matthias, thanks for opening the bug. I refined its description a little... Best regards Christoph
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 6. Februar 2019 10:00 To: Langer, Christoph <christoph.langer@sap.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 29/01/2019 08:58, Langer, Christoph wrote:
Hi Matthias,
New webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.4/ This is the one. Looks good ( and clean ). I agree, this version would be ok for me to be pushed. It would be good ,though, if there was a test for this (long paths on Windows).
I also like Alan's suggestion to open a follow up bug to explore using CreateFile on Windows right away rather than trying open. Looking at libjava/libnet/libnio, it's all CreateFileW on Windows... I think the right fix is to convert a relative path into absolute path, apply the prefix for long paths, and then use CreateFileW consistently. It's probably okay to get there in steps but I can't tell from the current webrev if it works with relative paths or not.
-Alan
Thanks . Looks like there are a few open calls in windows or shared code present , for example : Zlib: gzlib.c hotspot: osSupport_windows.cpp compactHashtable.cpp but mostly it is CreateFile(W) ( not sure if it is always with extended length path support). We could also compare what is done in JDK and in Windows HS os::open int os::open(const char *path, int oflag, int mode) { from os_windows.cpp (this creates an UNC path + uses _wopen ). Best regards, Matthias
-----Original Message----- From: Langer, Christoph Sent: Mittwoch, 6. Februar 2019 10:08 To: Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net; Alan Bateman <Alan.Bateman@oracle.com> Subject: RE: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
Hi Matthias,
thanks for opening the bug. I refined its description a little...
Best regards Christoph
-----Original Message----- From: Alan Bateman <Alan.Bateman@oracle.com> Sent: Mittwoch, 6. Februar 2019 10:00 To: Langer, Christoph <christoph.langer@sap.com>; Baesken, Matthias <matthias.baesken@sap.com> Cc: core-libs-dev@openjdk.java.net Subject: Re: RFR : 8217093: Support extended-length paths in parse_manifest.c on windows
On 29/01/2019 08:58, Langer, Christoph wrote:
Hi Matthias,
New webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8217093.4/ This is the one. Looks good ( and clean ). I agree, this version would be ok for me to be pushed. It would be good ,though, if there was a test for this (long paths on Windows).
I also like Alan's suggestion to open a follow up bug to explore using CreateFile on Windows right away rather than trying open. Looking at libjava/libnet/libnio, it's all CreateFileW on Windows... I think the right fix is to convert a relative path into absolute path, apply the prefix for long paths, and then use CreateFileW consistently. It's probably okay to get there in steps but I can't tell from the current webrev if it works with relative paths or not.
-Alan
participants (4)
-
Alan Bateman
-
Baesken, Matthias
-
Chris Hegarty
-
Langer, Christoph