RFR : 8217093: Support extended-length paths in parse_manifest.c on windows

[Langer, Christoph]

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 at oracle.com>; core-libs-
> dev at openjdk.java.net
> Cc: Langer, Christoph <christoph.langer at 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 at oracle.com>
> > Sent: Donnerstag, 24. Januar 2019 13:04
> > To: Baesken, Matthias <matthias.baesken at sap.com>; core-libs-
> > dev at 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