[RFC][icedtea-web]: Set plugin JVM options in itw-settings
Adam Domurad
adomurad at redhat.com
Wed Oct 31 07:07:51 PDT 2012
On 10/29/2012 03:30 PM, Saad Mohammad wrote:
> Hi,
>
> The patch attached contains a new feature allowing users to set plugin JVM
> options through itw-settings. The patch also cleans up itw-settings UI by
> organizing debugging, desktop integration, and security tab into a single tab
> named Advanced Settings.
>
> A simple way of testing this feature is by adding '-verbose' within settings and
> launching any applet in the web-browser.
>
> You'll also note the patch removes extra spaces after it retrieves jvm options
> from itw-settings. This is because extra spaces can sometimes break the launch
> of the plugin.
>
> Thanks.
>
> CHANGELOG
> ========================================================================
> 2012-10-29 Saad Mohammad <smohammad at redhat.com>
>
> Added new options in itw-settings which allows users to set JVM
> options when plugin is initialized.
> * netx/net/sourceforge/jnlp/config/Defaults.java (getDefaults):
> Added default for DeploymentConfiguration.KEY_PLUGIN_JVM_OPTIONS.
> * netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java:
> Added new property (KEY_PLUGIN_JVM_OPTIONS) that stores the value of
> JVM plugin options.
> * netx/net/sourceforge/jnlp/controlpanel/AdvancedSettingsPanel.java:
> New advanced settings panel containing a single organized view of
> older tabs and plugin settings.
> * netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> (createMainSettingsPanel):
> Added advanced settings to the list of tabs and moved debugging,
> desktop integration, and security from the tab list to the new
> panel.
> * netx/net/sourceforge/jnlp/controlpanel/PluginPanel:
> New plugin settings panel.
> * netx/net/sourceforge/jnlp/resources/Messages.properties:
> Added a new items (CPPluginJVMOptions, CPHeadPlugin,
> CPTabAdvancedSettings) and removed unused ones.
> * plugin/icedteanp/IcedTeaNPPlugin.cc:
> (plugin_start_appletviewer): Adds JVM options to the command that
> initalizes it.
> (get_JVM_options): Returns JVM options set in itw-settings.
> (trim_char): Cleans chars by removing extra spaces and new lines.
> * plugin/icedteanp/IcedTeaNPPlugin.h:
> Declaration of two new methods.
>
Omair already had good points, but I want to summarize some of my tips
regarding avoiding buffer overflows and the like. (and convince you to
use more C++ :)
(Re: what Omair said about static functions, it is worthy to clarify
(because it can be confusing with so many meanings of 'static'), static
function in C/C++ means approximately what 'private static' means in
Java. That is, it is not available outside the source file it was
defined in. Defining a static function in a header file will tell the
compiler there is a separate function in _every_ source file that has
the header.)
> diff --git a/plugin/icedteanp/IcedTeaNPPlugin.cc
> b/plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc
> @@ -1619,16 +1619,29 @@
>
> gchar** command_line;
> gchar** environment;
> -
> + gchar* jvm_options = get_JVM_options();
> +
> + int numberOfCommandsToAllocate = 0;
> int cmd_num = 0;
> +
> + if(jvm_options != NULL){
> + numberOfCommandsToAllocate++;
> + printf("Using user specified JVM options: '%s'\n", jvm_options);
> + }
> +
> if (plugin_debug)
> {
> - command_line = (gchar**) malloc(sizeof(gchar*)*11);
> + numberOfCommandsToAllocate += 11;
I notice a lot of details related to the amount of commands that might
be added. Since this plugin relies on C++ anyway please consider using:
std::vector<gchar*> command_line;
command_line.push_back("Whatever");
Or, better yet
std::vector<std::string> command_line;
command_line.push_back("Whatever");
gchar* -> std::string is perfectly valid.
> + command_line = (gchar**) malloc(sizeof(gchar*) *
> numberOfCommandsToAllocate);
> command_line[cmd_num++] = g_strdup(appletviewer_executable);
> command_line[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
> // set the classpath to avoid using the default (cwd).
> command_line[cmd_num++] = g_strdup("-classpath");
> command_line[cmd_num++] = g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE);
> +
> + if (jvm_options != NULL)
> + command_line[cmd_num++] = g_strdup(jvm_options);
> +
> command_line[cmd_num++] = g_strdup("-Xdebug");
> command_line[cmd_num++] = g_strdup("-Xnoagent");
> if (plugin_debug_suspend)
> @@ -1644,8 +1657,13 @@
> command_line[cmd_num] = NULL;
> } else
> {
> - command_line = (gchar**) malloc(sizeof(gchar*)*8);
> + numberOfCommandsToAllocate += 10;
> + command_line = (gchar**) malloc(sizeof(gchar*) *
> numberOfCommandsToAllocate);
> command_line[cmd_num++] = g_strdup(appletviewer_executable);
> +
> + if (jvm_options != NULL)
> + command_line[cmd_num++] = g_strdup(jvm_options);
> +
> command_line[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
> command_line[cmd_num++] = g_strdup("-classpath");
> command_line[cmd_num++] = g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE);
> @@ -1658,7 +1676,7 @@
> environment = plugin_filter_environment();
>
> if (!g_spawn_async (NULL, command_line, environment,
> - (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD,
> + (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD,
> NULL, NULL, &appletviewer_pid, &channel_error))
> {
> if (channel_error)
> @@ -1673,6 +1691,7 @@
> error = NPERR_GENERIC_ERROR;
> }
>
> + //Free memory
> g_strfreev (environment);
>
> for (int i = 0; i < cmd_num; i++) {
> @@ -1683,18 +1702,106 @@
> g_free(command_line);
> command_line = NULL;
>
> + if (jvm_options != NULL){
> + g_free(jvm_options);
> + jvm_options = NULL;
> + }
> +
> if (appletviewer_pid)
> {
> PLUGIN_DEBUG("Initialized VM with pid=%d\n", appletviewer_pid);
> appletviewer_watch_id = g_child_watch_add(appletviewer_pid,
> (GChildWatchFunc) appletviewer_monitor, (gpointer) appletviewer_pid);
> }
>
> -
> PLUGIN_DEBUG ("plugin_start_appletviewer return\n");
> return error;
> }
>
> /*
> + * Returns JVM options set in itw-settings
> + */
> +static gchar*
> +get_JVM_options()
> +{
> + gchar** commands;
> + gchar* cmd_line_str;
> + gchar *output;
> + int cmd_num = 0;
> + int cmd_length = 0; //Characters length of the full command
> +
> + commands = (gchar**) malloc(sizeof(gchar*) * 8);
> + commands[cmd_num++] = g_strdup(appletviewer_executable);
> + commands[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
> + commands[cmd_num++] = g_strdup("-classpath");
> + commands[cmd_num++] = g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE);
> + commands[cmd_num++] =
> g_strdup("net.sourceforge.jnlp.controlpanel.CommandLine");
> + commands[cmd_num++] = g_strdup("get");
> + commands[cmd_num++] = g_strdup("deployment.plugin.jvm.arguments");
> + commands[cmd_num] = NULL;
> +
> + //Calculate the character length of the commands
> + for (int i = 0; i < cmd_num; i++) {
> + cmd_length += strlen(commands[i]);
> + }
> +
> + cmd_line_str = (gchar*) malloc(sizeof(gchar*) * cmd_length + 1);
As I said on IRC, this is actually not enough buffer space. I'd say use
std::string and rest easy that you don't cause a buffer overflow :).
> + strcpy(cmd_line_str, "");
> +
> + for (int i = 0; i < cmd_num; i++){
> + strcat(cmd_line_str, commands[i]);
> + strcat(cmd_line_str, " ");
> +
> + //Free memory
> + g_free (commands[i]);
> + commands[i] = NULL;
> + }
> + strcat(cmd_line_str, "\0");
strcat'ing the null character is never necessary. The way strcat works
is by linearly scanning the string, finding where the '\0' is, and
adding the string to add. (Note, as a result, std::string can be more
efficient in this regard, since it stores length.)
Please use strncat wherever possible. Strcat is quite unsafe (it is not
buffer-size aware) (again, this is where std::string will safe you
headache.)
> +
> + //Get JVM options set in itw-settings
> + g_spawn_command_line_sync(cmd_line_str, &output, NULL, NULL,
> &channel_error);
> +
> + //Free memory
> + g_free(commands);
> + g_free(cmd_line_str);
> +
> + output = (gchar*) trim_char(output);
> + if (strlen(output) < 1 || strcmp(output, "null") == 0){
> + g_free(output);
> + output = NULL;
> + }
> + return output;
> +}
> +
> +/*
> + * Removes newlines and extra spaces from char
> + */
> +static char*
> +trim_char(char* to_trim)
> +{
> + if (to_trim == '\0'){
> + return to_trim;
> + }
> +
> + char* trimmed = (char*) calloc(((strlen(to_trim)) + 1), sizeof(char));
> + strcpy(trimmed, "");
> +
> + for (int i = 0; i < strlen(to_trim); i++){
This loop condition does a linear search of the string _every
iteration_. It will be O(N^2) time!
> + //Removes extra spaces and new lines
> + if (!(to_trim[i] == '\n' || i > 0 && to_trim[i] == ' ' &&
> to_trim[i - 1] == ' ')){
> + char* orig_char = (char*) calloc(2, sizeof(char));
No reason at all to dynamically allocate a 2 character string here. If
you really wanted the convenience of a 2 character string, you can do:
char orig_char[2];
And the rest of the code would be as-is, and the free would not be
needed. (But strcat is a lot to add one char anyway).
> + orig_char[0] = to_trim[i];
> + orig_char[1] = '\0';
> +
> + strcat(trimmed, orig_char);
> +
> + free(orig_char);
> + orig_char = NULL;
> + }
> + }
> + return trimmed;
> +}
Strongly recommend something like here:
http://stackoverflow.com/questions/216823/whats-the-best-way-to-trim-stdstring
Specifically:
> |std::string s;
> s.erase(s.find_last_not_of(" \n\r\t")+1);
> |
Seems sufficient.
I hope you don't find C/C++ too painful :)
Best regards,
- Adam
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121031/8b4b8123/attachment.html
More information about the distro-pkg-dev
mailing list