Address review comments

Author: nvazquez

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -882,7 +882,7 @@ protected enum HealthCheckResult {
     protected StorageSubsystemCommandHandler storageHandler;
 
     private boolean convertInstanceVerboseMode = false;
-    private String[] convertInstanceEnv = null;
+    private Map<String, String> convertInstanceEnv = null;
     protected boolean dpdkSupport = false;
     protected String dpdkOvsPath;
     protected String directDownloadTemporaryDownloadPath;
@@ -947,7 +947,7 @@ public boolean isConvertInstanceVerboseModeEnabled() {
         return convertInstanceVerboseMode;
     }
 
-    public String[] getConvertInstanceEnv() {
+    public Map<String, String> getConvertInstanceEnv() {
         return convertInstanceEnv;
     }
 
@@ -1437,14 +1437,14 @@ private void setConvertInstanceEnv(String convertEnvTmpDir, String convertEnvVir
             return;
         }
         if (StringUtils.isNotBlank(convertEnvTmpDir) && StringUtils.isNotBlank(convertEnvVirtv2vTmpDir)) {
-            convertInstanceEnv = new String[2];
-            convertInstanceEnv[0] = String.format("%s=%s", "TMPDIR", convertEnvTmpDir);
-            convertInstanceEnv[1] = String.format("%s=%s", "VIRT_V2V_TMPDIR", convertEnvVirtv2vTmpDir);
+            convertInstanceEnv = new HashMap<>(2);
+            convertInstanceEnv.put("TMPDIR", convertEnvTmpDir);
+            convertInstanceEnv.put("VIRT_V2V_TMPDIR", convertEnvVirtv2vTmpDir);
         } else {
-            convertInstanceEnv = new String[1];
+            convertInstanceEnv = new HashMap<>(1);
             String key = StringUtils.isNotBlank(convertEnvTmpDir) ? "TMPDIR" : "VIRT_V2V_TMPDIR";
             String value = StringUtils.isNotBlank(convertEnvTmpDir) ? convertEnvTmpDir : convertEnvVirtv2vTmpDir;
-            convertInstanceEnv[0] = String.format("%s=%s", key, value);
+            convertInstanceEnv.put(key, value);
         }
     }
 

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java

@@ -22,10 +22,11 @@
 import java.nio.charset.Charset;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Map;
 import java.util.UUID;
 
 import org.apache.cloudstack.storage.to.PrimaryDataStoreTO;
-import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.lang3.StringUtils;
 
 import com.cloud.agent.api.Answer;
@@ -245,8 +246,8 @@ protected boolean performInstanceConversion(String originalVMName, String source
 
         String logPrefix = String.format("(%s) virt-v2v ovf source: %s progress", originalVMName, sourceOVFDirPath);
         OutputInterpreter.LineByLineOutputLogger outputLogger = new OutputInterpreter.LineByLineOutputLogger(logger, logPrefix);
-        String[] convertInstanceEnv = serverResource.getConvertInstanceEnv();
-        if (ArrayUtils.isEmpty(convertInstanceEnv)) {
+        Map<String, String> convertInstanceEnv = serverResource.getConvertInstanceEnv();
+        if (MapUtils.isEmpty(convertInstanceEnv)) {
             script.execute(outputLogger);
         } else {
             script.execute(outputLogger, convertInstanceEnv);

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

@@ -30,6 +30,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Map;
 import java.util.Properties;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
@@ -42,8 +43,8 @@
 import java.util.stream.Collectors;
 
 import org.apache.cloudstack.utils.security.KeyStoreUtils;
+import org.apache.commons.collections4.MapUtils;
 import org.apache.commons.io.IOUtils;
-import org.apache.commons.lang3.ArrayUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.joda.time.Duration;
@@ -240,11 +241,11 @@ public String execute(OutputInterpreter interpreter) {
         return execute(interpreter, null);
     }
 
-    public String execute(OutputInterpreter interpreter, String[] environment) {
+    public String execute(OutputInterpreter interpreter, Map<String, String> environment) {
         return executeInternal(interpreter, environment);
     }
 
-    public String executeInternal(OutputInterpreter interpreter, String[] environment) {
+    private String executeInternal(OutputInterpreter interpreter, Map<String, String> environment) {
         String[] command = _command.toArray(new String[_command.size()]);
         String commandLine = buildCommandLine(command);
         if (_logger.isDebugEnabled() && !avoidLoggingCommand) {
@@ -254,23 +255,21 @@ public String executeInternal(OutputInterpreter interpreter, String[] environmen
         try {
             _logger.trace(String.format("Creating process for command [%s].", commandLine));
 
-            if (ArrayUtils.isEmpty(environment)) {
-                ProcessBuilder pb = new ProcessBuilder(command);
-                pb.redirectErrorStream(true);
-                if (_workDir != null)
-                    pb.directory(new File(_workDir));
+            ProcessBuilder pb = new ProcessBuilder(command);
+            pb.redirectErrorStream(true);
 
-                _logger.trace(String.format("Starting process for command [%s].", commandLine));
-                _process = pb.start();
-            } else {
-                // Since Runtime.exec() does not support redirecting the error stream, then append 2>&1 to the command
-                String[] commands = new String[] {"sh", "-c", String.format("%s 2>&1", commandLine)};
-                // The PATH variable must be added for indirect calls within the running command
-                // Example: virt-v2v invokes qemu-img, which cannot be found if PATH is not set
-                String[] env = ArrayUtils.add(environment, String.format("PATH=%s", System.getenv("PATH")));
-                _process = Runtime.getRuntime().exec(commands, env, _workDir != null ? new File(_workDir) : null);
+            if (MapUtils.isNotEmpty(environment)) {
+                Map<String, String> processEnvironment = pb.environment();
+                processEnvironment.putAll(environment);
+            }
+
+            if (_workDir != null) {
+                pb.directory(new File(_workDir));
             }
 
+            _logger.trace(String.format("Starting process for command [%s].", commandLine));
+            _process = pb.start();
+
             if (_process == null) {
                 _logger.warn(String.format("Unable to execute command [%s] because no process was created.", commandLine));
                 return "Unable to execute the command: " + command[0];