From 4535305c1d3aef15a575e22040f4c844394ed1be Mon Sep 17 00:00:00 2001
From: Arne Wiebalck <Arne.Wiebalck@cern.ch>
Date: Wed, 5 Jun 2019 16:42:37 +0200
Subject: [PATCH] Software RAID: Fix shell commands

Pass an array to utils.execute() or enforce string usage
via 'shell=True', but don't mix.

Follow-up to Id20302537f7994982c7584af546a7e7520e9612b

Change-Id: I2e6c628360aecf81039089af78b19fe6a956e564
---
 ironic_python_agent/hardware.py               | 24 +++++++++----------
 .../tests/unit/test_hardware.py               | 12 +++++-----
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py
index a750283a2..b1400f041 100644
--- a/ironic_python_agent/hardware.py
+++ b/ironic_python_agent/hardware.py
@@ -183,7 +183,7 @@ def _is_md_device(raid_device):
     :returns: True if the device is an md device, False otherwise.
     """
     try:
-        utils.execute("mdadm --detail {}".format(raid_device))
+        utils.execute('mdadm', '--detail', raid_device)
         LOG.debug("%s is an md device", raid_device)
         return True
     except processutils.ProcessExecutionError:
@@ -201,10 +201,9 @@ def _md_restart(raid_device):
     """
     try:
         component_devices = _get_component_devices(raid_device)
-        cmd = "mdadm --stop {}".format(raid_device)
-        utils.execute(cmd)
-        utils.execute("mdadm --assemble {} {}".format(
-                      raid_device, ' '.join(component_devices)))
+        utils.execute('mdadm', '--stop', raid_device)
+        utils.execute('mdadm', '--assemble', raid_device,
+                      *component_devices)
     except processutils.ProcessExecutionError as e:
         error_msg = ('Could not restart md device %(dev)s: %(err)s' %
                      {'dev': raid_device, 'err': e})
@@ -1438,8 +1437,8 @@ class GenericHardwareManager(HardwareManager):
         raid_device_count = len(block_devices)
         for index, logical_disk in enumerate(logical_disks):
             md_device = '/dev/md%d' % index
-            component_devices = ' '.join(
-                device.name + str(index + 1) for device in block_devices)
+            component_devices = [device.name + str(index + 1)
+                                 for device in block_devices]
             raid_level = logical_disk['raid_level']
             # The schema check allows '1+0', but mdadm knows it as '10'.
             if raid_level == '1+0':
@@ -1447,14 +1446,13 @@ class GenericHardwareManager(HardwareManager):
             try:
                 LOG.debug("Creating md device {} on {}".format(
                           md_device, component_devices))
-                cmd = ("mdadm --create {} --level={} --raid-devices={} {} "
-                       "--force --run --metadata=1").format(
-                           md_device, raid_level, raid_device_count,
-                           component_devices)
-                utils.execute(cmd)
+                utils.execute('mdadm', '--create', md_device, '--force',
+                              '--run', '--metadata=1', '--level', raid_level,
+                              '--raid-devices', raid_device_count,
+                              *component_devices)
             except processutils.ProcessExecutionError as e:
                 msg = "Failed to create md device {} on {}: {}".format(
-                    md_device, component_devices, e)
+                    md_device, ' '.join(component_devices), e)
                 raise errors.SoftwareRAIDError(msg)
 
         LOG.info("Successfully created Software RAID")
diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py
index 6f5a7b2b7..b3c8266de 100644
--- a/ironic_python_agent/tests/unit/test_hardware.py
+++ b/ironic_python_agent/tests/unit/test_hardware.py
@@ -2495,10 +2495,6 @@ class TestGenericHardwareManager(base.IronicAgentTest):
 
         result = self.hardware.create_configuration(self.node, [])
 
-        cmd_md0 = ("mdadm --create /dev/md0 --level=1 --raid-devices=2 "
-                   "/dev/sda1 /dev/sdb1 --force --run --metadata=1")
-        cmd_md1 = ("mdadm --create /dev/md1 --level=0 --raid-devices=2 "
-                   "/dev/sda2 /dev/sdb2 --force --run --metadata=1")
         mocked_execute.assert_has_calls([
             mock.call('parted', '/dev/sda', '-s', '--', 'mklabel', 'msdos'),
             mock.call('parted', '/dev/sdb', '-s', '--', 'mklabel', 'msdos'),
@@ -2510,8 +2506,12 @@ class TestGenericHardwareManager(base.IronicAgentTest):
                       'mkpart', 'primary', 102400, '-1'),
             mock.call('parted', '/dev/sdb', '-s', '-a', 'optimal', '--',
                       'mkpart', 'primary', 102400, '-1'),
-            mock.call(cmd_md0),
-            mock.call(cmd_md1)])
+            mock.call('mdadm', '--create', '/dev/md0', '--force', '--run',
+                      '--metadata=1', '--level', '1', '--raid-devices', 2,
+                      '/dev/sda1', '/dev/sdb1'),
+            mock.call('mdadm', '--create', '/dev/md1', '--force', '--run',
+                      '--metadata=1', '--level', '0', '--raid-devices', 2,
+                      '/dev/sda2', '/dev/sdb2')])
         self.assertEqual(raid_config, result)
 
     @mock.patch.object(utils, 'execute', autospec=True)