Regression after refactoring for XDG specification

Pavel Tisnovsky ptisnovs at redhat.com
Mon Nov 25 07:05:43 PST 2013


Hi Jiri,

the patch itself seems harmless at least :-) but it would be nice to have
a bigger set of reproducers for the most (IMHO common) issues - problems
with privileges, selinux modes, nfs related issues etc. etc. And the
validator class seems not to pass through all branches at the first sight.

A few notes:
+    /** The system'subdirResult deployment.config file */
-> this (missing space) seems to be some strange result of a find&replace?

+        public int getPasses() {
+        public int getFailures() {
-> Seems a bit tricky, need a javadoc!
-> Well all public methods in public classes (even inner static classes) should have a javadoc, please :)

+DCmaindircheckNotexists=After all tryes your configuration directory {0} do not exists
-> tryes??? -> "attempts" would be better AFAIK

Thanks,
Pavel



----- Jiri Vanek <jvanek at redhat.com> wrote:
> 
> 
> 
> -------- Original Message --------
> Subject: Re: [icedtea-web] Regression after refactoring for XDG specification
> Date: Thu, 24 Oct 2013 23:25:42 +0200
> From: Jacob Wisor <gitne at gmx.de>
> To: distro-pkg-dev at openjdk.java.net
> CC: Jiri Vanek <jvanek at redhat.com>
> 
> 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
> 
> 



More information about the distro-pkg-dev mailing list