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

[Andrew Azores]

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. 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... :)

Thanks,

-- 
Andrew A