There's an accompanying anaconda patch, coming right up.
mulhern (2): Add BTRFSValueError error and use in btrfs related code (#1019685) Check if device has enough members when setting RAID level (#1019685)
blivet/devicelibs/btrfs.py | 20 ++++++------ blivet/devices.py | 76 +++++++++++++++++++++++++++++++++++----------- blivet/errors.py | 3 ++ 3 files changed, 72 insertions(+), 27 deletions(-)
Related: rhbz#1019685
From BTRFS related methods that have ValueError-like exceptions raise
BTRFSValueError instead of ValueError. Note that this will not change the behavior of code that calls these method and catches a ValueError since BTRFSValueError extends ValueError. It can change the behavior of calling code where a BTRFSError is expected; since BTRFSValueError is also a subtype of BTRFSError the formerly ValueError exception will be caught where previously it would not. There are no instances like this in anaconda. There are two in blivet, in devices.py, and in both cases, this change seems to be an improvement.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devicelibs/btrfs.py | 20 ++++++++++---------- blivet/devices.py | 14 +++++++------- blivet/errors.py | 3 +++ 3 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/blivet/devicelibs/btrfs.py b/blivet/devicelibs/btrfs.py index 0e1171d..ea8a984 100644 --- a/blivet/devicelibs/btrfs.py +++ b/blivet/devicelibs/btrfs.py @@ -25,7 +25,7 @@ import re
from . import raid from .. import util -from ..errors import BTRFSError +from ..errors import BTRFSError, BTRFSValueError
import logging log = logging.getLogger("blivet") @@ -64,9 +64,9 @@ def create_volume(devices, label=None, data=None, metadata=None): recognizes the string. """ if not devices: - raise ValueError("no devices specified") + raise BTRFSValueError("no devices specified") elif any([not os.path.exists(d) for d in devices]): - raise ValueError("one or more specified devices not present") + raise BTRFSValueError("one or more specified devices not present")
args = [] if data: @@ -90,20 +90,20 @@ def create_volume(devices, label=None, data=None, metadata=None):
def add(mountpoint, device): if not os.path.ismount(mountpoint): - raise ValueError("volume not mounted") + raise BTRFSValueError("volume not mounted")
return btrfs(["device", "add", device, mountpoint])
def remove(mountpoint, device): if not os.path.ismount(mountpoint): - raise ValueError("volume not mounted") + raise BTRFSValueError("volume not mounted")
return btrfs(["device", "delete", device, mountpoint])
def create_subvolume(mountpoint, name): """Create a subvolume named name below mountpoint mountpoint.""" if not os.path.ismount(mountpoint): - raise ValueError("volume not mounted") + raise BTRFSValueError("volume not mounted")
path = os.path.normpath("%s/%s" % (mountpoint, name)) args = ["subvol", "create", path] @@ -111,7 +111,7 @@ def create_subvolume(mountpoint, name):
def delete_subvolume(mountpoint, name): if not os.path.ismount(mountpoint): - raise ValueError("volume not mounted") + raise BTRFSValueError("volume not mounted")
path = os.path.normpath("%s/%s" % (mountpoint, name)) args = ["subvol", "delete", path] @@ -123,7 +123,7 @@ _SUBVOL_REGEX = re.compile(_SUBVOL_REGEX_STR) # get a list of subvolumes from a mounted btrfs filesystem def list_subvolumes(mountpoint, snapshots_only=False): if not os.path.ismount(mountpoint): - raise ValueError("volume not mounted") + raise BTRFSValueError("volume not mounted")
args = ["subvol", "list", "-p", mountpoint] if snapshots_only: @@ -139,7 +139,7 @@ def list_subvolumes(mountpoint, snapshots_only=False):
def get_default_subvolume(mountpoint): if not os.path.ismount(mountpoint): - raise ValueError("volume not mounted") + raise BTRFSValueError("volume not mounted")
args = ["subvol", "get-default", mountpoint] buf = btrfs(args, capture=True) @@ -158,7 +158,7 @@ def create_snapshot(source, dest, ro=False): :keyword bool ro: whether to create a read-only snapshot """ if not os.path.ismount(source): - raise ValueError("source is not a mounted subvolume") + raise BTRFSValueError("source is not a mounted subvolume")
args = ["subvol", "snapshot"] if ro: diff --git a/blivet/devices.py b/blivet/devices.py index 7e4279c..fe87226 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -4996,7 +4996,7 @@ class BTRFSDevice(StorageDevice): args = ("btrfs.%d" % self.id,)
if kwargs.get("parents") is None: - raise ValueError("BTRFSDevice must have at least one parent") + raise errors.BTRFSValueError("BTRFSDevice must have at least one parent")
self.req_size = kwargs.pop("size", None) super(BTRFSDevice, self).__init__(*args, **kwargs) @@ -5206,13 +5206,13 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
def _addSubVolume(self, vol): if vol.name in [v.name for v in self.subvolumes]: - raise ValueError("subvolume %s already exists" % vol.name) + raise errors.BTRFSValueError("subvolume %s already exists" % vol.name)
self.subvolumes.append(vol)
def _removeSubVolume(self, name): if name not in [v.name for v in self.subvolumes]: - raise ValueError("cannot remove non-existent subvolume %s" % name) + raise errors.BTRFSValueError("cannot remove non-existent subvolume %s" % name)
names = [v.name for v in self.subvolumes] self.subvolumes.pop(names.index(name)) @@ -5454,13 +5454,13 @@ class BTRFSSnapShotDevice(BTRFSSubVolumeDevice): source = kwargs.pop("source", None) if not kwargs.get("exists") and not source: # it is possible to remove a source subvol and keep snapshots of it - raise ValueError("non-existent btrfs snapshots must have a source") + raise errors.BTRFSValueError("non-existent btrfs snapshots must have a source")
if source and not isinstance(source, BTRFSDevice): - raise ValueError("btrfs snapshot source must be a btrfs subvolume") + raise errors.BTRFSValueError("btrfs snapshot source must be a btrfs subvolume")
if source and not source.exists: - raise ValueError("btrfs snapshot source must already exist") + raise errors.BTRFSValueError("btrfs snapshot source must already exist")
self.source = source """ the snapshot's source subvolume """ @@ -5472,7 +5472,7 @@ class BTRFSSnapShotDevice(BTRFSSubVolumeDevice): if source and getattr(source, "volume", source) != self.volume: self.volume._removeSubVolume(self.name) self.parents = [] - raise ValueError("btrfs snapshot and source must be in the same volume") + raise errors.BTRFSValueError("btrfs snapshot and source must be in the same volume")
def _create(self): log_method_call(self, self.name, status=self.status) diff --git a/blivet/errors.py b/blivet/errors.py index e0e5871..4c4fe30 100644 --- a/blivet/errors.py +++ b/blivet/errors.py @@ -146,6 +146,9 @@ class LoopError(StorageError): class BTRFSError(StorageError): pass
+class BTRFSValueError(BTRFSError, ValueError): + pass + # DeviceTree class DeviceTreeError(StorageError): pass
Related: rhbz#1019685
RAID levels have minimum member requirements. Level setters now raise a BTRFSValueError if device does not have enough members to meet the minimum requirement. However, if the device exists its members may not be known yet, so the check is omitted.
Also, RaidError is rethrown as BTRFSValueError in level setter if the level is not valid for its use. Previously the RaidError was just propagated.
Signed-off-by: mulhern amulhern@redhat.com --- blivet/devices.py | 62 ++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 10 deletions(-)
diff --git a/blivet/devices.py b/blivet/devices.py index fe87226..0e02396 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -5127,6 +5127,50 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
self._defaultSubVolumeID = None
+ def _validateRaidLevel(self, level): + """ Returns an error message if the RAID level is invalid for this + device, otherwise None. + + :param level: a RAID level + :type level: :class:`~.devicelibs.raid.RAIDLevel` + :returns: an error message if the RAID level is invalid, else None + :rtype: str or NoneType + """ + if not self.exists and len(self.parents) < level.min_members: + return "RAID level %s requires that device have at least %d members, but device has only %d members." % (level, level.min_members, len(self.parents)) + return None + + def _setLevel(self, value, data): + """ Sets a valid level for this device and level type. + + :param object value: value for this RAID level + :param bool data: True if for data, False if for metadata + + :returns: a valid level for value, if any, else None + :rtype: :class:`~.devicelibs.raid.RAIDLevel` or NoneType + + :raises: :class:`~.errors.BTRFSValueError` if value represents + an invalid level. + """ + level = None + if value: + try: + levels = btrfs.RAID_levels if data else btrfs.metadata_levels + level = levels.raidLevel(value) + except errors.RaidError: + data_type_str = "data" if data else "metadata" + raise errors.BTRFSValueError("%s is an invalid value for %s RAID level." % (value, data_type_str)) + + error_msg = self._validateRaidLevel(level) + if error_msg: + raise errors.BTRFSValueError(error_msg) + + # pylint: disable=attribute-defined-outside-init + if data: + self._dataLevel = level + else: + self._metaDataLevel = level + @property def dataLevel(self): """ Return the RAID level for data. @@ -5140,12 +5184,11 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice): def dataLevel(self, value): """ Set the RAID level for data.
- :param value: new raid level - :param type: a valid raid level descriptor - :returns: None + :param object value: new raid level + :returns: None + :raises: :class:`~.errors.BTRFSValueError` """ - # pylint: disable=attribute-defined-outside-init - self._dataLevel = btrfs.RAID_levels.raidLevel(value) if value else None + self._setLevel(value, True)
@property def metaDataLevel(self): @@ -5160,12 +5203,11 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice): def metaDataLevel(self, value): """ Set the RAID level for metadata.
- :param value: new raid level - :param type: a valid raid level descriptor - :returns: None + :param object value: new raid level + :returns: None + :raises: :class:`~.errors.BTRFSValueError` """ - # pylint: disable=attribute-defined-outside-init - self._metaDataLevel = btrfs.metadata_levels.raidLevel(value) if value else None + self._setLevel(value, False)
@property def formatImmutable(self):
On Thu, 2014-09-11 at 13:34 -0400, mulhern wrote:
Related: rhbz#1019685
RAID levels have minimum member requirements. Level setters now raise a BTRFSValueError if device does not have enough members to meet the minimum requirement. However, if the device exists its members may not be known yet, so the check is omitted.
Also, RaidError is rethrown as BTRFSValueError in level setter if the level is not valid for its use. Previously the RaidError was just propagated.
Signed-off-by: mulhern amulhern@redhat.com
blivet/devices.py | 62 ++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 10 deletions(-)
diff --git a/blivet/devices.py b/blivet/devices.py index fe87226..0e02396 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -5127,6 +5127,50 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
self._defaultSubVolumeID = None
- def _validateRaidLevel(self, level):
""" Returns an error message if the RAID level is invalid for this
device, otherwise None.
:param level: a RAID level
:type level: :class:`~.devicelibs.raid.RAIDLevel`
:returns: an error message if the RAID level is invalid, else None
:rtype: str or NoneType
"""
if not self.exists and len(self.parents) < level.min_members:
return "RAID level %s requires that device have at least %d members, but device has only %d members." % (level, level.min_members, len(self.parents))
return None
- def _setLevel(self, value, data):
""" Sets a valid level for this device and level type.
:param object value: value for this RAID level
:param bool data: True if for data, False if for metadata
:returns: a valid level for value, if any, else None
:rtype: :class:`~.devicelibs.raid.RAIDLevel` or NoneType
:raises: :class:`~.errors.BTRFSValueError` if value represents
an invalid level.
"""
level = None
if value:
try:
levels = btrfs.RAID_levels if data else btrfs.metadata_levels
level = levels.raidLevel(value)
except errors.RaidError:
data_type_str = "data" if data else "metadata"
raise errors.BTRFSValueError("%s is an invalid value for %s RAID level." % (value, data_type_str))
error_msg = self._validateRaidLevel(level)
if error_msg:
raise errors.BTRFSValueError(error_msg)
# pylint: disable=attribute-defined-outside-init
Why not to define these in __init__?
if data:
self._dataLevel = level
else:
self._metaDataLevel = level
- @property def dataLevel(self): """ Return the RAID level for data.
@@ -5140,12 +5184,11 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice): def dataLevel(self, value): """ Set the RAID level for data.
:param value: new raid level
:param type: a valid raid level descriptor
:returns: None
:param object value: new raid level
:returns: None
:raises: :class:`~.errors.BTRFSValueError` """
# pylint: disable=attribute-defined-outside-init
self._dataLevel = btrfs.RAID_levels.raidLevel(value) if value else None
self._setLevel(value, True)
@property def metaDataLevel(self):
@@ -5160,12 +5203,11 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice): def metaDataLevel(self, value): """ Set the RAID level for metadata.
:param value: new raid level
:param type: a valid raid level descriptor
:returns: None
:param object value: new raid level
:returns: None
:raises: :class:`~.errors.BTRFSValueError` """
# pylint: disable=attribute-defined-outside-init
self._metaDataLevel = btrfs.metadata_levels.raidLevel(value) if value else None
self._setLevel(value, False)
@property def formatImmutable(self):
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: "anaconda patch review" anaconda-patches@lists.fedorahosted.org Sent: Friday, September 12, 2014 2:30:26 AM Subject: Re: [blivet:rhel7/master 2/2] Check if device has enough members when setting RAID level (#1019685)
On Thu, 2014-09-11 at 13:34 -0400, mulhern wrote:
Related: rhbz#1019685
RAID levels have minimum member requirements. Level setters now raise a BTRFSValueError if device does not have enough members to meet the minimum requirement. However, if the device exists its members may not be known yet, so the check is omitted.
Also, RaidError is rethrown as BTRFSValueError in level setter if the level is not valid for its use. Previously the RaidError was just propagated.
Signed-off-by: mulhern amulhern@redhat.com
blivet/devices.py | 62 ++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 10 deletions(-)
diff --git a/blivet/devices.py b/blivet/devices.py index fe87226..0e02396 100644 --- a/blivet/devices.py +++ b/blivet/devices.py @@ -5127,6 +5127,50 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice):
self._defaultSubVolumeID = None
- def _validateRaidLevel(self, level):
""" Returns an error message if the RAID level is invalid for this
device, otherwise None.
:param level: a RAID level
:type level: :class:`~.devicelibs.raid.RAIDLevel`
:returns: an error message if the RAID level is invalid, else
None
:rtype: str or NoneType
"""
if not self.exists and len(self.parents) < level.min_members:
return "RAID level %s requires that device have at least %d
members, but device has only %d members." % (level, level.min_members, len(self.parents))
return None
- def _setLevel(self, value, data):
""" Sets a valid level for this device and level type.
:param object value: value for this RAID level
:param bool data: True if for data, False if for metadata
:returns: a valid level for value, if any, else None
:rtype: :class:`~.devicelibs.raid.RAIDLevel` or NoneType
:raises: :class:`~.errors.BTRFSValueError` if value represents
an invalid level.
"""
level = None
if value:
try:
levels = btrfs.RAID_levels if data else
btrfs.metadata_levels
level = levels.raidLevel(value)
except errors.RaidError:
data_type_str = "data" if data else "metadata"
raise errors.BTRFSValueError("%s is an invalid value for
%s RAID level." % (value, data_type_str))
error_msg = self._validateRaidLevel(level)
if error_msg:
raise errors.BTRFSValueError(error_msg)
# pylint: disable=attribute-defined-outside-init
Why not to define these in __init__?
We actually do define these in __init__, not directly, but transitively, since we always assign to self.dataLevel and self.metatDataLevel in __init__.
As far as I knew at the time, pylint analysis would, however, never be strong enough to detect this fact, so I put this annotation here, instead of screening it out in false-positives.
Turns out, though, that they actually believe that they've fixed the problem in recent version of pylint (https://bitbucket.org/logilab/pylint/issue/192/w0201-message-may-be-a-little...), so soon we may be able to get rid of this annotation altogether. I'ld still like to keep it here, however, because I haven't yet seen whether their analysis is successful.
Meanwhile, we're confident that either these attributes are set in the object or the object is not created, which is just what we want so we don't ever get an AttributeError when accessing them.
if data:
self._dataLevel = level
else:
self._metaDataLevel = level
- @property def dataLevel(self): """ Return the RAID level for data.
@@ -5140,12 +5184,11 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice): def dataLevel(self, value): """ Set the RAID level for data.
:param value: new raid level
:param type: a valid raid level descriptor
:returns: None
:param object value: new raid level
:returns: None
:raises: :class:`~.errors.BTRFSValueError` """
# pylint: disable=attribute-defined-outside-init
self._dataLevel = btrfs.RAID_levels.raidLevel(value) if value else
None
self._setLevel(value, True)
@property def metaDataLevel(self):
@@ -5160,12 +5203,11 @@ class BTRFSVolumeDevice(BTRFSDevice, ContainerDevice): def metaDataLevel(self, value): """ Set the RAID level for metadata.
:param value: new raid level
:param type: a valid raid level descriptor
:returns: None
:param object value: new raid level
:returns: None
:raises: :class:`~.errors.BTRFSValueError` """
# pylint: disable=attribute-defined-outside-init
self._metaDataLevel = btrfs.metadata_levels.raidLevel(value) if
value else None
self._setLevel(value, False)
@property def formatImmutable(self):
-- Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern
On Thu, 2014-09-11 at 13:34 -0400, mulhern wrote:
There's an accompanying anaconda patch, coming right up.
mulhern (2): Add BTRFSValueError error and use in btrfs related code (#1019685) Check if device has enough members when setting RAID level (#1019685)
blivet/devicelibs/btrfs.py | 20 ++++++------ blivet/devices.py | 76 +++++++++++++++++++++++++++++++++++----------- blivet/errors.py | 3 ++ 3 files changed, 72 insertions(+), 27 deletions(-)
Other than that one comment on PATCH 2/2 these both look good to me.
anaconda-patches@lists.fedorahosted.org