[rfc][icedtea-web] replacing of hardcoded jre path by function

[Jiri Vanek]

On 03/12/2013 04:42 PM, Adam Domurad wrote:
> On 02/22/2013 08:03 AM, Jiri Vanek wrote:
>> Hi!
>>
>> This is replacing of hardoced path part 2. This is replacing the constants ICEDTEA_WEB_JRE by functions. However those functions are just returning the (ICEDTEA_WEB_JRE) consatnts now :) But they should prepare path for another parts...
>>
>> The goal is to have logic in those functions, which will provide the value from deploy.propertie file(s) (or use default (As is as now) if property will not exists....
>>
>> J
>>
>>
>> ps: this is part of make-jredir-configurable after install effort
>
>> diff -r 5501859497c2 plugin/icedteanp/IcedTeaNPPlugin.cc
>> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc Fri Feb 22 11:43:19 2013 +0100
>> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc Fri Feb 22 13:51:39 2013 +0100
>> @@ -156,8 +156,14 @@
>> // Data directory for plugin.
>> static std::string data_directory;
>>
>> -// Fully-qualified appletviewer executable.
>> -static const char* appletviewer_executable = ICEDTEA_WEB_JRE "/bin/java";
>> +// Fully-qualified appletviewer default executable and rt.jar
>> +static const char* appletviewer_default_executable = ICEDTEA_WEB_JRE "/bin/java";
>> +static const char* appletviewer_default_rtjar = ICEDTEA_WEB_JRE "/lib/rt.jar";
>> +// Fully-qualified appletviewer default executable and rt.jar
>> +// resolved by methods in runtime. It is expected they will be the same during
>> +//whole runtime
>> +static char appletviewer_executable[256];
>> +static char appletviewer_rtjar[256];
>
> Java user afraid of the heap ? :-) These static buffers are not needed.
>

I'm must admint I'm not sure how to achive this without static buffer and with new std::string :(

>>
>> // Applet viewer input channel (needs to be static because it is used in plugin_in_pipe_callback)
>> static GIOChannel* in_from_appletviewer = NULL;
>> @@ -284,6 +290,16 @@
>>
>> #endif
>>
>> +static char* getPluginExecutbale(char* dest){
>
> s/Executbale/Executable/
>
>
>> + strcpy(dest, appletviewer_default_executable);
>> + return dest;
>> +}
>> +
>> +static char* getPluginRtJar(char* dest){
>> + strcpy(dest, appletviewer_default_rtjar);
>> + return dest;
>> +}
>
> I prefer get_plugin_java_executable() & get_plugin_java_runtime_jar(). No big deal though, but I would use underscores for consistency however you name it.
>
> Also please use the signature:
>
> static std::string whatever_name() {
> return ICEDTEA_WEB_JRE "/bin/java"; // or lib/rt.jar
> }
>
> You can get rid of the constant this way, too. Note that char* is valid wherever std::string is taken (but not the other way around).
> Note that for now the signature *could* be const char* -- but I strongly encourage std::string for when this becomes read from the properties.
>
>
>> +
>>
>> /*
>> * Find first member in GHashTable* depending on version of glib
>> @@ -1517,13 +1533,14 @@
>> static NPError
>> plugin_test_appletviewer ()
>> {
>> - PLUGIN_DEBUG ("plugin_test_appletviewer: %s\n", appletviewer_executable);
>> +
>> + PLUGIN_DEBUG ("plugin_test_appletviewer: %s\n", getPluginExecutbale(appletviewer_executable));
>
> Just a hint, this will need a c_str() here when changed to std::string
>
>> NPError error = NPERR_NO_ERROR;
>>
>> gchar* command_line[3] = { NULL, NULL, NULL };
>> gchar** environment;
>>
>> - command_line[0] = g_strdup (appletviewer_executable);
>> + command_line[0] = g_strdup (getPluginExecutbale(appletviewer_executable));
>
> Same
>
>> command_line[1] = g_strdup("-version");
>> command_line[2] = NULL;
>>
>> @@ -1570,7 +1587,7 @@
>>
>> // Construct command line parameters
>>
>> - command_line.push_back(appletviewer_executable);
>> + command_line.push_back(getPluginExecutbale(appletviewer_executable));
>
> Same
>
>>
>> //Add JVM args to command_line
>> for (int i = 0; i < jvm_args->size(); i++)
>> @@ -1581,7 +1598,7 @@
>> command_line.push_back(PLUGIN_BOOTCLASSPATH);
>> // set the classpath to avoid using the default (cwd).
>> command_line.push_back("-classpath");
>> - command_line.push_back(ICEDTEA_WEB_JRE "/lib/rt.jar");
>> + command_line.push_back(getPluginRtJar(appletviewer_rtjar));
>>
>> // Enable coverage agent if we are running instrumented plugin
>> #ifdef COVERAGE_AGENT
>> @@ -1652,10 +1669,10 @@
>> gchar *output = NULL;
>> std::vector<std::string*>* tokenOutput = NULL;
>>
>> - commands.push_back(appletviewer_executable);
>> + commands.push_back(getPluginExecutbale(appletviewer_executable));
>> commands.push_back(PLUGIN_BOOTCLASSPATH);
>> commands.push_back("-classpath");
>> - commands.push_back(ICEDTEA_WEB_JRE "/lib/rt.jar");
>> + commands.push_back(getPluginRtJar(appletviewer_rtjar));
>> commands.push_back("net.sourceforge.jnlp.controlpanel.CommandLine");
>> commands.push_back("get");
>> commands.push_back("deployment.plugin.jvm.arguments");
>> @@ -2175,11 +2192,11 @@
>> }
>>
>> // Set appletviewer_executable.
>> - PLUGIN_DEBUG("Executing java at %s\n", appletviewer_executable);
>> + PLUGIN_DEBUG("Executing java at %s\n", getPluginExecutbale(appletviewer_executable));
>
> Same
>
>> np_error = plugin_test_appletviewer ();
>> if (np_error != NPERR_NO_ERROR)
>> {
>> - fprintf(stderr, "Unable to find java executable %s\n", appletviewer_executable);
>> + fprintf(stderr, "Unable to find java executable %s\n", getPluginExecutbale(appletviewer_executable));
>
> Same
>
>> return np_error;
>> }
>>
>> @@ -2191,7 +2208,7 @@
>>
>> plugin_instance_mutex = g_mutex_new ();
>>
>> - PLUGIN_DEBUG ("NP_Initialize: using %s\n", appletviewer_executable);
>> + PLUGIN_DEBUG ("NP_Initialize: using %s\n", getPluginExecutbale(appletviewer_executable));
>
> Same
>
>>
>> plugin_req_proc = new PluginRequestProcessor();
>> java_req_proc = new JavaMessageSender();
>
> The commands vector will not need the c_str(), because it is a vector of std::string (things are much nicer when everything uses std::string, we should move towards that!)

Hmmm.. I can not see the difference...:(


Itteration 2 attached. Thanx for patience!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: replacingConstWithFunction_2.diff
Type: text/x-patch
Size: 4356 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130319/1e775167/replacingConstWithFunction_2.diff