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

[Jiri Vanek]

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.