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

[Andrew Azores]

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,

-- 
Andrew A

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