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

[Jiri Vanek]

On 07/30/2014 08:08 PM, Andrew Azores wrote:
> 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.
>

I would check the mkdir.

I would go with


x = g_mkdir(s)
if (!x){
log_debug_only(creation of dir s failed)
}

Nothing more.

Maye you can have simple g_mkdir_checked function to avoid duplications.

If yo are ok with it then ok for 1.5 agter this change.

+- do as you wish. The patch is ok.
J.