[rfc][icedtea-web] C++-side small fixes (RH 1121549)

[Andrew Azores]

On 07/22/2014 10:12 AM, Omair Majid wrote:
> Hi,
>
> * Jiri Vanek <jvanek at redhat.com> [2014-07-22 05:23]:
>> On 07/21/2014 04:07 PM, Andrew Azores wrote:
>>> This patch addresses the issues covered in [0]. I'm not so familiar with the C++ side and my C++ is
>>> much much rustier than my Java, so this patch may be shockingly rough for its size ;)
> Patch looks mostly okay to me. As Jiri says, though, it does not address
> all issues in the bug you linked.
>
>> 1. Defect type: RESOURCE_LEAK
>> 10.
>> icedtea-web-1.5/plugin/icedteanp/IcedTeaPluginUtils.cc:1154:leaked_handle –
>> Handle variable "plugin_file_log_fd" going out of scope leaks the handle.
> Meh, I can live with this. This log file is opened on startup and used as
> long as the plugin process is live. So its lifetime is approximately
> the lifetime of the entire process. The OS will clean up everything when
> the process exits anyway.
>
>> 1. Defect type: CLANG_WARNING
>> 1. icedtea-web-1.5/plugin/icedteanp/IcedTeaScriptablePluginObject.cc:750:9:warning
>> – Dereference of undefined pointer value
>> 2. Defect type: CLANG_WARNING
>> 1. icedtea-web-1.5/plugin/icedteanp/IcedTeaScriptablePluginObject.cc:728:13:warning
>> – Value stored to 'java_result' is never read
>> 3. Defect type: CLANG_WARNING
>> 1. icedtea-web-1.5/plugin/icedteanp/IcedTeaNPPlugin.cc:377:12:warning –
>> Value stored to 'startup_error' during its initialization is never read
>  From what I can tell, these three should be addressed by the patch
> already. These are duplicates, I believe.
>
>>> diff --git a/plugin/icedteanp/IcedTeaParseProperties.cc b/plugin/icedteanp/IcedTeaParseProperties.cc
>>> --- a/plugin/icedteanp/IcedTeaParseProperties.cc
>>> +++ b/plugin/icedteanp/IcedTeaParseProperties.cc
>>> @@ -133,10 +133,14 @@ string  get_log_dir(){
>>>   			string r1= string(getenv ("XDG_CONFIG_HOME"))+"/icedtea-web";
>>>   			string r2 = r1+"/"+default_itw_log_dir_name;
>>>   			if (!IcedTeaPluginUtilities::file_exists(r1)){
>>> -				g_mkdir(r1.c_str(), 755);
>>> +				if (!g_mkdir(r1.c_str(), 755)) {
>>> +					PLUGIN_ERROR("Failed to create directory %s\n", r1.c_str());
>>> +				}
>>>   			}
>>>   			if (!IcedTeaPluginUtilities::file_exists(r2)){
>>> -				g_mkdir(r2.c_str(), 755);
>>> +				if (!g_mkdir(r2.c_str(), 755)) {
>>> +					PLUGIN_ERROR("Failed to create directory %s\n", r2.c_str());
>>> +				}
>>>   			}
>>>   			return r2;
> PLUGIN_ERROR just logs errors, right? It seems like this code still
> returns the unusable path `r2` in case of an error. Maybe an invalid
> path should be indicated explicitly?
>
>>> @@ -144,10 +148,14 @@ string  get_log_dir(){
>>>   		string r1 = string(mypasswd->pw_dir)+"/.config/icedtea-web";
>>>   		string r2 = r1+"/"+default_itw_log_dir_name;
>>>   		if (!IcedTeaPluginUtilities::file_exists(r1)){
>>> -			g_mkdir(r1.c_str(), 755);
>>> +			if (!g_mkdir(r1.c_str(), 755)) {
>>> +				PLUGIN_ERROR("Failed to create directory %s\n", r1.c_str());
>>> +			}
>>>   		}
>>>   		if (!IcedTeaPluginUtilities::file_exists(r2)){
>>> -			g_mkdir(r2.c_str(), 755);
>>> +			if (!g_mkdir(r2.c_str(), 755)) {
>>> +				PLUGIN_ERROR("Failed to create directory %s\n", r2.c_str());
>>> +			}
>>>   		}
> Same issue here.
>
> Thanks,
> Omair
>

I think really this method and its call site(s) just needs a bunch of 
refactoring. It's called "get_log_dir" so simply returning the location 
of the log directory makes sense, but why should it even be trying to 
create it? Maybe that belongs somewhere else eg the call site(s)? And 
either way this should probably be split into more than one method to 
reduce the duplicate code.

Thanks,

-- 
Andrew A