From 3b7f5be918587fcf14492c8d7a0abcdbedb62b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcus=20Sch=C3=A4fer?= Date: Wed, 18 Mar 2020 16:01:13 +0100 Subject: [PATCH] Complete refactoring of fstab handling With the new Fstab class from prior pull request there is an opportunity to handle all fstab related actions to be done by that class. This commit extends the Fstab class with an add_entry method such that we can avoid the extra lists holding raw fstab lines in e.g the disk builder. In the end all fstab related data is stored in an instance of the Fstab class. This also extends the KIWI api by an fstab management class. Related to #1329 and #1349 --- kiwi/builder/disk.py | 18 +++---- kiwi/system/setup.py | 22 +++------ kiwi/utils/fstab.py | 90 +++++++++++++++++++++------------- test/data/fstab | 3 ++ test/unit/builder/disk_test.py | 29 +++++------ test/unit/system/setup_test.py | 15 +++--- test/unit/utils/fstab_test.py | 13 ++++- 7 files changed, 103 insertions(+), 87 deletions(-) diff --git a/kiwi/builder/disk.py b/kiwi/builder/disk.py index a8d6afd9..1b2b8a2b 100644 --- a/kiwi/builder/disk.py +++ b/kiwi/builder/disk.py @@ -43,6 +43,7 @@ from kiwi.system.kernel import Kernel from kiwi.storage.subformat import DiskFormat from kiwi.system.result import Result from kiwi.utils.block import BlockID +from kiwi.utils.fstab import Fstab from kiwi.path import Path from kiwi.runtime_config import RuntimeConfig from kiwi.partitioner import Partitioner @@ -139,7 +140,7 @@ class DiskBuilder: self.boot_is_crypto = True if self.luks and not \ self.disk_setup.need_boot_partition() else False self.install_media = self._install_image_requested() - self.generic_fstab_entries = [] + self.fstab = Fstab() # an instance of a class with the sync_data capability # representing the entire image system except for the boot/ area @@ -893,8 +894,7 @@ class DiskBuilder: self.persistency_type, self.requested_filesystem ) for volume_fstab_entry in volume_fstab_entries: - if volume_fstab_entry not in self.generic_fstab_entries: - self.generic_fstab_entries.append(volume_fstab_entry) + self.fstab.add_entry(volume_fstab_entry) if device_map.get('spare') and \ self.spare_part_fs and self.spare_part_mountpoint: self._add_generic_fstab_entry( @@ -910,7 +910,7 @@ class DiskBuilder: device_map['swap'].get_device(), 'swap' ) setup.create_fstab( - self.generic_fstab_entries + self.fstab ) def _add_simple_fstab_entry( @@ -923,10 +923,7 @@ class DiskBuilder: device, mount_point, filesystem, ','.join(options), check ] ) - if fstab_entry not in self.generic_fstab_entries: - self.generic_fstab_entries.append( - fstab_entry - ) + self.fstab.add_entry(fstab_entry) def _add_generic_fstab_entry( self, device, mount_point, options=None, check='0 0' @@ -942,10 +939,7 @@ class DiskBuilder: block_operation.get_filesystem(), ','.join(options), check ] ) - if fstab_entry not in self.generic_fstab_entries: - self.generic_fstab_entries.append( - fstab_entry - ) + self.fstab.add_entry(fstab_entry) def _write_image_identifier_to_system_image(self): log.info('Creating image identifier: %s', self.mbrid.get_id()) diff --git a/kiwi/system/setup.py b/kiwi/system/setup.py index 95ae7598..ca276e93 100644 --- a/kiwi/system/setup.py +++ b/kiwi/system/setup.py @@ -32,7 +32,6 @@ from kiwi.system.root_init import RootInit from kiwi.command import Command from kiwi.command_process import CommandProcess from kiwi.utils.sync import DataSync -from kiwi.utils.fstab import Fstab from kiwi.defaults import Defaults from kiwi.system.users import Users from kiwi.system.shell import Shell @@ -585,9 +584,9 @@ class SystemSetup: working_directory=working_directory ) - def create_fstab(self, entries): + def create_fstab(self, fstab): """ - Create etc/fstab from given list of entries + Create etc/fstab from given Fstab object Custom fstab modifications are possible and handled in the following order: @@ -608,17 +607,17 @@ class SystemSetup: file in the image rootfs. Once called the fstab.script file will be deleted - :param list entries: list of line entries for fstab + :param list fstab: instance of Fstab """ fstab_file = self.root_dir + '/etc/fstab' fstab_append_file = self.root_dir + '/etc/fstab.append' fstab_patch_file = self.root_dir + '/etc/fstab.patch' fstab_script_file = self.root_dir + '/etc/fstab.script' - with open(fstab_file, 'w') as fstab: - for entry in entries: - fstab.write(entry + os.linesep) - if os.path.exists(fstab_append_file): + fstab.export(fstab_file) + + if os.path.exists(fstab_append_file): + with open(fstab_file, 'a') as fstab: with open(fstab_append_file, 'r') as append: fstab.write(append.read()) Path.wipe(fstab_append_file) @@ -635,13 +634,6 @@ class SystemSetup: ) Path.wipe(fstab_script_file) - # rewrite fstab after initial creation and after all - # optional modifications to make sure the canonical - # mount order is correct and no garbage exists. - fstab_final = Fstab() - fstab_final.read(fstab_file) - fstab_final.export(fstab_file) - def create_init_link_from_linuxrc(self): """ kiwi boot images provides the linuxrc script, however the kernel diff --git a/kiwi/utils/fstab.py b/kiwi/utils/fstab.py index 0f79963f..7a12be7d 100644 --- a/kiwi/utils/fstab.py +++ b/kiwi/utils/fstab.py @@ -15,12 +15,15 @@ # You should have received a copy of the GNU General Public License # along with kiwi. If not, see # +import logging import os from collections import namedtuple # project from kiwi.path import Path +log = logging.getLogger('kiwi') + class Fstab: """ @@ -38,42 +41,28 @@ class Fstab: """ Import specified fstab file + Read the given fstab file and initialize a new entry list + :param string filename: path to a fstab file """ self.fstab = [] with open(filename) as fstab: for line in fstab.readlines(): - mount_record = line.split() - if not mount_record or mount_record[0].startswith('#'): - continue - device = mount_record[0] - mountpoint = mount_record[1] - fstype = mount_record[2] - options = mount_record[3] - if device.startswith('UUID'): - device_path = ''.join( - ['/dev/disk/by-uuid/', device.split('=')[1]] - ) - elif device.startswith('LABEL'): - device_path = ''.join( - ['/dev/disk/by-label/', device.split('=')[1]] - ) - elif device.startswith('PARTUUID'): - device_path = ''.join( - ['/dev/disk/by-partuuid/', device.split('=')[1]] - ) - else: - device_path = device + self.add_entry(line) - self.fstab.append( - self.fstab_entry_type( - fstype=fstype, - mountpoint=mountpoint, - device_path=device_path, - device_spec=device, - options=options + def add_entry(self, line): + new_entry = self._parse_entry(line) + if new_entry: + for entry in self.fstab: + if entry.mountpoint == new_entry.mountpoint: + log.warning( + 'Mountpoint for "{0}" in use by "{1}", skipped'.format( + self._file_entry(new_entry), + self._file_entry(entry) + ) ) - ) + return + self.fstab.append(new_entry) def get_devices(self): return self.fstab @@ -94,9 +83,42 @@ class Fstab: ): entry = fstab_entries_by_path[device_path] fstab.write( - '{0} {1} {2} {3} 0 0{4}'.format( - entry.device_spec, entry.mountpoint, - entry.fstype, entry.options, - os.linesep - ) + self._file_entry(entry) + os.linesep ) + + def _file_entry(self, entry): + return '{0} {1} {2} {3} 0 0'.format( + entry.device_spec, entry.mountpoint, + entry.fstype, entry.options + ) + + def _parse_entry(self, line): + data_record = line.split() + if data_record and len(data_record) >= 4 \ + and not data_record[0].startswith('#'): + device = data_record[0] + mountpoint = data_record[1] + fstype = data_record[2] + options = data_record[3] + if device.startswith('UUID'): + device_path = ''.join( + ['/dev/disk/by-uuid/', device.split('=')[1]] + ) + elif device.startswith('LABEL'): + device_path = ''.join( + ['/dev/disk/by-label/', device.split('=')[1]] + ) + elif device.startswith('PARTUUID'): + device_path = ''.join( + ['/dev/disk/by-partuuid/', device.split('=')[1]] + ) + else: + device_path = device + + return self.fstab_entry_type( + fstype=fstype, + mountpoint=mountpoint, + device_path=device_path, + device_spec=device, + options=options + ) diff --git a/test/data/fstab b/test/data/fstab index ff6696d5..dadea8cc 100644 --- a/test/data/fstab +++ b/test/data/fstab @@ -5,5 +5,8 @@ LABEL=BOOT /boot xfs defaults 0 0 LABEL=foo /home ext4 defaults 0 0 PARTUUID=3c8bd108-01 /bar ext4 defaults 0 0 /dev/mynode /foo ext4 defaults 0 0 +# entry with the same mountpoint, expected to be skipped +LABEL=bar /home xfs defaults 0 0 + # this comment line and the line above should be ignored by the parser diff --git a/test/unit/builder/disk_test.py b/test/unit/builder/disk_test.py index e24a2120..7bbcbd26 100644 --- a/test/unit/builder/disk_test.py +++ b/test/unit/builder/disk_test.py @@ -188,6 +188,10 @@ class TestDiskBuilder: kiwi.builder.disk.LuksDevice = mock.Mock( return_value=self.luks_root ) + self.fstab = mock.Mock() + kiwi.builder.disk.Fstab = mock.Mock( + return_value=self.fstab + ) self.disk_builder = DiskBuilder( XMLState(description.load()), 'target_dir', 'root_dir', custom_args={'signing_keys': ['key_file_a', 'key_file_b']} @@ -754,22 +758,10 @@ class TestDiskBuilder: volume_manager.umount_volumes.call_args_list[0].assert_called_once_with( ) self.setup.create_fstab.assert_called_once_with( - [ - 'UUID=blkid_result / blkid_result_fs ro 0 0', - 'UUID=blkid_result /boot blkid_result_fs defaults 0 0', - 'UUID=blkid_result /boot/efi blkid_result_fs defaults 0 0', - 'fstab_volume_entries', - '/dev/systemVG/LVSwap swap swap defaults 0 0' - ] + self.disk_builder.fstab ) self.boot_image_task.setup.create_fstab.assert_called_once_with( - [ - 'UUID=blkid_result / blkid_result_fs ro 0 0', - 'UUID=blkid_result /boot blkid_result_fs defaults 0 0', - 'UUID=blkid_result /boot/efi blkid_result_fs defaults 0 0', - 'fstab_volume_entries', - '/dev/systemVG/LVSwap swap swap defaults 0 0' - ] + self.disk_builder.fstab ) @patch('kiwi.builder.disk.FileSystem') @@ -855,8 +847,13 @@ class TestDiskBuilder: 'boot/*', 'boot/.*', 'boot/efi/*', 'boot/efi/.*' ] ) - assert 'UUID=blkid_result /var blkid_result_fs defaults 0 0' in \ - self.disk_builder.generic_fstab_entries + assert [ + call('UUID=blkid_result / blkid_result_fs ro 0 0'), + call('UUID=blkid_result /boot blkid_result_fs defaults 0 0'), + call('UUID=blkid_result /boot/efi blkid_result_fs defaults 0 0'), + call('UUID=blkid_result /var blkid_result_fs defaults 0 0'), + call('UUID=blkid_result swap blkid_result_fs defaults 0 0'), + ] in self.disk_builder.fstab.add_entry.call_args_list self.disk.create_root_partition.reset_mock() self.disk.create_spare_partition.reset_mock() diff --git a/test/unit/system/setup_test.py b/test/unit/system/setup_test.py index 3909ef59..b9ddd18b 100644 --- a/test/unit/system/setup_test.py +++ b/test/unit/system/setup_test.py @@ -692,24 +692,23 @@ class TestSystemSetup: @patch('os.path.exists') @patch('kiwi.system.setup.Path.wipe') @patch('kiwi.command.Command.run') - @patch('kiwi.system.setup.Fstab') def test_create_fstab( - self, mock_Fstab, mock_command, mock_wipe, mock_exists + self, mock_command, mock_wipe, mock_exists ): - fstab_final = Mock() - mock_Fstab.return_value = fstab_final + fstab = Mock() mock_exists.return_value = True m_open = mock_open(read_data='append_entry') with patch('builtins.open', m_open, create=True): - self.setup.create_fstab(['fstab_entry']) + self.setup.create_fstab(fstab) + + fstab.export.assert_called_once_with('root_dir/etc/fstab') assert m_open.call_args_list == [ - call('root_dir/etc/fstab', 'w'), + call('root_dir/etc/fstab', 'a'), call('root_dir/etc/fstab.append', 'r') ] assert m_open.return_value.write.call_args_list == [ - call('fstab_entry\n'), call('append_entry') ] assert mock_command.call_args_list == [ @@ -721,8 +720,6 @@ class TestSystemSetup: call('root_dir/etc/fstab.patch'), call('root_dir/etc/fstab.script') ] - fstab_final.read.assert_called_once_with('root_dir/etc/fstab') - fstab_final.export.assert_called_once_with('root_dir/etc/fstab') @patch('kiwi.command.Command.run') @patch('kiwi.system.setup.NamedTemporaryFile') diff --git a/test/unit/utils/fstab_test.py b/test/unit/utils/fstab_test.py index 55c1fcba..2109554d 100644 --- a/test/unit/utils/fstab_test.py +++ b/test/unit/utils/fstab_test.py @@ -1,4 +1,6 @@ import io +import logging +from pytest import fixture from unittest.mock import ( MagicMock, patch, call ) @@ -6,9 +8,18 @@ from kiwi.utils.fstab import Fstab class TestFstab(object): + @fixture(autouse=True) + def inject_fixtures(self, caplog): + self._caplog = caplog + def setup(self): self.fstab = Fstab() - self.fstab.read('../data/fstab') + with self._caplog.at_level(logging.WARNING): + self.fstab.read('../data/fstab') + assert format( + 'Mountpoint for "LABEL=bar /home xfs defaults 0 0" ' + 'in use by "LABEL=foo /home ext4 defaults 0 0", skipped' + ) in self._caplog.text def test_get_devices(self): assert self.fstab.get_devices() == [