Script cleanup

- new negative tests
- some copy-paste replacement in the code

Signed-off-by: Laszlo Hornyak <laszlo.hornyak@gmail.com>

utils/src/com/cloud/utils/script/Script.java

@@ -19,11 +19,6 @@
 
 package com.cloud.utils.script;
 
-import com.cloud.utils.PropertiesUtil;
-import com.cloud.utils.concurrency.NamedThreadFactory;
-import com.cloud.utils.script.OutputInterpreter.TimedOutLogger;
-import org.apache.log4j.Logger;
-
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
@@ -43,6 +38,13 @@ import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 
+import org.apache.commons.io.IOUtils;
+import org.apache.log4j.Logger;
+
+import com.cloud.utils.PropertiesUtil;
+import com.cloud.utils.concurrency.NamedThreadFactory;
+import com.cloud.utils.script.OutputInterpreter.TimedOutLogger;
+
 public class Script implements Callable<String> {
     private static final Logger s_logger = Logger.getLogger(Script.class);
 
@@ -167,6 +169,16 @@ public class Script implements Callable<String> {
         return buildCommandLine(command);
     }
 
+    static String stackTraceAsString(Throwable throwable) {
+        //TODO: a StringWriter is bit to heavy weight
+        try(StringWriter out = new StringWriter(); PrintWriter writer = new PrintWriter(out);) {
+            throwable.printStackTrace(writer);
+            return out.toString();
+        } catch (IOException e) {
+            return "";
+        }
+    }
+
     public String execute(OutputInterpreter interpreter) {
         String[] command = _command.toArray(new String[_command.size()]);
 
@@ -259,28 +271,15 @@ public class Script implements Callable<String> {
             return error;
         } catch (SecurityException ex) {
             _logger.warn("Security Exception....not running as root?", ex);
-            StringWriter writer = new StringWriter();
+            return stackTraceAsString(ex);
-            ex.printStackTrace(new PrintWriter(writer));
-            return writer.toString();
         } catch (Exception ex) {
             _logger.warn("Exception: " + buildCommandLine(command), ex);
-            StringWriter writer = new StringWriter();
+            return stackTraceAsString(ex);
-            ex.printStackTrace(new PrintWriter(writer));
-            return writer.toString();
         } finally {
             if (_process != null) {
-                try {
+                IOUtils.closeQuietly(_process.getErrorStream());
-                    _process.getErrorStream().close();
+                IOUtils.closeQuietly(_process.getOutputStream());
-                } catch (IOException ex) {
+                IOUtils.closeQuietly(_process.getInputStream());
-                }
-                try {
-                    _process.getOutputStream().close();
-                } catch (IOException ex) {
-                }
-                try {
-                    _process.getInputStream().close();
-                } catch (IOException ex) {
-                }
                 _process.destroy();
             }
         }
@@ -318,23 +317,15 @@ public class Script implements Callable<String> {
             try {
                 result = interpreter.interpret(reader);
             } catch (IOException ex) {
-                StringWriter writer = new StringWriter();
+                result = stackTraceAsString(ex);
-                ex.printStackTrace(new PrintWriter(writer));
-                result = writer.toString();
             } catch (Exception ex) {
-                StringWriter writer = new StringWriter();
+                result = stackTraceAsString(ex);
-                ex.printStackTrace(new PrintWriter(writer));
-                result = writer.toString();
             } finally {
                 synchronized (this) {
                     done = true;
                     notifyAll();
                 }
-                try {
+                IOUtils.closeQuietly(reader);
-                    reader.close();
-                } catch (IOException ex) {
-                }
-                ;
             }
         }
 

utils/test/com/cloud/utils/ScriptTest.java

@@ -19,6 +19,9 @@
 
 package com.cloud.utils;
 
+import java.io.BufferedReader;
+import java.io.IOException;
+
 import org.apache.commons.lang.SystemUtils;
 import org.apache.log4j.Logger;
 import org.junit.Assert;
@@ -68,7 +71,8 @@ public class ScriptTest {
         script.add("foo");
         script.add("bar", "baz");
         script.set("blah", "blah");
-        Assert.assertEquals("/bin/echo foo bar baz blah blah ", script.toString());
+        Assert.assertEquals("/bin/echo foo bar baz blah blah ",
+                script.toString());
     }
 
     @Test
@@ -86,13 +90,40 @@ public class ScriptTest {
     @Test
     public void testRunSimpleBashScript() {
         Assume.assumeTrue(SystemUtils.IS_OS_LINUX);
-        Assert.assertEquals("hello world!", Script.runSimpleBashScript("echo 'hello world!'"));
+        Assert.assertEquals("hello world!",
+                Script.runSimpleBashScript("echo 'hello world!'"));
+    }
+
+    @Test
+    public void executeWithOutputInterpreter() {
+        Assume.assumeTrue(SystemUtils.IS_OS_LINUX);
+        Script script = new Script("/bin/bash");
+        script.add("-c");
+        script.add("echo 'hello world!'");
+        String value = script.execute(new OutputInterpreter() {
+
+            @Override
+            public String interpret(BufferedReader reader) throws IOException {
+                throw new IllegalArgumentException();
+            }
+        });
+        // it is a stack trace in this case as string
+        Assert.assertNotNull(value);
+    }
+
+    @Test
+    public void runSimpleBashScriptNotExisting() {
+        Assume.assumeTrue(SystemUtils.IS_OS_LINUX);
+        String output = Script.runSimpleBashScript("/not/existing/scripts/"
+                + System.currentTimeMillis());
+        Assert.assertNull(output);
     }
 
     @Test
     public void testRunSimpleBashScriptWithTimeout() {
         Assume.assumeTrue(SystemUtils.IS_OS_LINUX);
-        Assert.assertEquals("hello world!", Script.runSimpleBashScript("echo 'hello world!'", 1000));
+        Assert.assertEquals("hello world!",
+                Script.runSimpleBashScript("echo 'hello world!'", 1000));
     }
 
     @Test