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

[Jiri Vanek]

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.