[rfc][icedtea-web] policytool in itweb-settings

[Jiri Vanek]

On 01/22/2014 02:35 PM, Jiri Vanek wrote:
> On 01/21/2014 04:06 PM, Andrew Azores wrote:
>> On 01/21/2014 06:25 AM, Jiri Vanek wrote:
>>> On 01/20/2014 09:11 PM, Andrew Azores wrote:
>>>> On 01/20/2014 02:50 PM, Andrew Azores wrote:
>>>>> On 01/20/2014 07:27 AM, Jiri Vanek wrote:
>>>>>> On 01/17/2014 10:08 PM, Andrew Azores wrote:
>>>>>>> (snip)
>>>>>>
>>>>>> Ouch. See my reply to patch.
>>>>>
>>>>> Didn't see any mail, but we discussed on IRC. Here's a recap:
>>>>> - The reflection hack is horrible. It doesn't quite work, and it's super ugly.
>>>>> - The process not exiting is definitely a problem. We should not be playing around with reflection on the policytool like this.
>>>>> - Calling PolicyTool.main() directly breaks the automated build(?). We don't want to make it a build dep, and it's too minor to bother with a configure step/flag(?).
>>>>> - Solution: Runtime#exec() makes its return, with a fallback method of reflectively invoking PolicyTool.main() if for any reason the exec fails (non-POSIX system, executable not on PATH, etc)
>>>>>
>>>>> So here's a patch implementing this solution, with lots of extra added documentation.
>>>>>
>>>>
>>>> And this time with better respect for event dispatch thread, I hope. (thanks omajid)
>>>>
>>>> Thanks,
>>>
>>> Only one *the* nit :)
>>>
>>>
>>>> /**
>>>> + * We do this so we can asynchronously monitor the exit status of the policytool process.
>>>> + * If it fails immediately and we need to fall back to using reflectivePolicyToolLaunch,
>>>> + * then the Thread spawned here is very short lived. However, if the policytool process
>>>> + * successfully launches, then it takes some unknown amount of time before it closes
>>>> + * again. We do not want to call waitFor() on the ptool process in the main thread
>>>> + * because this will cause the itweb-settings window to lock up.
>>>> + * @param frame a {@link JFrame} to act as parent to warning dialogs which may appear
>>>> + * @param filePath a {@link String} representing the path to the file to be opened
>>>> + * @throws IOException if an IOException occurs during the exec of the policytool process
>>>> + */
>>>> + private static void policyToolLaunchHelper(final JFrame frame, final String filePath) {
>>>> + final String[] launchCommand = new String[] { "policytool", "-file", filePath };
>>>> + new Thread(new Runnable() {
>>>> + @Override
>>>> + public void run() {
>>>> + try {
>>>> + final Process ptool = Runtime.getRuntime().exec(launchCommand);
>>>> + if (ptool.waitFor() != 0) {
>>>> + reflectivePolicyToolLaunch(filePath);
>>>> + }
>>>> + } catch (Exception e) {
>>>> + OutputController.getLogger().log(e);
>>>> + showCouldNotOpenFileDialog(frame, filePath, R("CPPolicyEditorNotFound"));
>>>> + }
>>>> + }
>>>> + }).start();
>>>> + }
>>>> +
>>>
>>>
>>> I would discourage you from this, By doing this you will lunch the main classed fallback even in case of failure in spawned jvm.
>>>
>>> I would go with:
>>>
>>> Integer lunched = null;
>>> ...
>>> if (lunched == null){
>>> final Process ptool = Runtime.getRuntime().exec("policytool --help");
>>> //it only prints somethig to stdou
>>> lunched = ptool.waitFor();
>>> }
>>> if (lunched == null){ throw/log Something human readable or run fallback or whatewer}
>>> if (lunched ==0){run async proces} else {run fallback}
>>>
>>>
>>> otherwise ok!
>>>
>>> J.
>>>
>>>
>>
>> Well, the first null check seems useless if lunched isn't used anywhere else.
>
>
> Mostly correct, anyway, it was pseudocode.
>
>  > Then, policytool doesn't actually accept anything except '-file' - it has no '-help' or '--help'-. So that process you're exec'ing always fails just about immediately with a non-zero (1) exit status. lunched will never be null after assigning ptool.waitFor() to it, unless some exception is thrown and handling logic is added, so the lunched == null case also seems useless. Then, lunched is always non-zero because there is no --help, so we never run async process to launch policytool properly with a -file, and always resort to fallback. *If* there was a --help switch which could be used to run policytool headless with a zero exit
>> status, then this would all work nicely... :)
>
> hmhm. unluckily correct. My overlook.
>
>>
>> Thanks,
>>
>
> Instead of wait for, you should use process builderwith its start. The process is then synchronous.
>
>
> Also there is "This method checks that the command is a valid operating system command. " in javadoc, so then the fallback should be used.
>
It is the something like:
java.io.IOException: Cannot run program "file.exe": error=2, No such file or...
>
> As for backport. I'm still for it. Bit as +1 for me, and -1 for Jacob, then its up to you... I still think that its is minor modification. Just making long time implemented stuff visible (like console for 1.4.2)