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

[Mikael Vidstedt]

Looks good to me, thanks for making the changes!

The portola/jdk10 repo is, at least until proven otherwise, effectively read-only since it’s supposed to track the jdk10 upstream repo. Can you please rebase the change on portola/portola, verify that it still works, and then send me an hg export patch which I’ll help push.

Cheers,
Mikael

> On Mar 19, 2018, at 1:24 AM, Siebenborn, Axel <axel.siebenborn at sap.com> wrote:
> 
> 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
>>>> 
>>>> 
>