From b91207ae47e50ebbf689f9e9d5fd2147a8575e76 Mon Sep 17 00:00:00 2001
From: Ian Wienand <iwienand@redhat.com>
Date: Wed, 10 May 2017 09:59:26 +1000
Subject: [PATCH] Remove _config_error thrower
"log and throw" is arguably an anti-pattern; the error message either
bubles-up into the exception, or the handler figures it out. We have
an example where this logs, and then the handler in blockdevice.py
catches it and logs it again.
Less layers is better; just raise the exception, and use log.exception
to get tracebacks where handled.
Change-Id: I8efd94fbe52a3911253753f447afdb7565849185
---
.../block_device/level1/partitioning.py | 32 +++++++++----------
diskimage_builder/block_device/level2/mkfs.py | 28 +++++++---------
.../block_device/level3/mount.py | 20 ++++--------
.../block_device/level4/fstab.py | 6 ----
4 files changed, 35 insertions(+), 51 deletions(-)
diff --git a/diskimage_builder/block_device/level1/partitioning.py b/diskimage_builder/block_device/level1/partitioning.py
index 937379cba..1b2d17f0c 100644
--- a/diskimage_builder/block_device/level1/partitioning.py
+++ b/diskimage_builder/block_device/level1/partitioning.py
@@ -152,14 +152,19 @@ class Partitioning(PluginBase):
# Parameter check
if 'base' not in config:
- self._config_error("Partitioning config needs 'base'")
+ raise BlockDeviceSetupException("Partitioning config needs 'base'")
self.base = config['base']
+ if 'partitions' not in config:
+ raise BlockDeviceSetupException(
+ "Partitioning config needs 'partitions'")
+
if 'label' not in config:
- self._config_error("Partitioning config needs 'label'")
+ raise BlockDeviceSetupException(
+ "Partitioning config needs 'label'")
self.label = config['label']
if self.label not in ("mbr", ):
- self._config_error("Label must be 'mbr'")
+ raise BlockDeviceSetupException("Label must be 'mbr'")
# It is VERY important to get the alignment correct. If this
# is not correct, the disk performance might be very poor.
@@ -176,15 +181,13 @@ class Partitioning(PluginBase):
if 'align' in config:
self.align = parse_abs_size_spec(config['align'])
- if 'partitions' not in config:
- self._config_error("Partitioning config needs 'partitions'")
-
self.partitions = []
prev_partition = None
for part_cfg in config['partitions']:
if 'name' not in part_cfg:
- self.config_error("Missing 'name' in partition config")
+ raise BlockDeviceSetupException(
+ "Missing 'name' in partition config")
part_name = part_cfg['name']
flags = set()
@@ -195,12 +198,13 @@ class Partitioning(PluginBase):
elif f == 'primary':
flags.add(Partitioning.flag_primary)
else:
- self._config_error("Unknown flag [%s] in "
- "partitioning for [%s]"
- % (f, part_name))
+ raise BlockDeviceSetupException(
+ "Unknown flag [%s] in partitioning for [%s]"
+ % (f, part_name))
+
if 'size' not in part_cfg:
- self._config_error("No 'size' in partition [%s]"
- % part_name)
+ raise BlockDeviceSetupException("No 'size' in partition [%s]"
+ % part_name)
size = part_cfg['size']
ptype = int(part_cfg['type'], 16) if 'type' in part_cfg else 0x83
@@ -211,10 +215,6 @@ class Partitioning(PluginBase):
prev_partition = np
logger.debug(part_cfg)
- def _config_error(self, msg):
- logger.error(msg)
- raise BlockDeviceSetupException(msg)
-
def _size_of_block_dev(self, dev):
with open(dev, "r") as fd:
fd.seek(0, 2)
diff --git a/diskimage_builder/block_device/level2/mkfs.py b/diskimage_builder/block_device/level2/mkfs.py
index fe27dda88..4ce4cdd27 100644
--- a/diskimage_builder/block_device/level2/mkfs.py
+++ b/diskimage_builder/block_device/level2/mkfs.py
@@ -43,16 +43,13 @@ file_system_max_label_length = {
class Filesystem(Digraph.Node):
- def _config_error(self, msg):
- logger.error(msg)
- raise BlockDeviceSetupException(msg)
-
def __init__(self, config):
logger.debug("Create filesystem object; config [%s]" % config)
# Parameter check (mandatory)
for pname in ['base', 'name', 'type']:
if pname not in config:
- self._config_error("Mkfs config needs [%s]" % pname)
+ raise BlockDeviceSetupException(
+ "Mkfs config needs [%s]" % pname)
setattr(self, pname, config[pname])
# Parameter check (optional)
@@ -71,20 +68,19 @@ class Filesystem(Digraph.Node):
self.label = "img-rootfs"
if self.label in file_system_labels:
- self._config_error(
- "File system label [%s] used more than once" %
- self.label)
+ raise BlockDeviceSetupException(
+ "File system label [%s] used more than once" % self.label)
file_system_labels.add(self.label)
if self.type in file_system_max_label_length:
- if file_system_max_label_length[self.type] < \
- len(self.label):
- self._config_error(
- "Label [%s] too long for filesystem [%s]: "
- "maximum length [%d] provided length [%d]" %
- (self.label, self.type,
- file_system_max_label_length[self.type],
- len(self.label)))
+ if file_system_max_label_length[self.type] < len(self.label):
+ raise BlockDeviceSetupException(
+ "Label [{label}] too long for filesystem [{type}]: "
+ " [{len}] > [{max_len}]".format({
+ 'label': self.label,
+ 'type': self.type,
+ 'len': len(self.label),
+ 'max': file_system_max_label_length[self.type]}))
else:
logger.warning("Length of label [%s] cannot be checked for "
"filesystem [%s]: unknown max length" %
diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py
index b247b1f8e..a0f0e91c6 100644
--- a/diskimage_builder/block_device/level3/mount.py
+++ b/diskimage_builder/block_device/level3/mount.py
@@ -35,17 +35,13 @@ sorted_mount_points = None
class MountPoint(Digraph.Node):
- @staticmethod
- def _config_error(msg):
- logger.error(msg)
- raise BlockDeviceSetupException(msg)
-
def __init__(self, mount_base, config):
# Parameter check
self.mount_base = mount_base
for pname in ['base', 'name', 'mount_point']:
if pname not in config:
- self._config_error("MountPoint config needs [%s]" % pname)
+ raise BlockDeviceSetupException(
+ "MountPoint config needs [%s]" % pname)
setattr(self, pname, config[pname])
Digraph.Node.__init__(self, self.name)
logger.debug("MountPoint created [%s]" % self)
@@ -57,8 +53,9 @@ class MountPoint(Digraph.Node):
def insert_node(self, dg):
global mount_points
if self.mount_point in mount_points:
- self._config_error("Mount point [%s] specified more than once"
- % self.mount_point)
+ raise BlockDeviceSetupException(
+ "Mount point [%s] specified more than once"
+ % self.mount_point)
logger.debug("Insert node [%s]" % self)
mount_points[self.mount_point] = self
dg.add_node(self)
@@ -136,17 +133,14 @@ class Mount(object):
type_string = "mount"
tree_config = TreeConfig("mount")
- def _config_error(self, msg):
- logger.error(msg)
- raise BlockDeviceSetupException(msg)
-
def __init__(self, config, params):
logger.debug("Mounting object; config [%s]" % config)
self.config = config
self.params = params
if 'mount-base' not in self.params:
- MountPoint._config_error("Mount default config needs 'mount-base'")
+ raise BlockDeviceSetupException(
+ "Mount default config needs 'mount-base'")
self.mount_base = self.params['mount-base']
self.mount_points = {}
diff --git a/diskimage_builder/block_device/level4/fstab.py b/diskimage_builder/block_device/level4/fstab.py
index dc91ebc79..7f3cacd75 100644
--- a/diskimage_builder/block_device/level4/fstab.py
+++ b/diskimage_builder/block_device/level4/fstab.py
@@ -14,8 +14,6 @@
import logging
-from diskimage_builder.block_device.blockdevice \
- import BlockDeviceSetupException
from diskimage_builder.block_device.tree_config import TreeConfig
from diskimage_builder.graph.digraph import Digraph
@@ -28,10 +26,6 @@ class Fstab(Digraph.Node):
type_string = "fstab"
tree_config = TreeConfig("fstab")
- def _config_error(self, msg):
- logger.error(msg)
- raise BlockDeviceSetupException(msg)
-
def __init__(self, config, params):
logger.debug("Fstab object; config [%s]" % config)
self.config = config