[icedtea-web] Regression after refactoring for XDG specification

[Jacob Wisor]

Hello!

Jiri Vanek wrote:
> Hi!
> 
> Can i push? This patch will be soon inapplicable, and it is heavily needed.

Generally speaking, as I already mentioned this approach is too complex in my 
opinion. And, I don't care really anymore because the initial requirement or 
intention was to
1. check whether the system implements the XDG specification and
2. if it does, make sure that all necessary parent directories are created in a 
"virgin" user's home directory.
Not much else. So I think this has got a little bit out of hand. But, I won't 
oppose this patch as long as it does what it should do. And, I like the new 
DirectoryValidator class. ;-)
Nevertheless, I would be happier if some other IcedTea-Web hacker would review 
this patch too, with my remarks on it in mind.

I also have to mention that I did not have the time to actually apply this patch 
and test it.

 > diff -r 6124fd87eaba
 > tests/netx/unit/net/sourceforge/jnlp/config/DirectoryValidatorTest.java
 > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
 > +++ b/tests/netx/unit/net/sourceforge/jnlp/config/DirectoryValidatorTest.java
 > Thu Sep 12 11:14:54 2013 +0200
 > @@ -0,0 +1,271 @@
 > +/*
 > + Copyright (C) 2012 Red Hat, Inc.

It's 2013 already ;-)

Since you are so eager to have three different error messages, please at least 
rewrite them into proper and rather simple English. See the examples below.

 > diff -r 6124fd87eaba netx/net/sourceforge/jnlp/resources/Messages.properties
 > --- a/netx/net/sourceforge/jnlp/resources/Messages.properties	Wed Sep 11
 > 12:05:44 2013 +0200
 > +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties	Thu Sep 12
 > 11:14:54 2013 +0200
 > @@ -304,6 +304,9 @@
 >  DCInternal=Internal error: {0}
 >  DCSourceInternal=<internal>
 >  DCUnknownSettingWithName=Property "{0}" is unknown.
 > +DCmaindircheckNotexists=After all tryes your configuration directory {0} do
 > not exists

IcedTea-Web's configuration directory {0} does not exist.

I do not actually understand what you have meant with this message. Should it 
rather say: Could not create IcedTea-Web's configuration directory {0}.?

 > +DCmaindircheckNotdir=Your configuration directory {0} is not directory

IcedTea-Web's configuration directory {0} is a file instead of a directory.

 > +DCmaindircheckRwproblem=Your configuration directory {0} can not be
 > read/written properly

Cannot write to or read from IcedTea-Web's configuration directory {0}.

> […]
> 
> Ok, painfull review indeed!
> 
> I did my best, see below:
> 
> On 09/10/2013 09:34 PM, Jacob Wisor wrote:
>> Jiri Vanek schrieb:
>>> On 07/30/2013 06:14 PM, Jacob Wisor wrote:
> ..snip...
>>>> Nevertheless, thank you for addressing this issue, so please keep up 
>>>> the good work.
>>
>>> +    private static void ensureMainDirs() {
>>
>> I am especially unhappy with this method's naming. I am aware that 
>> naming is a
>> difficult task in CS, but it pays off investing time into it. What 
>> does it
>> ensure main directories about? What happens if something cannot be or 
>> has not
>> been ensured? Does possibly a RuntimeException be thrown? Except from 
>> that, what
>> are main directories supposed to be? Perhaps root directories? What 
>> makes a
>> directory so distinct to be "main"? So, please find an adequate name 
>> and add
>> some documentation.
> 
> Ok. I refactored it a bit and added javadoc. I hope it is much cleaner 
> now. Thanx for well deserved
> kick.

Well, my comment was not meant to sound or feel like a kick. But, it obviously 
has met its purpose. :-D

As a side note; some quote from Disney's Mulan comes to my mind: "But I don't 
want to kick the other kid's butt" :-D

>>
>> Some language stuff:
>>> +                    System.out.println("WARNING: key " + key + " do 
>>> not have
>> value, switching to default");
>>
>> "WARNING: key " + key + " has no value, setting to default value"
>>
>>> +                    System.out.println("WARNING: key " + key + " do 
>>> not have
>> value, skipping");
>>
>> "WARNING: key " + key + " has no value, skipping"
>>
>>> +                    System.err.println("ERROR: key " + key + " 
>>> value: " +
>> value + " not existed, and was NOT created");
>>
>> "ERROR: Directory " + value + " denoted by key " + key + " does not 
>> exist and
>> has not been created"
>>
>>> +                    System.out.println("OK: key " + key + " value: " 
>>> + value
>> + " not existed, and was created");
>>
>> "OK: Directory " + value + " denoted by key " + key + " did not exist 
>> but has
>> been created"
> 
> Used. Thank you.
>>
>>> +    //not private for testing
>>> +    static String testMainDir(File f, boolean verbose) {
>>
>> Since you have made this method not private please add some 
>> documentation on
>> what this method's purpose is and what it does. It is definitely not
>> self-explanatory, even after reading its code. And, as mentioned below 
>> it is
>> probably unnecessary complex and seems to be mixing functional code 
>> with message
> I hope fixed by refactoring.
> 
>> generating code. If you really want to keep this differentiated error 
>> reporting
>> feature, I would strongly advise to deglomerate functional and message
>> generating code.
> 
> Well, nothing else then pure laziness was on my side here:( Sorry!  I 
> have added wrapper class which
> is keeping the symptoms of failures, and message is generated later, 
> based on those flags.
> 
>>
>>> +        StringBuilder messages = new StringBuilder();
>>> +        if (!f.exists()) {
>>> +            String s = (R("DCmaindircheckNotexists", 
>>> f.getAbsolutePath()));
>>> +            if (verbose) {
>>> +                System.err.println(s);
>>> +            }
>>> +            messages.append(s).append("\n");
>>> +        }
>>> +        if (!f.isDirectory()) {
>>> +            String s = (R("DCmaindircheckNotdir", 
>>> f.getAbsolutePath()));
>>> +            if (verbose) {
>>> +                System.err.println(s);
>>> +            }
>>> +            messages.append(s).append("\n");
>>> +        }
>>
>> These two tests are redundant. File.isDirectory() implies a check for 
>> existence.
>> It does not matter to the user or the application whether the desired 
>> directory
>> does not exist or is not a directory. In effect the program cannot read
>> from/write to the desired location.
> 
> Partially yes, and aprtially no. IsDirectory really implies fileExists, 
> but the message then would
> be misleading. I really would like to keep those most common error cases 
> separately.
>>
>>> +        File testFile = null;
>>> +        boolean ok = false;
>>
>> Naming again. What is ok?
> 
> renamed.
>>
>>> +        try {
>>> +            testFile = File.createTempFile("maindir", "check", f);
>>> +            if (testFile.exists()) {
>>> +                ok = true;
>>> +            }
>>> +            try {
>>> +                FileUtils.saveFile("ww", testFile);
>>> +                String s = FileUtils.loadFileAsString(testFile);
>>> +                if (!s.trim().equals("ww")) {
>>> +                    ok = false;
>>> +                }
>>
>> This test misses probably its point because it is testing for creating 
>> a *file*
>> not a *directory*. Most file systems with DACLs distinguish between 
>> /creating/
>> files or directories, and between /creating/ and /writing/ into files or
>> directories, so it is important to be precise here. Furthermore, it tests
>> writing into a file instead of perhaps writing into a file in a 
>> subdirectory.
> 
> Fair enough. I forgot about this case utterly. The test is now testing 
> also subdirectory artificaly
> created inisde real directory (call to testMainDir one more times)
>>
>>> +            } catch (Exception ex) {
>>> +                if (JNLPRuntime.isDebug()) {
>>> +                    ex.printStackTrace();
>>> +                }
>>> +                ok = false;
>>> +            }
>>
>> No really, you should *not* catch arbitrary exceptions. It ain't just 
>> some empty
>> words.
> 
> Well.. There are cases where it brings moore good then wrong. Endless 
> loops are very often
> implemented like:
> public void run(){
> while (true){
> try{
> }catch(Throwable r){
> log(t)
> }
> }
> }
> 
> By otherwords, when exception should not , by no means, disturb the 
> program - eg my case above - it
> does not sound so bad...

Yes indeed, but this is because exceptions in such constructs usually get 
handled properly by restoring or resetting whatever state is necessary so that 
the code in the try block can continue or restart.

> I would like to keep the exception cough as it is.

The core of the problem is that you actually cannot guard any piece of code from 
being subject to exceptions. You can only try handling an exception. In Java, 
there is no way to define a critical section that guarantees successful code 
execution unconditionally (of course, as long as the code is logically and 
formally correct). Btw, this is exactly one reason why Java is not suitable for 
real-time applications per se.
The most commonly forgotten kind of exception is an OutOfMemoryException. This 
exception can disturb the flow of code at any time - not only when creating new 
objects! Almost exactly the same is true for a general RuntimeException. Please 
do not forget that not only Java code can throw exceptions but native code can 
do too. Ignoring all exceptions is neither actually handling an exception nor a 
solution. It is only acceptable if and only if the state of the application 
after the exception has been ignored is consistent or in other words can be 
dealt with by the following code. I am not really sure about it in this case.

>>
>> After all, this method is overly complex and does not add any value 
>> for the
>> user. So I would advise you to discard this method.
>>
>>> diff -r 79bdc074df81 
>>> netx/net/sourceforge/jnlp/resources/Messages.properties
>>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties    Fri 
>>> Aug 30
>> 11:02:08 2013 -0400
>>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties    Tue 
>>> Sep 03
>> 13:56:53 2013 +0200
>>> @@ -304,6 +304,9 @@
>>>  DCInternal=Internal error: {0}
>>>  DCSourceInternal=<internal>
>>>  DCUnknownSettingWithName=Property "{0}" is unknown.
>>> +DCmaindircheckNotexists=After all tryes your configuration directory 
>>> {0} do
>> not exists
>>> +DCmaindircheckNotdir=Your configuration directory {0} is not directory
>>> +DCmaindircheckRwproblem=Your configuration directory {0} can not be
>> read/written properly
>>
>> Although it is nice of you to make precise distinctions between error 
>> types to
>> help the user it is actually not necessary and does not offer any 
>> additional
>> help when analyzing this problem. All these messages can be fused into 
>> one
> 
> I really think that two most common cases file instead of dir and 
> insufficient privledges  should be
> distinguished.
> 
> Unless you stongly insists, I would like to keep the three messages as 
> it was.
>> sufficiently explanatory error message: DCCannotAccessConfig="Cannot 
>> access
>> configuration in file {full path to config file}." This should 
>> probably be
>> enough for an admin, even for a user. I would really advise this 
>> because it
>> makes the code simpler and yet serves its purpose.
> 
> Well.. eh.. yes.m I admit that the code is complex. Now it is even a bit 
> more complex. I hope that
> refactoring made it much more readable, and taht testing is strong enough.
> 
> I would like to keep the "three distinguish messages" but any idea to 
> keep them, check all parts,
> and make the check algorithm a bit simpler is welcomed.
>>
>>

Finally I would like to mention that it have been nice of you that you have 
taken care of it. Good luck!
Jacob