[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.
>

Okay, I'll post again for the 1.5 patch. In the meantime here's the 
hopefully final patch for HEAD for approval. It's the same as last time 
but basically with s/create_log_dir/create_dir/g and the warning output 
squelched during the tests.

Thanks,

-- 
Andrew A

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