RFR: Fix jtreg test java/lang/ProcessBuilder/Basic.java

[Siebenborn, Axel]

Hi Mikael,
thank you for your looking at my change.

New webrev:

http://cr.openjdk.java.net/~asiebenborn/jtreg_ProcessBuilder/webrev.01/

Comments inline below:

> -----Original Message-----
> From: Mikael Vidstedt [mailto:mikael.vidstedt at oracle.com]
> Sent: Donnerstag, 15. März 2018 21:57
> To: Siebenborn, Axel <axel.siebenborn at sap.com>
> Cc: Volker Simonis <volker.simonis at gmail.com>; portola-
> dev at openjdk.java.net
> Subject: Re: RFR: Fix jtreg test java/lang/ProcessBuilder/Basic.java
> 
> 
> Axel,
> 
> Thanks for looking into and fixing this! A few questions and nits:
> 
> childproc.c:
> 
> The whole exec dance is fascinating to decode, but AFAICT your change is
> correct. Feel free to leave the code as-is, but just for completeness I’ll
> mention that if I read the existing code correctly, removing the first if-clause
> completely should also work (that said, there could well be something subtle
> I’m missing).
> 

Yes, I think you're right. However, I wanted to keep the diff small.

> Can I suggest the following comment:
> 
> "ENOEXEC indicates that the file header was not recognized. The musl C
> library does not implement the fallback to /bin/sh for that case, so fall
> through to the code below which implements that fallback using
> execve_with_shell_fallback."
> 

I changed the comment according to your suggestion.

> 
> Basic.java:
> 
> * There are a couple of additional reference to /bin/false and /bin/true, lines
> 374 and 1997 in the new version, was that deliberate or should they also be
> updated?
> 
I just updated the references, where the executables are copied. 
Running /bin/false and /bin/true works fine as long as we don’t copy them to 
a file with another name. This keeps the diff smaller and we are closer to the 
original test.

Line 373 was a mistake and I changed it back to /bin/true. 
I changed line 1997. Actually, it doesn't matter if we copy the busybox executable
here or the generated shell script, as the test expects an IOException to be thrown 
and the return value doesn't matter. However, using TrueExe here follows the schema 
to use TrueExe, whenever we copy the executable 

> * Missing space after BusyBox on line 616

Fixed.

> 
> * The formatting of the initialization of the "boolean is” on lines 619-620
> seem off
> 

Fixed.

> * Can you please add a comment somewhere about why BusyBox needs
> special treatment - basically what you said in your initial email?
> 

I added a comment.

> * Why was the change to EnglishUnix needed? That is, what is the difference
> in the message?
> 
I have seen the problem during my tests, but I can't reproduce it now. So I reverted that part.

> * There’s an extra space after the added check on line 696
> 
> Cheers,
> Mikael
> 

Thanks,
Axel

> > On Mar 14, 2018, at 1:00 AM, Siebenborn, Axel
> <axel.siebenborn at sap.com> wrote:
> >
> > Good catch Volker!
> > I forgot the link to the webrev:
> > I'm a bit out of practice.
> >
> > http://cr.openjdk.java.net/~asiebenborn/jtreg_ProcessBuilder/webrev/
> >
> > Thanks,
> > Axel
> >
> >
> >
> > -----Original Message-----
> > From: Volker Simonis [mailto:volker.simonis at gmail.com]
> > Sent: Dienstag, 13. März 2018 19:39
> > To: Siebenborn, Axel <axel.siebenborn at sap.com>
> > Cc: portola-dev at openjdk.java.net
> > Subject: Re: RFR: Fix jtreg test java/lang/ProcessBuilder/Basic.java
> >
> > You probably also wanted to post this URL to your webrev :)
> >
> > http://cr.openjdk.java.net/~asiebenborn/jtreg_ProcessBuilder/webrev/
> >
> > In general, how should we proceed with fixes for portola? Just post
> > webrevs or do you expect us to open bugs in JBS for every issue?
> >
> > Thanks,
> > Volker
> >
> >
> > On Tue, Mar 13, 2018 at 12:28 PM, Siebenborn, Axel
> > <axel.siebenborn at sap.com> wrote:
> >> Hi,
> >>
> >> I'm currently looking into jtreg failures,  that occur with the portola jdk.
> >>
> >> A first issue I tackled, was the failing test
> 'java/lang/ProcessBuilder/Basic.java'
> >>
> >> The test copies the executable '/bin/true' and '/bin/false'. However, on
> alpine linux, these are just links to /bin/busybox.
> >> Copying the busbox executable into a file with a different name won't
> result in the expected return codes.
> >>
> >> Another problem with this test is a bug in musl, that probably won't be
> fixed [1]. 'execvp' has not the fallback to /bin/sh.
> >>
> >> Regards,
> >> Axel
> >>
> >> [1] http://www.openwall.com/lists/musl/2018/03/09/2
> >>
> >>