From 5f800f2d229b83bef239b2fc494e43112a048215 Mon Sep 17 00:00:00 2001 From: Laszlo Hornyak Date: Sun, 30 Mar 2014 22:33:20 +0200 Subject: [PATCH] Script cleanup - new negative tests - some copy-paste replacement in the code Signed-off-by: Laszlo Hornyak --- utils/src/com/cloud/utils/script/Script.java | 59 +++++++++----------- utils/test/com/cloud/utils/ScriptTest.java | 37 +++++++++++- 2 files changed, 59 insertions(+), 37 deletions(-) diff --git a/utils/src/com/cloud/utils/script/Script.java b/utils/src/com/cloud/utils/script/Script.java index dd71cabc6d4..d2656a8e574 100755 --- a/utils/src/com/cloud/utils/script/Script.java +++ b/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 { private static final Logger s_logger = Logger.getLogger(Script.class); @@ -167,6 +169,16 @@ public class Script implements Callable { 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 { return error; } catch (SecurityException ex) { _logger.warn("Security Exception....not running as root?", ex); - StringWriter writer = new StringWriter(); - ex.printStackTrace(new PrintWriter(writer)); - return writer.toString(); + return stackTraceAsString(ex); } catch (Exception ex) { _logger.warn("Exception: " + buildCommandLine(command), ex); - StringWriter writer = new StringWriter(); - ex.printStackTrace(new PrintWriter(writer)); - return writer.toString(); + return stackTraceAsString(ex); } finally { if (_process != null) { - try { - _process.getErrorStream().close(); - } catch (IOException ex) { - } - try { - _process.getOutputStream().close(); - } catch (IOException ex) { - } - try { - _process.getInputStream().close(); - } catch (IOException ex) { - } + IOUtils.closeQuietly(_process.getErrorStream()); + IOUtils.closeQuietly(_process.getOutputStream()); + IOUtils.closeQuietly(_process.getInputStream()); _process.destroy(); } } @@ -318,23 +317,15 @@ public class Script implements Callable { try { result = interpreter.interpret(reader); } catch (IOException ex) { - StringWriter writer = new StringWriter(); - ex.printStackTrace(new PrintWriter(writer)); - result = writer.toString(); + result = stackTraceAsString(ex); } catch (Exception ex) { - StringWriter writer = new StringWriter(); - ex.printStackTrace(new PrintWriter(writer)); - result = writer.toString(); + result = stackTraceAsString(ex); } finally { synchronized (this) { done = true; notifyAll(); } - try { - reader.close(); - } catch (IOException ex) { - } - ; + IOUtils.closeQuietly(reader); } } diff --git a/utils/test/com/cloud/utils/ScriptTest.java b/utils/test/com/cloud/utils/ScriptTest.java index 48489886cc7..99059bf926c 100644 --- a/utils/test/com/cloud/utils/ScriptTest.java +++ b/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