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

[Andrew Azores]

On 07/30/2014 09:47 AM, Jiri Vanek wrote:
> On 07/28/2014 11:53 PM, Andrew Azores wrote:
>> On 07/28/2014 11:14 AM, Jiri Vanek wrote:
>>>
>>>> diff --git a/plugin/icedteanp/IcedTeaPluginUtils.cc 
>>>> b/plugin/icedteanp/IcedTeaPluginUtils.cc
>>>> --- a/plugin/icedteanp/IcedTeaPluginUtils.cc
>>>> +++ b/plugin/icedteanp/IcedTeaPluginUtils.cc
>>>> @@ -59,6 +59,19 @@
>>>>   /* Plugin async call queue */
>>>>   static std::vector< PluginThreadCall* >* 
>>>> pendingPluginThreadRequests = new std::vector<
>>>> PluginThreadCall* >();
>>>>
>>>> +int
>>>> +IcedTeaPluginUtilities::createLogDirectory(std::string dir)
>>>> +{
>>>> +    if (!IcedTeaPluginUtilities::file_exists(dir))
>>>> +    {
>>>> +        if (!g_mkdir(dir.c_str(), 755))
>>>> +        {
>>>> +            return errno;
>>>> +        }
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>> ...
>>> > +        int parentErr = 
>>> IcedTeaPluginUtilities::createLogDirectory(r1);
>>> > +        if (!parentErr){
>>> > +            PLUGIN_ERROR("FATAL: Failed to create IcedTea-Web 
>>> config directory %s: %s\n",
>>> r1.c_str(), strerror(parentErr));
>>> > +            return NULL;
>>>          }
>>>
>>> When you are her, you should also verify (if it exists then) whether 
>>> it is directory or file. If
>>> File, return failure.
>>>
>>> I would discourage you from returning errno.  I would say Print 
>>> error messages here, and return
>>> true/false.
>>>
>>> true/false is the only indicator you need if you need to decide 
>>> wheter continue or not.
>>>
>>> And I would suggest continue at all cost, but print out a warning 
>>> that it probably will nto work.
>>>
>>> So I would go with:
>>>
>>> bool IcedTeaPluginUtilities::createLogDirectory(std::string dir)
>>> {
>>> if (!file_exists(dir) )
>>>     {
>>>         if (!g_mkdir(dir.c_str(), 755))
>>>             int x=errno
>>>             echo "warning, creation of $dir failed. Itw need this 
>>> direcotry to work. You can
>>> expect failre later. reason: "error_toString(x)
>>> //always save errorn
>>>             return false;
>>>         }
>>>     } else {
>>>         if (!is_dir(dir)){
>>>             echo "warning, cannot create  $dir  becasue there is 
>>> already an file"
>>>             return false
>>>         }
>>>     }
>>>     return true;
>>> //feel free to add some debug only outputs here like exists, 
>>> continuing, is dir, continuing...
>>> }
>>> int parentErr = IcedTeaPluginUtilities::createLogDirectory(r1);
>>> // warnings have been printed
>>> // no need to die, trying to continue
>>>
>>>
>>>
>>>
>>> Also.. :-/ .. your patch kills ITW at my machine
>>>
>>> [unknown user][ITW-C-PLUGIN][MESSAGE_DEBUG][Mon Jul 28 16:11:03 CEST
>>> 2014][/home/jvanek/hg/icedtea-web/plugin/icedteanp/IcedTeaParseProperties.cc:141] 
>>> ITNPP Thread#
>>> 139757752895360, gthread 0x7f1bd2614120: FATAL: Failed to create 
>>> IcedTea-Web config directory
>>> /home/jvanek/.config/icedtea-web: Success
>>>
>>> and firefox dies.  Please run your code on soem usecases before 
>>> posting.
>>>
>>> Well the /home/jvanek/.config/icedtea-web do exist at my pc - so 
>>> somethig is wrong :)
>>
>> :S I could swear it worked for me, not sure what happened there. 
>> Maybe I made a change and failed to
>> test that one before sending :/
>>
>>>
>>>
>>> My reason to try continuing is simple - There may be cases, when 
>>> .confing/itw may be some special
>>> device - well nearly impossible, but still possible.
>>>
>>>
>>> Another note - sinc you created new method - please unittest it in 
>>> cpp-unit-tests.
>>>
>>>
>>> And positive note at the end. I run covescan on your patch and not 
>>> sure what to read -
>>> http://cov01.lab.eng.brq.redhat.com/covscanhub/task/13258/ :))
>>> J.
>>>
>>>
>>>
>>
>> New patch has tests added and some style cleanup done, and works 
>> again (hopefully - seems to work
>> fine for me and unit tests are a little reassuring).
>>
>> Thanks,
>>
>
> Is this output of test expected? (yes, the code is doing it :) , but 
> should not be :) )
> Passed: is_directory
> WARNING: Needed to create directory /tmp/fileSGg0Vp but there is 
> already a file of the same name at this location.
> WARNING: Failed to create new directory /tmp/filecvV1sH/test. Reason: 
> Permission denied
> Passed: create_log_dir
>
>
> I mean the warnings. I thing they are disturbing the output and should 
> not be here.
>
> I think there are exampels in ccp tests, how to redirect the output. 
> May you try them?
>
> Except this I'm moreover happy with patch.
>
> One nit
>
> you called it "create_log_dir"  I would go with generic "create_dir" 
> If the redirect of output and renaming  will be smooth, ok for head.
>
>
> For 1.5 - I'm for a bit less invasive  patch. One of the earlier 
> versions was more suitable. I would Discourage you from any behaviour 
> changes (like new error/warning messages). Please reconsider benefits 
> and post 1.5 patch separately.  (unless it is completely same as this 
> one)
>
> tyvm!
>
> J.
>

Attached is the promised 1.5 backport patch. The rest of the coverity 
issues are handled exactly the same as in HEAD. Creating the log 
directory however has hardly changed now, with simply reduced duplicate 
code. No new methods or error messages. This means that the coverity 
issue of the mkdir results not being checked will still be present in 
1.5/1.5.1 though.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: rh1121549-1.5-backport.patch
Type: text/x-patch
Size: 4238 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140730/c9b7b4e9/rh1121549-1.5-backport.patch>