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

[Jiri Vanek]

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.


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)